Refresh module settings for diagnostics

[IgorArkhipov]

EN

Problem

Entropy read left/right module settings only during connection. After a physical key changed controller mode, the app could not show whether persistent QMK values changed or whether the issue existed only in firmware runtime state.

Fix

• Add a Saved values / Refresh row to Modules settings. It re-reads every unique module QSID without reconnecting and updates displayed values only after successful reads.
• Record a before/after diagnostic entry for each QSID with original firmware group and field labels, width, value, and failure details.
• Mark snapshots as persistent QMK data with runtime mode unobservable, so unchanged values do not imply that firmware runtime state is healthy.
• Stop issuing further HID reads after the first failure to avoid multiplying one device timeout across every module setting.

Verification

• cargo test --locked: 193 passed.
• cargo test module_setting_: 7 passed.
• cargo clippy --locked --all-targets: passed with existing repository warnings.
• python3 scripts/check_i18n.py: passed.
• Scoped rustfmt --check and git diff --check: passed.
• TARGET=aarch64-apple-darwin scripts/build_macos_app.sh: arm64 app, DMG, and ZIP built; plist validation passed.

Hardware validation

Enable Diagnostics, open Modules settings, and refresh once for a baseline. Switch the right controller Normal → Scroll → Normal with its keyboard key while alternating right-pointer and left-scroll use, then refresh again. Logs should either identify changed persistent QSIDs or report changed=0 with runtime_mode_observable=false, narrowing the left-scroll dropout to runtime firmware behavior.

Repository-wide cargo fmt -- --check still reports pre-existing formatting baseline outside this change.

RU

Проблема

Entropy читала настройки левого и правого модулей только при подключении. После переключения режима контроллера физической клавишей приложение не могло показать, изменились ли сохраненные значения QMK или проблема существует только во временном состоянии прошивки.

Исправление

• На странице настроек модулей добавлена строка Сохраненные значения / Обновить. Она повторно читает каждый уникальный QSID без переподключения и обновляет показанные значения только после успешного чтения.
• Для каждого QSID в диагностику записываются значения до и после обновления, исходные названия группы и поля из прошивки, ширина значения и ошибки чтения.
• Снимок явно помечается как сохраненные данные QMK, а runtime-режим как недоступный для наблюдения. Поэтому отсутствие изменений не означает, что временное состояние прошивки исправно.
• После первой ошибки новые HID-чтения не отправляются, чтобы один timeout устройства не повторялся для каждой настройки модуля.

Проверка

• cargo test --locked: прошли 193 теста.
• cargo test module_setting_: прошли 7 тестов.
• cargo clippy --locked --all-targets: прошел с существующими предупреждениями репозитория.
• python3 scripts/check_i18n.py: прошел.
• Scoped rustfmt --check и git diff --check: прошли.
• TARGET=aarch64-apple-darwin scripts/build_macos_app.sh: собраны arm64-приложение, DMG и ZIP; plist прошел проверку.

Проверка на устройстве

Включить диагностику, открыть настройки модулей и один раз обновить значения для baseline. Переключить правый контроллер Normal → Scroll → Normal назначенной клавишей, чередуя управление указателем справа и прокрутку слева, затем обновить значения снова. Логи должны либо показать изменившиеся сохраненные QSID, либо changed=0 вместе с runtime_mode_observable=false, что сузит причину пропадания левой прокрутки до runtime-поведения прошивки.

Общий cargo fmt -- --check по репозиторию по-прежнему сообщает о ранее существовавшем formatting baseline вне этого изменения.

[kissetfall]

Thanks for adding the refresh diagnostics. I found three issues that need to be addressed before this can merge:

1. The Refresh click handler performs get_qmk_setting_u8/u16 synchronously on the egui/UI thread. The USB path can spend up to 20 read attempts with timeouts and retry delays, so a failed device can freeze the entire UI for roughly 20 seconds. Please move the refresh operation into the app's owning background HID task/queue, expose an in-progress state to prevent duplicate refreshes, and only apply a result if it still belongs to the currently connected device. Add lifecycle coverage proving that clicking Refresh schedules background work instead of doing HID I/O inline, including disconnect/device-change handling.

2. After the first real read failure, all remaining QSIDs are returned as Err("skipped ..."), but failed_count() counts those skipped reads as device failures. That makes the status and diagnostic snapshot misleading. Please model success, failed, and skipped as distinct outcomes and report/count them separately. Add a regression test where the first QSID fails and multiple later QSIDs are skipped; it should report exactly one attempted failure and the correct skipped count.

