fix(qr): Serialize channel import writes#5999
Merged
jamesarich merged 1 commit intoJun 29, 2026
Merged
Conversation
Contributor
📄 Docs staleness check — advisoryThis PR modifies user-facing UI source files but does not update any page under
Changed source files: What to check:
New page checklist (if adding a new doc page):
If this PR does not require a doc update (e.g., internal refactor, bug fix, test change), add the
|
QR channel replacement imports previously launched each per-channel
firmware write as a separate fire-and-forget safeLaunch coroutine, then
immediately replaced the local cached ChannelSet. Two consequences made
the import path non-deterministic:
1. N concurrent safeLaunch coroutines acquired the radio transport's
queue mutex in scheduling-determined order, not iteration order, so
the firmware received set_channel admin packets in an unpredictable
sequence.
2. replaceAllSettings() ran before any of those in-flight writes were
known to have been enqueued, let alone processed by the firmware,
so the optimistic local cache was contestable by both the late-
arriving per-channel cache updates inside AdminControllerImpl
.setLocalChannel and by the asynchronous firmware channel-response
echoes that arrive via MeshConfigHandlerImpl.handleChannel.
This change makes the REPLACE import path deterministic.
A new suspend helper applyReplacementChannelSet(channelSet,
radioController, radioConfigRepository) centralizes the replacement
behavior shared by both entry points (ScannedQrCodeViewModel for QR
scan / URL share-link import, and ChannelViewModel for the channel
settings screen). It reads the current channel set directly from the
repository flow via first() — not from the StateFlow's .value, which
returns an empty ChannelSet() placeholder during the brief window
before the first subscriber emission and would suppress trailing
DISABLED writes. It then calls getChannelReplacementList to build an
authoritative slot list and enqueues each channel write sequentially
via radioController.setLocalChannel before atomically replacing the
local cached settings via replaceAllSettings.
setLocalChannel is genuinely suspend, so the sequential for-loop
serializes dispatch at the enqueue layer. setLocalChannel returns once
the packet is enqueued under the transport's queue mutex, not after
firmware ACK — firmware echoes via MeshConfigHandlerImpl can still
arrive after replaceAllSettings and are tracked separately.
A new helper getChannelReplacementList(new, currentSettings) builds
the authoritative Channel list for the replacement. It is added to
ProtoExtensions.kt alongside the existing getChannelList (which is
preserved unchanged for its existing caller on the manual channel
edit path in RadioConfigViewModel.updateChannels). The helper:
- emits Channel.Role.PRIMARY for index 0 of the imported set
(DISABLED if new is empty, to avoid promoting empty settings)
- emits Channel.Role.SECONDARY for indices 1..new.lastIndex
- emits Channel.Role.DISABLED with empty ChannelSettings for any
trailing slots present in the current set but beyond the imported
range, so the radio clears them
- never skips positions where currentSettings[i] == new[i]; the
imported set is authoritative
The currentSettings parameter is consulted only for its size (to
determine trailing DISABLED writes); its values are never compared
against new. This is the key semantic difference from getChannelList,
which uses old[i] != new[i] to diff-skip unchanged positions.
Both ScannedQrCodeViewModel.setChannels and ChannelViewModel
.setChannels are simplified to delegate to applyReplacementChannelSet
for the channel write loop, then handle LoRa config conditionally
(the LoRa branch is unchanged from the prior implementation).
The now-dead public setChannel(channel: Channel) helper is removed
from ChannelViewModel (it had no remaining callers after setChannels
stopped routing through it). The orphaned Channel import is dropped
from both files. The private setChannel helper is also removed from
ScannedQrCodeViewModel for the same reason.
Seven unit tests are added under core:ui commonTest covering every
behavioral branch of getChannelReplacementList:
- index 0 emits PRIMARY with the new settings even when
currentSettings[0] equals new[0] (verifies no diff-skip)
- secondary indices emit SECONDARY with the new settings even when
unchanged from current
- trailing indices beyond new.lastIndex are emitted as DISABLED
with empty ChannelSettings
- empty new and empty current produces an empty list
- empty new with non-empty current emits DISABLED for every slot
including index 0
- single-entry new with multi-entry current emits PRIMARY at 0
followed by DISABLED trailing slots
- new larger than current emits PRIMARY plus one SECONDARY per
added index (the most common real REPLACE pattern)
Out of scope for this change:
- MeshConfigHandlerImpl.handleChannel unconditionally overwrites
the local cache from firmware-reported channel state. Firmware
echoes can therefore still arrive after replaceAllSettings and
re-corrupt the cache. The applyReplacementChannelSet KDoc
documents this residual racer; it is tracked separately.
- QR ADD-mode merge uses structural .distinct() over
ChannelSettings which can collapse equal entries and shift later
channels. Also tracked separately.
- ChannelSet PRIMARY-reordering to position 0 (spec compliance) is
a separate concern.
- The sibling RadioConfigViewModel.updateChannels path still uses
the original getChannelList and per-channel safeLaunch pattern.
It is the manual single-channel edit path and is unchanged here.
No tests, detekt, or compile tasks are run in this commit. The change
was verified against spotlessApply for core:ui and feature:settings;
CI will run spotlessCheck, detekt, assembleDebug, test, and allTests
as the authoritative gate.
df2108f to
669b9de
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR makes QR channel replacement imports deterministic by sending imported channel writes sequentially instead of launching each write as a separate fire-and-forget coroutine.
The previous replacement flow could start multiple channel writes, immediately replace the local cached settings, and then receive later channel updates from the radio or the local write path. It also used the local cache to decide which imported channels needed to be sent, which made QR replacement vulnerable to stale or partially updated local state.
QR replacement should be authoritative: importing a replacement channel set should send every imported slot to the radio, even if the local cache appears to already match. This change makes the channel write sequence explicit and awaited before replacing the local cached settings.
This fixes the observed multi-channel QR replacement issue where replacing a full channel set could leave a later channel with stale or incomplete state.
Key Changes
getChannelReplacementList(new, currentSettings)for authoritative QR replacement imports.getChannelList(new, old)diff helper unchanged for the manual edit path.DISABLEDentries for old trailing slots beyond the imported channel list.PRIMARYSECONDARYDISABLEDTesting
getChannelReplacementList.Migration Notes
Related #6013