Skip to content

fix(qr): Serialize channel import writes#5999

Merged
jamesarich merged 1 commit into
meshtastic:mainfrom
jeremiah-k:bugfix/qr-serialize-channel-imports
Jun 29, 2026
Merged

fix(qr): Serialize channel import writes#5999
jamesarich merged 1 commit into
meshtastic:mainfrom
jeremiah-k:bugfix/qr-serialize-channel-imports

Conversation

@jeremiah-k

@jeremiah-k jeremiah-k commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

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

  • Added getChannelReplacementList(new, currentSettings) for authoritative QR replacement imports.
  • Kept the existing getChannelList(new, old) diff helper unchanged for the manual edit path.
  • Replaced per-channel fire-and-forget writes in QR/channel replacement paths with sequential awaited channel writes.
  • Emitted every imported slot during QR replacement, without skipping positions where the local cache appears unchanged.
  • Emitted DISABLED entries for old trailing slots beyond the imported channel list.
  • Preserved positional role assignment:
    • index 0 is PRIMARY
    • later imported slots are SECONDARY
    • trailing old slots are DISABLED
  • Read the current channel set directly from the repository flow before building trailing disabled writes, avoiding the StateFlow placeholder window.
  • Preserved existing LoRa config handling.
  • Kept radio channel responses as the source of truth after import.

Testing

  • Added unit-test coverage for getChannelReplacementList.
  • Covered unchanged imported slots, secondary slots, old trailing disabled slots, empty inputs, empty replacement over existing channels, single-entry replacement, and replacement lists larger than the current cache.
  • Verified manually on device that QR replacement correctly replaces the full channel set and fixes the previously observed channel replacement issue.

Migration Notes

  • No user data migration is required.
  • Existing channel data remains unchanged until the user imports or edits channels.
  • This only changes the ordering and cache timing of QR channel replacement imports.

Related #6013

@github-actions github-actions Bot added the bugfix PR tag label Jun 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📄 Docs staleness check — advisory

This PR modifies user-facing UI source files but does not update any page under docs/en/user/ or docs/en/developer/.

⚠️ Doc changes propagate to 3 consumers: in-app docs browser, Jekyll site (GitHub Pages), and meshtastic.org (Docusaurus sync). Updating a page in docs/en/ automatically flows to all three.

Changed source files:

core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/qr/ScannedQrCodeViewModel.kt
core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/ProtoExtensions.kt

What to check:

Changed area Likely doc page
feature/messaging/ docs/en/user/messages-and-channels.md
feature/node/ docs/en/user/nodes.md or docs/en/user/node-metrics.md
feature/map/ docs/en/user/map-and-waypoints.md
feature/connections/ docs/en/user/connections.md
feature/settings/ docs/en/user/settings-radio-user.md or docs/en/user/settings-module-admin.md
feature/firmware/ docs/en/user/firmware.md
feature/intro/ docs/en/user/onboarding.md
feature/discovery/ docs/en/user/discovery.md
feature/docs/ Internal docs infrastructure
core/ui/ docs/en/developer/codebase.md or component-specific user pages

New page checklist (if adding a new doc page):

  1. Create the .md file in docs/en/user/ or docs/en/developer/ with last_updated frontmatter
  2. Register in DocBundleLoader.kt with string resources (in-app browser)
  3. Jekyll and Docusaurus sync pick up new pages automatically — no config change needed

If this PR does not require a doc update (e.g., internal refactor, bug fix, test change), add the skip-docs-check label to dismiss this check.

Cross-platform note: This check is advisory while doc coverage matures. Both Android and Apple repos use the same skip-docs-check label and advisory severity. See meshtastic/design standards for shared conventions.

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.
@jeremiah-k jeremiah-k force-pushed the bugfix/qr-serialize-channel-imports branch from df2108f to 669b9de Compare June 28, 2026 23:31
@jeremiah-k jeremiah-k marked this pull request as ready for review June 28, 2026 23:40

@jamesarich jamesarich left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@jamesarich jamesarich added this pull request to the merge queue Jun 29, 2026
Merged via the queue into meshtastic:main with commit cbd72be Jun 29, 2026
22 checks passed
@jeremiah-k jeremiah-k deleted the bugfix/qr-serialize-channel-imports branch June 29, 2026 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix PR tag

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants