F-19: Unbounded encoder delta: one corrupt I2C read can command full speed #22

Severity: HIGH · Effort: Small · Batch: 1 · Status: Code done in #21 (7c980f9), awaiting bench verification

Found in the second full review of the firmware. Recorded with the rest of the pass (F-19…F-44) in docs/REVIEW_REMEDIATION_PLAN_2.md, which is where its status is kept.

Description

RotaryEncoderHal::readEncoderDelta assembles a 32-bit signed delta from four I2C bytes and
pollOnce passes it to the rotation callback unchecked. ThrottleController::onKnobRotation
computes delta * stepsPerClick — signed overflow, which is undefined behaviour, for a large
delta — and clamps the result to ±126. So one corrupted upper byte on a bus that runs alongside
DCC wiring commands full speed, in either direction. A garbage delta in SELECTING state
also reaches Knob::handleRotation, whose while wrap loops can run for millions of iterations
while holding m_stateMutex.

The double-read workaround makes this likelier than it looks. If the Seesaw is answering the
previous request (the likely reason the second read is the one that "works"), one failed
transaction shifts the pipeline and a GPIO-bulk word — bit 24 set, about 16.7 million — is read
as a delta.

Plan

1. In RotaryEncoderHal::pollOnce, treat a delta outside ±MAX_PLAUSIBLE_DELTA as a failed
   read: log it at WARN with the raw value and drop it. One full revolution per poll is far
   beyond a human hand; that is the bound.
2. In ThrottleController::onKnobRotation, clamp delta before multiplying, so no caller can
   reach the overflow.
3. Replace the while wrap loops in Knob::handleRotation with modular arithmetic.
4. Unit tests for the controller and the knob with an absurd delta.

Files: RotaryEncoderHal.cpp, ThrottleController.cpp, Knob.cpp, tests

Acceptance Criteria

• A delta beyond the plausibility bound never reaches the rotation callback
  (RotaryEncoderHal::isPlausibleDelta, ±24).
• onKnobRotation(knob, INT_MIN) is well-defined: the delta is clamped before the
  multiply, and the result saturates at the end of the range.
• Knob::handleRotation with a huge delta returns at once with a valid index.
• Unit tests test_encoder_implausible_delta_is_refused,
  test_controller_extreme_delta_is_well_defined and
  test_knob_rotation_huge_delta_wraps_directly pass on the board (they compile).
• Bench: log raw deltas while spinning fast, and while unplugging an encoder mid-spin; no
  out-of-bound value reaches the controller.