Skip to content
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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

delihus
Copy link
Contributor

@delihus delihus commented May 10, 2024

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

delihus and others added 4 commits January 11, 2023 14:19
@christophfroehlich
Copy link
Contributor

When there are joints what don't have implemented limits rqt-jtc throws a lot of errors.

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.

@delihus
Copy link
Contributor Author

delihus commented May 15, 2024

@christophfroehlich
I'm wandering why RQT-JTC checks limits for all joints described in URDF if it has an access for joints controlled by JTC via jtc_info.

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.
Here is a video where I cannot select the controller.

Screencast.from.15.05.2024.16.49.21.webm

@christophfroehlich
Copy link
Contributor

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.

@delihus
Copy link
Contributor Author

delihus commented May 15, 2024

Take a look to robotiq_description. The controlled revolute joint ${prefix}robotiq_85_right_knuckle_joint has limits defined but its mimic joints does not. That is why the exceptions throw.

I don't really know if limits should be implemented in continuous mimic joints or not.

The issue with RQT-JTC it that it is not possible to select a controller if the RQT-JTC cannot read the limits of all joints defined in robot_description. I don't really understand why RQT-JTC reads the limits of all joints. I think RQT-JTC should focus on its controlled joints.

@christophfroehlich
Copy link
Contributor

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
This PR is also related to #894 (you fixed the URDF source, now where we subscribe always from topic).

Copy link
Contributor

@christophfroehlich christophfroehlich left a 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 add robot_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?
    image

btw: I found a bug, but this also happens with the head version #1136

Comment on lines 178 to 181
self._update_robot_description_list_timer = QTimer(self)
self._update_robot_description_list_timer.setInterval(
int(1000.0 / self._ctrlrs_update_freq)
)
Copy link
Member

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

@delihus
Copy link
Contributor Author

delihus commented May 23, 2024

@christophfroehlich I moved the reading of joints to the another PR #1146

After that I change the robot_description feature.

Copy link
Member

@saikishor saikishor left a 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

Copy link
Contributor

mergify bot commented Jun 5, 2024

This pull request is in conflict. Could you fix it @delihus?

@bmagyar
Copy link
Member

bmagyar commented Jun 10, 2024

@delihus could you please rebase this one? Or just resolve the conflicts

@delihus
Copy link
Contributor Author

delihus commented Jun 14, 2024

@bmagyar Updated

@bmagyar
Copy link
Member

bmagyar commented Jun 15, 2024

Thank you!

@christophfroehlich
Copy link
Contributor

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

@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 /robot_description etc) and we can go ahead with this 😊

Copy link
Member

@bmagyar bmagyar left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants