-
Notifications
You must be signed in to change notification settings - Fork 365
feat: adds enable_overrun parameter to CM #2540
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: master
Are you sure you want to change the base?
feat: adds enable_overrun parameter to CM #2540
Conversation
@BarisYazici, all pull requests must be targeted towards the |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## jazzy #2540 +/- ##
==========================================
+ Coverage 88.98% 89.02% +0.03%
==========================================
Files 145 145
Lines 16491 16491
Branches 1396 1396
==========================================
+ Hits 14675 14681 +6
+ Misses 1284 1278 -6
Partials 532 532
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This pull request is in conflict. Could you fix it @BarisYazici? |
Introduces a new parameter `enable_overrun` to the controller manager, allowing users to disable the overrun detection and warning mechanism. This parameter defaults to true, maintaining the existing behavior.
7709cba
to
55dffe7
Compare
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 added more refined parameters in #2546
Please take a look. Test it on your hardware and let us know
cm->get_clock()->sleep_until(current_time + period); | ||
} | ||
else | ||
else if(enable_overrun) |
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.
With this we are simply removing the needed sleep too. Is this intended?
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.
Yes, as stated in your PR -> It seems to be 'impossible' (without more complicated hardware setups), to have a synced loop. Aka, relative to my notebook, the master controller of the robot doesn't run with 1000 hz but in the example yesterday with 1003 hz. Hence, the sleep only works against us but not for us...
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.
IMO, I don't think that part should ever be removed. Sleep is the one that ensures the frequency for everyone.
Based on the issue #2529, wanted make it more concrete with the draft PR.
The problem is for Franka robots the read function will be blocking until there is a response from the robot. So the read function will already dictates the control loop frequency. In general for Franka robots, we wouldn't need to even sleep in the ros2_control_node.
Introduces a new parameter
enable_overrun
to the controller manager, allowing users to disable the overrun detection and warning mechanism. This parameter defaults to true, maintaining the existing behavior.Contributions via pull requests are much appreciated. Before sending us a pull request, please ensure that:
To send us a pull request, please:
colcon test
andpre-commit run
(requires you to install pre-commit bypip3 install pre-commit
)