3. The Refresh button's hover tooltip is always installed even while suppress_tooltips is true. Please suppress it consistently with the row-label tooltip while the settings list is scrolling or being dragged.

After these changes, please validate refresh on both USB and Bluetooth: the UI must remain responsive, a second refresh must show changed persistent values, and the diagnostic log must distinguish actual read failures from skipped QSIDs.

[IgorArkhipov]

Addressed all three issues:

• module-setting refresh now transfers HID ownership to a background task, prevents duplicate refreshes, and rejects results after disconnect or device change.
• failed and skipped QSID reads are modeled and reported separately, and the Refresh tooltip now respects tooltip suppression.
• added regression and lifecycle coverage; all 227 local tests and all hosted CI builds pass.
• Physical validation remains pending and will be reported separately.

[kissetfall]

Thanks — f4a4634 correctly moves the refresh reads off the UI thread, separates failed and skipped results, and suppresses the button tooltip while scrolling. Those original issues are resolved.

The new background task still has lifecycle blockers before this can merge:

1. module_settings_refresh_task is a separate HID owner rather than part of one shared HID lifecycle. It takes self.hid_device, but the existing Macro, Combo, Combo Timeout, Tap Hold, Tap Dance, and pending-import paths continue to run. With the handle temporarily absent, those paths can consume dirty or pending state, report a false Saved status, or lose a firmware write. Please serialize all HID operations through one owning abstraction, or defer every competing path without consuming its state until the handle returns.

2. Device switching, start_connect, and Refresh Device Data still gate only on layer_write_task. During a module refresh they can drop the refresh receiver and open a second handle while the first worker is still using the device. Refresh Device Data can also clear the cache before canceling the refresh. Please include this task in the shared connect, reconnect, cache-refresh, device-switch, undo, scan, and import lifecycle, or implement a safe cancellation and handoff contract.

3. A read error is converted to String and the worker always returns HidDevice. A physical disconnect is therefore restored as an apparently connected handle instead of returning no handle and entering the disconnected lifecycle. Preserve disconnect classification and do not restore a disconnected device.

4. The new tests cover a generic spawned closure, boolean start checks, and an identity helper, but not the production EntropyApp lifecycle. Please add regression coverage through the actual production path for competing dirty writers, disconnect, device change, reconnect, stale results, and exactly-once processing after handle return.

After this is integrated with the single HID owner, please repeat the USB and Bluetooth validation from the previous review.

[IgorArkhipov]

Module refresh now owns HID access while competing writes, scans, device switches, cache refresh, undo, and imports defer.

Disconnects clear connection state, stale results are discarded, and new EntropyApp tests cover competing writers, device refresh, disconnect, and stale results.

Local tests and all CI builds are green.

[kissetfall]

Thanks — f13937d fixes the main in-session serialization issues from the previous review. Competing Macro/Combo/Tap Hold writes now wait while refresh owns HID, device switch/reconnect/undo/cache refresh are gated, and a disconnect no longer restores an invalid handle.

There are still lifecycle blockers before merge:

1. on_exit does not participate in the HID handoff. If the app closes while module refresh owns the handle, flush_pending_tap_hold_numeric_writes() drains pending values with no device available, and the display-preset fallback also cannot reach firmware. Please finish or safely cancel/join the active HID task before exit writes, without consuming pending state prematurely.

2. The disconnect path still loses deferred user changes. finish_module_settings_refresh() calls clear_connected_keyboard_state() when the worker returns no handle, which clears pending Tap Hold values; the subsequent reconnect path resets dirty Macro/Combo state. Please preserve pending/dirty state through disconnect/reconnect, or define an explicit rollback/retry contract that cannot silently discard it.

3. The new lifecycle test uses a synthetic hid_device: None result and then verifies only a local Key Override application. It does not cover a successful handle return, actual exactly-once firmware writes after handoff, disconnect/reconnect preservation, or app exit. Please add production-path regression coverage for those cases.

4. The disconnect status in finish_module_settings_refresh() is hardcoded English. Please route it through i18n.

After these changes, please repeat the USB and Bluetooth validation with refresh concurrent with pending Macro/Combo/Tap Hold state, physical disconnect/reconnect, and application close.

[IgorArkhipov]

