Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Check dt in updateFromVelocity #1481

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

tonynajjar
Copy link
Contributor

updateFromVelocity is not only called from update but also directly if position_feedback is false so we should also check there is dt is too small

Signed-off-by: Tony Najjar <tony.najjar.1997@gmail.com>
@christophfroehlich
Copy link
Contributor

christophfroehlich commented Jan 9, 2025

I'm fine with this in general, but in case dt=0 nothing bad happens anyways?
we should also rewrite the API here, see #1394 for a first draft

@tonynajjar
Copy link
Contributor Author

but in case dt=0 nothing bad happens anyways?

if it's 0, then you get a division by 0 further down

@tonynajjar
Copy link
Contributor Author

There is more to be debugged as discussed but this PR can already be merged I would say. It already fixes a major bug for me because if one dt=0 goes through, the accumulators always return nan afterwards

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.

Sorry, I got confused because the arguments left/right_vel are no velocities but position delta values. If it would be velocities we would not need to divide by dt in this method, and integrating with a step size of zero would just do nothing.

We really should rewrite this API, but I understand your point and we can merge this as a temporary fix.

@christophfroehlich christophfroehlich added the backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. label Jan 9, 2025
@christophfroehlich christophfroehlich merged commit 0736e6c into ros-controls:master Jan 10, 2025
24 checks passed
mergify bot pushed a commit that referenced this pull request Jan 10, 2025
Signed-off-by: Tony Najjar <tony.najjar.1997@gmail.com>
(cherry picked from commit 0736e6c)
christophfroehlich pushed a commit that referenced this pull request Jan 11, 2025
Co-authored-by: Tony Najjar <tony.najjar.1997@gmail.com>
mamueluth pushed a commit to StoglRobotics-forks/ros2_controllers that referenced this pull request Mar 14, 2025
Signed-off-by: Tony Najjar <tony.najjar.1997@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants