Skip to content

fix(confirm): a screen will not accept the previous hold's release bounce - #486

Closed
BitHighlander wants to merge 1 commit into
release/7.14.2from
fix/484-confirm-release-bounce
Closed

fix(confirm): a screen will not accept the previous hold's release bounce#486
BitHighlander wants to merge 1 commit into
release/7.14.2from
fix/484-confirm-release-bounce

Conversation

@BitHighlander

Copy link
Copy Markdown
Owner

Closes #484. Found on hardware during the rc30 round; the emulator cannot reach it.

The defect

A confirmation screen opening right after another one completes accepts the
release bounce of the previous hold as consent. The user holds once and two
consecutive screens both pass.

This weakens #482 specifically. That PR added a second ButtonRequest so the user
is asked to consent to a body the device admits it cannot fully display. On
hardware the user is asked for the second hold but does not have to perform it.

It also makes host-side aborts racy: a Cancel can arrive to find the bounce has
already approved the screen. We hit exactly that while reproducing, and initially
mistook it for a broken Cancel.

Measurement

255-char Ping body (trips Cut Off, so two screens back to back). Tester cleared
the Cut Off screen, released, and did not touch the button again. Only the timing
of the host's ButtonAck for the second request varied:

run ButtonAck for BR2 result
carry-over immediate Success at 1.602s, no second hold
control never sent silence 15s — the screen is gated
delayed +5s silence 15s — bounce window has passed

The never-acked control rules out "the screen was never gated". Delaying the ack
past the bounce window removes the effect, which places the spurious press edge
immediately after the release of hold #1.

Root cause

lib/board/keepkey_button.c has no debounce — EXTI is EXTI_TRIGGER_BOTH and the
ISR decides press vs release from the GPIO level at interrupt time:

if (gpio_get(BUTTON_PORT, BUTTON_PIN) & BUTTON_PIN) { on_release_handler(...); }
else                                                { on_press_handler(...);  }

A mechanical release bounces low and dispatches on_press_handler.
confirm_screen() resets state_info to HOME and re-registers both handlers,
so the newly armed screen takes that bounce as a fresh press out of HOME.

The change

Track whether the button has been observed released while this screen owned it.

  • confirm_screen() init: armed = keepkey_button_up() — a screen that opens with
    the button down starts disarmed
  • handle_screen_press(): returns early unless armed
  • handle_screen_release(): sets armed = true

One bool, two branches, no driver change and no timing constant. A debounce delay
would have needed a tuned constant and would still be a race; requiring an observed
release is exact.

EMULATOR: keepkey_button_up() is a constant false there, so armed is set
unconditionally under #ifdef EMULATOR — otherwise emulated presses would never
land and every emulator confirm test would hang.

Verification plan

CI here proves it compiles and does not regress the emulator suite; it cannot
prove the fix, because there is no button and no bounce to reproduce against.

Hardware re-run on the artifact from this PR, before rc31:

  • cutoff_428_carryover.py — expect silence where it previously returned
    Success at 1.602s
  • cutoff_428_delayack.py — unchanged (silence)
  • a deliberate second hold still completes the confirmation normally
  • T6's three-page ETH pager, the worst case for consecutive screens

Scope note for rc30

Two rc30 tests are invalid until this lands: any test with consecutive confirm
screens can pass on a single hold (T5, T6), and any test whose pass condition is a
host Cancel races the bounce (T2, T3). Running them first would have produced
passes needing a re-run anyway.

Evidence: docs/release/rc30-evidence/T5-cutoff-findings.md. Reproduction scripts
are in deps/python-keepkey/tests/ (not in this commit — the submodule gitlink is
handled separately).

…unce

The button driver has no debounce. EXTI is EXTI_TRIGGER_BOTH and
buttonisr_usr() decides press vs release from the GPIO level at interrupt
time, so a mechanical release bounces the line low again and dispatches
on_press_handler.

confirm_screen() resets state_info to HOME and re-registers both handlers,
so a screen opening right after another one completes is armed in time to
receive that bounce and accept it as a fresh press out of HOME. On hardware
the user holds once and two consecutive screens both pass.

That weakens #482 specifically: the Cut Off screen exists so the user is
asked to consent to a body the device admits it cannot fully display. The
user was asked for the second hold but did not have to perform it. It also
makes host-side aborts racy -- a Cancel can arrive to find the bounce has
already approved the screen.

Track whether the button has been observed released while this screen owned
it. A screen that opens with the button down waits for a real release before
it will accept anything; the release handler arms it. keepkey_button_up() is
a constant false under EMULATOR, so arm unconditionally there or emulated
presses never land.

Measured on hardware, 7.14.2 at 23af308, 255-char Ping body, varying only
when the host's ButtonAck for the second request lands:

  immediate ack  -> Success at 1.602s with no second hold
  ack withheld   -> silence for 15s (the screen is gated, not ungated)
  ack after 5s   -> silence for 15s (the bounce window has passed)

The emulator cannot reach this: it has no button and no bounce.

Closes #484
@BitHighlander

Copy link
Copy Markdown
Owner Author

Closing: the premise is wrong.

The "device confirmed with no press" measurements were the tester pressing the
button and reporting it as untouched -- not reflex or bounce, simply that the
press was not caught in the moment. There is no release-bounce defect and this
fix corrects nothing.

The tell was in the numbers and I talked past it. This change moved the result
from 1.602s to 1.662s -- i.e. not at all. A fix that does not move the metric it
targets has not been validated by the metric staying put; that is a refutation.
I read it as noise and kept going.

The decisive control was a SINGLE confirm screen, untouched: it completed at
5.076s. With no preceding screen there is no transition and no bounce to inherit,
so the two-screen framing could not have been the cause. That control cost one
minute and should have run before the issue was filed, let alone the PR.

What survives, on evidence unaffected by this:

What survives independently of any timing measurement: #485. That one is read
directly out of confirm_sm.c:441 -- the post-warning screen re-draws the same
truncated body -- and does not depend on hardware behaviour at all.

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.

1 participant