Keep the release that ends an arming gesture - #156
subtilitas wants to merge 30 commits into
Conversation
The control task forwarded touch events with a non-blocking send to a 32-entry queue, so a full queue refused the new event. Every other queue in the file gives up its oldest entry instead: the GT911's own event queue, the command queue and the sample queue each say so in a comment. The one carrying touch kept the oldest. The last event of a gesture is its release. A release the screen never sees leaves it holding a press that is no longer on the glass, and ui_hold_tick() completes that hold on the frame timer, so ARM goes out with nothing on the panel. The screen also refuses DISARM until a contact arrives carrying the same track id, and consumes the first such tap as the phantom's release. The touch-health watchdog does not cover it: the controller is answering normally. A still finger emits no events, so filling 32 slots needs coordinate wobble on the held contact or a second contact; a palm on the glass reaches it in about 90 ms of undrained frame. Evict the oldest and count the eviction. The frame log carries the count as TOUCHEVICT beside DRAW and WAIT, so a bench can report whether the render loop has ever fallen far enough behind for it to happen. Separately, cap what one frame can credit to a hold. The duration is measured at the top of the frame and applied at the end of it, after the touch queue is drained, so a late frame credits a hold that began while that same frame was dispatching its own events -- an arm on a press milliseconds old. UI_HOLD_MAX_CREDIT_S is 250 ms, which puts at least eight frames with the press standing between a press and an arm, and is far above an ordinary 26 ms frame. test_widgets gains one_late_frame_cannot_complete_a_hold: eight 2.5 s frames to fire rather than one, and a frame of zero or negative time credits nothing. Three existing cases fired a hold with a single 2 s tick and now advance in frame-sized steps through a helper.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (3)
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f26c0efd98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| bool routed = (xQueueSend(s_touch_q, &evt, 0) == pdTRUE); | ||
| if (!routed) { | ||
| touch_event_t stale; | ||
| if (xQueueReceive(s_touch_q, &stale, 0) == pdTRUE) { |
There was a problem hiding this comment.
When s_touch_q is full with an already queued TOUCH_EVENT_UP at its head—for example, after its matching DOWN was dispatched and other contacts kept producing MOVE events during a stalled render—this blindly removes that release to admit a newer MOVE. touch_tracker_update() emits releases before events for remaining contacts, so this ordering is reachable; the UI then retains the earlier press and ui_hold_tick() can still complete an ARM with no finger present, recreating the safety failure this patch intends to prevent. Evict or coalesce only nonterminal MOVE events, or explicitly cancel the affected gesture rather than dropping an arbitrary oldest event.
Useful? React with 👍 / 👎.
| if (dt_s > UI_HOLD_MAX_CREDIT_S) { | ||
| dt_s = UI_HOLD_MAX_CREDIT_S; |
There was a problem hiding this comment.
The new cap only affects callers of ui_hold_tick(), while busfault_screen.c::tick() still advances its deliberate acknowledgement hold with the raw s.held_s += dt_s. If a long frame dispatches a press on ACK before calling ui_router_tick(), that screen can therefore consume the entire stale frame duration and acknowledge the fault immediately from a millisecond-old touch, despite the newly documented guarantee that a hold receives at most 250 ms per frame. Reuse ui_hold_t there or apply the same cap to its custom hold logic.
Useful? React with 👍 / 👎.
Two findings, both confirmed. Evicting the oldest entry could throw away a release. A queued UP reaches the head once the MOVEs ahead of it have been drained or evicted, and another contact producing MOVEs during the same stalled frame then displaced it -- the defect this branch exists to close, one step further along. A movement is a position and the next one supersedes it: ui_slider's by_delta() measures a drag from its own origin, so a MOVE that never arrives costs an intermediate frame and no travel. A press and a release are the ends of a gesture and are not replaceable. The queue therefore gives up its oldest entry only when that entry is a movement, or when the arriving event is itself a release; otherwise the arriving event is the one refused. A press that never registers is inert, and a release that never arrives is what leaves a screen holding a gesture. The frame log carries both counts as TOUCH <evicted>/<dropped>. The bus-fault acknowledgement kept its own timer. busfault_screen's tick() advanced s.held_s with the raw frame duration, so a late frame that dispatched the press it then credited would acknowledge a fault from a touch milliseconds old -- the gesture that screen exists to refuse. The cap moved out of ui_hold_tick() into ui_hold_credit(), which both callers now use. test_busfault_screen gains one_late_frame_cannot_acknowledge_a_fault: eight frames of 3 s each rather than one. It fails with the raw addition restored.
|
Both addressed in the push above. P1 — preserve queued release events during eviction. Confirmed, and the reachability is as described: a queued The queue now distinguishes what can be spared from what cannot. A MOVE is a position and the next one supersedes it — So the oldest entry is given up only when it is a MOVE, or when the arriving event is itself an UP. Otherwise the arriving event is the one refused — a press that never registers is inert, whereas a release that never arrives is exactly what leaves a screen holding a gesture. The frame log now carries both counts as The residual, stated rather than hidden: with the head a lifecycle event and an arriving UP, the head is still displaced. That needs 32 queued events with a DOWN or UP at the head and at most P2 — apply the frame-credit cap to fault acknowledgements. Confirmed.
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98c2bbd11d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const bool have_head = | ||
| (xQueuePeek(s_touch_q, &head, 0) == pdTRUE); |
There was a problem hiding this comment.
When the render task drains s_touch_q between this xQueuePeek() and the later xQueueReceive(), the receive can remove a different head than the one classified here. Because control_task and app_main run concurrently on separate cores, a peeked MOVE can be consumed by app_main and the receive can then evict the following DOWN or UP; if the consumer empties the queue, the receive fails and an arriving UP is never retried despite there now being room. Under queue pressure either interleaving can again discard a gesture endpoint and leave the UI holding a released contact, so inspection and removal must operate on the same entry (or the send must be retried after concurrent drainage).
Useful? React with 👍 / 👎.
| * A MOVE is a position, and the next one supersedes it: | ||
| * ui_slider's by_delta() measures a drag from its own origin, so | ||
| * a MOVE that never arrives costs an intermediate frame and no | ||
| * travel. A DOWN and an UP are the ends of a gesture and are | ||
| * not replaceable, so a MOVE is the only class this throws away | ||
| * while one of them is waiting. |
There was a problem hiding this comment.
When the evicted MOVE is the event where a held finger first leaves ARM, the next position does not supersede its state transition: motor_screen.c, servo_screen.c, and busfault_screen.c all permanently cancel their hold upon observing an out-of-control MOVE. If the queue drops that MOVE and a later position is back inside the button, the screen never learns that the gesture was broken and can eventually arm or acknowledge the fault. Only coalesce positions for consumers whose behavior depends solely on the latest coordinate, rather than treating every MOVE as spare.
Useful? React with 👍 / 👎.
| (xQueuePeek(s_touch_q, &head, 0) == pdTRUE); | ||
| const bool head_spare = have_head | ||
| && head.type == TOUCH_EVENT_MOVE; | ||
| const bool new_spare = (evt.type != TOUCH_EVENT_UP); |
There was a problem hiding this comment.
When a full queue headed by a DOWN or UP refuses a new contact's DOWN, the tracker still emits that contact's eventual UP; this classification then treats the orphan UP as indispensable and evicts the queued lifecycle event to admit it. During one stalled frame, a short contact can therefore have its DOWN dropped and its useless UP subsequently displace another contact's valid release, recreating the stuck hold this policy is intended to prevent without requiring a queue full of lifecycle events. Track whether the corresponding DOWN reached the router and treat an UP for a refused press as spare.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 98c2bbd11d
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| } else if (have_head) { | ||
| touch_event_t stale; | ||
| if (xQueueReceive(s_touch_q, &stale, 0) == pdTRUE) { | ||
| atomic_fetch_add(&s_touch_evicted, 1u); | ||
| routed = (xQueueSend(s_touch_q, &evt, 0) == pdTRUE); |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
With a dispatched ARM press and a stalled renderer, a palm can fill the 32-entry queue with the first finger's UP at its head. If another contact then lifts, head_spare and new_spare are both false, so this branch evicts that queued UP to admit the newer UP. The screen ignores the second ID and keeps ticking the first press until it arms with no finger present. Preserve all terminal releases, or cancel affected gestures, rather than exchanging one UP for another. Fresh evidence beyond the prior MOVE case is that two UPs still select this eviction branch. This can silently re-arm configured outputs, which the trusted policy treats as priority.
SECURITY.md reference: SECURITY.md:L9-L14
Useful? React with 👍 / 👎.
Four findings on the eviction policy, all confirmed, and together they say the policy was the wrong shape: no choice of which event to drop is safe on its own. A release that never arrives leaves a screen holding a press. A press that never arrives orphans the release after it, and that orphan then displaces a valid release. A movement is not spare either -- the movement where a finger leaves ARM is what abandons the hold, and the position after it is back inside the button. And the render task drains the queue from the other core, so inspecting the head decides nothing: the entry classified is not necessarily the entry removed, and a consumer that empties the queue between the two leaves the arriving event unsent. So the queue goes back to one rule -- drop the oldest, take the newest -- and the loss is recorded rather than reasoned about. The frame that observes the count move tells the screen on top that its record of the glass is stale, and the screen drops any gesture in progress. Which event went missing stops mattering. ui_screen_t gains an optional cancel(). A screen whose controls all act on a release can leave it NULL: losing an event there costs a highlight. A screen with a gesture that completes on a timer cannot, and the three that have one implement it -- MOTOR & ESC, SERVO and CAN BUS FAULT. Cancelling commands nothing, which is what letting go early already does. The count is read before the drain, so an event lost while that loop runs is answered by the next frame rather than missed. The frame log carries it as TOUCHLOST. Two cases, each failing with cancel() stubbed out: a_cancelled_gesture_does_not_arm holds ARM, cancels, ticks past two seconds and checks nothing was commanded, that the eventual release commands nothing either, and that a fresh press still arms; a_cancelled_gesture_acknowledges_nothing does the same for the fault acknowledgement.
|
All four addressed, and they changed the shape of the fix rather than its details. Thank you — taken together they say the eviction policy was the wrong instrument, so it is gone. What the four findings establish. No choice of which event to drop is safe on its own:
What replaces it. The queue goes back to one rule — drop the oldest, take the newest — and the loss is recorded rather than reasoned about. The frame that observes the count move tells the screen on top that its record of the glass is stale, and the screen drops any gesture in progress. Which event went missing stops mattering, so every one of the five interleavings above resolves the same way.
The count is read before the drain, so an event lost while that loop runs is answered by the next frame rather than missed; the count only rises, so a difference is always one or more real losses. The frame log carries it as Tests, both failing with
The queue itself is in
@codex review |
The motor and bus-fault cancels each had a case and the servo's did not, which left the patch below the 90 % codecov holds new code to. The servo arming hold completes on the same frame timer as the other two, so the same gesture is reachable there. a_cancelled_gesture_does_not_arm holds ARM, cancels, ticks past two seconds and checks nothing was commanded, that the eventual release commands nothing either, and that a fresh press still arms. It fails with the servo screen's cancel() stubbed out.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e19c2899b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * while this loop runs is answered by the next frame rather than | ||
| * missed: the count only rises. | ||
| */ | ||
| const unsigned lost_now = atomic_load(&s_touch_lost); |
There was a problem hiding this comment.
If the control task overflows the full queue after this load but while app_main is draining it, it can remove an undispatched UP and increment s_touch_lost; the render task then processes the remaining events and reaches ui_router_tick() in this same iteration, before the next frame observes the increment. A hold already near two seconds can therefore fire with its release missing. Fresh evidence beyond the earlier queue race is that the new cancellation path samples the counter only before the concurrent drain; recheck/exchange it after draining and cancel before ticking.
Useful? React with 👍 / 👎.
| ui_slider_release(&s.speed); | ||
| ui_hold_reset(&s.arm); | ||
| s.arm_down = false; |
There was a problem hiding this comment.
When the dropped event is the UP ending a servo-dial drag, this cancellation releases the speed slider and ARM hold but leaves s.dragging set. Once the GT911 reuses that track ID, a later contact that began elsewhere can move across the dial and satisfy the stale drag path, issuing a new servo position without a DOWN on the dial; while armed, that can move the connected servo unexpectedly. Clear s.dragging along with the other gesture ownership.
Useful? React with 👍 / 👎.
| s.pressed = false; | ||
| s.held_s = 0.0f; |
There was a problem hiding this comment.
If the lost event is the ACK finger's UP, cancel() stops the timer but leaves s.have_press true. Every subsequent DOWN is then rejected until an UP with the old track ID happens to arrive, so the operator may be unable to acknowledge the fault and leave this screen after the queue-overflow recovery. Clear have_press as part of abandoning the gesture.
Useful? React with 👍 / 👎.
| const ui_screen_t *scr = screen_for(s.current); | ||
| if (scr != NULL && scr->cancel != NULL) { | ||
| scr->cancel(); |
There was a problem hiding this comment.
When the missing event is the UP for HOME or STOP, the gesture is owned by ui_router rather than the current screen, but this function leaves s.band_press latched. A later body contact with the reused ID is consequently consumed as the old band gesture and can either lose the user's input or activate the stale HOME/STOP action based on where it lifts. Clear the router's band press before delegating cancellation to the screen.
Useful? React with 👍 / 👎.
| if (xQueueReceive(s_touch_q, &stale, 0) == pdTRUE) { | ||
| routed = (xQueueSend(s_touch_q, &evt, 0) == pdTRUE); | ||
| } | ||
| atomic_fetch_add(&s_touch_lost, 1u); |
There was a problem hiding this comment.
This counter records only overflows of s_touch_q, but the upstream queue in firmware/panel/components/gt911/touch.c::publish() also evicts its oldest event when full. If the touch task fills that queue while the control task is delayed, the control task subsequently forwards every remaining event into an empty s_touch_q, so this increment never occurs and no gesture is cancelled. A release or canceling MOVE lost in the first queue can therefore still leave an ARM hold running; propagate that queue's loss indication into the same cancellation path.
Useful? React with 👍 / 👎.
| * mattering. | ||
| */ | ||
| touch_event_t stale; | ||
| if (xQueueReceive(s_touch_q, &stale, 0) == pdTRUE) { |
There was a problem hiding this comment.
When a STOP release is successfully routed, s_stop_counted is set, but a later overflow can evict that release here before the router consumes it. The router then raises no matching s_stop_request, leaving the marker set; a subsequent STOP that genuinely needs the router backstop—for example, during the frame after entering a banded screen while s_stop_live is still false—has its request ignored by service_arming() when it exchanges the stale marker. Clear or associate this marker with the specific routed event whenever the touch stream is invalidated.
Useful? React with 👍 / 👎.
| * A screen whose controls all act on a release can leave this NULL -- | ||
| * losing an event there costs a highlight. A screen with a gesture that | ||
| * completes on a timer cannot: a press whose release went missing goes | ||
| * on being held, and the timer finishes it with nothing on the glass. |
There was a problem hiding this comment.
Release-driven screens cannot safely leave this callback NULL: they retain DOWN state until an UP arrives. For example, overview_screen.c keeps have_press, press_id, and the selected tile, so if that UP is evicted, a later contact reusing the ID can lift on the stale tile and navigate without having pressed it; outputs_screen.c similarly retains hit_kind and can apply a stale binding change. Because ui_router_cancel_gestures() does nothing for these screens, event loss costs more than a highlight; give stateful release controls a cancellation path as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 5e19c2899b
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| * for nothing, which is what letting go early already does. | ||
| */ | ||
| lost_seen = lost_now; | ||
| ui_router_cancel_gestures(); |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
When the bench is already armed and touch traffic overflows after a DISARM DOWN but before its queued UP is drained, this cancellation runs first. Both MOTOR and SERVO callbacks clear the press owner, so their UP handlers ignore the release and never post DISARM; the control task's independent fast path only hit-tests top-band STOP. The heartbeat and link therefore stay healthy and output keeps driving until the operator notices and retries or presses STOP. Make cancellation fail safe for an armed ARM/DISARM gesture.
SECURITY.md reference: SECURITY.md:L9-L14
Useful? React with 👍 / 👎.
Seven findings, all confirmed. Together they say the cancel had the right shape and too small a reach. The loss count was read before the drain, so an event lost while that loop was running went unanswered until the next frame -- and ui_router_tick() runs in the same iteration, so a hold already near two seconds completed in it. The count is now read after the drain and before the tick. The driver's own event queue evicts its oldest for the same reason, and that loss reached nothing: the control task then forwarded what remained into an empty queue and never counted anything. touch_lost() publishes it and the render loop folds it into the same difference. s_stop_counted marks a stop this task applied and the router will latch again. The event it refers to can be the one evicted, in which case the router raises no request and the marker stands, to be consumed by the next stop that genuinely needs the backstop. A loss clears it. The cancels themselves left state behind. The servo screen kept its dial drag, so a later contact on a reused track id could command a position with no press on the dial, which on an armed bench moves the servo. The bus-fault screen kept ownership of its track id, so every later press was refused and the operator could not acknowledge the fault or leave. And HOME and STOP are the router's gesture rather than a screen's, so the band press was cancelled by nobody. The contract was also too narrow. A screen whose controls act on the release keeps state between the two, and a press left latched owns a track id the controller reuses: the overview's tiles navigate, the outputs and picker screens' cells apply a binding change, and the settings screen's keys repeat on the frame timer. All five now cancel, and the header says that a release-driven control is not exempt. Five cases, each failing with the cancel it covers stubbed out: a cancelled tile press navigates nowhere, a cancelled cell press applies nothing on the outputs screen and toggles nothing on the picker, a cancelled hit opens nothing from the settings screen, and cancelling lets go of the band.
…-release # Conflicts: # STATUS.md
|
All seven addressed in the push above. Together they say the cancel had the right shape and too small a reach. P1 — recheck touch loss before advancing holds. Confirmed. The count was read before the drain, so a loss during it went unanswered until the next frame — and P1 — propagate losses from the GT911 event queue. Confirmed, and this was the bigger hole: P1 — clear stale STOP markers after eviction. Confirmed. P1 — clear the servo dial drag. Confirmed, and it is the one that reaches a pin: a stale P2 — clear ownership on the fault acknowledgement. Confirmed. A latched P2 — cancel router-owned band gestures. Confirmed. HOME and STOP are the router's gesture, not a screen's, so P2 — require cancellation for release-driven controls. Confirmed, and the header now says so rather than inviting NULL. A screen whose controls act on the release keeps state between the two, and a latched press owns a track id the controller reuses. The overview's tiles navigate, the outputs and picker screens' cells apply a binding change, and the settings screen's keys repeat on the frame timer — all five now cancel. Tests, five new, each failing with the cancel it covers stubbed out:
Worth flagging, because it nearly cost a vacuous test: the first version of the settings case used the screen's own
@codex review |
Cancelling a gesture asks for nothing, which is the safe direction for arming and the wrong one for its opposite. Disarming is a press, so the release is the whole command: a release lost to a full touch queue is a disarm the operator made and the bench never saw, and the cancel then dropped it silently. The heartbeat and the link stay healthy, so nothing else stops the outputs -- the bench goes on driving until somebody notices and presses again or reaches for STOP. The control task's own fast path does not cover it either; it hit-tests the band's STOP and nothing else. The motor and servo screens post their disarm from cancel() when the bench is armed and the press being abandoned is on that control and has not already fired. An arm cancelled part way still asks for nothing, because the hold is the gesture and it did not complete. Two cases: a_cancelled_disarm_still_disarms, which fails without it, and a_cancelled_arm_asks_for_nothing, which holds the other direction where it was.
|
Confirmed, and this is the most important finding on the branch — the cancellation I added was fail-safe in one direction and fail-dangerous in the other, which is worse than the defect it replaced for that one gesture. Arming has already sent its command by the time the finger lifts, so a hold abandoned part way asks for nothing. Correct. Disarming is a press, so its release is the whole command. A release lost to a full queue is a disarm the operator made and the bench never saw, and The motor and servo screens now post their disarm from if (s.armed && s.pressed == 1 && !s.arm.fired) {
post(MOTOR_CMD_DISARM, 0.0f);
}Two cases:
@codex review |
The paragraph on cancelling said it asks for nothing, and the paragraph four lines below it says a disarm is the case where that is wrong. The first now points at the second.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dedcade52c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const unsigned lost_now = atomic_load(&s_touch_lost) + touch_lost(); | ||
| if (lost_now != lost_seen) { | ||
| lost_seen = lost_now; | ||
| ui_router_cancel_gestures(); |
There was a problem hiding this comment.
When either loss counter has already changed before a frame begins, the loop at main.c:3849-3851 dispatches every surviving event before this first comparison. For example, after a servo-drag UP is dropped, a queued MOVE from a contact reusing that ID can call command(), or an orphan UP can apply an output binding, before the new cancellation callbacks run; cancelling afterward cannot undo those actions. Fresh evidence beyond the prior hold-tick race is that this revision adds release/drag cancellation hooks that are bypassed by this ordering. Check and cancel before draining, while retaining a post-drain check for concurrent losses.
Useful? React with 👍 / 👎.
| (void)xQueueReceive(s_touch.events, &dropped, 0); | ||
| (void)xQueueSend(s_touch.events, &evts[i], 0); | ||
| atomic_fetch_add(&s_touch.lost, 1u); |
There was a problem hiding this comment.
When the GT911 queue drops the UP belonging to a STOP press, this counter eventually cancels the router and screen gestures, but it never clears the independent s_stop_press state owned by control_pump(). After the controller reuses s_stop_id, a contact that starts elsewhere and is released over STOP satisfies the stale branch at main.c:593-614, unexpectedly stopping the bench and aborting the run without a new STOP press. Propagate driver loss to the control task as well so it abandons its fast-path STOP ownership.
Useful? React with 👍 / 👎.
| * the marker stands -- to be consumed by the next stop that | ||
| * genuinely needs the backstop, which would then be ignored. | ||
| */ | ||
| atomic_store(&s_stop_counted, false); |
There was a problem hiding this comment.
When a successfully routed STOP release filled the queue, s_stop_counted is true and that release is at the tail; if the next event overflows the queue, eviction normally removes a different entry from the head, yet this unconditional clear discards the still-valid marker. The renderer then processes the surviving STOP release and raises s_stop_request, and service_arming() sees the marker false and calls arming_stop() a second time for the same press. This advances the stop generation twice and repeats its command-invalidation and gesture-cancellation side effects; clear the marker only when the associated STOP release was actually evicted.
Useful? React with 👍 / 👎.
Three findings, all confirmed. The cancellation ran after the drain and not before it. A loss recorded before the frame began was therefore answered only once every surviving event had been dispatched -- and those events were captured around the one that went missing, so a queued movement on a reused track id commands a servo position and an orphan release applies a binding change, neither of which a later cancel can take back. The count is now read twice: once before the drain, and once before the tick, which is the concurrent case the previous round added. The control task owns its STOP press independently of the screens, and nothing cancelled it. The driver's queue can drop that press's release before this loop ever sees it; the controller then reuses the track id, and a contact that begins elsewhere and lifts over STOP satisfies the stale branch and stops a run nobody asked to stop. It drops that ownership on a driver loss. Only the driver's count is watched there: this task evicts from its own queue after the event has already been through that branch, so its own losses cannot orphan the press. And s_stop_counted was cleared for any eviction, where it refers to one particular release. Clearing it when that release is still queued is the opposite error to leaving it stand: the router raises its request, the marker reads false, and arming_stop() runs a second time for one press, advancing the stop generation twice. The marker now carries the id of the release it refers to and is cleared only when that release is the entry evicted.
|
All three addressed in the push above. P1 — cancel known losses before dispatching survivors. Confirmed, and it is the mirror of the finding from the previous round: I moved the check after the drain to catch a loss occurring during it, and that left a loss recorded before the frame answered only once every surviving event had been dispatched. Those survivors were captured around the one that went missing, so a queued MOVE on a reused track id commands a servo position and an orphan UP applies a binding change — neither of which a later cancel can take back. The count is read twice now: once before the drain, once before the tick. Both cases are covered and neither displaces the other. P2 — cancel the control task's STOP gesture on driver loss. Confirmed. It owns It drops that ownership on a driver loss. Only the driver's count is watched there, deliberately: this task evicts from P2 — preserve markers for STOP releases that remain queued. Confirmed, and thank you for catching the direction — I introduced this one answering the round before. Clearing The marker now carries the track id of the release it refers to, and is cleared only when that release is the entry actually evicted. Suite 44/44, coverage 95.9%, docs, screenshot, sanitizer-instrumentation and clang-tidy checks clean. @codex review |
The block above the send still said the eviction direction matters more here than anywhere else on the panel, which is what the block below it was written to replace: no choice of which event to drop is safe, and that is why the loss is recorded instead. The first now carries only what it takes to fill the queue and points at the second.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5645a93605
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const unsigned lost_now = touch_losses(); | ||
| if (lost_now != lost_seen) { | ||
| lost_seen = lost_now; | ||
| ui_router_cancel_gestures(); |
There was a problem hiding this comment.
When the control task refills and overflows s_touch_q immediately after this load but before ui_router_tick(), it can still evict the held contact's UP and increment the counter too late for this frame's cancellation. If the hold is within one capped credit of completion, the following tick can therefore ARM with no contact on the glass. Fresh evidence beyond the earlier pre-drain race is that the added post-drain check remains a TOCTOU operation on the other core; the loss sample and hold advancement need synchronization.
Useful? React with 👍 / 👎.
| touch_event_t dropped; | ||
| (void)xQueueReceive(s_touch.events, &dropped, 0); | ||
| (void)xQueueSend(s_touch.events, &evts[i], 0); | ||
| atomic_fetch_add(&s_touch.lost, 1u); |
There was a problem hiding this comment.
If the control task drains the driver queue between the failed xQueueSend() and this xQueueReceive(), the receive finds nothing and the subsequent send successfully enqueues the new event, so no event was lost; nevertheless the counter is incremented unconditionally. The render loop then treats this false increment as a broken touch stream, cancelling a valid gesture and potentially posting a DISARM when an armed screen currently has its ARM/DISARM control pressed. Increment only when either the receive actually discarded an entry or the retry also failed.
Useful? React with 👍 / 👎.
Two findings. The loss counter is sampled on one core and raised on the other, so the post-drain check is a read that the control task can invalidate before ui_router_tick() advances the hold. A hold within one capped credit of completing therefore fires on a contact that has already gone. The frame ordering closes it without synchronising the two. A command posted by a screen is forwarded at flush_screen_commands() on the frame after the one that posted it, and the pre-drain cancellation of that next frame runs first. So cancelling drops an arm the screen has posted and the application has not yet collected. A posted disarm is kept: it is the direction that fails safe, and the same asymmetry the gesture itself has. And both queues counted a loss they had not taken. The consumer can drain between the failed send and the receive, in which case nothing was evicted and the retry succeeds. Counting that told the render loop the stream had broken when it had not, cancelling a gesture still on the glass -- and on an armed bench a cancelled disarm gesture posts a disarm. Both count only when an event actually went. a_cancel_drops_an_arm_the_hold_already_posted fails without the first; a_cancel_keeps_a_disarm_already_posted holds the other direction.
|
Both addressed in the two pushes above. P1 — synchronise the final loss check with hold ticking. Confirmed: the counter is raised on the control task's core and sampled on the render task's, so the post-drain check is a read the other core can invalidate before I did not add synchronisation, because the frame ordering already gives a place to close it. A command posted by a screen is not forwarded in the frame that posted it — So If you think the ordering argument is too load-bearing to rest on and would rather see an explicit handshake, say so — it is a real trade and I would rather be told than guess. P2 — count a driver loss only after actually dropping an event. Confirmed, and it applies to the panel's queue as well, which had the identical unconditional increment. The consumer can drain between the failed send and the receive, in which case nothing was evicted and the retry succeeds. That false increment is not harmless in this branch: the render loop would read it as a broken stream and cancel a gesture still on the glass — and on an armed bench a cancelled disarm gesture posts a disarm, so a spurious count could stop a run. Both queues now count only when an event actually went ( Tests:
Suite 44/44, coverage 95.9%, docs, screenshot and sanitizer checks clean. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae93b2e207
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const unsigned drv_lost = touch_lost(); | ||
| if (drv_lost != s_stop_lost_seen) { | ||
| s_stop_lost_seen = drv_lost; | ||
| s_stop_press = false; |
There was a problem hiding this comment.
When the GT911 queue has already dropped the UP for s_stop_press, the surviving events are processed by the preceding while (touch_wait_event(...)) loop before this check clears ownership. A later contact reusing the ID can therefore start elsewhere and lift over STOP during that drain, satisfying the stale release branch and aborting the run. Fresh evidence beyond the earlier missing-propagation report is that the new propagation check is positioned after the entire drain; check before processing survivors, while retaining a post-drain check for concurrent losses.
Useful? React with 👍 / 👎.
| if (dropped && stale.type == TOUCH_EVENT_UP | ||
| && atomic_load(&s_stop_counted) | ||
| && stale.point.id == atomic_load(&s_stop_counted_id)) { | ||
| atomic_store(&s_stop_counted, false); |
There was a problem hiding this comment.
When the full render queue contains an older ordinary UP with the same recycled GT911 track ID ahead of the marked STOP release, a subsequent overflow evicts that older UP and this comparison clears s_stop_counted even though the actual STOP release remains queued. The renderer later consumes that release and raises the backstop, causing service_arming() to count and apply the same stop again, including cancelling a hold begun after the real stop. Fresh evidence beyond the prior unconditional-clear issue is that the replacement association uses only the reusable 8-bit contact ID rather than a unique event sequence.
Useful? React with 👍 / 👎.
| const bool dropped = | ||
| (xQueueReceive(s_touch_q, &stale, 0) == pdTRUE); | ||
| if (dropped) { | ||
| routed = (xQueueSend(s_touch_q, &evt, 0) == pdTRUE); | ||
| } |
There was a problem hiding this comment.
When the render task empties s_touch_q after the initial send reports full but before xQueueReceive(), dropped is false and this conditional skips the retry even though the queue now has space. The incoming event is consequently discarded and !routed records a loss, cancelling an otherwise intact gesture and potentially interrupting a run. Fresh evidence beyond the earlier atomic-eviction report is that the revised code still gates the retry on a successful eviction; retry the send after the receive attempt regardless of whether an old entry was removed.
Useful? React with 👍 / 👎.
Three findings. The STOP marker named the release it refers to by track id. A GT911 id is eight bits and the controller reuses them, so an older ordinary release carrying a recycled id, ahead of the marked one in the queue, cleared the marker when it was evicted -- and the marked release was still queued. The router then raised its request, the marker read false, and the stop was applied a second time for one press. The queue now carries a sequence alongside each event, unique for the life of the run and written by the one task that sends, and the marker names that. The retry after an eviction was gated on the eviction taking something. The render task can empty this queue between the failed send and the receive, in which case there is room without anything having been evicted; the incoming event was then discarded for no reason, and counted as a loss. It is retried either way. And the control task answered a driver loss only after draining the events that loss left behind. Those events were captured around the one that went missing, so a contact reusing the track id of a STOP press whose release the driver dropped could lift over STOP inside that very loop and abort a run. The check runs before the drain as well as after, which is the same shape the render loop already has.
|
All three addressed in the push above. P2 — associate STOP markers with a unique queued event. Confirmed, and this is the third iteration on that marker, which is a signal in itself. A GT911 track id is eight bits and the controller reuses them, so an older ordinary release carrying a recycled id — sitting ahead of the marked one in the queue — cleared the marker when it was evicted, while the marked release was still queued. The router then raised its request, the marker read false, and the stop was applied a second time for one press. The queue now carries a P2 — retry the incoming event after a concurrent drain. Confirmed. The retry was gated on the eviction having taken something, so if the render task emptied the queue between the failed send and the receive there was room without anything evicted — and the incoming event was discarded anyway and counted as a loss, which then cancelled an intact gesture. It is retried either way now, and the loss counted only when an event actually went. P2 — check driver loss before draining surviving touches. Confirmed; the same before/after shape the render loop already got two rounds ago, and I missed applying it here. The control task answered a driver loss only after draining the events that loss left behind — and those events were captured around the missing one, so a contact reusing the track id of a STOP press whose release the driver dropped could lift over STOP inside that very loop and abort a run. Suite 44/44, coverage 95.9%, docs, screenshot and sanitizer checks clean; CI dispatched on the new head. @codex review |
A tab row holds a press between DOWN and UP and switches the pane on the release. A cancel left that press latched with its track id, which the controller reuses, so a later contact that began elsewhere and lifted over the tab switched the pane. ui_tabs_cancel() drops the press; MOTOR & ESC calls it with the rest of its cancellation, and ANALYSER and BALANCE, whose tab row is the one control that holds state, gain a cancel of their own.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6509b2da6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const ui_screen_t *scr = screen_for(s.current); | ||
| if (scr != NULL && scr->cancel != NULL) { | ||
| scr->cancel(); |
There was a problem hiding this comment.
If touch loss is recorded concurrently with the queue drain and a later surviving HOME release navigates before the post-drain loss check, this cancels only the newly current overview screen. Gesture state on the screen that processed the events remains stale; for example, ANALYSER and BALANCE have no leave()/enter() reset, so a tab DOWN whose UP was lost can survive navigation and a later recycled-ID UP can switch its pane. Cancel every screen's gesture state on a global stream loss, or retain and cancel each screen active during the drain.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. ui_router_cancel_gestures() now tells every screen, not the one on top: a screen off the top keeps its gesture state until it is entered again, and cancelling one with no gesture in progress asks for nothing. Test in test_nav: a tab DOWN on ANALYSER, a navigation to the overview, the cancel, a return, and an UP with the same id over the tab leaves the pane as it was; it fails against a cancel that reaches only the current screen.
| const unsigned lost_now = touch_losses(); | ||
| if (lost_now != lost_seen) { | ||
| lost_seen = lost_now; | ||
| ui_router_cancel_gestures(); |
There was a problem hiding this comment.
If a loss is first observed here while an armed MOTOR or SERVO ARM/DISARM press is active, ui_router_cancel_gestures() posts the fail-safe DISARM after this frame's only flush_screen_commands() call at main.c:4210. The request is therefore not transferred to s_disarm_request until the next render iteration; because queue loss is specifically associated with a stalled renderer, drawing or display_flip() can leave the output driving for an unbounded extra frame instead of dropping the heartbeat promptly. Fresh evidence beyond the earlier preserve-DISARM report is that the callback now creates the command, but this post-drain call occurs too late to forward it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. The post-drain cancellation is followed by flush_screen_commands(stops_now) in the same frame, so a DISARM it posts reaches s_disarm_request before the render and the flip. Nothing else can be pending at that point: this frame's flush took the events, the cancellation drops any ARM, and the tick has not run, so the ordering the pre-drain cancellation relies on is unchanged.
A loss is observed after the frame's events were dispatched, and one of them can have navigated, so the screen holding the press is no longer the one on top. The router tells every screen. The DISARM a bench screen posts from that cancellation is flushed in the same frame rather than behind the render and the flip.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: caacba8994
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (scr != NULL && scr->cancel != NULL) { | ||
| scr->cancel(); |
There was a problem hiding this comment.
Although the overview and output screens now implement cancellation, log_viewer_screen() still leaves cancel NULL even though its browse, import, and plot handlers retain press_btn/pressing across DOWN and act on the next UP. If a known queue loss occurs after a button DOWN, this loop skips that screen, so a surviving or later UP can refresh the card, open a file, change import settings, or load data without a corresponding press. Clear the log viewer's pending button/row/drag state during global cancellation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. The log viewer gains a cancel that drops the press, the row and the drag; it was the one screen with press state and no cancel, checked across every screen in shared/ui. Test in test_logview: a selected row, a DOWN on it, the cancel, then the UP leaves the view at BROWSE where a second tap would have opened the file, and a fresh tap still opens; it fails against a cancel that drops nothing. Safety.md and the CHANGELOG entry name the log viewer in the list.
Every view of the log viewer acts on the release: a button refreshes the card, opens a file or changes an import setting, and a row selects or opens. A press left standing after a lost event let a later contact's release do any of those. The screen gains a cancel that drops the press.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
1 similar comment
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7d2e72b11
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| */ | ||
| static void cancel(void) | ||
| { | ||
| s.have_press = false; |
There was a problem hiding this comment.
When touch loss is detected after a tile DOWN, this clears ownership but leaves s.pressed pointing at the tile and does not invalidate s.drawn_mask. If cancellation happens before that frame renders, the tile is painted as pressed; if it happens afterward, the cached pressed rendering is retained. In either case the overview can show a permanently held tile with no contact until another event invalidates it, so cancellation should also reset s.pressed and redraw the overview.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. The overview's cancel now clears the pressed tile and invalidates the cached render as well as dropping the press's ownership. The existing test_nav case renders after the cancel and checks the frame equals the idle one; it fails against a cancel that leaves the tile pressed.
The overview's cancel dropped the press's ownership and left the tile drawn pressed, with no contact left to release it. The cancel clears the pressed tile and invalidates the cached render.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8dfca6698
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (s.pending.kind == MOTOR_CMD_ARM) { | ||
| s.pending.kind = MOTOR_CMD_NONE; | ||
| } |
There was a problem hiding this comment.
When touch loss first occurs after app_main() flushes the previous frame's pending ARM at main.c:4210 but before the post-drain loss check, this block cannot retract the now-queued command, and s.armed can still be false because the frame snapshot was taken before the control task processed it. Cancellation therefore neither clears the queued ARM nor posts a DISARM/advances s_lets_go, allowing the control task to arm after the initiating gesture has been abandoned. The equivalent logic in servo_screen.c::cancel() has the same window; cancellation needs to invalidate an ARM that may already be in flight, not only one still in the screen's local slot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, for both screens. The render side cannot retract what it has handed over, so the check moves to the side that can: every command now carries the count of lost touch events it was posted under (touch_lost, from touch_losses(), which both tasks read), and the control task drops a MOTOR or SERVO ARM whose count has moved since it was queued. Nothing is predicted about the other task; the arm and the loss are compared on the same counter. Only an arm is dropped: a loss makes no throttle or position suspect, and a disarm is never dropped. The window between the flush and the post-drain check is closed by this rather than narrowed, since the comparison happens at the taking.
The render side cancels only what it has not yet handed over. An arm already queued carries the count of lost touch events it was posted under; the control task compares it with the count both tasks read and drops an arm whose count has moved. Only an arm: a loss makes no throttle or position suspect, and a disarm is never dropped.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
1 similar comment
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 653d538348
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (arms && pc.touch_lost != touch_losses()) { | ||
| continue; |
There was a problem hiding this comment.
When the GT911 task drops an event concurrently, this comparison can read the old loss count and then accept the ARM before the other core increments s_touch.lost; the same race exists if a loss begins immediately after the comparison. If the render loop observes that increment before the control task publishes its newly armed snapshot, cancellation still sees the screen as disarmed, consumes the loss, and posts no DISARM, after which the control task can publish the accepted arm with no further cancellation pending. Fresh evidence beyond the prior forwarded-ARM finding is that the added generation check is itself a TOCTOU check against the independently running touch task. Synchronize loss publication with validation/application, or recheck after applying and fail safe when the count moved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. The check at the taking cannot be atomic with an arm that is two exchanges away, so the count is looked at twice: the control task keeps the count the accepted arm carried and compares it again after outputs_arm(true), in both the linked and the unlinked branch. A count that moved disarms at once through disarm_here(), the same path a DISARM command takes, with an alert. A loss before the first look drops the arm, a loss between the looks disarms, and a loss after the second look is seen by the render side against an armed bench and answered as a disarm there. Safety.md and the CHANGELOG entry say so.
The check when the arm is taken from the queue and the arm itself are up to two seconds apart, and the touch task counts on its own clock. A loss in that gap is answered by nobody: the render side holds nothing for a bench its snapshot still calls disarmed. The control task keeps the count the arm carried and compares it again after the bank arms; a count that moved disarms through the same path a DISARM takes.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 202fc3a4c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .stops = stops_now, | ||
| .lets_go = atomic_load(&s_lets_go) }; | ||
| .lets_go = atomic_load(&s_lets_go), | ||
| .touch_lost = touch_losses() }; |
There was a problem hiding this comment.
If an ARM remains pending from the previous frame and a touch event is lost after the pre-drain check at main.c:4195 but before this flush, this line stamps the already-posted command with the new loss count. The flush removes ARM from the screen before the post-drain cancellation at main.c:4396, and the control-side generation check consequently accepts it because the stamped count still matches. Fresh evidence after the earlier thread response is that the generation is sampled here in flush_screen_commands(), not when motor_screen.c::post() or servo_screen.c::post() actually records ARM, so the stated cancellation of an in-flight ARM is still bypassed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. The stamp is now the loss count the frame's cancellation last acted on (lost_seen), passed into flush_screen_commands(), rather than touch_losses() at the flush. A loss between the pre-drain check and the flush leaves the stamp behind the count the control task reads, so the arm is dropped there; a loss before the check is answered by the cancellation, which drops the pending arm before any flush; a loss after the flush is answered by the post-drain check and by the same stamp comparison at the taking.
A loss between the frame's cancellation and its flush is one no cancellation has answered. Stamped with the count at the flush, an arm posted the frame before would carry that loss past the control task's check; stamped with the count the cancellation last acted on, it fails the check and is dropped.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 684a86a9dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (touch_losses() == s_arm_touch_lost) { | ||
| return; | ||
| } | ||
| (void)disarm_here(link_up); |
There was a problem hiding this comment.
When a touch loss lands immediately after this comparison but before publish_snapshot() at line 3934, the renderer can consume the loss while its snapshot still says disarmed, so the screen cancellation posts no DISARM; the control task then publishes the already-armed bank, with no unobserved loss left to trigger another cancellation. Fresh evidence beyond the prior thread response is that the added post-outputs_arm() check still precedes snapshot publication and therefore leaves this final race window. Publish the armed transition before the final validation or synchronize loss observation through the publication step.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. The second look now runs after publish_snapshot(), in the same control pass, on a flag the arm act sets. The snapshot the screens read turns armed at that publication, so a loss before the look is answered here by a disarm and a loss after it is seen by the screens against an armed bench; no loss falls between the two. Safety.md and the CHANGELOG entry say so.
The render side's snapshot turns armed at publish_snapshot(). A loss between the arm and that publication was answered by nobody: the control task had looked already, and the screens cancelled against a bench they still called disarmed. The second look now follows the publication, so a loss before it disarms here and a loss after it is seen against an armed bench.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
2 participants
The defect
firmware/panel/main/main.cforwards touch events to the render loop with anon-blocking send:
A full 32-entry queue refuses the new event. Every other queue in the same
file evicts the oldest, each with a comment saying why:
gt911/touch.c:80-87main.c:3548-3553main.c:3401-3404The last event of a gesture is its release. A release the screen never sees
leaves
s.pressed == 1with no finger on the glass, andui_hold_tick()completes the hold on the frame timer — ARM goes out with nothing on the
panel. The screen then also refuses DISARM until a contact arrives carrying
the same track id, and consumes the first such tap as the phantom's release.
The touch-health watchdog does not help: the controller is answering normally,
so
arming_touch_deadis false. STOP is unaffected — the control taskhit-tests the band itself, independent of the render loop.
What it takes. A still finger emits nothing (
touch_tracker_updateemits aMOVE only on a coordinate change), so a dead-still finger cannot fill 32 slots
by itself. It needs coordinate wobble on the held contact, or a second contact —
a palm or the other hand resting on the glass multiplies the event rate by up to
TOUCH_MAX_POINTSand fills the queue in about 90 ms of undrained frame.Two fixes
1. Evict the oldest, and count it. The same pattern the command queue uses.
The frame log now carries the count beside DRAW and WAIT:
A nonzero
TOUCHEVICTon a bench is this condition occurring. Nothing measurespanel frame time today, so that number is the cheapest way to learn whether the
render loop ever falls far enough behind for it to matter.
2. Cap what one frame can credit to a hold.
dt_sis measured at the top ofthe frame and applied at
ui_router_tick()at the end of it — after the touchqueue is drained. So a late frame credits its own full duration to a hold that
began while that same frame was dispatching the press. A 2.5 s frame arms on a
press milliseconds old.
UI_HOLD_MAX_CREDIT_Sis 250 ms, so a hold spans at least eight frames with thepress standing. An ordinary frame is 26 ms, far below the cap.
Tests
test_widgets, 33 cases, all passing. New:one_late_frame_cannot_complete_a_hold— eight 2.5 s frames to fire ratherthan one; a frame of zero or negative time credits nothing. Fails without
the cap (verified by reverting
ui_widgets.calone).Three existing cases fired a hold with a single 2 s tick, which is the shape the
cap forbids; they now advance in frame-sized steps through a helper.
What is not covered. The queue itself is in
firmware/panel/main/main.c,which is outside the host suite, so the eviction is not under test. The half
that is testable — the hold's per-frame credit — is. Neither has run on a board.
Unsettled, and it is the whole reachability question: whether an
app_mainframe on the motor screen ever runs 90-400 ms on the real board.
TOUCHEVICTanswers it directly the next time the panel is powered.
docs/Safety.mdanddocs/Safety-de.mdrecord both behaviours under thedeliberate list. Coverage 95.8%, unchanged floor.