Document panda_simulated_config.yaml's loading contract (ParameterBuilder namespace + update_period) - #3834
Conversation
The file reads like a ROS 2 parameter file but is not one: it has no node
name / ros__parameters wrapper, and the demo launch files load it through
ParameterBuilder("moveit_servo"), which nests every key under the
moveit_servo namespace that servo's ParamListener expects
(servo_node.cpp). Passing it directly via parameters=[...] fails at
startup with an unhelpful error.
The file also enables online_signal_smoothing::AccelerationLimitedPlugin,
whose required update_period parameter cannot be added here: the plugin's
ParamListener is constructed without a prefix (acceleration_filter.cpp)
and reads from the node's root namespace, while every key in this file
lands under moveit_servo. - which is why all four demo launch files supply
{"update_period": 0.01} as a separate node-level parameter.
Document both constraints in a header comment so users starting from this
config find the contract in the file itself instead of reverse-engineering
it from the demos.
Closes moveit#3830
Signed-off-by: Lang Li <langlipro@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe YAML configuration now documents that callers must load it through ChangesServo configuration documentation
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This is a localized documentation-only change with no runtime or API behavior changes, and no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The PR documents both requirements from issue Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
1 participant
Description
Closes #3830.
panda_simulated_config.yamlreads like a ROS 2 parameter file but cannot be loaded as one, and the smoothing plugin it enables requires a parameter that lives only in the demo launch files. This PR documents both constraints in a header comment, so a user starting from this config finds the contract in the file itself instead of reverse-engineering it from the demos.#3830 offered maintainers two options: (a) make the file self-contained, or (b) document the loading contract. This implements (b), because while preparing (a) I found it is not actually achievable without breaking changes — which itself seems worth recording:
/**: ros__parameters:wrapper would break every existing consumer. The demo launch files (and downstream users following them) load this file throughParameterBuilder("moveit_servo").yaml(...), which reads the whole document and nests it under themoveit_servonamespace expected by servo's listener (servo_node.cppL105-106). With a wrapper, those keys would land undermoveit_servo./**.ros__parameters.*— garbage.update_periodcannot be moved into this file at all.AccelerationLimitedPluginconstructs its listener without a prefix (acceleration_filter.cppL181) and therefore readsupdate_periodfrom the node's root namespace, while every key in this yaml lands undermoveit_servo.. That is why all four demo launch files (demo_twist,demo_pose,demo_joint_jog,demo_ros_api) each pass{"update_period": 0.01}as a separate node-level parameter.Comment-only change: no behavior, API, or formatting impact. Happy to reword or trim if maintainers prefer a shorter note.
Checklist
Summary by CodeRabbit
update_periodsetting for acceleration-limited operation.