-
Notifications
You must be signed in to change notification settings - Fork 327
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
[RQT-JTC] Find limits only in jtc joints | add robot_description combo #1131
base: master
Are you sure you want to change the base?
Conversation
changed default value from `odom` -> `base_link`
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
…cription Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
I see the need for the namespace issue, but what do you mean by that? A revolute/prismatic joint without limits is not a genuine URDF, and we throw an error since ros-controls/ros2_control#1256, or ros2_control 4.9.0 respectively. |
@christophfroehlich I'm asking about this because when the errors about unknown limits of mimic joints throw then there JTC doen't appear in controllers combo. Screencast.from.15.05.2024.16.49.21.webm |
Ok, so the problem comes from mimic joints, where no limits are given? That does not make a lot of sense to add it, but it is not explicitly given in the XML specification that it can be omitted. I'll come back later to check what we excpect in the component parser. |
Take a look to I don't really know if limits should be implemented in continuous mimic joints or not. The issue with |
Thanks for the background information. I agree that it should check the to-be-controlled joints only, but I'm still curious what should be the correct XML in this case. We have the limit tag in our examples and the release notes, but this should not block this PR but related to: #891 |
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 was able to find some time to test it now.
Maybe you should split the joint-parsing from the the combo-box PR. The first one is great and works well, but I'm not yet convinced from the combo-box:
- Maybe I don't see it, but couldn't you just run
ros2 run rqt_joint_trajectory_controller rqt_joint_trajectory_controller --ros-args -r /robot_description:=/my_robot_descr
instead of changing the gui? Yes, it is more comfortable with the combo box but at least we should addrobot_description
as default. - The console output is rather noisy now (>40x
Waiting for the robot_description!
) after selecting the topic (if the controller_manager was selected before). I don't see what has changed in the logic, please have a look. - The three combo boxes side by side are not very userfriendly in the default window size. should we change it into several lines?
btw: I found a bug, but this also happens with the head version #1136
self._update_robot_description_list_timer = QTimer(self) | ||
self._update_robot_description_list_timer.setInterval( | ||
int(1000.0 / self._ctrlrs_update_freq) | ||
) |
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.
this may be a little too fast, no? same frequency as the controller :/ should be capped to something fairly low in my opinion... or simply set to something low
@christophfroehlich I moved the reading of joints to the another PR #1146 After that I change the robot_description feature. |
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 agree with @christophfroehlich on this. Having 3 dropdowns is not very user-friendly. Moreover, Now it is up to the user to choose the proper combination of controller_manager
and the robot description. This is very error prone right?
If we need this kind of setup, does it make sense to have the controller_manager
republish the robot_description
it is currently using?. This way applications such as rqt_joint_trajectory_controller
or others can make use of it. What's your opinion on this? @bmagyar @destogl @christophfroehlich
This pull request is in conflict. Could you fix it @delihus? |
@delihus could you please rebase this one? Or just resolve the conflicts |
…cription Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
@bmagyar Updated |
Thank you! |
@delihus We discussed this in the ros2_control meeting tonight, we want to go forward with your proposal. Please address my points above (defaulting to |
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'm happy once @christophfroehlich 's points are addressed
When there are joints what don't have implemented limits rqt-jtc throws a lot of errors.
I added looking for limits for joints what are controlled by actual JTC.
The main feature is that it is possible to change robot description topic to control specific robot. Unfortunately it is not possible to do this using namespace in my case. My namespace issue: ros-controls/ros2_control#1506
Result video:
Screencast from 10.05.2024 11:52:20.webm