Adding better support for missed heartbeat messages #144
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,17 @@ | |
|
|
||
| #include "pstop/protocol.h" | ||
|
|
||
| static | ||
| uint64_t | ||
| get_diff(uint64_t lhs, uint64_t rhs) | ||
| { | ||
| if(lhs <= rhs) { | ||
| return rhs - lhs; | ||
| } | ||
|
|
||
| return lhs - rhs; | ||
| } | ||
|
|
||
| static | ||
| int | ||
| is_checksum_valid(const pstop_msg_t *req) | ||
|
|
@@ -35,28 +46,40 @@ check_counter(const pstop_application_config_t *app_config, const pstop_remote_d | |
|
|
||
| static | ||
| pstop_error_t | ||
| check_timestamp(const pstop_application_t *app, const protocol_data_t *client, const pstop_msg_t *req) | ||
| { | ||
| if(req->stamp <= client->last_received_stamp) { | ||
| return PSTOP_MSG_OUT_OF_ORDER; | ||
| } | ||
|
|
||
| // did the other end miss a message? | ||
| // example of missed messages | ||
| // last sent stamp = 500, last received stamp = 500 | ||
| // now = 600, current request stamp = 900 | ||
| // diff = 100, remote diff = 400 | ||
| // diff should be close to the same if no messages have been lost | ||
|
|
||
| uint64_t diff_received_stamp = get_diff(req->stamp, client->last_received_stamp); | ||
|
|
||
| uint64_t now = app->env.get_time_cb(); | ||
|
|
||
| uint64_t diff_sent_stamp = get_diff(now, client->last_timestamp); | ||
|
|
||
| uint64_t diff_remote_vs_local = get_diff(diff_sent_stamp, diff_received_stamp); | ||
|
|
||
| if(diff_sent_stamp == 0U) { | ||
| // the chances of this happening are extremely unlikely. | ||
| // It would mean that the amount of time to run through processing | ||
| // a single message takes 0ms. | ||
|
|
||
| // If this does happen then the received timestamps should also be | ||
| // close to 0ms. Setting diff_sent_stamp = 1 will mean that the | ||
| // diff_remote_vs_local is the max number of missed hearbeats. | ||
| diff_sent_stamp = 1U; | ||
| } | ||
| uint64_t missed = diff_remote_vs_local / diff_sent_stamp; | ||
| if(missed > (uint64_t)(app->app_config.max_missed_heartbeats + 1U)) { | ||
| return PSTOP_MSG_LOST; | ||
|
Comment on lines
+49
to
+82
There was a problem hiding this comment. 🔴 This PR edits pstop_c protocol/timestamp logic directly in this repo, which CLAUDE.md designates off-limits: pstop_c is pre-qualified, "never edited here, never re-verified"; protocol and machine-safety changes must go upstream and return as a version bump. check_timestamp's signature and missed-heartbeat arithmetic (protocol.c:49-75) and pstop_application_init's default limits (pstop_application.c:47) are exactly the counter/stamp/heartbeat-timeout logic the manual reserves for the upstream track, and per CLAUDE.md this is also automatic Class C (touches timing/heartbeat-timeout on the stop path) requiring two-person sign-off before implementation, not present here. … Extended reasoning...…Fix: revert this change from pstop_c/ in this repo and land it upstream as a version bump with Class C review, per CLAUDE.md. CLAUDE.md states 'pstop_c/ is off-limits. Pre-qualified, separate upstream track. Referenced at the interface, never edited here, never re-verified. Protocol and machine-safety changes go upstream and return as a version bump.' This diff modifies pstop_c/pstop/src/pstop/protocol.c (check_timestamp signature and body) and pstop_c/pstop/src/pstop/pstop_application.c (default protocol limits), both inside pstop_c/. Any change touching counter/stamp/heartbeat-timeout logic is automatic Class C requiring two-person authorization before implementation; nothing in the diff or PR description shows that sign-off. Landing this as-is bypasses the pre-qualification boundary that keeps the safety case for the missed-heartbeat diagnostic valid; any downstream user pulling this repo now gets an unvetted, locally-modified version of pre-qualified safety code instead of the upstream-verified one. Verification: normal — the diff directly edits pstop_c/, which CLAUDE.md §1.2 declares off-limits ("Pre-qualified, separate upstream track. Referenced at the interface, never edited here, never re-verified. Protocol and machine-safety changes go upstream and return as a version bump"). The violation is not hypothetical; it is the changeset itself. Confirmed in the diff: pstop_c/pstop/src/pstop/protocol.c… |
||
| } | ||
|
|
||
| return PSTOP_OK; | ||
|
Comment on lines
+49
to
85
There was a problem hiding this comment. 🔴 check_timestamp() no longer validates req->received_stamp (the remote's echo of our last sent stamp) after bonding, unlike check_counter() which still checks req->received_counter against msg_counter. A corrupted or forged received_stamp on the untrusted black channel is no longer rejected as PSTOP_MSG_OUT_OF_ORDER as long as req->stamp keeps advancing and the new heartbeat ratio stays under threshold. Fix: keep validating that req->received_stamp bounds/echoes client->last_timestamp (mirroring the received_counter check in check_counter), in addition to the new missed-heartbeat ratio math, so black-channel corruption of the echoed stamp is still caught for every message, not only at bond time (line 125). Extended reasoning...Base check_timestamp used req->received_stamp: Verification: nit. The candidate reads the code correctly and the scenario is reachable. Base check_timestamp (git show 9d3b0e4:protocol.c) rejected |
||
|
|
@@ -92,7 +115,7 @@ protocol_handle_message(pstop_machine_t *machine, const pstop_msg_t *req, pstop_ | |
| return err; | ||
| } | ||
|
|
||
| err = check_timestamp(machine->application, &(client->remote_data), req); | ||
| if(err != PSTOP_OK) { | ||
| return err; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| // SPDX-FileCopyrightText: 2026 Polymath Robotics, Inc. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| #include "pstop/protocol.h" | ||
|
|
||
| #include <unity/unity.h> | ||
|
|
||
| #include "pstop/checksum.h" | ||
| #include "pstop/requirements/test_utils.h" | ||
|
|
||
| #define MACHINE_ID 1236 | ||
| #define PSTOP_ID 1234 | ||
|
|
||
| static | ||
| device_id_t MACHINE = { | ||
| .data = MACHINE_ID | ||
| }; | ||
|
|
||
| static | ||
| device_id_t REMOTE = { | ||
| .data = PSTOP_ID | ||
| }; | ||
|
|
||
| static | ||
| pstop_application_t pstop_app; | ||
|
|
||
| #define MAX_CLIENTS 2U | ||
|
|
||
| static pstop_remote_data_t pstop_clients[MAX_CLIENTS]; | ||
|
|
||
| // 2-04-1: Shall be able to detect delayed messages | ||
| // Description: Set number of missed heartbeats to 2. Send BOND message | ||
| // with timestamp 100. Send OK message with timestamp 200, current | ||
| // clock is 200. Then send another OK message with timestamp 900 and | ||
| // current clock is 300. This will indicate we missed too many | ||
| // messages and will return PSTOP_MSG_LOST. | ||
|
|
||
| static | ||
|
There was a problem hiding this comment.
fans out to 7 callees (efferent coupling). Grounded coupling-delta finding (deterministic), not an LLM guess. There was a problem hiding this comment.
fans out to 7 callees (efferent coupling). Grounded coupling-delta finding (deterministic), not an LLM guess. There was a problem hiding this comment.
fans out to 7 callees (efferent coupling). Grounded coupling-delta finding (deterministic), not an LLM guess. There was a problem hiding this comment.
fans out to 7 callees (efferent coupling). Grounded coupling-delta finding (deterministic), not an LLM guess. |
||
| void | ||
| req_2_04_1_test(void) | ||
| { | ||
| pstop_machine_t machine; | ||
| machine_init(&machine, &pstop_app, pstop_clients, MAX_CLIENTS); | ||
| configure_app_defaults(&pstop_app, &MACHINE, 0U, 2U, 1000U); | ||
|
|
||
| set_operator_allowed(true, false, 500U); | ||
|
|
||
| pstop_msg_t req; | ||
| pstop_message_init(&req); | ||
| req.message = PSTOP_MESSAGE_BOND; | ||
| req.counter = 10; | ||
| req.stamp = 100; | ||
| req.id.data = PSTOP_ID; | ||
| req.receiver_id.data = MACHINE_ID; | ||
| req.received_counter = 0U; | ||
| req.received_stamp = 0U; | ||
|
|
||
| pstop_msg_t resp; | ||
| pstop_message_init(&resp); | ||
|
|
||
| set_time(100); | ||
| // succesfull bond | ||
| TEST_ASSERT_EQUAL(PSTOP_OK, machine_process_message(&machine, &req, &resp)); | ||
| TEST_ASSERT_EQUAL(PSTOP_MESSAGE_BOND, resp.message); | ||
|
|
||
| const protocol_data_t *remote = machine_get_protocol_data(&machine, &REMOTE); | ||
| TEST_ASSERT_NOT_NULL(remote); | ||
| uint64_t last_heartbeat = remote->last_timestamp; | ||
|
|
||
| // now send message with a valid stop | ||
| req.message = PSTOP_MESSAGE_OK; | ||
| req.counter = 11; | ||
| req.stamp = 200; | ||
| req.received_counter = resp.counter; | ||
| req.received_stamp = resp.stamp; | ||
| set_time(200); | ||
| TEST_ASSERT_EQUAL(PSTOP_OK, machine_process_message(&machine, &req, &resp)); | ||
| remote = machine_get_protocol_data(&machine, &REMOTE); | ||
| TEST_ASSERT_NOT_NULL(remote); | ||
| TEST_ASSERT_NOT_EQUAL(last_heartbeat, remote->last_timestamp); | ||
| last_heartbeat = remote->last_timestamp; | ||
|
|
||
| // now send message with timestamp too far in the future | ||
| req.message = PSTOP_MESSAGE_OK; | ||
| req.counter = 12; | ||
| req.stamp = 900; | ||
| req.received_counter = resp.counter; | ||
| req.received_stamp = resp.stamp; | ||
| set_time(300); | ||
| TEST_ASSERT_EQUAL(PSTOP_MSG_LOST, machine_process_message(&machine, &req, &resp)); | ||
| remote = machine_get_protocol_data(&machine, &REMOTE); | ||
| TEST_ASSERT_NOT_NULL(remote); | ||
| TEST_ASSERT_EQUAL(last_heartbeat, remote->last_timestamp); | ||
| } | ||
|
|
||
| void | ||
| main_req_2_04_test(void) | ||
| { | ||
| UnitySetTestFile("req_2_04_test.c"); | ||
|
|
||
| RUN_TEST(req_2_04_1_test); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 This regresses the missed-heartbeat detector: missed is diff_remote_vs_local/diff_sent_stamp, where diff_sent_stamp is the machine's own arrival-time gap (protocol.c:66,80), not the base's heartbeat_ms/round-trip-echo count. A late-arriving prior message shrinks diff_sent_stamp, so missed balloons and PSTOP_MSG_LOST fires though nothing was lost. If delay stays constant while a real heartbeat is skipped, both deltas grow together and missed stays near 0, masking real loss. Fix: derive missed from a value immune to arrival jitter, e.g. heartbeat_ms or the round-trip echo the base used. [also at: pstop_c/pstop/src/pstop/protocol.c:81 - Machine operators now get spurious PSTOP_MSG_LOST stops after ordinary network jitter/delay recovery, even though no message was actually lost. diff_sent_stamp (line 66) is the real wall-clock gap between successive message PROCESSING times, not a fixed interval; a burst of quick arrivals right…]
Extended reasoning...
protocol_handle_message updates client->remote_data.last_timestamp=now (machine clock) and last_received_stamp=req->stamp (remote clock) on every processed message. check_timestamp (protocol.c:62-81) compares diff_received_stamp=|req->stamp-last_received_stamp| (remote clock delta) to diff_sent_stamp=|now-last_timestamp| (machine clock delta) and flags LOST when their difference/diff_sent_stamp exceeds max_missed_heartbeats+1. Example, heartbeat_ms=100: message N-1 is network-delayed 90ms so it's processed late; message N arrives on schedule 10ms after that. diff_sent_stamp=10, diff_received_stamp=100 (unaffected by transport delay, it's remote send-time gap). missed=90/10=9>threshold, LOST fires though nothing was lost. Reverse: remote genuinely skips one send but transport delay stays constant, so both deltas grow by the same skipped interval and missed stays ~0 — real loss goes undetected. Base algorithm compared only machine-clock-domain values…
Verification: normal. The new detector (protocol.c:62-82) computes
missed = get_diff(diff_sent_stamp, diff_received_stamp) / diff_sent_stamp, withdiff_sent_stamp = |now - last_timestamp|(line 64-66) being the machine's own inter-arrival gap (last_timestamp is set tonowper processed message, line 159) anddiff_received_stamp = |req->stamp - last_received_stamp|(line 62) the remote-clock gap. The…