-
Notifications
You must be signed in to change notification settings - Fork 2
TF Listener + Reset Wrapped #72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
schromya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've requested some changes to this branch. Please look at the comments on each file. Some of these changes are relics from the previous IK task that didn't get fixed due to the last milestone time constraint.
These are also some changes that are not file specific:
- Please remove both
/logdirectories and make sure it is added to the .gitignore - Please remove the
/testfolder - Please move
robot_description/compositesintolibs/robot_description/ros/src/robot_description/urdfand delete all folders underrobot_descriptionacceptros(to avoid duplicates since we moved all the urdfs into the ros package)
| # path_to_current_package = get_package_share_directory('robot_motion') + '/robot_motion/ik/' | ||
| # setting_file_path = path_to_current_package + 'config/' + 'bimanual_ik_settings.yaml' # need to change this probably as currently not working for settings.yaml | ||
| # print("Setting file path in display.launch.py:", setting_file_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove these
| # TODO: parameterize these | ||
| urdf_path = path_to_pkg + '/urdf/bimanual_arms.urdf' | ||
| urdf_file = open(urdf_path, 'r') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this a substitution so the relative path (i.e. so /urdf/bimanual_arms.urdf can be subbed out if needed). Here is an example of substitutions
| urdf_file = open(urdf_path, 'r') | ||
| urdf_string = urdf_file.read() | ||
|
|
||
| rviz_cfg_path = path_to_pkg + '/config/bimanual_arm.rviz' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also make a substitution for this to (see previous comment).
| output='screen', | ||
| arguments=['-d',rviz_cfg_path], | ||
| ), | ||
| # TODO: Allow parameter to update /joint_state topic to other topic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do this with an argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this from this branch as its not being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please update all the xacros that generate this with the changes that were added to here (i.e. the extra link offsets). May need to look at git history to get all the changes.
- Please update all the xacros that build this file to take custom filename prefix that's different from file source prefix (which is what the xacros currently use as an argument). Then add instructions for re-generating this file from the xacros and add that to the readme. Make sure to regenerate this file to check that it works properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add example of requesting tf frame between to joints. For example: ros2 run tf2_ros tf2_echo table left_delto_offset_link
| """ | ||
| frames = self.tf_buffer.all_frames_as_string() | ||
| frames_yaml = self.tf_buffer.all_frames_as_yaml() | ||
| # self.get_logger().info(f"\nCurrent TF frames:\n{frames}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commend out text.
| print(f"\nCurrent TF frames (YAML):\n{frames_yaml}") | ||
|
|
||
|
|
||
| def main(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add function comment.
| return None | ||
|
|
||
|
|
||
| def print_available_frames(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this function if not in use.
|
|
||
| self._joint_names = settings["joint_names"] | ||
|
|
||
| def reset(self, joint_state): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add type hint to this.
| Reset the internal state of the solver with a new joint_state seed. | ||
| Args: | ||
| joint_state (list[float]): List of joint angles (in radians) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please adjust this to take in np.ndarray (and then convert to list if needed) to keep consistent high-level api.
Wrote a new new file in robot_description/ros/src/robot_description called frame_listener.py.
the reset also added to ranged_ik.py