feat(hardware_interface): implement dynamic command toggling for MockHardware - #3241
feat(hardware_interface): implement dynamic command toggling for MockHardware #3241shlok-mehndiratta wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3241 +/- ##
==========================================
+ Coverage 89.56% 89.59% +0.02%
==========================================
Files 168 168
Lines 21419 21475 +56
Branches 1662 1666 +4
==========================================
+ Hits 19184 19240 +56
+ Misses 1533 1530 -3
- Partials 702 705 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
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. |
There was a problem hiding this comment.
Pull request overview
This PR enhances the mock_components/GenericSystem mock hardware plugin to allow dynamically enabling/disabling command-to-state mirroring at runtime via a ROS 2 service, enabling more realistic controller/state-machine tests (e.g., simulating “stuck” or unresponsive hardware).
Changes:
- Added a
~/set_command_propagation(std_srvs/SetBool) service to toggle command propagation inGenericSystem. - Switched the propagation flag to
std::atomic<bool>for safe toggling from a service callback. - Added a new unit test and extended the
TestableResourceManagerhelper to run with an injected executor so the service can be exercised in-test.
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/mock_components/generic_system.cpp | Adds the SetBool service and makes the propagation toggle atomic. |
| hardware_interface/include/mock_components/generic_system.hpp | Declares the atomic flag and service member; adds required includes. |
| hardware_interface/test/mock_components/test_generic_system.cpp | Adds an executor-driven unit test that toggles propagation via service. |
| hardware_interface/CMakeLists.txt | Adds std_srvs dependency/targets to build/link configuration. |
| hardware_interface/package.xml | Declares runtime/build dependency on std_srvs. |
Comments suppressed due to low confidence (2)
hardware_interface/test/mock_components/test_generic_system.cpp:2790
- Same issue as above: after joining the executor thread, the test continues past FAIL() and will attempt to join the thread again at the end. Return immediately after FAIL once cleanup is complete.
{
executor->cancel();
executor_thread.join();
FAIL() << "Service call timed out";
}
hardware_interface/test/mock_components/test_generic_system.cpp:2774
- Same issue as above: FAIL() does not abort, so the test continues after joining the executor thread and will later join again. Return immediately after FAIL once cleanup is complete.
{
executor->cancel();
executor_thread.join();
FAIL() << "Service call timed out";
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| executor->cancel(); | ||
| executor_thread.join(); | ||
| FAIL() << "Service not available"; | ||
| } |
| #include <cmath> | ||
| #include <string> | ||
| #include <unordered_map> | ||
| #include <vector> | ||
|
|
| ${TinyXML2_LIBRARIES} | ||
| ${pal_statistics_LIBRARIES} | ||
| ${control_msgs_TARGETS} | ||
| ${lifecycle_msgs_TARGETS} | ||
| ${std_srvs_TARGETS} |
christophfroehlich
left a comment
There was a problem hiding this comment.
Thank you, this works as expected!
Please address the comments from copilot if applicable and update the documentation
| if (command_propagation_disabled_) | ||
| if (command_propagation_disabled_.load(std::memory_order_acquire)) | ||
| { | ||
| RCLCPP_WARN(get_logger(), "Command propagation is disabled - no values will be returned!"); |
There was a problem hiding this comment.
This should better be a throttled log.
Closes #3214
While working on some controller tests, I realized that the
GenericSystemmock always mirrors commands to states perfectly. This makes it hard to test how high-level controllers or state machines react when the hardware actually stops responding or gets stuck (e.g., communication loss or a physical state-lock).This PR adds a
~/set_command_propagationservice (usingstd_srvs/SetBool) to theGenericSystemmock. It lets you manually "freeze" the command propagation at runtime:data: false: The mock stops updating its state and stays at its last value, ignoring new commands.data: true: Resumes normal mirroring.Implementation Notes
std::atomic<bool>withmemory_order_acquire/releasefor the toggle. This keeps the real-timeread()loop lock-free and safe from the asynchronous service thread without adding mutex overhead.mock_components/GenericSystemplugin. I haven't tested this on physical hardware since it's a simulation-only feature, and it shouldn't have any side effects on real drivers.TestableResourceManagerhelper to support injecting an executor, which allowed me to verify the service logic in a standard unit test.I've verified this both with a new test case (
toggle_command_propagation_service) and manually withros2 service call. All 29 existing tests pass.Hope this helps anyone else trying to simulate hardware gremlins in their tests!
Verification Screenshot
(I'll attach a screenshot here showing the join states freezing after the service call)