Skip to content

Conversation

@saikishor
Copy link

@saikishor saikishor commented Feb 16, 2025

Needs ros/urdfdom_headers#83

This is needed to be able to parse the newly added limits of acceleration, deceleration and jerk, this would be pretty much useful to integrate them into the ros2_control architecture

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/allow-for-more-complex-joints-in-urdf/42234/3

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/proposal-to-extend-jointlimits-in-urdf/42831/1

const char* deceleration_str = config->Attribute("deceleration");
if (deceleration_str == NULL){
CONSOLE_BRIDGE_logDebug("urdfdom.joint_limit: no deceleration, using default value");
jl.deceleration = std::numeric_limits<double>::infinity();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a full review, but an isolated comment. If the deceleration is unspecified, wouldn't it make more sense to default to the acceleration value?

Unrelated nit: Bracing and spacing style is inconsistent in this changeset.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. That's a very good point. I'll do the changes soon

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@saikishor saikishor force-pushed the extend/joint/limits branch from 998b548 to 22d0e79 Compare March 27, 2025 08:37
@saikishor saikishor requested a review from adolfo-rt March 27, 2025 08:38
Copy link

@forrest-rm forrest-rm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is someone working on enforcing these in ros2_control? Otherwise these new limits will just confuse folks.

(@bmagyar)

@saikishor
Copy link
Author

saikishor commented Apr 18, 2025

Is someone working on enforcing these in ros2_control? Otherwise these new limits will just confuse folks.

@forrest-rm We already have the first version of the limit enforcement already integrated into the ros2_control (ros-controls/ros2_control#2047). Once this is merged, we will add support for considering the acceleration, deceleration and jerk limits too

Copy link

@forrest-rm forrest-rm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ready to merge once the ros2_control changes are also good to go.

@scpeters
Copy link
Contributor

can you retarget this to rolling? Also, I think we should add parsing logic based on the URDFVersion similar to #235 so that the new attributes are only parsed if the file version is new enough (1.1 or higher), and warnings are printed if the new attributes are present in an old (1.0) file

@saikishor saikishor changed the base branch from master to rolling January 27, 2026 21:47
@saikishor
Copy link
Author

can you retarget this to rolling? Also, I think we should add parsing logic based on the URDFVersion similar to #235 so that the new attributes are only parsed if the file version is new enough (1.1 or higher), and warnings are printed if the new attributes are present in an old (1.0) file

@scpeters done!

saikishor and others added 3 commits January 28, 2026 10:19
Co-authored-by: Jasper van Brakel <[email protected]>
Signed-off-by: Sai Kishor Kothakota <[email protected]>
Signed-off-by: Sai Kishor Kothakota <[email protected]>
Signed-off-by: Sai Kishor Kothakota <[email protected]>
@saikishor saikishor force-pushed the extend/joint/limits branch from 081cf71 to ff9b5e3 Compare January 28, 2026 09:19
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.

6 participants