Skip to content

refactor(dc_measurements,dc_util): unify parameter declaration on dc_util helpers - #317

Merged
Minipada merged 1 commit into
jazzyfrom
feature/178-unify-parameter-declaration-across-measu
Aug 11, 2026
Merged

refactor(dc_measurements,dc_util): unify parameter declaration on dc_util helpers#317
Minipada merged 1 commit into
jazzyfrom
feature/178-unify-parameter-declaration-across-measu

Conversation

@Minipada

Copy link
Copy Markdown
Owner

Summary

  • Unifies parameter declaration across dc_measurements/dc_group onto dc_util::get_*_type_param()/get_*_param(): all 34 Measurement/Condition plugin .cpp files and measurement_server.cpp's own 5 raw declare_parameter + 3 direct nav2_util::declare_parameter_if_not_declared call sites now go through dc_util, instead of ~90 call sites split across three inconsistent patterns.
  • dc_util/include/dc_util/node_utils.hpp rewritten: the 14 near-identical helper bodies collapse onto one shared detail::declare_and_get<T>() core, extended with the type coverage plugins actually needed (double, mandatory vector<bool>/vector<int64_t>/vector<double>) and a node-level (unprefixed) variant. The broken, unused get_float_type_param (declared PARAMETER_DOUBLE, returned int, zero call sites) was deleted.
  • Decision recorded in docs/adr/0008-dc-util-owns-parameter-declaration.md: nav2_util stays a dependency (MeasurementServer already inherits nav2_util::LifecycleNode), dc_util becomes the single sanctioned wrapper around it, and dc_group's existing plain Python declare_parameter calls are left as-is (no equivalent idempotency problem exists there).
  • Convention documented for plugin authors in doc/src/dc/contributing.md.
  • Two pre-existing, unrelated bugs surfaced while cross-checking every declared-vs-read parameter name/type and were deliberately left untouched (flagged inline, not fixed, per "no behaviour change"): bool_equal.cpp's value_ field is double despite a PARAMETER_BOOL declaration; distance_traveled.cpp declares transform_tolerance but reads back the different, undeclared transform_timeout.

Closes #178

Test plan

  • colcon build --packages-select dc_util dc_core dc_measurements dc_interfaces dc_common dc_group — clean, 6/6 packages, zero errors/warnings (real ROS 2 Jazzy Podman environment)
  • colcon test --packages-select dc_util dc_measurements dc_group — 46 tests, 0 errors, 0 failures, 0 skipped
  • pre-commit run on all changed files — clean (clang-format, black, codespell, XML/package metadata checks); only the pre-existing, environment-only poetry-requirements failure reproduces, unrelated to this diff

🤖 Generated with Claude Code

https://claude.ai/code/session_01L56mFyxLcigqQxzZ7BzNgs

…util helpers

Every Measurement/Condition plugin declared its own parameters by calling
nav2_util::declare_parameter_if_not_declared + get_parameter directly,
duplicating the same declare/get/fatal-on-failure boilerplate that
dc_util::get_*_type_param() already provided but only ~20 of ~90 call
sites actually used; measurement_server.cpp additionally had 5 raw
declare_parameter calls (throw-on-redeclare, not idempotent) for its own
node-level parameters.

Collapse dc_util/node_utils.hpp's 14 near-identical helpers onto one
shared core, extend it with the missing double/array-of-scalar types and
a node-level (unprefixed) variant, and route every plugin and
measurement_server.cpp through it. nav2_util stays a dependency
(MeasurementServer already inherits nav2_util::LifecycleNode) but is now
only referenced from dc_util itself -- recorded in ADR-0008, along with
why dc_group's Python side keeps its existing plain declare_parameter
calls. Two pre-existing, unrelated bugs (bool_equal.cpp's double-typed
field vs. its PARAMETER_BOOL declaration; distance_traveled.cpp reading
back a different parameter name than it declares) were left untouched
and flagged inline rather than silently fixed.

Verified with a real colcon build + colcon test in a ROS 2 Jazzy
environment: 6/6 packages build clean, 46/46 tests pass.

Closes #178

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L56mFyxLcigqQxzZ7BzNgs
Signed-off-by: David Bensoussan <d.bensoussan@proton.me>
@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 36.64596% with 102 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.82%. Comparing base (2b7cc67) to head (14be548).

Files with missing lines Patch % Lines
dc_measurements/plugins/measurements/camera.cpp 0.00% 15 Missing ⚠️
dc_util/include/dc_util/node_utils.hpp 62.86% 13 Missing ⚠️
dc_measurements/plugins/measurements/ip_camera.cpp 0.00% 10 Missing ⚠️
dc_measurements/src/measurement_server.cpp 62.50% 6 Missing ⚠️
dc_measurements/plugins/measurements/map.cpp 0.00% 5 Missing ⚠️
...easurements/plugins/conditions/double_inferior.cpp 0.00% 3 Missing ⚠️
...easurements/plugins/conditions/double_superior.cpp 0.00% 3 Missing ⚠️
...asurements/plugins/conditions/integer_inferior.cpp 0.00% 3 Missing ⚠️
...asurements/plugins/conditions/integer_superior.cpp 0.00% 3 Missing ⚠️
...easurements/plugins/conditions/list_bool_equal.cpp 0.00% 3 Missing ⚠️
... and 19 more
Additional details and impacted files
@@            Coverage Diff             @@
##            jazzy     #317      +/-   ##
==========================================
+ Coverage   40.48%   40.82%   +0.34%     
==========================================
  Files          82       82              
  Lines        5094     4920     -174     
==========================================
- Hits         2062     2008      -54     
+ Misses       3032     2912     -120     
Flag Coverage Δ
cpp-jazzy 40.82% <36.65%> (+0.34%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Minipada
Minipada merged commit 0f0088e into jazzy Aug 11, 2026
8 checks passed
@Minipada
Minipada deleted the feature/178-unify-parameter-declaration-across-measu branch August 16, 2026 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant