Add missing controller async parameter declarations - #3481
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3481 +/- ##
==========================================
+ Coverage 89.51% 89.54% +0.02%
==========================================
Files 164 164
Lines 21270 21275 +5
Branches 1656 1659 +3
==========================================
+ Hits 19040 19050 +10
+ Misses 1530 1527 -3
+ Partials 700 698 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
|
||
| auto_declare<int>("async_parameters.thread_priority", 50); | ||
| auto_declare<std::string>("async_parameters.scheduling_policy", "synchronized"); | ||
| auto_declare<std::vector<int64_t>>("async_parameters.cpu_affinity", std::vector<int64_t>()); | ||
| auto_declare<int>("async_parameters.execution_rate", impl_->ctrl_itf_params_.update_rate); | ||
| auto_declare<bool>("async_parameters.wait_until_initial_trigger", true); | ||
| auto_declare<bool>("async_parameters.print_warnings", true); |
There was a problem hiding this comment.
Maybe, it is wiser to add declare_parameters method to the asyncParams and then handle it in the realtime_tools, this way any new parameter need not be reflected again and again here
There was a problem hiding this comment.
yeah, that is a good catch. Added a commit for this
|
As per this comment, moved parameter declaraion to To sum up, this is the merging order:
Nice to have, on this topic:Printing CPU cores we pin threads on PR: ros-controls/realtime_tools#551 |
| realtime_tools::AsyncFunctionHandlerParams::declare( | ||
| impl_->node_, "async_parameters.", impl_->ctrl_itf_params_.update_rate); |
There was a problem hiding this comment.
Why not use async_params_.declare?
There was a problem hiding this comment.
I figured the parameters are a property of the node that declares them, and this should only be a utility function. Also for loading parameters from yaml.
So it goes something like this:
- in
init, node is just being created, so we declare the parameters. There is noasync_params_member yet. - after init, parameters are loaded from yaml file
- then in
configure(), the parameters are read and stored inasync_params_.
that is why declare() is a static function, and not tied to the async_params_ instance. The instance does not even exist in init, so we might miss param setting from yaml.
Description
Setting
async_parameterskey in controller config does not applycpu_affinityorthread_priorityconfig, as per the documentationThis config:
Results in these logs:
When this is expected:
Solution:
Declare the parameters in question automatically, here
Not sure why this is not declared automatically. Probably at some point rclcpp does not automatically declare parameters set for nodes, so these configs get ignored.
And no one seemed to be pinning async controllers to any of the cores 😄
Is this user-facing behavior change?
yep, now when you put the parameters described in the docs, they actually get applied.
Did you use Generative AI?
Nah