rasprover: add ROS 2 CDR golden-bytes tests, fix BatteryState health enum - #73
Merged
Conversation
app_ros_cdr.c hand-rolls the CDR byte layout for sensor_msgs/BatteryState and sensor_msgs/JointState. A wrong pad byte does not fail the build, it makes a ROS 2 subscriber silently misparse, so the layout needs pinning. Ten tests over golden byte arrays derived by hand from the message IDL plus the CDR alignment rule (working shown in comments), not from running the code under test. They cover the encapsulation header, every alignment transition that padding depends on (float32 after a string, uint32 after a run of uint8, float64 after a uint32), the NaN "unmeasured" sentinels, the buffer-too-small contract, and an encode/decode round trip. Platform is qemu_cortex_m3. The code under test is pure and would suit `unit_testing`, but ztest registers cases through ELF iterable sections and Mach-O rejects the section attribute, so unit_testing does not build on macOS; native_sim is Linux-only. qemu_cortex_m3 ships with the Zephyr SDK and runs everywhere. native_sim stays in platform_allow for Linux CI. test_battery_state_power_supply_enums documents a defect rather than hiding it: the encoder writes 2 for power_supply_health and comments it POWER_SUPPLY_HEALTH_GOOD, but in sensor_msgs/BatteryState GOOD is 1 and 2 is OVERHEAT. The test asserts current behaviour so the suite stays green, with a BUG note above it. No behaviour change here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
app_ros_encode_battery_state() wrote 2 for power_supply_health under a /* POWER_SUPPLY_HEALTH_GOOD */ comment. In sensor_msgs/BatteryState the constants are POWER_SUPPLY_HEALTH_UNKNOWN = 0, GOOD = 1, OVERHEAT = 2 (mirroring the Linux power_supply enum), so every battery message the rover published claimed the pack was overheating. Nothing in firmware reads the value back, so this was only ever visible to a ROS 2 subscriber. Caught by the golden-bytes suite added in the previous commit, while deriving the expected wire bytes from the message IDL. The neighbouring status and technology writes now spell out their numeric values too. A bare 2 sitting next to a constant name is what let this through; naming the number makes the comment self-checking. Message length is unchanged at 73 bytes, and app_zenoh.c is the only caller. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two commits. The second one is a real bug fix and is the reason this PR matters.
790c59arasprover: add Ztest suite for ROS 2 CDR encoding5061147rasprover: fix BatteryState health enum (was OVERHEAT, not GOOD)The bug
app_ros_encode_battery_state()wrote2forpower_supply_healthunder a/* POWER_SUPPLY_HEALTH_GOOD */comment. Insensor_msgs/BatteryStatethe constants arePOWER_SUPPLY_HEALTH_UNKNOWN = 0,GOOD = 1,OVERHEAT = 2(mirroring the Linuxpower_supplyenum).Every battery message the rover published claimed the pack was overheating. Nothing in firmware reads the value back, so it was only ever visible to a ROS 2 subscriber — exactly the class of defect that is invisible from the firmware side and that golden-bytes tests exist to catch. It was found while deriving expected wire bytes from the message IDL, not by running the code.
power_supply_status = 2(DISCHARGING) andpower_supply_technology = 0(UNKNOWN) are both correct. The neighbouring writes now spell out their numeric values too — a bare2sitting next to a constant name is what let this through.app_zenoh.c:298is the only caller and just publishes the buffer. Message length unchanged at 73 bytes.The suite
10 tests over
app_ros_cdr.c, which is pure (writes into a caller-supplied buffer, no kernel/net/fs) and was previously uncovered for BatteryState.frame_id, the absence of a pad for the u32 after four u8 flags (rel 48 already aligned), the 4-byte pad for the f64 after the velocity count. Alignment measured from the post-encapsulation origin, as FastCDR expects.Platform:
qemu_cortex_m3unit_testingwould be the natural choice for pure code, but it does not build on macOS: ztest registers suites viaSTRUCT_SECTION_ITERABLE→__attribute__((section(...))), which Mach-O rejects (mach-o section specifier requires a segment and section separated by a comma).native_simis Linux-only.qemu_cortex_m3ships with the Zephyr SDK and runs on any host;native_simstays inplatform_allowfor Linux CI.Verification
mise run agent-build rasprover --sysbuild— succeeds.tests/rasprover/test_ros_cdr.py—3 passed, unaffected (JointState only; it never covered BatteryState, which is why this gap existed).Deliberately out of scope
tests/rasprover/test_ros_cdr.pyleft in place despite some JointState overlap — consolidating test infrastructure is not this PR's job.west twister -T applicationscurrently fails onmotor_controller(error: 'usb_dc_detach' is deprecated [-Werror=deprecated-declarations], 4 configs), so such a task would be red on arrival. That failure is pre-existing and worth its own look.🤖 Generated with Claude Code