Enable dynamic server parameter change#93
Conversation
|
@Nicodaube thanks for the PR, i will take a look when i have time! |
There was a problem hiding this comment.
Pull request overview
This PR enables the parameter server to react at runtime to changes in configuration parameters (notably storing_period and must_save_on_update) by registering parameter-change callbacks, and adds a new test client to exercise dynamic reconfiguration behavior (Issue #58).
Changes:
- Added
ParameterEventHandlercallbacks in the server to apply updated values formust_save_on_update,storing_period, andallow_dynamic_typing. - Added a new test client (
client_dynamic_param_change) and integrated it into the existing test runner script. - Extended the test client utilities to get/set server parameters via ROS parameter services.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
server/src/parameter_server.cpp |
Adds parameter-change callbacks to apply runtime changes (timer recreation, save-on-update flag, etc.). |
server/include/parameter_server.h |
Adds new callback/handler members and stores configuration state. |
test/include/persist_parameter_client.hpp |
Adds get/set-parameter service helpers used by the new dynamic-param test. |
test/src/persist_parameter_client.cpp |
Instantiates new get/set-parameter service clients. |
test/include/test_common.h |
Adds helper methods to change/read server parameters and assert values in tests. |
test/src/test_dynamic_param_change.cpp |
New test program covering dynamic changes (save-on-update, storing timer, dynamic typing). |
test/test.py |
Runs the new dynamic-param-change test stage and includes its return code in the final success condition. |
CMakeLists.txt |
Builds and installs the new client_dynamic_param_change executable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@Nicodaube thanks for fixing this up, sorry for being late to review this. but this is on my list, i will do that when i can allocate time for. |
No problem at all, I took time to correct the copilot remarks too |
|
Sorry for not having done the changes requested, I was in a bit of a rush at work 😆 . I will try and do that this week ! |
|
@Nicodaube no worries, thanks for replying! |
|
@Nicodaube thanks for the fixes. unfortunately this still needs a few fixes before merge. |
|
I corrected the errors you pointed, sorry for the messy PR, I still got things to learn on those parameter callback 😛 |
Solves issue #58 i think