• Exit now cancels close while module refresh owns HID, then flushes pending Tap Hold/display writes only after the handle returns.
• Dirty Macro, Combo, Combo Timeout, Tap Dance, and pending Tap Hold state is preserved on refresh disconnect and restored only for the same device after reconnect.
• Added i18n for the refresh-disconnect message.
• Added production-lifecycle coverage using a test HID backend: successful handle return, exactly-once firmware writes after handoff, disconnect/reconnect preservation, and deferred exit.

[kissetfall]

Thanks — f51bc92 fixes the previously reported refresh-disconnect localization and adds a real test HID backend, successful handle-return coverage, and same-device dirty-state preservation on its old base.

The PR is not mergeable after #91 and #82. GitHub reports CONFLICTING; the real merge conflicts in app_init, app_state, hid, app_lifecycle, device_connect_task, and device_connection. The refresh task must join the HID owner and exit lifecycle now in main rather than reintroducing parallel helpers/recorder state.

Additional blocking issues found in the updated code:

1. Ordinary native window close still bypasses refresh deferral. handle_close_to_tray() calls defer_exit_until_hid_write_returns() only for tray quit and force_close_requested; the normal Close path falls through. The new test calls the helper directly, so it cannot catch the production Linux close path. Rebase onto #82 and test a real ViewportEvent::Close through handle_close_to_tray().
2. Even the manual deferral does not drain all state marked dirty in the test. After refresh returns, finish_deferred_exit_after_hid_write() flushes Tap Hold/display and immediately issues Close before the later Macro, Combo, Combo Timeout, Tap Dance, and Key Override writers run. The test marks those states but only asserts Key Override/Tap Hold requests, and manually reorders Key Override before final close. The production close lifecycle must drain every deferred HID mutation exactly once before closing, using the real update order.
3. restore_deferred_hid_settings_after_connect() calls .take() before checking identity. Connecting any other device once permanently discards the original device's pending edits. Keep the deferred state until the matching stable device identity reconnects or the user explicitly discards it.
4. Reconnect restoration replaces whole Combo and Tap Dance entry/synced arrays captured before disconnect. That discards freshly read firmware state for clean slots and can either overwrite unrelated firmware changes or leave UI and firmware divergent. Preserve dirty deltas (slot/QSID plus intended value) and overlay only those deltas onto the freshly loaded same-device snapshot; keep clean fields from reconnect.

Please rebase on current main, reuse #82's test HID backend and single hid_write_task_active() owner (Layer + Combo + Module Refresh), and add production lifecycle tests for normal close, all deferred writers, a different-device reconnect followed by the original device, and clean-slot firmware changes across reconnect. Physical USB/Bluetooth validation remains required after the code path is correct.

[IgorArkhipov]

Addressed in follow-up commits after rebasing onto current main.

• Normal native close now goes through handle_close_to_tray() and defers while module refresh owns HID.
• Deferred exit uses real lifecycle order and drains Macro, Combo, Combo Timeout, Tap Dance, Tap Hold, and Key Override writes before closing. Regression coverage drives an actual ViewportEvent::Close.
• Deferred settings remain associated with their original device until that same stable identity reconnects; connecting another device does not discard them.
• Reconnect restoration preserves dirty Combo/Tap Dance deltas and overlays them onto freshly read state, leaving clean slots intact.
• Follow-up disconnect handling retains unsaved state without HID, and stale deferred state from an earlier device no longer blocks exit when a healthy HID is available.

[kissetfall]

The rebase and the new production close ordering are substantial improvements. The successful module-refresh handoff now runs pending synchronous writers before Close, the Combo and Tap Dance restore uses dirty slot deltas, and the UI follows the shared settings-list controls. Local verification passed 251 tests, Clippy, i18n, scoped rustfmt, diff-check, and release build. There are still lifecycle and data-loss blockers.

1. The deferred-settings identity is not stable across a physical replug. ModuleSettingsDeviceIdentity uses HID path plus Vial keyboard_id. On Linux the path is a hidraw node and can change after unplug/replug; keyboard_id identifies a definition, not a physical unit. The pending state can therefore remain orphaned for the same keyboard, while two identical units cannot be distinguished safely without serial-backed identity. Use the existing Device serial plus VID/PID/keyboard id as the authority, with an explicit fallback policy when serial is unavailable. Add tests for the same device returning on a different path and two identical keyboards.

2. Disconnect preservation still covers only part of the mutations that are allowed while refresh owns HID. A pending picker result and its target are not stored. clear/start_connect resets Key Override, Alt Repeat, Combo picker, key/encoder targets, so a selection made while refresh is active is silently consumed or dropped after reconnect. The real close test also does not include Key Override. Preserve a typed deferred mutation for every blocked writer rather than a partial collection of page snapshots.

