Repository navigation
Bridge link fixes2 - #1636
Merged
Merged
Bridge link fixes2#1636
Conversation
Frame/ack/resend counters, RX stop reasons, and ack latency histograms (sender: uart_tx -> ack parsed; receiver: frame parsed -> ack handed to uart_tx). Printed by 'uhk uartStats' and included in the 'uhk recover' dump. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
After any RX teardown, bridgeOnRxDisabled resyncs the parser while the peer's frame may still be streaming in; the rest of it arrived as 'unexpected' bytes, each of which reset RX again (10ms deaf, mid-frame re-enable) until the frame ended. Let the parser resync on the next Start byte instead, as the low-power build already did. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Restore the watermark check removed in 8d055e5: a frame whose ack got lost is resent with the same watermark and must not be applied twice (left key states carry cursor deltas). rxIdxValid exempts the first frame after a local watermark reset. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The ack loop is ~3ms for a key-state frame and ~13ms for a maximum-length one; every retry blinds the left key scanner for the whole delay. Backoff still doubles per retry. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
shared/uart_parser.c never included debug.h, so the flag was undefined there and the byte-corruption block was compiled out regardless of its value (since a689d1f). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
uart_tx stores the buffer pointer and returns; EasyDMA reads it afterwards. Control bytes (Ack/Nack/Ping) and the wake byte were sent from stack variables that went out of scope before the transfer, so what reached the wire was not guaranteed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The CRC-invalid dump ran in the UART RX ISR and emitted one deferred log message per byte; a single bad frame overflowed the log buffer regardless of log thread priority. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The exponential backoff let one unacked frame hold txBufferBusy for ~1s (was ~8s at 64ms); on the left half that blocks the key scanner, so key changes made meanwhile are coalesced or lost. Worst case is now UART_RESEND_COUNT * UART_RESEND_DELAY = 45ms. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Under DEBUG_STRESS_UART, ack/nack/ping bytes are discarded with ~5% probability, so the timeout -> resend -> duplicate path gets exercised, not only CRC failures. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
uartLoop took its sleep deadline before the resend branch, and resend() restamps lastMessageSentTime, so after every retry the thread slept to the next ping tick instead of to the next retry. The effective retry interval was therefore max(UART_RESEND_DELAY, time to next ping) == UART_BRIDGE_PING_INTERVAL == 200ms, which the logs show verbatim: gaps of 20/80/200/200ms against a nominal 15ms. That made the retry path unable to recover anything: five transmissions at 200ms is 1000ms against a 700ms UART_BRIDGE_TIMEOUT, so the link was always declared dead mid-retry, which then triggered a BLE fallback and a StateSync link reset - a burst of frames that lost another ack, and round again. Latent since at least 17.1.3; it only bit once the delay dropped below the ping interval (64ms << tries against a 100ms ping only quantised the first retry). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N5oDHHomxRbhs7jTqA9rgW
resendTries was cleared only on an ack or on give-up, so a burst that disconnected mid-retry left it at UART_RESEND_COUNT+1. The first frame sent after the reconnect then hit the give-up branch on its very first timeout and was discarded. Visible in the logs as a "Repeatedly failed to send a message!" immediately after the first "didn't receive ack" of each burst, rather than after the fifth. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N5oDHHomxRbhs7jTqA9rgW
The retry budget and UART_BRIDGE_TIMEOUT were independent constants, and had drifted into a state where the budget could not complete: 5 transmissions x 200ms effective delay against a 700ms timeout. With the delay fixed it is 5 x 15 = 75ms, but nothing stopped the two from drifting apart again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N5oDHHomxRbhs7jTqA9rgW
An ack was a bare control byte with no sequence information, and the sender closed whatever
frame happened to be outstanding. A resend produces two acks for one frame - the peer acks
the duplicate too, because the ack is raised in the UART layer before the messenger's
watermark dedup runs - so the second ack closed the *next* frame:
L sends F1 waiting
L resends F1 (timeout) waiting
ack(F1) arrives -> closes F1 sender pushes F2
ack(dup F1) arrives -> closes F2 F2 may still be on the wire
ack(F2) arrives -> closes F3 ... permanently one frame ahead
While desynchronised the link has no error detection at all: a genuinely lost frame gets its
predecessor's trailing ack and is never resent, so a dropped key-state frame disappears
silently. The stream only resynchronises when an ack lands with nothing outstanding, which a
StateSync reset burst never allows. 344e84e fixed the duplicate *delivery* half of this;
this is the ack aliasing half.
A single bit is enough, since the window is one frame. Two new control bytes carry it -
UartControlByte_Ack0 for an even watermark, Ack1 for an odd one - and an ack whose parity
does not match the outstanding frame is counted as stale and ignored. It also makes the
desync observable: ackWhileIdle cannot see it, because in that state the acks are never
idle-time ones.
The pre-existing UartControlByte_Ack keeps its value and its unsequenced behaviour, so a
peer that predates Ack0/Ack1 is still understood and needs no flag day. Note the
compatibility is one-directional: such a peer cannot parse Ack0/Ack1 and will read them as
out-of-frame bytes, so a mixed pair still degrades in that direction.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N5oDHHomxRbhs7jTqA9rgW
The deadline is anchored where uart_tx starts the DMA, not where the transfer completes, so the frame's own time on the wire was being spent out of the ack budget. A maximum-length frame occupies the wire for ~23ms on its own, which expires a 15ms timer before the peer has finished receiving it - no collision with another frame required. UART_RESEND_DELAY now means what its comment claims: the ack turnaround alone. The wire time is computed from the frame length and added, which is exact to within microseconds since transmission time is just byte count times baud - and is a smaller change than hooking UART_TX_DONE, which would also have to tell a data frame's completion apart from an ack's and would add ISR latency to the same number. Measured on a healthy pair for calibration: ack turnaround 133us-1815us mean, 10.3ms worst case, against ackLoop maxima of 12.1-13.3ms that were nearly all frame transmission. The budget assert now uses the worst-case frame, so it stays honest: 5 * (15 + 24) = 195ms against UART_BRIDGE_TIMEOUT 700ms. UART_BYTE_TIME_US moves to the header on the way, since it is a property of the link rather than of the low-power scheme that happened to contain it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N5oDHHomxRbhs7jTqA9rgW
9265da9 added `#include "debug.h"` to shared/uart_parser.c to make the flag visible, but put it *inside* `#ifdef DEVICE_ID` - and that guard is itself always false there, because DEVICE_ID comes from CONFIG_DEVICE_ID via device.h, which the file never includes. So the `#else` branch kept defining DEBUG_STRESS_UART to false locally and the whole stress block stayed compiled out. Confirmed by the compile line carrying no -DDEVICE_ID, and by the injector's log string being absent from the ELF before this change and present after. The harness has therefore never run since a689d1f, 9265da9 and 5019550 notwithstanding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N5oDHHomxRbhs7jTqA9rgW
With the frame's wire time charged separately, UART_RESEND_DELAY budgets the ack turnaround alone - measured at 133-346us mean with a sub-millisecond p90 on the left half and 1.8ms mean on the right. 15ms was sized against the observed maximum (~10ms, which was slot contention during a StateSync burst); sizing against the mean instead puts 7ms at roughly 20x it, and brings the recovery latency on a genuine loss into the same range as the 7ms NUS figure. The asymmetry justifies it: a spurious duplicate is cheap now that acks carry a watermark parity and no longer alias the ack stream, whereas the delay is also how long the sender stalls on a real loss, which blocks the key scanner. UART_RESEND_DELAY, UART_RESEND_COUNT, UART_BYTE_TIME_US and the wire-time helper move to uart_defs.h so the link tests can derive their own spacing from them instead of restating it. The stress injection rates become macros for the same reason, and the ack-drop rate goes from 1/32 to 1/16: over a link test's ~420 frames that puts two consecutive drops at ~80% per run and three at ~10%. Those cannot both be made comfortable - P(k drops) scales as p^k, so the separation between k and k+1 is only a factor of p. Budget assert still holds: 5 * (7 + 24) = 155ms against UART_BRIDGE_TIMEOUT 700ms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N5oDHHomxRbhs7jTqA9rgW
The give-up test was `resendTries++ > UART_RESEND_COUNT`, so a count of 3 actually permitted four resends and the budget macro carried a +2 to compensate. Corrected to >=, so the constant is now the number of resends and a frame gets UART_RESEND_COUNT+1 transmissions. Raised to 5 at the same time, which the shorter delay pays for: 5 retries at 7ms plus wire time is a ~54ms worst-case sender stall, against ~85ms for the previous 4 at 15ms. More chances to recover a lost frame, in less time than before - and the stall matters because on the left half the blocked sender is the key scanner. Fixing the off-by-one in isolation would have been a behaviour change disguised as a rename; doing it alongside the deeper count makes the net effect an improvement on both axes. Budget assert: (5+1) * (7+24) = 186ms against UART_BRIDGE_TIMEOUT 700ms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N5oDHHomxRbhs7jTqA9rgW
Both halves run the same link test script; each presses only the keys it owns, so what the right half validates has actually crossed the wire. The right sends MessengerCommand_StartTest before every repetition; the left picks it up in its scanner thread (starting it from the receive path raced the end of the previous run) and publishes its key states over the UART bridge on change only. Stress is switched on only between two quiet steps, so the StartTest exchange itself is never stressed - a dropped ack there resent it and restarted the left. Link tests are found by their id in any module (Tests_FindLinkTest, ids in tests.h). Link/probe: left change L1, left change L2 4ms later, right change R at 32ms that L2 must beat. Lost acks on L1 stall the sender, holding L2 back. Measured: a timeout retry ~9.3ms, a nack retry ~3ms, L2 lands ~6ms after the sender frees up, so R passes two consecutive timeouts and fails three. Two probes share a 40ms slot pair on six keys, every key phase clearing debounce. 70 cycles, sized so a healthy run fails ~20% of the time. Test suite: - Delays run on one schedule from the test's start (start plus all delays so far) instead of from when each began, so the ticks of other actions no longer drift the two halves apart. A doubletap test that only passed on that tick slack gets real debounce margin. - repeatCount: a repeated test runs exactly that many repetitions, counts each failing one once, is never auto-rerun and never verbose; timeouts are logged. A failing one reports e.g. "FAIL (2/70 failed, 98% fail confidence)", assuming a healthy test fails 20% of the time (binomial). The summary lists discrete and repeated tests separately, combining the repeated ones by Fisher's method (test_statistics.c, no libm). - The suite ticker is split into evaluating a test's outcome, concluding a repetition or a test, the verbose rerun, moving on and finishing the suite. - The left half logs none of a test's actions. Stress injection draws from xorshift32: the one-bit LFSR made the rolls of back-to-back acks share 13 of 16 bits, making two and three dropped acks in a row ~2x and ~4x likelier than nominal. The stress rates are marked as calibrated against by the link tests. uhk recover also dumps where the main thread sits (state, pc/lr, return addresses on its stack), to tell a blocked main from an idle one without a debugger. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N5oDHHomxRbhs7jTqA9rgW
Take read node->next and then swapped fifo.first, so a put from the uart isr in between got linked behind an unlinked node: the message was lost and its pool slots leaked. Nested producers could also lose a message or run into the retry panic. Guard put and take by DISABLE_IRQ instead. This was never encountered in practice as far as we know. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
After a simultaneous cold start of both halves (battery-less, powered over the bridge cable) the UARTE receiver can end up corrupting the first byte after every idle gap, without raising any rx error: frames lose their Start byte, lone pings and acks are lost, and the link flaps. One rx restart cures it for good. The reset on every unexpected byte used to hide this, and was removed in 5d560fd (and compiled out with UART_LOWPOWER in 18.0.0-18.0.2). Bring the recovery back in a bounded form: three consecutive rx chunks with out-of-frame bytes and nothing valid in between restart RX, at most once per 100ms. Co-Authored-By: Claude Opus 5.5 <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
1 participant
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.
Changelog: