Debounce rapid QMK settings writes#90
Conversation
d9c98cc to
c961c7c
Compare
kissetfall
left a comment
There was a problem hiding this comment.
The debounce scheduler itself looks sound, but the surrounding write lifecycle is not safe to merge yet.
Required changes:
- Rebase/integrate this scheduler after #89 is corrected and route debounced writes through the single background HID worker. This PR currently keeps synchronous HID set/readback operations in the UI lifecycle and substantially overlaps with #89.
- Do not silently drop a pending value when HID is temporarily unavailable. A layer write can take the HID handle before the 180 ms deadline; the pending request is then removed, the write is skipped, and the UI can diverge from firmware. Pending work must be preserved/retried or explicitly cancelled with the UI restored to the confirmed firmware value. The same rule must cover disconnect.
- Add lifecycle/regression coverage for a delayed setting colliding with a layer write, disconnect/reconnect, device switch, and application shutdown. The current unit tests validate scheduler bookkeeping but not HID ownership or handoff.
Please also verify the integrated path over both USB and Bluetooth.
c961c7c to
11ca324
Compare
|
Addressed in commit 11ca324 after rebasing.
|
kissetfall
left a comment
There was a problem hiding this comment.
Thanks — the 11ca324 update addresses the previous debounce-specific blockers on its old stack: delayed slider values are handed to the background Settings worker, unavailable transport restores the confirmed value, device switching is deferred, and stale worker/readback generations are guarded.
This cannot merge on current main. The branch still contains the blocked #89 stack, GitHub reports CONFLICTING, and the real merge conflicts in eight lifecycle files, including app_lifecycle, device_connect_task, device_connection, layer_operations, and window_lifecycle.
Blocking integration work:
- Rebase after #89 is corrected on current main (#91 + #82). The debounce scheduler must feed the single Layer/Combo/Settings HID owner. If Combo temporarily owns HID when a 180 ms value becomes due, keep the newest value pending; do not classify the missing handle as a disconnected device or restore the old UI value.
- Remove the parallel Settings-only close state/path.
pending_close_after_settings_writeand the rewrittenhandle_close_to_tray()conflict with #82's production exit chain and would bypass its Combo, Tap Hold, and display-fallback exactly-once guarantees. Pending debounce requests and active Settings writes must participate in the existing shared exit deferral, which drains every HID owner before the final close. - Apply the same shared busy contract to device switch, Refresh Device Data, imports, undo, disconnect, and reconnect. A pending debounce value, a queued Settings request, and an active worker must all transition deterministically through handoff or terminal cancellation.
- Replace the synthetic
start_settings_write_for_test()cases with the real test HID backend. The helper drops its sender immediately and the tests do not prove successful handle return or firmware requests. Add lifecycle coverage for Debounce → Combo/Layer collision, device switch, disconnect/reconnect, and normal close, asserting that only the newest value is written and read back exactly once.
Please keep the scheduler/coalescing logic, but integrate it only after #89 has one current-main HID lifecycle owner.
11ca324 to
1add7df
Compare
|
Updated in 1add7df:
|
kissetfall
left a comment
There was a problem hiding this comment.
Head 1add7df is rebased and the four release-build checks are green, but it is not mergeable yet:
-
The local test target does not compile.
cargo test --all-targetsfails insrc/ui/qmk_settings_write_queue.rs:309-310: the sibling test module calls privateSettingsWriteQueueState::status()and refers toSettingsWriteStatuswithout importing/exposing it (E0624,E0433). CI currently runs release builds only, so it misses this. Please make the regression compile and add tests to CI. -
The debounce fallback is not guaranteed to be a confirmed firmware value. If 8 -> 10 is already in the Settings worker and the user moves 10 -> 12, the pending debounce records optimistic 10 as
old_value. If the active worker then disconnects,finish_settings_write()suppresses reconciliation because a newer debounce exists; the no-transport path later restores 10 even though firmware may still contain 8. Track the last confirmed/read-back value across the active worker and add a mid-worker disconnect regression with a newer debounced value. -
This stack still inherits #89's missing Combo -> Settings completion handoff and non-terminal deferred-exit failure path.
poll_combo_write()returns the handle but only resumes a pending device connect; it does not start the Settings request queued behind Combo. A Settings disconnect during close can also leave other dirty HID writes holding the cancelled close indefinitely. Please fix these in #89 first, then keep #90's scheduler on that corrected owner.
cargo clippy, i18n, and diff checks complete successfully. The hard local failure is the test compile; the lifecycle cases above remain functional blockers.
1add7df to
0569d51
Compare
|
Addressed:
|
kissetfall
left a comment
There was a problem hiding this comment.
Re-reviewed head 0569d51. The previous blockers are addressed: the test target compiles and Linux CI runs cargo test --all-targets; rollback now tracks the last verified readback across an active worker; Combo/Layer completion continues queued Settings work; and close flushes debounce requests through the shared HID lifecycle.
The new regressions cover a newer debounced value during worker disconnect, successful and mismatched readback baselines, Combo handoff with only the newest value, reconnect, and debounce arriving after close was requested. Local verification passed 265/265 tests, Clippy, scoped rustfmt, i18n, and diff-check. All four hosted builds are green.
Approved.
EN
Problem
Touchpad sensitivity sliders sent a QMK HID command for every UI change while dragging. Slow USB or Bluetooth transports could receive a burst of stale writes for the same QSID, increasing latency and making the final firmware value less predictable.
Fix
This PR complements #89; if #89 merges first, this scheduler will be rebased onto its background HID worker so debounce remains separate from transport threading.
Verification
cargo test --locked: 194 passed.cargo test qmk_settings_write_queue --locked: 3 passed.cargo clippy --locked --all-targets: passed with existing repository warnings.python3 scripts/check_i18n.py: passed.rustfmt --checkandgit diff --check: passed.TARGET=aarch64-apple-darwin scripts/build_macos_app.sh: arm64 app, DMG, and ZIP built; plist and code-signature validation passed.RU
Проблема
Слайдеры чувствительности тачпада отправляли QMK HID-команду при каждом изменении во время перетаскивания. Медленный USB- или Bluetooth-транспорт мог получить очередь устаревших записей одного QSID, что увеличивало задержку и делало итоговое значение в прошивке менее предсказуемым.
Исправление
PR намеренно дополняет #89; если #89 будет смержен первым, планировщик будет изменен на его фоновый HID-worker, чтобы debounce оставался отдельным от transport threading.
Проверка
cargo test --locked: прошли 194 теста.cargo test qmk_settings_write_queue --locked: прошли 3 теста.cargo clippy --locked --all-targets: прошел с существующими предупреждениями репозитория.python3 scripts/check_i18n.py: прошел.rustfmt --checkиgit diff --check: прошли.TARGET=aarch64-apple-darwin scripts/build_macos_app.sh: собраны arm64-приложение, DMG и ZIP; plist и подпись прошли проверку.