3. Macro restoration replaces the entire pre-disconnect macro buffer when macros_dirty is true. Unlike Combo and Tap Dance, this can overwrite fresh clean macro slots read on reconnect. Track macro slot deltas against the synced readback and overlay only intended edits. The single deferred_hid_settings slot can also be overwritten if another device accumulates dirty state and disconnects before the original device returns; pending state needs per-device ownership or an explicit discard decision.

4. The disconnect contract is still specific to module refresh. Combo and Layer workers use the same hid_write_task_active gate, but their disconnect paths do not call the new preservation/reconnect owner. A Combo disconnect followed by device scan can still clear pending Macro, Combo Timeout, Tap Dance, Tap Hold, or picker state. Route all three workers through one shared finish/disconnect handoff so #82 is fixed for success and disconnect, not only the module-refresh path.

5. The new real-close test proves presence of some writes with any(), not exactly-once behavior, and covers only the all-success path. finish_deferred_exit_after_hid_write still sends Close whenever no background task is active even if a drained Macro, Combo Timeout, Tap Dance, Key Override, or Tap Hold write failed. In particular Tap Dance clears its dirty flag after a failed attempt. Keep the close cancelled until all queued mutations are verified or the user explicitly discards them, and assert exact request counts plus retry/failure behavior for every writer and display fallback.

Please retain the current background refresh/reporting and design-system work, but move deferred ownership into the shared HID lifecycle. Physical USB and Bluetooth validation should include path-changing replug, a second identical device, write failure during deferred close, and disconnect from each HID worker.

[IgorArkhipov]

Addressed in 6a22e4c:

• Deferred ownership now uses serial + VID/PID + keyboard ID. Serial-backed entries survive HID path changes; devices without a serial remain path-bound to avoid cross-applying state to an identical sibling device. Regression coverage includes path-changing replug and a second identical device.
• Added typed deferred picker mutations for Combo, Key Override, Alt Repeat, key, and encoder targets. Completed selections now survive a worker-owned HID interval and replay after reconnect. Key Override and Alt Repeat retain their picker mutation for retry if their write fails.
• Deferred macros now store edited slot deltas against synced readback, then overlay only those slots after reconnect. Deferred state is per device, so pending state for one device no longer overwrites another.
• Module refresh, Combo, and Layer disconnects now use one shared HID-worker disconnect handoff, preserving deferred state before connected keyboard state is cleared.
• Deferred close remains cancelled while any HID-backed mutation is pending or fails. Tap Dance retains dirty state after failure; Tap Hold retries; display fallback must write successfully before Close. The real-close regression now drives the real lifecycle and checks exact-once Macro, Combo, Combo Timeout, Tap Dance, Tap Hold, and Key Override requests.

[kissetfall]

The new per-device delta overlay, typed picker intents, shared disconnect entry point, Tap Dance retry, and exact-once happy-path assertions are good improvements. Head 6a22e4c is merge-clean; all four hosted builds are green; local cargo test --all-targets passes 251/251, and Clippy, i18n, and diff-check pass. The lifecycle still has release-blocking gaps.

1. A normal close is still cancelled only when a worker already owns HID. defer_exit_until_hid_write_returns returns false when hid_write_task_active is false, even if macros, Combo/term, Tap Dance, Tap Hold, or a picker mutation is pending. handle_close_to_tray then lets the original Close proceed; finish_deferred_exit_after_hid_write never gets a chance to drain those queues, and on_exit only attempts Tap Hold/display fallback. The new real-close test starts a module refresh first, so it does not cover the broken ordinary case. Cancel the initial Close whenever a worker is active OR has_pending_hid_mutations is true (and while required display fallback is pending), set exit_after_hid_write, then drive the lifecycle until all verified writes finish. Add a ViewportEvent::Close test with dirty state and no pre-existing worker.

2. Layer disconnects still lose the operation. finish_layer_write returns immediately when result.hid_device is None, and the shared handoff only preserves macro/combo/tap-dance/tap-hold/picker state. It does not retain result.context.desired, partial progress, or the LayerWriteFallback; clear_connected_keyboard_state then drops layout, task, and undo state. After reconnect the user sees whichever partial firmware writes completed, while the intended layer mutation and its undo/retry are gone. Store a typed deferred layer operation/progress before clearing and reconcile/retry it after fresh readback. Cover disconnect after one key and after keys but before encoders.

