Repository navigation
feat(nav_msgs): add COLLISION_APPROACH_ACTIVE reason (#34) - #23
Conversation
Signed-off-by: Kunal Yadav <kunal780954@gmail.com>
|
Software review: current head is a focused additive constant-only change, the compatibility snapshot matches it, and both hosted ROS 2 Jazzy and repository-quality runs are green. No blocking software issue found from my side. Keep the producer-side use as a separate follow-up and do not treat this constant alone as runtime validation. |
panthera-momagdii
left a comment
There was a problem hiding this comment.
Approved for the additive reason constant and matching compatibility snapshot.
Review preconditions: openAMRobot/openamrobot-interfaces; branch feat/collision-approach-reason; reviewed head 5a16c9897b9c8d24862bdb57321fa2a316a7feae; base main at 77950470a87f1f5815346b2f000a662ee8c56f94. Expected outcome: formal interface review only, with no merge or source changes.
The two-file diff adds COLLISION_APPROACH_ACTIVE=6008 to NavigationStatus and its generated compatibility snapshot. Existing constants and message fields are unchanged. Keeping CONTRACT_VERSION at 1 is consistent with CONTRACT.md's rule for adding reason constants; this is not the message-field extension discussed in platform-sw #36.
I inspected the actual hosted Repository quality build log, run 37711776383, job 113099092844. It builds both interface packages, exposes 6008 in the generated interface, validates 13 interfaces, passes 15 checker tests and 14 compatibility tests, and reports no incompatible changes against the base snapshot. The installed C++ consumer and retained-message consumers pass, and the deliberate removed-field fixture is rejected as CONTRACT_MISMATCH. The log does contain publisher shutdown tracebacks during exchange-test cleanup; the successful consumer assertions are not a claim that teardown was error-free. That harness code is unchanged by this PR.
No further technical changes requested for this constant-only scope. The producer and consumer behaviour that uses the new reason must be reviewed and tested separately; adding the constant does not implement constraints reporting, demonstrate collision avoidance, or close #34/#35.
Not verified: I did not rerun these jobs locally, validate the future producer mapping in simulation, or test hardware. Contributor provenance remains the author's responsibility. No merge, source or hardware changes were made.
AI assistance: ChatGPT assisted this source/diff and hosted-evidence review at the software lead's request.
Pull request
Adds one additive reason constant, COLLISION_APPROACH_ACTIVE=6008, to NavigationStatus.msg — no field or shape change, so CONTRACT_VERSION stays unchanged.
Scope: ros2/openamr_nav_msgs/msg/NavigationStatus.msg (one new constant) and compatibility/jazzy.json (regenerated snapshot). Nothing else touched.
Why: Feeds a follow-up producer change in openamr-platform-sw for #34's constraints/speed-limit reporting. ActiveConstraint.TYPE_COLLISION_APPROACH already exists in ActiveConstraint.msg; this gives the producer a reason code to pair with it when collision_monitor's existing /collision_monitor_state reports its approach action on the configured polygon.
Validation: Built both openamr_nav_msgs and openamr_ui_msgs with a real colcon build (colcon build --symlink-install --packages-select openamr_nav_msgs openamr_ui_msgs), regenerated the snapshot with tools/check_compatibility.py --source ros2 --output compatibility/jazzy.json, and reviewed the diff — it adds exactly one new entry (msg/COLLISION_APPROACH_ACTIVE, uint16, 6008) and changes nothing else.
Compatibility: Additive only. Existing consumers are unaffected — nothing currently reads this constant, same as #22's MOTION_SOURCE_PARTIALLY_PROTECTED. No contract-version bump needed.
Migration: None required.
Safety impact: None. This is a status-reporting constant with no effect on motion, control, or any safety-relevant behavior.
Intellectual property and provenance