Repository navigation
Adding Exida recommended msg safety improvements - #196
johnpolymath wants to merge 3 commits into
Conversation
- Switching to a CRC-32.
- Adding an inverted value to catch stuck 0/1 bits.
This inverted value is calculated as:
~((message << 24) | (id.data & 0x00FFFFFFU))
- PSTOP version has been changed to 0x03
- Tests have been added to verify:
- pstop version is correct
- inverted value is correct
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Formal verification. PR-changed functions: 0/39 verified (0 proven, 0 may-equivalent, 0 distinguished) · 39 not verified (25 unsupported, 14 over cap).
Not verified on this run: checksum\_crc16 (unsupported), protocol\_handle\_message (unsupported), pstop\_create\_generic\_message (unsupported), pstop\_message\_decode (unsupported), pstop\_message\_encode (unsupported), pstop\_message\_init (unsupported), read\_device\_id (unsupported), write\_device\_id (unsupported), main\_protocol\_test (unsupported), test\_protocol\_bond\_correct\_timestamp (unsupported), and 15 more; 14 changed function(s) beyond the cap and 0 skipped when the time budget ran out.
Graphify review — findings
Bumps the PSTOP wire protocol to version 3, growing messages to 50 bytes and replacing the trailing CRC-16 with a CRC-32 (checksum_crc32). The old second padding word is now an inverted_value field holding the bitwise inverse of the message type and the low 3 bytes of the sender ID; pstop_message_encode fills it in automatically. protocol_handle_message now rejects messages whose version isn't PSTOP_VERSION (PSTOP_INVALID_VERSION) or whose inverted value doesn't match (PSTOP_INVALID_INVERTED_VALUE), so v2 peers can no longer interoperate.
No blocking issues surfaced. 9 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 215 functions depend on the 117 functions this change touches.
Health — this change adds coupling hotspots:
- new:
configure_app_defaults()— 32 callers, 7 callees - new:
machine_handle_message()— 30 callers, 5 callees - new:
send_bond()— 23 callers, 5 callees - new:
send_stop()— 18 callers, 5 callees - new:
send_ok()— 16 callers, 5 callees - new:
pstop_message_encode()— 7 callers, 6 callees - new:
pstop_message_decode()— 7 callers, 5 callees - new:
send_unbond()— 4 callers, 5 callees - …and 49 more — each is listed as a finding
Verification — 215 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 210 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify checksum\_crc16.
The verifier did not have enough to check checksum\_crc16, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: uint16_t
Could not verify: Could not verify protocol\_handle\_message.
The verifier did not have enough to check protocol\_handle\_message, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: pstop_error_t
Could not verify: Could not verify pstop\_create\_generic\_message.
The verifier did not have enough to check pstop\_create\_generic\_message, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify pstop\_message\_decode.
The verifier did not have enough to check pstop\_message\_decode, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify pstop\_message\_encode.
The verifier did not have enough to check pstop\_message\_encode, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify pstop\_message\_init.
The verifier did not have enough to check pstop\_message\_init, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify read\_device\_id.
The verifier did not have enough to check read\_device\_id, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify write\_device\_id.
The verifier did not have enough to check write\_device\_id, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify main\_protocol\_test.
The verifier did not have enough to check main\_protocol\_test, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify test\_protocol\_bond\_correct\_timestamp.
The verifier did not have enough to check test\_protocol\_bond\_correct\_timestamp, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify test\_protocol\_bond\_invalid\_echo\_counter.
The verifier did not have enough to check test\_protocol\_bond\_invalid\_echo\_counter, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify test\_protocol\_bond\_invalid\_timestamp.
The verifier did not have enough to check test\_protocol\_bond\_invalid\_timestamp, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify test\_protocol\_bond\_missed\_message\_timestamps.
The verifier did not have enough to check test\_protocol\_bond\_missed\_message\_timestamps, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify test\_protocol\_bond\_missed\_too\_many\_messages.
The verifier did not have enough to check test\_protocol\_bond\_missed\_too\_many\_messages, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify test\_protocol\_bond\_missing\_sent\_messages.
The verifier did not have enough to check test\_protocol\_bond\_missing\_sent\_messages, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify test\_protocol\_bond\_received\_counter\_invalid.
The verifier did not have enough to check test\_protocol\_bond\_received\_counter\_invalid, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify test\_protocol\_bond\_request.
The verifier did not have enough to check test\_protocol\_bond\_request, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify test\_protocol\_bond\_then\_unbond.
The verifier did not have enough to check test\_protocol\_bond\_then\_unbond, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify test\_protocol\_invalid\_checksum\_req\_2\_01.
The verifier did not have enough to check test\_protocol\_invalid\_checksum\_req\_2\_01, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify test\_protocol\_invalid\_counter.
The verifier did not have enough to check test\_protocol\_invalid\_counter, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify test\_protocol\_invalid\_message\_req\_2\_08.
The verifier did not have enough to check test\_protocol\_invalid\_message\_req\_2\_08, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify test\_protocol\_invalid\_receiver\_id\_req\_2\_07.
The verifier did not have enough to check test\_protocol\_invalid\_receiver\_id\_req\_2\_07, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify test\_protocol\_lost\_messages\_req\_2\_03.
The verifier did not have enough to check test\_protocol\_lost\_messages\_req\_2\_03, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify test\_protocol\_new\_client\_invalid\_received\_counter\_req\_4\_05.
The verifier did not have enough to check test\_protocol\_new\_client\_invalid\_received\_counter\_req\_4\_05, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
Could not verify: Could not verify test\_protocol\_new\_client\_invalid\_received\_counter\_req\_4\_06.
The verifier did not have enough to check test\_protocol\_new\_client\_invalid\_received\_counter\_req\_4\_06, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: unsupported return type: void
· 2 grounded finding(s) anchored inline below; 55 more finding(s) on lines outside this diff (see the check run).
| @@ -65,11 +65,19 @@ check_timestamp(const pstop_application_config_t *app_config, const protocol_dat | |||
| pstop_error_t | |||
There was a problem hiding this comment.
protocol_handle_message()
fans out to 8 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| msg->calculated_checksum = checksum_crc32(data, PSTOP_MESSAGE_SIZE - 4U); | ||
| } | ||
|
|
||
| void |
There was a problem hiding this comment.
pstop_message_encode()
fans out to 6 callees (efferent coupling); 7 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
There was a problem hiding this comment.
Beyond the inline findings, I also checked the other in-tree consumers that call into protocol_handle_message (machn/main/main.c, pstop_c/examples/machine/machine_app.c) for the same kind of stale-format breakage — they only reference checksum/calculated_checksum/receiver_id, fields that still exist, and are compiled directly against the updated pstop_c headers, so they pick up the new wire format automatically and aren't affected the way the hardcoded tools/*.py scripts and host/test_aux_channel.c are.
Extended reasoning...
The change bumps the PSTOP protocol version (0x02 to 0x03), reworks the 48-to-50-byte wire format to use CRC-32 plus a new inverted_value anti-stuck-bit field, and updates protocol_handle_message to reject mismatched versions/inverted values; it touches safety-relevant message validation but not auth or external input parsing beyond the existing protocol. Two confirmed, already-inline findings show real compiled/runtime breakage in tools/pstop_test_remote.py, tools/pstop_multi_remote_test.py (hardcoded old version/format) and host/test_aux_channel.c (references a removed struct field), none of which this diff updates. I additionally verified other in-tree consumers of the message struct (machn, pstop_c examples) are not similarly broken, since they compile against the updated headers and only touch fields that still exist.
Findings marked 🟡 are optional suggestions and need no follow-up push.
|
|
||
| #ifndef PSTOP_VERSION | ||
| # define PSTOP_VERSION 0x02U | ||
| # define PSTOP_VERSION 0x03U |
There was a problem hiding this comment.
🔴 Bumping PSTOP_VERSION to 0x03 and the wire format (50-byte msgs, CRC-32, inverted_value) breaks tools/pstop_test_remote.py and tools/pstop_multi_remote_test.py, which still hardcode PSTOP_VERSION=0x02, SIZE=48, and CRC-16 and are not updated by this diff. Every run of these scripts against a machine built from this change now fails bond() after a 15s timeout ("bond failed") instead of validating the min-STOP-duration arming policy, because protocol_handle_message now also rejects the mismatched version/size via the new PSTOP_INVALID_VERSION/checksum checks. Fix: update tools/pstop_test_remote.py and tools/pstop_multi_remote_test.py to the v3 layout (50 bytes, CRC-32, inverted_value field) alongside the C library change.
Why this was flagged
tools/pstop_test_remote.py:27 hardcodes PSTOP_VERSION=0x02, SIZE=48, FMT with 2 padding fields, and crc16() over 46 bytes (tools/pstop_test_remote.py:36-47,63-78); tools/pstop_multi_remote_test.py:51-121 does the same. After this diff, a real machine running pstop_c rejects any message whose version != PSTOP_VERSION (protocol.c new check) and whose checksum doesn't match CRC-32 over 46 of 50 bytes (pstop_msg.c). These scripts are documented (pstop_test_remote.py:4-12) as the safety-arming-policy test harness against host/machine_app_runner and the ROS2 machine_bridge_node. No safeguard in the diff updates or warns these callers; they silently stop functioning and exit 1, blocking the documented safety validation workflow until manually fixed.
Verification: config.h now defines PSTOP_VERSION 0x03U and PSTOP_MESSAGE_SIZE 50U (was 0x02U/48U), and protocol.c protocol_handle_message() adds a first check if(req->version != PSTOP_VERSION) return PSTOP_INVALID_VERSION;, plus CRC-32 and inverted_value checks. tools/pstop_test_remote.py:45 and tools/pstop_multi_remote_test.py:51 still hardcode PSTOP_VERSION = 0x02, SIZE = 48, and CRC-16, so bond() times out after 15s and exits "bond failed" instead of validating the arming policy.
There was a problem hiding this comment.
Fixed in #197 (stacked on this branch): tools/pstop_test_remote.py and tools/pstop_multi_remote_test.py now speak v3 (50 bytes, CRC-32, inverted_value). Both pass against host/machine_app_runner built from this branch: 51/51 in the multi-remote suite, including new wrong-version and bad-inverted-value cases.
| /** | ||
| * Inverted bits of the message and the bottom 3 bytes of the id: | ||
| * ~((message << 24) | (id & 0xFFFFFF)); | ||
| */ | ||
| uint32_t inverted_value; | ||
|
|
||
| /** | ||
| * extra padding bytes | ||
| */ | ||
| uint32_t padding1; |
There was a problem hiding this comment.
🟡 (optional) Removing pstop_msg_t.padding2 here breaks host/test_aux_channel.c:35, which does CHECK(m.padding2 == 0u); and will now fail to compile with no such member. That test is built by make -C host test, run in .github/workflows/host-check.yml, but that workflow only triggers on paths host/, components/, firmware/main/** -- not pstop_c/**, so this PR's own CI never runs it and the break ships silently. Fix: when removing/renaming a pstop_msg_t field, grep all consumers outside pstop_c (host/, firmware/, machn/, ros2/) for direct field access and update them in the same PR; here update host/test_aux_channel.c to drop the padding2 check.
Why this was flagged
host/test_aux_channel.c includes common/pstop_aux_channel.h which includes pstop/pstop_msg.h from ../pstop_c, so it is compiled directly against the struct defined here. Line 35 of that file reads m.padding2, a field this diff deletes from pstop_msg_t (replaced by inverted_value/padding1 at lines 75-84). make -C host test (host/Makefile's test_aux_channel target) will fail to build with a missing-member error. host-check.yml's path filter (host/, components/, firmware/main/, etc.) does not include pstop_c/, so this PR's CI never exercises that build, and the base branch's CI config gives no protection either -- the breakage is latent until someone else touches host/ or components/.
Verification: The diff removes uint32_t padding2; from pstop_msg_t in pstop_c/pstop/include/pstop/pstop_msg.h. host/test_aux_channel.c:35 reads CHECK(m.padding2 == 0u);, compiled via host/Makefile's test_aux_channel target, a prerequisite of make -C host test. After merge this fails to compile; it builds cleanly on the base branch. host-check.yml's path filters exclude pstop_c/**, so this PR's CI never catches it.
|
I checked what in the repo depends on the v2 wire format and tested this on hardware. The pstop_c change itself looks right: the 70 unit and 32 requirement tests pass, and my CRC-32 matches the golden vector in Breaks without #197
Checked, no change needed: Hardware: a remote built from this branch bonded to the v3 host machine over USB-NCM and ran for 305 s with 1524 frames each way, 0 rejects, no counter gaps, every checksum good, ~3 ms RTT. OK/arm was not exercised (no E-stop loops wired on that unit). For this PR
|
davidt315
left a comment
There was a problem hiding this comment.
remove the now dead checksum_crc16 function. This also makes read_uint16_le/_be and write_uint16_le/_be dead code as well since the only callers were read_uint16 and write_uint16 which this PR deletes. all those dead functions and their tests should be removed.
| static const uint32_t POLY_CRC32 = 0x9D7F97D6U; | ||
|
|
||
| uint16_t | ||
| checksum_crc16(const uint8_t *data, size_t data_length) |
There was a problem hiding this comment.
This is dead code. Nothing calls it anymore so we should remove it before merging this. Strive for no dead code :)
There was a problem hiding this comment.
Agreed, done in #197 (stacked on this branch): checksum_crc16 and its polynomial are removed. pstop_c unit and requirement suites still pass (68 and 32; the two removed tests account for the drop from 70).
|
|
||
| static | ||
| uint16_t | ||
| read_uint16(const uint8_t *data, size_t *pos) |
There was a problem hiding this comment.
removing these functions here makes the read_unit16_le/be functions uncalled other than tests
There was a problem hiding this comment.
Done in #197: read_uint16_le/_be and write_uint16_le/_be are removed from endian.c/endian.h, along with their two tests in endian_test.c. Nothing else referenced them.
make -C host test compiles pstop_c and common/pstop_aux_channel.h, but the workflow did not trigger on those paths, so #196 broke test_aux_channel with a green CI.
Resolve pstop_msg.h: main documented padding2 (#202); this branch removes it.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 2 findings from earlier reviews are still open above.
Still open from earlier reviews (2):
- 🔴
pstop_c/pstop/include/pstop/config.h:8—Bumping PSTOP_VERSION to 0x03 and the wire format (50-byte msgs, CRC-32, inverted_value) breaks tools/pstop_test_remote… - Also unresolved: 1 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
Migration Guide