diff --git a/core/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/settings/ProcessRadioResponseUseCase.kt b/core/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/settings/ProcessRadioResponseUseCase.kt index 1a7b4898cfb..614d69ea49a 100644 --- a/core/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/settings/ProcessRadioResponseUseCase.kt +++ b/core/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/settings/ProcessRadioResponseUseCase.kt @@ -51,6 +51,13 @@ sealed class RadioResponseResult { data class Error(val message: UiText, val routingError: Routing.Error? = null) : RadioResponseResult() data object Success : RadioResponseResult() + + /** + * A routing ACK for one of our requests arrived from [from], not the node the request was addressed to. For a + * request addressed to the connected node this means the radio renumbered itself between the request and its ACK — + * firmware 2.8 moves `my_node_num` to `crc32(public_key)` when it mints the key on the first region set. + */ + data class UnexpectedAckSender(val from: Int) : RadioResponseResult() } /** Use case for processing incoming [MeshPacket]s that are responses to admin requests. */ @@ -90,7 +97,7 @@ open class ProcessRadioResponseUseCase { packet.from == destNum -> RadioResponseResult.Success - else -> null + else -> RadioResponseResult.UnexpectedAckSender(from = packet.from) } } diff --git a/core/domain/src/commonTest/kotlin/org/meshtastic/core/domain/usecase/settings/ProcessRadioResponseUseCaseTest.kt b/core/domain/src/commonTest/kotlin/org/meshtastic/core/domain/usecase/settings/ProcessRadioResponseUseCaseTest.kt index 9946a99791e..f843e717def 100644 --- a/core/domain/src/commonTest/kotlin/org/meshtastic/core/domain/usecase/settings/ProcessRadioResponseUseCaseTest.kt +++ b/core/domain/src/commonTest/kotlin/org/meshtastic/core/domain/usecase/settings/ProcessRadioResponseUseCaseTest.kt @@ -73,6 +73,24 @@ class ProcessRadioResponseUseCaseTest { assertEquals(RadioResponseResult.Success, result) } + @Test + fun `routing ack from a node other than the addressed one is reported instead of dropped`() { + val packet = + MeshPacket( + from = 456, + decoded = + Data( + portnum = PortNum.ROUTING_APP, + request_id = 42, + payload = Routing(error_reason = Routing.Error.NONE).encode().toByteString(), + ), + ) + + val result = useCase(packet, 123, setOf(42)) + + assertEquals(RadioResponseResult.UnexpectedAckSender(from = 456), result) + } + @Test fun `invoke with metadata response returns metadata result`() { // Arrange 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 879878715b7..99a9c92b059 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 @@ -73,6 +73,7 @@ import org.meshtastic.core.repository.LocationService import org.meshtastic.core.repository.LockdownCoordinator import org.meshtastic.core.repository.LockdownPassphraseStore import org.meshtastic.core.repository.MapConsentPrefs +import org.meshtastic.core.repository.MeshConnectionManager import org.meshtastic.core.repository.MqttManager import org.meshtastic.core.repository.NodeRepository import org.meshtastic.core.repository.NodeRestartTracker @@ -152,7 +153,7 @@ data class RadioConfigState( @KoinViewModel @Suppress("LongParameterList", "LargeClass") open class RadioConfigViewModel( - @InjectedParam private val destNum: Int?, + @InjectedParam initialDestNum: Int?, private val radioConfigRepository: RadioConfigRepository, private val packetRepository: PacketRepository, private val serviceRepository: ServiceRepository, @@ -175,9 +176,20 @@ open class RadioConfigViewModel( private val securityKeyBackupStore: SecurityKeyBackupStore, private val snackbarManager: SnackbarManager, private val nodeRestartTracker: NodeRestartTracker, + private val connectionManager: MeshConnectionManager, private val analytics: PlatformAnalytics, ) : ViewModel() { + /** + * The node this session addresses. Starts as the injected destination (null = the connected node, whatever its + * number). A session opened on the connected node *by number* — Node detail → Administration — drops to null when + * that number moves under it (firmware 2.8 renumbers on the first region set), so later writes follow the connected + * node through [destNode] instead of a number the radio no longer answers to. + */ + private val activeDestNum = MutableStateFlow(initialDestNum) + private val destNum: Int? + get() = activeDestNum.value + val lockdownTokenInfo = serviceRepository.lockdownTokenInfo val sessionAuthorized = serviceRepository.sessionAuthorized val lockdownState = serviceRepository.lockdownState @@ -309,8 +321,9 @@ open class RadioConfigViewModel( locationService.getCurrentLocation() init { - nodeRepository.nodeDBbyNum - .map { nodes -> if (destNum != null) nodes[destNum] else nodes.values.firstOrNull() } + combine(nodeRepository.nodeDBbyNum, activeDestNum) { nodes, dest -> + if (dest != null) nodes[dest] else nodes.values.firstOrNull() + } .distinctUntilChanged() .onEach { _destNode.value = it @@ -325,11 +338,10 @@ open class RadioConfigViewModel( // Derive isLocal from the immutable destNum and the (possibly changing) myNodeInfo. // flatMapLatest cancels the previous inner flow on every change, so there is // no window where stale local config can leak through. - nodeRepository.myNodeInfo - .map { ni -> - val isLocal = (destNum == null) || (destNum == ni?.myNodeNum) - isLocal to if (isLocal) ni?.pioEnv else null - } + combine(nodeRepository.myNodeInfo, activeDestNum) { ni, dest -> + val isLocal = (dest == null) || (dest == ni?.myNodeNum) + isLocal to if (isLocal) ni?.pioEnv else null + } .distinctUntilChanged() .flatMapLatest { (isLocal, pioEnv) -> if (isLocal) { @@ -1235,6 +1247,27 @@ open class RadioConfigViewModel( } } + is RadioResponseResult.UnexpectedAckSender -> { + // The connected radio ACKed our local write from a node number other than the one we addressed: + // firmware 2.8 renumbers itself (num = crc32(public_key)) when it mints the PKI key on the first + // region set, live and without a reboot, so our cached number is now stale and every further admin + // write would NAK PKI_SEND_FAIL_PUBLIC_KEY until we re-learn it. Re-run the config handshake to pick + // up the new my_node_num, and treat this ACK as the save's confirmation. Scoped to a local save + // (route empty, isLocal): a remote admin target legitimately answers from a different node. + if (requestId != null && !isLateRemoteRead && route.isEmpty() && radioConfigState.value.isLocal) { + clearRequestIds() + // A session opened on the connected node by number would otherwise keep addressing the old + // number (and stop counting as local) once the handshake reports the new one. + activeDestNum.value = null + connectionManager.startConfigOnly() + setResponseStateSuccess() + } + // Otherwise an unaddressed node's ack says nothing about our request: leave it pending, exactly as + // when the use case returned null for it. Falling through would let the generic completion below + // retire the request and resolve a remote save on a foreign ack. + return + } + is RadioResponseResult.Metadata -> { _radioConfigState.update { it.copy(metadata = result.metadata) } if (!isLateRemoteRead) incrementCompleted() diff --git a/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/ProfileRoundTripTest.kt b/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/ProfileRoundTripTest.kt index cedc8f94554..f8222fe645d 100644 --- a/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/ProfileRoundTripTest.kt +++ b/feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/ProfileRoundTripTest.kt @@ -122,7 +122,7 @@ class ProfileRoundTripTest { viewModel = RadioConfigViewModel( - destNum = null, + initialDestNum = null, radioConfigRepository = radioConfigRepository, packetRepository = packetRepository, serviceRepository = serviceRepository, @@ -146,6 +146,7 @@ class ProfileRoundTripTest { securityKeyBackupStore = securityKeyBackupStore, snackbarManager = snackbarManager, nodeRestartTracker = nodeRestartTracker, + connectionManager = mock(MockMode.autofill), ) } 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 52226f50d18..f52f14d18e8 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 @@ -44,6 +44,7 @@ import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.resetMain import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.setMain +import okio.ByteString import okio.ByteString.Companion.encodeUtf8 import org.meshtastic.core.domain.usecase.settings.AdminActionsUseCase import org.meshtastic.core.domain.usecase.settings.ExportProfileUseCase @@ -64,6 +65,7 @@ import org.meshtastic.core.repository.HomoglyphPrefs import org.meshtastic.core.repository.LocationRepository import org.meshtastic.core.repository.LocationService import org.meshtastic.core.repository.MapConsentPrefs +import org.meshtastic.core.repository.MeshConnectionManager import org.meshtastic.core.repository.MqttManager import org.meshtastic.core.repository.NodeRestartTracker import org.meshtastic.core.repository.PacketRepository @@ -92,6 +94,7 @@ import org.meshtastic.proto.LoRaRegionPresetMap import org.meshtastic.proto.LocalConfig import org.meshtastic.proto.LocalModuleConfig import org.meshtastic.proto.MeshPacket +import org.meshtastic.proto.PortNum import org.meshtastic.proto.Routing import org.meshtastic.proto.User import kotlin.test.AfterTest @@ -170,6 +173,7 @@ class RadioConfigViewModelTest { private val uiPrefs: UiPrefs = mock(MockMode.autofill) private val securityKeyBackupStore: SecurityKeyBackupStore = mock(MockMode.autofill) private val snackbarManager: SnackbarManager = mock(MockMode.autofill) + private val connectionManager: MeshConnectionManager = mock(MockMode.autofill) private val trackerScope = CoroutineScope(SupervisorJob()) private val nodeRestartTracker = NodeRestartTracker(trackerScope) @@ -222,34 +226,131 @@ class RadioConfigViewModelTest { private fun runTest(block: suspend TestScope.() -> Unit) = kotlinx.coroutines.test.runTest(testDispatcher, testBody = block) - private fun createViewModel(destNum: Int? = null, snackbarManager: SnackbarManager = this.snackbarManager) = - RadioConfigViewModel( - destNum = destNum, - radioConfigRepository = radioConfigRepository, - packetRepository = packetRepository, - serviceRepository = serviceRepository, - nodeRepository = nodeRepository, - locationRepository = locationRepository, - mapConsentPrefs = mapConsentPrefs, - analyticsPrefs = analyticsPrefs, - homoglyphEncodingPrefs = homoglyphEncodingPrefs, - importProfileUseCase = importProfileUseCase, - exportProfileUseCase = exportProfileUseCase, - importSecurityConfigUseCase = importSecurityConfigUseCase, - securityKeyBackupStore = securityKeyBackupStore, - snackbarManager = snackbarManager, - nodeRestartTracker = nodeRestartTracker, - installProfileUseCase = installProfileUseCase, - radioConfigUseCase = radioConfigUseCase, - adminActionsUseCase = adminActionsUseCase, - processRadioResponseUseCase = processRadioResponseUseCase, - locationService = locationService, - fileService = fileService, - mqttManager = mqttManager, - lockdownCoordinator = FakeLockdownCoordinator(), - analytics = mock(MockMode.autofill), + private fun createViewModel( + destNum: Int? = null, + snackbarManager: SnackbarManager = this.snackbarManager, + processRadioResponseUseCase: ProcessRadioResponseUseCase = this.processRadioResponseUseCase, + ) = RadioConfigViewModel( + initialDestNum = destNum, + radioConfigRepository = radioConfigRepository, + packetRepository = packetRepository, + serviceRepository = serviceRepository, + nodeRepository = nodeRepository, + locationRepository = locationRepository, + mapConsentPrefs = mapConsentPrefs, + analyticsPrefs = analyticsPrefs, + homoglyphEncodingPrefs = homoglyphEncodingPrefs, + importProfileUseCase = importProfileUseCase, + exportProfileUseCase = exportProfileUseCase, + importSecurityConfigUseCase = importSecurityConfigUseCase, + securityKeyBackupStore = securityKeyBackupStore, + snackbarManager = snackbarManager, + nodeRestartTracker = nodeRestartTracker, + connectionManager = connectionManager, + installProfileUseCase = installProfileUseCase, + radioConfigUseCase = radioConfigUseCase, + adminActionsUseCase = adminActionsUseCase, + processRadioResponseUseCase = processRadioResponseUseCase, + locationService = locationService, + fileService = fileService, + mqttManager = mqttManager, + lockdownCoordinator = FakeLockdownCoordinator(), + analytics = mock(MockMode.autofill), + ) + .also { createdViewModels += it } + + @Test + fun `local save acked by a renumbered radio re-runs the handshake and completes the save`() = runTest { + val node = Node(num = 123, user = User(id = "!123")) + nodeRepository.setNodes(listOf(node)) + nodeRepository.setMyNodeInfo(myNodeInfo(myNodeNum = 123)) + val packetFlow = MutableSharedFlow() + every { serviceRepository.meshPacketFlow } returns packetFlow + everySuspend { radioConfigUseCase.setConfig(any(), any(), any()) } calls + { + it.args.onRequestIdArg()(77) + 77 + } + viewModel = createViewModel(processRadioResponseUseCase = ProcessRadioResponseUseCase()) + + viewModel.setConfig(Config(lora = Config.LoRaConfig(region = Config.LoRaConfig.RegionCode.US))) + runCurrent() + assertTrue(viewModel.radioConfigState.value.responseState is ResponseState.Loading) + + // Firmware 2.8 renumbered itself on the first region set: the ACK comes from the new number. + packetFlow.emit(routingAck(requestId = 77, from = 456)) + runCurrent() + + verify { connectionManager.startConfigOnly() } + assertTrue(viewModel.radioConfigState.value.responseState is ResponseState.Success) + } + + @Test + fun `a session opened on the connected node by number follows it after the renumber`() = runTest { + nodeRepository.setNodes(listOf(Node(num = 123, user = User(id = "!123")))) + nodeRepository.setMyNodeInfo(myNodeInfo(myNodeNum = 123)) + val packetFlow = MutableSharedFlow() + every { serviceRepository.meshPacketFlow } returns packetFlow + val writeTargets = mutableListOf() + var nextPacketId = 90 + everySuspend { radioConfigUseCase.setConfig(any(), any(), any()) } calls + { + writeTargets += it.args[0] as Int + val id = ++nextPacketId + it.args.onRequestIdArg()(id) + id + } + // Node detail → Administration opens the connected node's settings by its number, not as a null local session. + viewModel = createViewModel(destNum = 123, processRadioResponseUseCase = ProcessRadioResponseUseCase()) + assertTrue(viewModel.radioConfigState.value.isLocal) + + viewModel.setConfig(Config(lora = Config.LoRaConfig(region = Config.LoRaConfig.RegionCode.US))) + runCurrent() + packetFlow.emit(routingAck(requestId = 91, from = 456)) + runCurrent() + verify { connectionManager.startConfigOnly() } + assertTrue(viewModel.radioConfigState.value.responseState is ResponseState.Success) + + // The re-handshake lands: the radio reports its new number and is installed under it. + nodeRepository.setMyNodeInfo(myNodeInfo(myNodeNum = 456)) + nodeRepository.setNodes(listOf(Node(num = 456, user = User(id = "!456")))) + runCurrent() + assertTrue(viewModel.radioConfigState.value.isLocal) + + viewModel.setConfig(Config(lora = Config.LoRaConfig(region = Config.LoRaConfig.RegionCode.US))) + runCurrent() + assertEquals(listOf(123, 456), writeTargets) + } + + @Test + fun `remote save acked by an unexpected node neither re-handshakes nor completes`() = runTest { + nodeRepository.setNodes( + listOf(Node(num = 123, user = User(id = "!123")), Node(num = 200, user = User(id = "!200"))), ) - .also { createdViewModels += it } + nodeRepository.setMyNodeInfo(myNodeInfo(myNodeNum = 123)) + val packetFlow = MutableSharedFlow() + every { serviceRepository.meshPacketFlow } returns packetFlow + everySuspend { radioConfigUseCase.setConfig(any(), any(), any()) } calls + { + it.args.onRequestIdArg()(78) + 78 + } + viewModel = createViewModel(destNum = 200, processRadioResponseUseCase = ProcessRadioResponseUseCase()) + + viewModel.setConfig(Config(lora = Config.LoRaConfig(region = Config.LoRaConfig.RegionCode.US))) + runCurrent() + assertTrue(viewModel.radioConfigState.value.responseState is ResponseState.Loading) + + packetFlow.emit(routingAck(requestId = 78, from = 456)) + runCurrent() + + verify(exactly(0)) { connectionManager.startConfigOnly() } + val state = viewModel.radioConfigState.value + assertTrue( + state.responseState is ResponseState.Loading, + "expected Loading, got ${state.responseState} (isLocal=${state.isLocal}, route='${state.route}')", + ) + } @Test fun `setConfig calls useCase`() = runTest { @@ -2092,3 +2193,14 @@ class RadioConfigViewModelTest { /** Extracts the trailing `onRequestId` callback from a mocked request method's args. */ @Suppress("UNCHECKED_CAST") private fun List.onRequestIdArg(): (Int) -> Unit = last() as (Int) -> Unit + +/** A real ROUTING_APP ack for [requestId] as the radio would deliver it, sent by node [from]. */ +private fun routingAck(requestId: Int, from: Int) = MeshPacket( + from = from, + decoded = + Data( + portnum = PortNum.ROUTING_APP, + request_id = requestId, + payload = ByteString.of(*Routing(error_reason = Routing.Error.NONE).encode()), + ), +)