3. Picker failure handling is inconsistent and can either spin or lose the edit. Key Override and Alt Repeat put result/target back on any write failure; apply_picker_results consumes them again on the next frame, so a persistent HID/readback error causes a synchronous write attempt every frame and keeps Close cancelled forever with no explicit retry/cancel. Key and encoder failures do the opposite: the picker result is consumed and not retained; encoder assignment has already changed the local layout before the failed firmware write. Route all five typed picker mutations through one queued/verified worker state with bounded/backed-off or explicit retry, and preserve old/readback values on failure. Test persistent non-disconnect errors as well as disconnect.

4. Deferred identity is still unsafe/incomplete for common no-serial Vial devices. Those entries remain bound to the hidraw path, so the same keyboard re-enumerating at a new path never restores them. Because deferred_hid_settings is not included in has_pending_hid_mutations, the test deferred_settings_do_not_block_exit_with_healthy_hid explicitly allows closing while that unmatched in-memory state exists; the edits are then lost. Conversely, treating any non-empty serial as unique can cross-apply on firmware that gives identical units the same generic serial. Add a durable or user-confirmed reattachment path for ambiguous identities and do not silently close/drop unmatched deferred edits. Cover no-serial path change and duplicate serials, not only unique-serial replug.

Please keep the PR blocked until these cases are covered with failure-injectable HID tests and real USB/Bluetooth disconnect/reconnect checks.

[IgorArkhipov]

Addressed:

• Close now cancels when any HID mutation is pending, not only while a worker owns HID. It drains verified writes before closing. Added real ViewportEvent::Close coverage for dirty state with no pre-existing worker.
• Layer writes now preserve a typed desired snapshot across HID disconnect, then reconcile after fresh layout/HID readback. Regression tests inject disconnect after one key and after keys complete but before encoder writes, verifying remaining diffs replay.
• Picker failures now retain typed retry state with backoff instead of retrying every frame. Key/encoder updates wait for verified writes; KO/Alt Repeat restore prior local state and dirty flags on failure. Added persistent-failure coverage.
• Deferred settings are now path-bound even when a serial exists, preventing cross-application from duplicated firmware serials. No-serial path changes retain deferred state and surface reattachment guidance instead of auto-applying it. Unmatched state has an intentional close-discard path, while normal pending writes still drain first.

[kissetfall]

Re-reviewed head 0616596. Layer replay, per-device deltas, typed picker preservation, normal-close deferral, and the expanded HID tests are meaningful improvements. All four hosted builds are green and local tests pass 269/269. Two release blockers remain.

1. Ambiguous/path-changing reconnect still has no safe recovery path. ModuleSettingsDeviceIdentity::matches requires the old HID path even with a serial, and a no-serial path change only leaves a hardcoded English status. The deferred edits cannot be attached to the re-enumerated keyboard. On explicit close, discard_deferred_hid_settings_for_exit silently clears those unmatched edits after only logging a warning. Provide a localized user-confirmed reattachment/discard flow for path changes and duplicate serials; never silently drop the deferred record on close.

2. Picker failures still retry synchronously every 100 ms forever. apply_picker_results restores the mutation and picker_retry_due after every failure, so a persistent non-disconnect/readback error repeatedly performs HID I/O on the UI thread and can keep close cancelled indefinitely. Use the shared queued worker with bounded backoff or an explicit Retry/Discard state, preserving confirmed UI/readback values.

Add regressions for confirmed reattachment/discard, close with unmatched deferred edits, and persistent picker failure without per-frame retries.

[IgorArkhipov]

Addressed:

• Deferred HID edits now stay path-bound by default. A path change, whether no serial or a duplicate serial, is surfaced as a localized confirmation flow before reattaching; close also requires explicit discard confirmation. Cancel and a close request preserve an active reattach prompt.
• Picker write failures no longer retry on the UI loop. The failed choice is cleared after rollback, so retry requires a new explicit selection; confirmed UI state remains intact.
• Restored confirmed touchpad values after disconnect-state reset, and preserved incomplete layer writes for reconnect replay.
• Added regression coverage for reattach/discard confirmation, duplicate serials, close interruption, persistent picker errors, QMK restoration, and layer replay.

[kissetfall]

After extensive review and testing, we decided not to accept this PR.

The saved-values refresh addresses a narrow diagnostic case while introducing a large, high-risk HID lifecycle layer, and it still cannot observe the runtime firmware state that originally motivated the feature. The product value does not justify the maintenance cost and regression risk.

Thank you for the substantial work and iterations. We are closing this PR without merge.