Make HID settings writes nonblocking#89
Conversation
abdd668 to
c777b48
Compare
kissetfall
left a comment
There was a problem hiding this comment.
Thanks for moving module/touchpad settings writes off the UI thread. The FIFO direction is sound, but the shared HID lifecycle still has several correctness issues that need to be fixed before merge:
-
settings_write_tasktakesself.hid_device, while the existing macro/combo/combo-term/tap-dance lifecycle continues to run later in the same update. With the handle temporarily absent, macro saving clearsmacros_dirtywith an error, and combo/combo-term saving can leave its success flag true, clear the dirty state, and report “Saved” without writing anything. Please coordinate every existing HID writer with the owning transport lifecycle so temporary queue ownership defers work without clearing dirty state or emitting false success. Add lifecycle tests for dirty macro/combo data while a settings write is in flight. -
qmk_setting_transport_available()treats an activelayer_write_taskas available, so module/touchpad requests can be queued behind it. If the layer write finishes with a disconnect or worker failure, no HID handle is restored and the settings request remains pending forever.qmk_settings_write_busy()then blocks scanning, device switching, and reconnect. Please provide an explicit successful handoff or terminal failure/cancellation path for every handle-loss case, and prove that the queue always leaves the busy state after a layer-write disconnect. -
ModuleSettingWritebackError::ReadbackMismatchcontains the actual firmware value, butfinish_settings_write()discards it. Module state remains at the old value and touchpad state remains at the optimistic requested value, so the UI can disagree with the device even though the real readback is known. Please reconcile firmware-backed state with the actual readback and add regression coverage for both module and touchpad targets. -
draw_settings_write_status()always installs its hover tooltip. Please pass through the settings list'ssuppress_tooltipsstate so status tooltips are hidden while scrolling or dragging, consistently with the rest of the page.
The current three queue tests cover FIFO/status bookkeeping only. Please add app-lifecycle coverage for the cross-owner cases above. Afterward, validate on both USB and Bluetooth: settings writes remain nonblocking, dirty macro/combo work is preserved, disconnect cannot wedge the app, and UI values match firmware readback.
|
Addressed in 5056858:
|
kissetfall
left a comment
There was a problem hiding this comment.
Thanks — head 5056858 addresses the four previously reported issues on its old base: dirty synchronous writers are gated, queued settings terminate after transport loss, readback mismatch reconciles module/touchpad state, and status tooltips respect suppression.
The PR is not mergeable after #82: GitHub reports CONFLICTING, and the real merge conflicts in eight lifecycle files (app_init, app_state, app_lifecycle, device_connect_task, device_connection, layer_operations, layout_device_dropdown, and layout_top_tabs). These are not mechanical conflicts: #82 made Combo writes a background HID owner and added close/exit chaining, so the two parallel lifecycle implementations must be unified before this can merge.
Blocking requirements for the rebase:
- Keep one production HID ownership contract for Layer, Combo, and Settings tasks. A module/touchpad request made while Combo owns the handle must remain pending, not fail as “device not connected”; Combo and synchronous writers must likewise defer while Settings owns it. Device scan/switch, Refresh Device Data, imports, undo, and every real close path must use the same owner/busy state.
- Preserve the #82 exit guarantee when Settings has an active task or a queued request behind another owner: chain all pending HID work, return the real handle, flush exit writes once, then close. Do not let a queue-only state fall through as idle.
- Replace
start_settings_write_for_test()coverage with the real test HID backend and production worker result. Its sender is dropped immediately and it does not prove handle return or post-handoff writes. Add a lifecycle regression that starts a real Settings task, marks Macro/Combo/Combo Timeout/Tap Dance/Key Override/Tap Hold pending, proves no competing request while Settings owns HID, then proves each deferred write executes exactly once after return. Also exercise a real Layer-task disconnect and show the queued Settings state leaves busy terminally.
Please rebase on current main (including #91 and #82), resolve the ownership path in the owning abstraction rather than keeping duplicate hid_write_task_active() implementations, and rerun the full merge lifecycle suite.
5056858 to
6008a00
Compare
|
Rebased on current main and unified HID ownership at 6008a00.
|
kissetfall
left a comment
There was a problem hiding this comment.
Head 6008a00 fixes the earlier rebase conflicts and most of the old-base issues, and the local suite passes 247/247. Two lifecycle blockers remain before this is safe to merge:
-
The shared owner still has no explicit Combo -> Settings handoff.
poll_settings_write()runs beforepoll_combo_write()in the background update; when Combo returns the HID handle,poll_combo_write()only finishes Combo and does not start the queued Settings request. The request therefore waits for an unrelated later frame (up to the normal 250 ms cadence while visible and 5 s while hidden to tray). The Layer completion path has the same missing start in this PR. Please start the next owner at the completion boundary and add real-HID coverage for Settings queued behind both Combo and Layer, rather than only the opposite direction. -
Deferred exit is not terminal on transport/worker failure. If Settings loses the HID handle while Macro/Combo/Combo Timeout/Tap Dance/Key Override/Tap Hold work is dirty,
finish_deferred_exit_after_hid_write()keeps seeing those flags and repainting, while the synchronous writers cannot clear them without a handle. The window can remain stuck in a cancelled-close loop. Please define the failure policy (preserve/report the unwritten state, then allow close) and add a production-worker close regression for disconnect and channel failure, with exactly-once assertions for writes completed before failure.
The four platform build checks are green, i18n and diff checks pass, and cargo clippy completes. This review is specifically blocking on lifecycle handoff and terminal exit behavior.
|
Addressed:
|
kissetfall
left a comment
There was a problem hiding this comment.
Head 84790d4 resolves the two remaining lifecycle blockers. Combo and Layer now hand queued Settings work off at their completion boundary in the same frame, including transport-loss paths. Settings disconnect and worker-channel failure terminate the queue, preserve the other dirty HID state, clear deferred-exit ownership, and emit one final Close instead of looping indefinitely. The new failure-injectable test HID covers both terminal paths and verifies exactly-once completed Combo/Settings requests.
Local verification passed 251/251 tests, Clippy, i18n, diff-check, and a release build. All four hosted platform builds are green, the head stayed stable, and the PR is merge-clean. The changed Settings controls continue to use the existing Entropy design-language helpers; no new UI-system violation found.
EN
Problem
Module and touchpad controls performed QMK HID writes synchronously inside UI callbacks. A slow write, retry, or readback could block Entropy for hundreds of milliseconds or longer and leave no clear indication whether firmware saved the value.
Fix
Debouncing and coalescing rapid slider writes remain separate follow-up work.
Verification
cargo test --locked: 194 passed.cargo test settings_write --locked: 3 passed.cargo clippy --locked --all-targets: passed with existing repository warnings.python3 scripts/check_i18n.py: passed.rustfmtandgit 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-запись синхронно внутри UI-callback. Медленная запись, повтор или контрольное чтение могли блокировать Entropy на сотни миллисекунд или дольше, при этом интерфейс не показывал, сохранила ли прошивка значение.
Исправление
Debounce и объединение быстрых изменений слайдера остаются отдельной следующей задачей.
Проверка
cargo test --locked: прошли 194 теста.cargo test settings_write --locked: прошли 3 теста.cargo clippy --locked --all-targets: прошел с существующими предупреждениями репозитория.python3 scripts/check_i18n.py: прошел.rustfmtиgit diff --check: прошли.TARGET=aarch64-apple-darwin scripts/build_macos_app.sh: собраны arm64-приложение, DMG и ZIP; plist и подпись прошли проверку.