From 6d4ea50ca3a4bd7249056101952a35fc5f1c255d Mon Sep 17 00:00:00 2001 From: James Rich Date: Fri, 12 Jun 2026 09:19:59 -0500 Subject: [PATCH] =?UTF-8?q?fix:=20address=20review=20triage=20=E2=80=94=20?= =?UTF-8?q?transition-based=20ham=20routing,=20local-only=20guard,=20calls?= =?UTF-8?q?ign=20guidance?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Route to set_ham_mode only when the licensed toggle transitions OFF→ON; subsequent saves of an already-licensed node use set_owner so other owner edits still propagate and the node doesn't reboot on every save (F-1) - Enforce the local-node-only contract in AdminControllerImpl, not just the ViewModel: setHamMode ignores remote destinations (F-3) - Show 'Your amateur radio call sign, up to 8 characters' as the field's supporting text in ham mode, giving sighted and screen-reader users the constraint (F-4, F-6) - Document that saveUserConfig is the preferred entry point over setOwner (F-7) Co-Authored-By: Claude Fable 5 --- .skills/compose-ui/strings-index.txt | 1 + .../core/repository/AdminController.kt | 12 +++++------ .../composeResources/values/strings.xml | 1 + .../core/service/AdminControllerImpl.kt | 4 ++++ .../core/service/RadioControllerImplTest.kt | 14 +++++++++++-- .../settings/radio/RadioConfigViewModel.kt | 15 +++++++++---- .../radio/component/UserConfigItemList.kt | 2 ++ .../radio/RadioConfigViewModelTest.kt | 21 +++++++++++++++++++ 8 files changed, 58 insertions(+), 12 deletions(-) diff --git a/.skills/compose-ui/strings-index.txt b/.skills/compose-ui/strings-index.txt index d40af8e52..73cc216ed 100644 --- a/.skills/compose-ui/strings-index.txt +++ b/.skills/compose-ui/strings-index.txt @@ -123,6 +123,7 @@ button_gpio buzzer_gpio calculating call_sign +call_sign_summary cancel cancel_reply canned_message diff --git a/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/AdminController.kt b/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/AdminController.kt index b146ceaff..72d332634 100644 --- a/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/AdminController.kt +++ b/core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/AdminController.kt @@ -56,12 +56,12 @@ interface AdminController { /** * Enables amateur-radio (ham) mode on a node via `AdminMessage.set_ham_mode`. * - * Must target only the locally connected node — firmware ham onboarding is a local operation and the UI gates it - * accordingly. The firmware handler rewrites the owner (long_name = call_sign), flips `is_licensed`, disables - * encryption, applies [HamParameters.tx_power]/[HamParameters.frequency] to the LoRa config verbatim, and reboots. - * The implementation echoes the local node's current LoRa values into those two fields so a re-send never wipes the - * node's overrides; caller-supplied [HamParameters.tx_power]/[HamParameters.frequency] are ignored. Intentionally - * absent from [AdminEditScope]: ham enablement is not a batch-edit operation. + * Must target only the locally connected node — firmware ham onboarding is a local operation; the implementation + * ignores requests for any other node. The firmware handler rewrites the owner (long_name = call_sign), flips + * `is_licensed`, disables encryption, applies [HamParameters.tx_power]/[HamParameters.frequency] to the LoRa config + * verbatim, and reboots. The implementation echoes the local node's current LoRa values into those two fields so a + * re-send never wipes the node's overrides; caller-supplied [HamParameters.tx_power]/[HamParameters.frequency] are + * ignored. Intentionally absent from [AdminEditScope]: ham enablement is not a batch-edit operation. */ suspend fun setHamMode(destNum: Int, hamParameters: HamParameters, packetId: Int) diff --git a/core/resources/src/commonMain/composeResources/values/strings.xml b/core/resources/src/commonMain/composeResources/values/strings.xml index 8a93589ba..5bd0d5065 100644 --- a/core/resources/src/commonMain/composeResources/values/strings.xml +++ b/core/resources/src/commonMain/composeResources/values/strings.xml @@ -141,6 +141,7 @@ Buzzer GPIO Calculating… Call sign + Your amateur radio call sign, up to 8 characters Cancel Cancel reply Canned Message diff --git a/core/service/src/commonMain/kotlin/org/meshtastic/core/service/AdminControllerImpl.kt b/core/service/src/commonMain/kotlin/org/meshtastic/core/service/AdminControllerImpl.kt index 81c8b6923..9f0473733 100644 --- a/core/service/src/commonMain/kotlin/org/meshtastic/core/service/AdminControllerImpl.kt +++ b/core/service/src/commonMain/kotlin/org/meshtastic/core/service/AdminControllerImpl.kt @@ -68,6 +68,10 @@ internal class AdminControllerImpl( } override suspend fun setHamMode(destNum: Int, hamParameters: HamParameters, packetId: Int) { + if (destNum != nodeManager.myNodeNum.value) { + Logger.w { "Ignoring setHamMode for node $destNum — ham onboarding targets the local node only" } + return + } // Firmware applies tx_power/frequency to the LoRa config verbatim, so echo the node's current // values to keep a re-send (e.g. a callsign edit while already licensed) from wiping overrides. val lora = radioConfigRepository.localConfigFlow.firstOrNull()?.lora ?: Config.LoRaConfig() diff --git a/core/service/src/commonTest/kotlin/org/meshtastic/core/service/RadioControllerImplTest.kt b/core/service/src/commonTest/kotlin/org/meshtastic/core/service/RadioControllerImplTest.kt index 16e2c6c52..bd7a868e1 100644 --- a/core/service/src/commonTest/kotlin/org/meshtastic/core/service/RadioControllerImplTest.kt +++ b/core/service/src/commonTest/kotlin/org/meshtastic/core/service/RadioControllerImplTest.kt @@ -363,7 +363,7 @@ class RadioControllerImplTest { @Test fun setHamModeSendsAdminWithEchoedLoraValuesAndUpdatesUser() = runTest { - val controller = createController() + val controller = createController(myNodeNum = 123) val existingUser = User(id = "!0000007b", long_name = "Old Name", short_name = "OLD") every { nodeManager.nodeDBbyNodeNum } returns mapOf(123 to Node(num = 123, user = existingUser)) every { radioConfigRepository.localConfigFlow } returns @@ -396,7 +396,7 @@ class RadioControllerImplTest { @Test fun setHamModeWithNoCachedLoraConfigSendsProtoDefaults() = runTest { - val controller = createController() + val controller = createController(myNodeNum = 123) every { nodeManager.nodeDBbyNodeNum } returns emptyMap() every { radioConfigRepository.localConfigFlow } returns MutableStateFlow(LocalConfig()) @@ -423,6 +423,16 @@ class RadioControllerImplTest { } } + @Test + fun setHamModeIgnoresRemoteDestinations() = runTest { + val controller = createController(myNodeNum = 123) + + controller.setHamMode(456, HamParameters(call_sign = "KK7ABC", short_name = "KK7A"), 42) + + verifySuspend(exactly(0)) { commandSender.sendAdmin(any(), any(), any(), any()) } + verify(exactly(0)) { nodeManager.handleReceivedUser(any(), any(), any(), any()) } + } + @Test fun importContactReturnsEarlyWhenDisconnected() = runTest { val controller = createController(myNodeNum = null) diff --git a/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt b/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt index e0f4a3e45..bc5c6d190 100644 --- a/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt +++ b/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.kt @@ -292,13 +292,16 @@ open class RadioConfigViewModel( } /** - * Routes the User config save: ham onboarding (`set_ham_mode`) when the licensed toggle is on and the target is the - * locally connected node, [setOwner] otherwise. The local-node guard is the backstop for the UI gate — - * `set_ham_mode` must never be sent to a remote node. + * Routes the User config save: ham onboarding (`set_ham_mode`) when the licensed toggle transitions OFF→ON on the + * locally connected node, [setOwner] otherwise. Routing on the transition — not the toggle state — keeps subsequent + * saves of an already-licensed node on the `set_owner` path, so edits to other owner fields still reach the device + * and the node doesn't reboot on every save (firmware reboots on `set_ham_mode`). The local-node guard is the + * backstop for the UI gate — `set_ham_mode` must never be sent to a remote node. */ fun saveUserConfig(user: User) { val destNum = destNum ?: destNode.value?.num ?: return - if (user.is_licensed && destNum == myNodeNum) setHamMode(destNum, user) else setOwner(user) + val enablingHam = user.is_licensed && !radioConfigState.value.userConfig.is_licensed + if (enablingHam && destNum == myNodeNum) setHamMode(destNum, user) else setOwner(user) } private fun setHamMode(destNum: Int, user: User) { @@ -315,6 +318,10 @@ open class RadioConfigViewModel( } } + /** + * Sends a plain `set_owner` with [user]. Prefer [saveUserConfig] for User config screen saves — it routes ham + * onboarding to `set_ham_mode` when the licensed toggle is first enabled; calling this directly bypasses that. + */ fun setOwner(user: User) { val destNum = destNum ?: destNode.value?.num ?: return safeLaunch(tag = "setOwner") { diff --git a/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/UserConfigItemList.kt b/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/UserConfigItemList.kt index 904355c6b..d9a384381 100644 --- a/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/UserConfigItemList.kt +++ b/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/UserConfigItemList.kt @@ -32,6 +32,7 @@ import org.meshtastic.core.model.Capabilities import org.meshtastic.core.model.isUnmessageableRole import org.meshtastic.core.resources.Res import org.meshtastic.core.resources.call_sign +import org.meshtastic.core.resources.call_sign_summary import org.meshtastic.core.resources.hardware_model import org.meshtastic.core.resources.licensed_amateur_radio import org.meshtastic.core.resources.licensed_amateur_radio_text @@ -87,6 +88,7 @@ fun UserConfigScreen(viewModel: RadioConfigViewModel, onBack: () -> Unit) { EditTextPreference( title = stringResource(if (hamMode) Res.string.call_sign else Res.string.long_name), value = formState.value.long_name, + summary = if (hamMode) stringResource(Res.string.call_sign_summary) else null, maxSize = longNameMax, enabled = state.connected, isError = !validLongName, diff --git a/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt b/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt index 4a8f3288a..76b3a2fbe 100644 --- a/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt +++ b/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt @@ -421,6 +421,27 @@ class RadioConfigViewModelTest { verifySuspend(exactly(0)) { radioConfigUseCase.setHamMode(any(), any()) } } + @Test + fun `saveUserConfig routes subsequent licensed saves to setOwner`() = runTest { + val node = Node(num = 123, user = User(id = "!123")) + nodeRepository.setNodes(listOf(node)) + nodeRepository.setMyNodeInfo(myNodeInfo(myNodeNum = 123)) + viewModel = createViewModel() + + val user = User(long_name = "KK7ABC", short_name = "KK7A", is_licensed = true) + everySuspend { radioConfigUseCase.setHamMode(any(), any()) } returns 42 + everySuspend { radioConfigUseCase.setOwner(any(), any()) } returns 43 + + // First save transitions OFF→ON and onboards via set_ham_mode. + viewModel.saveUserConfig(user) + // A later save while already licensed must use set_owner so other owner fields propagate. + val edited = user.copy(short_name = "KK7B") + viewModel.saveUserConfig(edited) + + verifySuspend(exactly(1)) { radioConfigUseCase.setHamMode(any(), any()) } + verifySuspend { radioConfigUseCase.setOwner(123, edited) } + } + @Test fun `saveUserConfig routes licensed save to setOwner when myNodeInfo is absent`() = runTest { val node = Node(num = 123, user = User(id = "!123"))