Skip to content

Adding Exida recommended msg safety improvements - #196

Open
johnpolymath wants to merge 3 commits into
mainfrom
task/hinke/update-pstop-msg
Open

johnpolymath wants to merge 3 commits into
mainfrom
task/hinke/update-pstop-msg

Conversation

@johnpolymath

Copy link
Copy Markdown
Contributor
  • 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

Migration Guide

- 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

@graphify-labs graphify-labs Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Health regression — 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Health regression — 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.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +75 to 84
/**
* 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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 (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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #197: host/test_aux_channel.c checks inverted_value instead of padding2. #197 also adds pstop_c/** and common/** to the host-check.yml path filters, so a change like this one now runs make -C host test.

@iliabaranov

Copy link
Copy Markdown
Contributor

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 pstop_msg_test.c. Several things outside pstop_c still speak v2 and break, so I put the fixes in a stacked PR on top of this branch instead of pushing here: #197.

Breaks without #197

  • host/test_aux_channel.c no longer compiles (padding2 is gone).
  • tools/pstop_test_remote.py and tools/pstop_multi_remote_test.py hardcode v2 (48 bytes, CRC-16). Against a v3 machine the bond just times out, because the length check drops the packet and nothing is logged.
  • The runner logs the two new errors as ?, and the docs still describe v2.

Checked, no change needed: firmware/, machn/, host/machine_app_runner and ros2/protective_stop_machine take their buffer sizes from PSTOP_MESSAGE_SIZE and don't use padding2. The remote firmware builds from this branch and the new code is in the binary. machn and the ROS 2 package were not built.

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

  • v3 is a hard cutover with no negotiation. A v2 and a v3 endpoint cannot talk and the failure is silent, so remotes, machines and the host runner have to be updated together.
  • The title and body should carry ! / BREAKING CHANGE:, and the Migration Guide section is empty.
  • tools/stop_reset_battery.py was already stale on main and still is. docs/safety/* still describe the 40-byte layout and CRC-16 and need a proper pass against v3.

@iliabaranov
iliabaranov added this pull request to stack #204 October 15, 2026 17:00

@davidt315 davidt315 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is dead code. Nothing calls it anymore so we should remove it before merging this. Strive for no dead code :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing these functions here makes the read_unit16_le/be functions uncalled other than tests

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

iliabaranov added a commit that referenced this pull request Oct 16, 2026
checksum_crc16 has no callers after #196, and read_uint16 / write_uint16
were the only users of the 16-bit endian helpers. Remove checksum_crc16,
its polynomial, read/write_uint16_le/_be and their two tests.

Requested in review on #196.
iliabaranov added a commit that referenced this pull request Oct 16, 2026
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.
iliabaranov added a commit that referenced this pull request Oct 20, 2026
Picks up main (#189, #194, #201, #202) via #196. Resolve common/pstop_aux_channel.h:
keep main's Doxygen @file block with the v3 wording.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This branch has not been deployed

No deployments
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.

3 participants