Remove hardcoded default update rate - #3273
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the implicit 100 Hz fallback for ResourceManager update rates so hardware initialization now requires an explicit non-zero update_rate. It aligns ResourceManagerParams and constructor defaults with the new sentinel-based validation intended to prevent simulators and custom ResourceManager setups from silently running at the wrong frequency.
Changes:
- Changed
ResourceManagerandResourceManagerParamsdefault update rates from100to0. - Added a validation in
load_and_initialize_components()that logs an error and returnsfalsewhenupdate_rateis unset/zero. - Updated two tests that constructed
ResourceManagerParamsdirectly to pass an explicitupdate_rate.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
hardware_interface/src/resource_manager.cpp |
Switches internal default rate to 0 and rejects zero-rate initialization. |
hardware_interface/include/hardware_interface/types/resource_manager_params.hpp |
Changes ResourceManagerParams::update_rate default and documents the new requirement. |
hardware_interface/include/hardware_interface/resource_manager.hpp |
Updates public constructor/helper defaults to use the new zero sentinel. |
hardware_interface_testing/test/test_resource_manager.cpp |
Fixes a test setup to provide an explicit update rate. |
hardware_interface_testing/test/test_resource_manager_prepare_perform_switch.cpp |
Fixes another test fixture to provide an explicit update rate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (params.update_rate == 0) | ||
| { | ||
| RCLCPP_ERROR( | ||
| params.logger, "update_rate is not set or is zero - cannot initialize hardware components."); | ||
| components_are_loaded_and_initialized_ = false; | ||
| return false; |
| rclcpp::node_interfaces::NodeClockInterface::SharedPtr clock_interface, | ||
| rclcpp::node_interfaces::NodeLoggingInterface::SharedPtr logger_interface, | ||
| bool activate_all = false, const unsigned int update_rate = 100); | ||
| bool activate_all = false, const unsigned int update_rate = 0); |
| explicit ResourceManager( | ||
| const std::string & urdf, rclcpp::Clock::SharedPtr clock, rclcpp::Logger logger, | ||
| bool activate_all = false, const unsigned int update_rate = 100); | ||
| bool activate_all = false, const unsigned int update_rate = 0); |
christophfroehlich
left a comment
christophfroehlich
left a comment
There was a problem hiding this comment.
Please fix the failing pre-commit tests (better also install pre-commit install for future commits)
|
@naitikpahwa18 you'll also have to either merge & fix conflicts or rebase the PR onto the latest |
e294179 to
fd3e949
Compare
|
I'm currently looking into the CI failures, but I'm not yet sure whether they're related to changes in this PR or to unrelated CI issues. |
Most of the jobs are green on the master branch (except of rolling-main ones). I suggest to compare the list of failing jobs with other PRs, for example the latest merged one https://github.com/ros-controls/ros2_control/commits/master/ |
|
@naitikpahwa18 all tests are failing because now they need the old default set manually. |
|
@bmagyar Thanks for pointing that out. Should I update the tests to explicitly set the update rate where they currently rely on the old default, or is there a preferred approach you’d like me to follow? |
You should of course update the tests if they rely on former default. |
|
This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete. |
Description
The update_rate in ResourceManagerParams and cm_update_rate_ in ResourceStorage were hardcoded to 100 Hz as defaults. This caused simulators that override load_and_initialize_components or construct ResourceManager without explicitly setting the rate to silently run at the wrong frequency with no error. This PR changes the default to 0 as a sentinel value and adds an explicit validation in load_and_initialize_components that logs an error and returns false if update_rate is not set. Two tests that constructed ResourceManagerParams without setting update_rate are also fixed to explicitly pass 100.
Fixes #1575