From 844a59c749afdbc331074666eff9ae2d6caf2045 Mon Sep 17 00:00:00 2001 From: James Rich Date: Tue, 28 Jul 2026 18:49:15 -0500 Subject: [PATCH 1/2] fix(ui): stop discarding measured-zero sensor and RSSI readings The protobuf models are Wire-generated, so presence IS nullability. A `(x ?: 0f) != 0f` guard collapses "absent" and "measured zero" into one state and throws away a real reading. Ten live sites did this. Replace the zero-guards with `?.let { }` presence checks, and keep RSSI nullable end-to-end so an unknown signal no longer renders as 0 dBm -- the strongest value on that scale. Humidity keeps its zero-guard: 0 %RH is not physically reachable. Co-Authored-By: Claude Opus 5 --- .../no-float-metric-zero-sentinel.yml | 20 +++ .../ast-grep-rules/no-rssi-zero-default.yml | 15 +++ .../org/meshtastic/core/ble/BleDevice.kt | 7 +- .../core/ble/MeshtasticBleDevice.kt | 4 +- .../core/ble/MeshtasticBleDeviceRssiTest.kt | 55 ++++++++ .../kotlin/org/meshtastic/core/model/Node.kt | 28 ++--- .../core/model/NodeTelemetryStringsTest.kt | 72 +++++++++++ .../core/network/radio/BleRadioTransport.kt | 2 +- .../org/meshtastic/core/testing/FakeBle.kt | 6 +- .../meshtastic/core/ui/component/NodeItem.kt | 17 +-- .../core/ui/component/NodeItemCompact.kt | 6 +- .../ui/component/NodeItemZeroMetricsTest.kt | 119 ++++++++++++++++++ feature/connections/build.gradle.kts | 7 ++ .../component/ConnectionsPreviews.kt | 2 +- .../ui/components/CurrentlyConnectedInfo.kt | 5 +- .../ui/components/DeviceListItem.kt | 12 +- .../ui/components/DeviceListItemRssiTest.kt | 64 ++++++++++ feature/node/build.gradle.kts | 7 ++ .../feature/node/component/PowerMetrics.kt | 30 +++-- .../component/PowerMetricsZeroVoltageTest.kt | 60 +++++++++ 20 files changed, 477 insertions(+), 61 deletions(-) create mode 100644 .coderabbit/ast-grep-rules/no-float-metric-zero-sentinel.yml create mode 100644 .coderabbit/ast-grep-rules/no-rssi-zero-default.yml create mode 100644 core/ble/src/commonTest/kotlin/org/meshtastic/core/ble/MeshtasticBleDeviceRssiTest.kt create mode 100644 core/model/src/commonTest/kotlin/org/meshtastic/core/model/NodeTelemetryStringsTest.kt create mode 100644 core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/NodeItemZeroMetricsTest.kt create mode 100644 feature/connections/src/jvmTest/kotlin/org/meshtastic/feature/connections/ui/components/DeviceListItemRssiTest.kt create mode 100644 feature/node/src/jvmTest/kotlin/org/meshtastic/feature/node/component/PowerMetricsZeroVoltageTest.kt diff --git a/.coderabbit/ast-grep-rules/no-float-metric-zero-sentinel.yml b/.coderabbit/ast-grep-rules/no-float-metric-zero-sentinel.yml new file mode 100644 index 0000000000..1070d309a2 --- /dev/null +++ b/.coderabbit/ast-grep-rules/no-float-metric-zero-sentinel.yml @@ -0,0 +1,20 @@ +# Recurring defect class A — see "Presence vs sentinel zero" in .skills/code-review/SKILL.md. +# +# The protobuf models are Wire-generated, so presence IS nullability: EnvironmentMetrics.temperature and friends are +# declared `Float? = null` and there is no hasX() accessor. A `(x ?: 0f) != 0f` guard collapses "absent" and +# "measured zero" into one state and silently discards a real reading. Use `x?.let { ... }` instead — see +# `gatherSensors` in core/ui/.../NodeItem.kt for the reference pattern. +# +# Humidity is the deliberate exception: 0 %RH is not physically reachable, so relative_humidity / co2_humidity keep +# their zero-guards and are not listed here. Same for barometric_pressure (0 hPa is a vacuum). +id: no-float-metric-zero-sentinel +language: kotlin +severity: error +message: "0 is a real reading on this scale — use `?.let { }` instead of a zero-guard." +rule: + any: + - pattern: "($X ?: 0f) != 0f" + - pattern: "($X ?: 0f) == 0f" +constraints: + X: + regex: "(temperature|voltage|current|soil_moisture)$" diff --git a/.coderabbit/ast-grep-rules/no-rssi-zero-default.yml b/.coderabbit/ast-grep-rules/no-rssi-zero-default.yml new file mode 100644 index 0000000000..c732a159b2 --- /dev/null +++ b/.coderabbit/ast-grep-rules/no-rssi-zero-default.yml @@ -0,0 +1,15 @@ +# Recurring defect class A — see "Presence vs sentinel zero" in .skills/code-review/SKILL.md. +# +# 0 dBm is the STRONGEST value on the RSSI scale, so defaulting a missing reading to 0 renders an unknown signal as an +# excellent one. Keep the value nullable end-to-end and let `MetricFormatter.rssi(null)` render an em dash. +id: no-rssi-zero-default +language: kotlin +severity: error +message: "0 dBm is the strongest RSSI, not \"unknown\" — keep the value nullable." +rule: + any: + - pattern: "$X ?: 0" + - pattern: "($X ?: 0) != 0" +constraints: + X: + regex: "(?i)rssi$" diff --git a/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/BleDevice.kt b/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/BleDevice.kt index a6fecb9e3b..ea51dad158 100644 --- a/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/BleDevice.kt +++ b/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/BleDevice.kt @@ -44,8 +44,11 @@ interface BleDevice { val rssi: Int? get() = null - /** Reads the current RSSI value. */ - suspend fun readRssi(): Int + /** + * Reads the current RSSI value in dBm, or `null` when no reading is available (no live connection and no scan + * advertisement). 0 dBm is the strongest value on this scale, so it must never stand in for "unknown". + */ + suspend fun readRssi(): Int? /** Bond the device. */ suspend fun bond() diff --git a/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/MeshtasticBleDevice.kt b/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/MeshtasticBleDevice.kt index b4e1f03f61..3e00ccd025 100644 --- a/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/MeshtasticBleDevice.kt +++ b/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/MeshtasticBleDevice.kt @@ -55,12 +55,12 @@ class MeshtasticBleDevice( override val rssi: Int? = advertisement?.rssi @OptIn(ExperimentalKableApi::class) - override suspend fun readRssi(): Int { + override suspend fun readRssi(): Int? { val active = ActiveBleConnection.active return if (active != null && active.address == address) { active.peripheral.rssi() } else { - advertisement?.rssi ?: 0 + advertisement?.rssi } } diff --git a/core/ble/src/commonTest/kotlin/org/meshtastic/core/ble/MeshtasticBleDeviceRssiTest.kt b/core/ble/src/commonTest/kotlin/org/meshtastic/core/ble/MeshtasticBleDeviceRssiTest.kt new file mode 100644 index 0000000000..8cbbef353e --- /dev/null +++ b/core/ble/src/commonTest/kotlin/org/meshtastic/core/ble/MeshtasticBleDeviceRssiTest.kt @@ -0,0 +1,55 @@ +/* + * Copyright (c) 2026 Meshtastic LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.meshtastic.core.ble + +import com.juul.kable.Advertisement +import dev.mokkery.MockMode +import dev.mokkery.answering.returns +import dev.mokkery.every +import dev.mokkery.mock +import kotlinx.coroutines.test.runTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull + +/** + * 0 dBm is the strongest value on the RSSI scale, so a bonded-only device with no advertisement must report `null` + * rather than collapse into an excellent-signal reading. Both cases are pinned together. + */ +class MeshtasticBleDeviceRssiTest { + + @Test + fun `advertised zero rssi is reported`() = runTest { + val advertisement: Advertisement = mock(MockMode.autofill) { every { rssi } returns 0 } + val device = MeshtasticBleDevice(address = ADDRESS, advertisement = advertisement) + + assertEquals(0, device.rssi) + assertEquals(0, device.readRssi()) + } + + @Test + fun `bonded-only device without an advertisement reports no rssi`() = runTest { + val device = MeshtasticBleDevice(address = ADDRESS) + + assertNull(device.rssi) + assertNull(device.readRssi()) + } + + private companion object { + const val ADDRESS = "AA:BB:CC:DD:EE:FF" + } +} diff --git a/core/model/src/commonMain/kotlin/org/meshtastic/core/model/Node.kt b/core/model/src/commonMain/kotlin/org/meshtastic/core/model/Node.kt index 15a67bd32c..02f54367f3 100644 --- a/core/model/src/commonMain/kotlin/org/meshtastic/core/model/Node.kt +++ b/core/model/src/commonMain/kotlin/org/meshtastic/core/model/Node.kt @@ -142,30 +142,16 @@ data class Node( fun gpsString(): String = GPSFormat.toDec(latitude, longitude) - @Suppress("CyclomaticComplexMethod") private fun EnvironmentMetrics.getDisplayStrings(isFahrenheit: Boolean): List { - val temp = - if ((temperature ?: 0f) != 0f) { - MetricFormatter.temperature(temperature ?: 0f, isFahrenheit) - } else { - null - } + // These fields carry presence: `null` means "no sensor", so 0 °C / 0 V / 0 A / 0% are real readings and must + // still render. Humidity keeps its zero-guard because 0% RH is not physically reachable. + val temp = temperature?.let { MetricFormatter.temperature(it, isFahrenheit) } val humidity = if ((relative_humidity ?: 0f) != 0f) MetricFormatter.humidity(relative_humidity ?: 0f) else null - val soilTemperatureStr = - if ((soil_temperature ?: 0f) != 0f) { - MetricFormatter.temperature(soil_temperature ?: 0f, isFahrenheit) - } else { - null - } + val soilTemperatureStr = soil_temperature?.let { MetricFormatter.temperature(it, isFahrenheit) } val soilMoistureRange = 0..100 - val soilMoisture = - if ((soil_moisture ?: Int.MIN_VALUE) in soilMoistureRange && (soil_temperature ?: 0f) != 0f) { - MetricFormatter.percent(soil_moisture ?: 0) - } else { - null - } - val voltage = if ((this.voltage ?: 0f) != 0f) MetricFormatter.voltage(this.voltage ?: 0f) else null - val current = if ((current ?: 0f) != 0f) MetricFormatter.current(current ?: 0f) else null + val soilMoisture = soil_moisture?.takeIf { it in soilMoistureRange }?.let { MetricFormatter.percent(it) } + val voltage = this.voltage?.let { MetricFormatter.voltage(it) } + val current = current?.let { MetricFormatter.current(it) } val iaq = if ((iaq ?: 0) != 0) "IAQ: $iaq" else null return listOfNotNull( diff --git a/core/model/src/commonTest/kotlin/org/meshtastic/core/model/NodeTelemetryStringsTest.kt b/core/model/src/commonTest/kotlin/org/meshtastic/core/model/NodeTelemetryStringsTest.kt new file mode 100644 index 0000000000..10c50c9f66 --- /dev/null +++ b/core/model/src/commonTest/kotlin/org/meshtastic/core/model/NodeTelemetryStringsTest.kt @@ -0,0 +1,72 @@ +/* + * Copyright (c) 2026 Meshtastic LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.meshtastic.core.model + +import org.meshtastic.proto.EnvironmentMetrics +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * `EnvironmentMetrics` fields are Wire-generated and nullable, so `null` is the only "not reported" signal. Each metric + * is pinned twice — absent and measured-zero — because either assertion alone lets the two states collapse back into + * one. + */ +class NodeTelemetryStringsTest { + + private fun telemetry(metrics: EnvironmentMetrics) = + Node(num = 1, environmentMetrics = metrics).getTelemetryStrings() + + @Test + fun absent_environment_metrics_render_nothing() { + assertEquals(emptyList(), telemetry(EnvironmentMetrics())) + } + + @Test + fun zero_temperature_is_reported() { + assertEquals(listOf("0.0°C"), telemetry(EnvironmentMetrics(temperature = 0f))) + } + + @Test + fun zero_voltage_and_current_are_reported() { + val strings = telemetry(EnvironmentMetrics(voltage = 0f, current = 0f)) + assertEquals(listOf("0.00 V", "0.0 mA"), strings) + } + + @Test + fun zero_soil_readings_are_reported() { + val strings = telemetry(EnvironmentMetrics(soil_temperature = 0f, soil_moisture = 0)) + assertEquals(listOf("0.0°C", "0%"), strings) + } + + @Test + fun soil_moisture_no_longer_requires_a_soil_temperature() { + assertEquals(listOf("42%"), telemetry(EnvironmentMetrics(soil_moisture = 42))) + } + + @Test + fun out_of_range_soil_moisture_is_still_rejected() { + assertEquals(emptyList(), telemetry(EnvironmentMetrics(soil_moisture = 101))) + } + + @Test + fun zero_humidity_stays_filtered() { + // 0 %RH is not physically reachable, so unlike the other metrics its zero-guard is intentional. + assertTrue(telemetry(EnvironmentMetrics(relative_humidity = 0f)).isEmpty()) + assertEquals(listOf("41%"), telemetry(EnvironmentMetrics(relative_humidity = 41f))) + } +} diff --git a/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt b/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt index 70a01b1208..9eac91d4c0 100644 --- a/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt +++ b/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt @@ -506,7 +506,7 @@ class BleRadioTransport( try { bleConnection.deviceFlow.first()?.let { device -> val rssi = retryBleOperation(tag = address) { device.readRssi() } - Logger.d { "[$address] Connection confirmed. Initial RSSI: $rssi dBm" } + Logger.d { "[$address] Connection confirmed. Initial RSSI: ${rssi?.let { "$it dBm" } ?: "unknown"}" } } } catch (e: CancellationException) { throw e diff --git a/core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeBle.kt b/core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeBle.kt index 4d26be4e0e..adb0c05572 100644 --- a/core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeBle.kt +++ b/core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeBle.kt @@ -56,7 +56,7 @@ class FakeBleDevice( override val isConnected: Boolean get() = _state.value == BleConnectionState.Connected - override suspend fun readRssi(): Int = DEFAULT_RSSI + override suspend fun readRssi(): Int? = rssi override suspend fun bond() { _isBonded.value = true @@ -65,10 +65,6 @@ class FakeBleDevice( fun setState(newState: BleConnectionState) { _state.value = newState } - - companion object { - private const val DEFAULT_RSSI = -60 - } } class FakeBleScanner : diff --git a/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItem.kt b/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItem.kt index ea878d69fe..531c3cffae 100644 --- a/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItem.kt +++ b/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItem.kt @@ -389,26 +389,27 @@ private fun gatherSensors(node: Node, tempInFahrenheit: Boolean, contentColor: C ) } } - if ((env.soil_temperature ?: 0f) != 0f) { - val temp = MetricFormatter.temperature(env.soil_temperature ?: 0f, tempInFahrenheit) + // Soil temperature, moisture, voltage and current all carry presence: `null` is "no sensor", 0 is a real reading. + env.soil_temperature?.let { soilTemperature -> + val temp = MetricFormatter.temperature(soilTemperature, tempInFahrenheit) items.add { SoilTemperatureInfo(temp = temp, contentColor = contentColor) } } - if ((env.soil_moisture ?: 0) != 0 && (env.soil_temperature ?: 0f) != 0f) { - items.add { SoilMoistureInfo(moisture = "${env.soil_moisture}%", contentColor = contentColor) } + env.soil_moisture?.let { soilMoisture -> + items.add { SoilMoistureInfo(moisture = "$soilMoisture%", contentColor = contentColor) } } - if ((env.voltage ?: 0f) != 0f) { + env.voltage?.let { voltage -> items.add { PowerInfo( - value = MetricFormatter.voltage(env.voltage ?: 0f), + value = MetricFormatter.voltage(voltage), label = stringResource(Res.string.voltage), contentColor = contentColor, ) } } - if ((env.current ?: 0f) != 0f) { + env.current?.let { current -> items.add { PowerInfo( - value = MetricFormatter.current(env.current ?: 0f), + value = MetricFormatter.current(current), label = stringResource(Res.string.current), contentColor = contentColor, ) diff --git a/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItemCompact.kt b/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItemCompact.kt index 74671cf288..25d7b5962b 100644 --- a/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItemCompact.kt +++ b/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItemCompact.kt @@ -449,8 +449,10 @@ private fun CompactMetricsRow(thatNode: Node, tempInFahrenheit: Boolean, content val env = thatNode.environmentMetrics val segments = buildList<@Composable () -> Unit> { - if ((env.temperature ?: 0f) != 0f) { - val temp = MetricFormatter.temperature(env.temperature ?: 0f, tempInFahrenheit) + // Temperature carries presence, so `null` already means "no sensor" — testing against 0 hid an ordinary + // 0 °C reading. Mirrors the fix already made in NodeItem's gatherSensors. + env.temperature?.let { temperature -> + val temp = MetricFormatter.temperature(temperature, tempInFahrenheit) add { IconInfo( icon = MeshtasticIcons.Temperature, diff --git a/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/NodeItemZeroMetricsTest.kt b/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/NodeItemZeroMetricsTest.kt new file mode 100644 index 0000000000..63c50e73da --- /dev/null +++ b/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/NodeItemZeroMetricsTest.kt @@ -0,0 +1,119 @@ +/* + * Copyright (c) 2026 Meshtastic LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.meshtastic.core.ui.component + +import androidx.compose.material3.MaterialTheme +import androidx.compose.ui.test.ComposeUiTest +import androidx.compose.ui.test.ExperimentalTestApi +import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.v2.runComposeUiTest +import org.meshtastic.core.model.ConnectionState +import org.meshtastic.core.model.Node +import org.meshtastic.proto.Config.DisplayConfig.DisplayUnits +import org.meshtastic.proto.EnvironmentMetrics +import org.meshtastic.proto.User +import kotlin.test.Test + +/** + * `EnvironmentMetrics` fields are Wire-generated and nullable, so `null` is the only "not reported" signal. Each metric + * is pinned twice — absent and measured-zero — because either assertion alone lets the two states collapse back into + * one. + */ +@OptIn(ExperimentalTestApi::class) +class NodeItemZeroMetricsTest { + + @Test + fun nodeItem_showsZeroTemperature() = runComposeUiTest { + setNodeItem(EnvironmentMetrics(temperature = 0f)) + onNodeWithText("0.0°C").assertIsDisplayed() + } + + @Test + fun nodeItem_hidesAbsentTemperature() = runComposeUiTest { + setNodeItem(EnvironmentMetrics()) + onNodeWithText("0.0°C").assertDoesNotExist() + } + + @Test + fun nodeItem_showsZeroVoltageAndCurrent() = runComposeUiTest { + setNodeItem(EnvironmentMetrics(voltage = 0f, current = 0f)) + onNodeWithText("0.00 V").assertIsDisplayed() + onNodeWithText("0.0 mA").assertIsDisplayed() + } + + @Test + fun nodeItem_hidesAbsentVoltageAndCurrent() = runComposeUiTest { + setNodeItem(EnvironmentMetrics()) + onNodeWithText("0.00 V").assertDoesNotExist() + onNodeWithText("0.0 mA").assertDoesNotExist() + } + + @Test + fun nodeItem_showsZeroSoilReadings() = runComposeUiTest { + setNodeItem(EnvironmentMetrics(soil_temperature = 0f, soil_moisture = 0)) + onNodeWithText("0.0°C").assertIsDisplayed() + onNodeWithText("0%").assertIsDisplayed() + } + + @Test + fun nodeItem_hidesAbsentSoilReadings() = runComposeUiTest { + setNodeItem(EnvironmentMetrics()) + onNodeWithText("0.0°C").assertDoesNotExist() + onNodeWithText("0%").assertDoesNotExist() + } + + @Test + fun nodeItem_showsSoilMoistureWithoutSoilTemperature() = runComposeUiTest { + // The old guard required a non-zero soil temperature before moisture was drawn at all. + setNodeItem(EnvironmentMetrics(soil_moisture = 42)) + onNodeWithText("42%").assertIsDisplayed() + } + + @Test + fun nodeItemCompact_showsZeroTemperature() = runComposeUiTest { + setNodeItemCompact(EnvironmentMetrics(temperature = 0f)) + onNodeWithText("0.0°C").assertIsDisplayed() + } + + @Test + fun nodeItemCompact_hidesAbsentTemperature() = runComposeUiTest { + setNodeItemCompact(EnvironmentMetrics()) + onNodeWithText("0.0°C").assertDoesNotExist() + } + + private fun ComposeUiTest.setNodeItem(metrics: EnvironmentMetrics) = setContent { + MaterialTheme { + NodeItem( + thisNode = null, + thatNode = node(metrics), + distanceUnits = DisplayUnits.METRIC.value, + tempInFahrenheit = false, + connectionState = ConnectionState.Connected, + ) + } + } + + private fun ComposeUiTest.setNodeItemCompact(metrics: EnvironmentMetrics) = setContent { + MaterialTheme { + NodeItemCompact(thisNode = null, thatNode = node(metrics), distanceUnits = DisplayUnits.METRIC.value) + } + } + + private fun node(metrics: EnvironmentMetrics) = + Node(num = 2, user = User(id = "!2", long_name = "Sensor"), environmentMetrics = metrics) +} diff --git a/feature/connections/build.gradle.kts b/feature/connections/build.gradle.kts index b1c2c0d5df..31c784b73b 100644 --- a/feature/connections/build.gradle.kts +++ b/feature/connections/build.gradle.kts @@ -41,5 +41,12 @@ kotlin { } androidMain.dependencies { implementation(libs.usb.serial.android) } + + // Compose UI tests live in jvmTest, not commonTest: this module enables android host tests, and the + // androidHostTest stubs leave Build.FINGERPRINT null, which the Compose Robolectric idling strategy NPEs on. + jvmTest.dependencies { + implementation(libs.compose.multiplatform.ui.test) + implementation(compose.desktop.currentOs) + } } } diff --git a/feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/component/ConnectionsPreviews.kt b/feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/component/ConnectionsPreviews.kt index 130d557fc4..44d96319d1 100644 --- a/feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/component/ConnectionsPreviews.kt +++ b/feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/component/ConnectionsPreviews.kt @@ -262,7 +262,7 @@ private class PreviewBleDevice( override val isBonded: Boolean = true override val isConnected: Boolean = false - override suspend fun readRssi(): Int = rssi ?: PREVIEW_BLE_RSSI + override suspend fun readRssi(): Int? = rssi override suspend fun bond() = Unit } diff --git a/feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/components/CurrentlyConnectedInfo.kt b/feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/components/CurrentlyConnectedInfo.kt index 01ed30d3e7..35b918d420 100644 --- a/feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/components/CurrentlyConnectedInfo.kt +++ b/feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/components/CurrentlyConnectedInfo.kt @@ -30,7 +30,7 @@ import androidx.compose.runtime.Composable import androidx.compose.runtime.Immutable import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue -import androidx.compose.runtime.mutableIntStateOf +import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment @@ -85,7 +85,8 @@ fun CurrentlyConnectedInfo( modifier: Modifier = Modifier, bleDevice: DeviceListEntry.Ble? = null, ) { - var rssi by remember { mutableIntStateOf(0) } + // Null until the first successful read: 0 dBm is the strongest value on this scale, not "unknown". + var rssi by remember(bleDevice?.device?.address) { mutableStateOf(null) } LaunchedEffect(bleDevice) { if (bleDevice == null) return@LaunchedEffect while (bleDevice.device.isConnected) { diff --git a/feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/components/DeviceListItem.kt b/feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/components/DeviceListItem.kt index b8f24aa38d..9f4ae158ab 100644 --- a/feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/components/DeviceListItem.kt +++ b/feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/components/DeviceListItem.kt @@ -37,7 +37,7 @@ import androidx.compose.material3.Text import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue -import androidx.compose.runtime.mutableIntStateOf +import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.rememberUpdatedState import androidx.compose.runtime.setValue @@ -82,13 +82,15 @@ fun DeviceListItem( onDelete: (() -> Unit)? = null, rssi: Int? = null, ) { - // Throttle the RSSI updates to match the connected device polling rate - var displayedRssi by remember { mutableIntStateOf(rssi ?: 0) } + // Throttle the RSSI updates to match the connected device polling rate. The value stays nullable end-to-end: + // 0 dBm is the strongest reading on this scale, so defaulting to it would render an unknown signal as excellent. + // Keyed by address so a recycled list slot drops the previous device's reading instead of showing it for a tick. + var displayedRssi by remember(device.address) { mutableStateOf(rssi) } val currentRssi by rememberUpdatedState(rssi) - LaunchedEffect(Unit) { + LaunchedEffect(device.address) { while (true) { delay(RSSI_UPDATE_RATE_MS) - displayedRssi = currentRssi ?: 0 + displayedRssi = currentRssi } } diff --git a/feature/connections/src/jvmTest/kotlin/org/meshtastic/feature/connections/ui/components/DeviceListItemRssiTest.kt b/feature/connections/src/jvmTest/kotlin/org/meshtastic/feature/connections/ui/components/DeviceListItemRssiTest.kt new file mode 100644 index 0000000000..934be791eb --- /dev/null +++ b/feature/connections/src/jvmTest/kotlin/org/meshtastic/feature/connections/ui/components/DeviceListItemRssiTest.kt @@ -0,0 +1,64 @@ +/* + * Copyright (c) 2026 Meshtastic LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.meshtastic.feature.connections.ui.components + +import androidx.compose.material3.MaterialTheme +import androidx.compose.ui.test.ComposeUiTest +import androidx.compose.ui.test.ExperimentalTestApi +import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.v2.runComposeUiTest +import org.meshtastic.core.model.ConnectionState +import org.meshtastic.feature.connections.model.DeviceListEntry +import kotlin.test.Test + +/** + * 0 dBm is the strongest value on the RSSI scale, so an unknown signal must not render as one. The null and zero cases + * are pinned together — either alone lets the two states collapse back into a single "0". + */ +@OptIn(ExperimentalTestApi::class) +class DeviceListItemRssiTest { + + @Test + fun zeroRssi_rendersAsZeroDbm() = runComposeUiTest { + setDeviceListItem(rssi = 0) + onNodeWithText("0 dBm", substring = true).assertIsDisplayed() + } + + @Test + fun negativeRssi_rendersTheReading() = runComposeUiTest { + setDeviceListItem(rssi = -70) + onNodeWithText("-70 dBm", substring = true).assertIsDisplayed() + } + + @Test + fun absentRssi_rendersNoReadingAtAll() = runComposeUiTest { + setDeviceListItem(rssi = null) + onNodeWithText("dBm", substring = true).assertDoesNotExist() + } + + private fun ComposeUiTest.setDeviceListItem(rssi: Int?) = setContent { + MaterialTheme { + DeviceListItem( + connectionState = ConnectionState.Disconnected, + device = DeviceListEntry.Tcp(name = "Sensor", fullAddress = "t192.168.0.2"), + onSelect = {}, + rssi = rssi, + ) + } + } +} diff --git a/feature/node/build.gradle.kts b/feature/node/build.gradle.kts index 4e0a843541..56205802ca 100644 --- a/feature/node/build.gradle.kts +++ b/feature/node/build.gradle.kts @@ -54,5 +54,12 @@ kotlin { } androidMain.dependencies { implementation(libs.markdown.renderer.android) } + + // Compose UI tests live in jvmTest, not commonTest: this module enables android host tests, and the + // androidHostTest stubs leave Build.FINGERPRINT null, which the Compose Robolectric idling strategy NPEs on. + jvmTest.dependencies { + implementation(libs.compose.multiplatform.ui.test) + implementation(compose.desktop.currentOs) + } } } diff --git a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/component/PowerMetrics.kt b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/component/PowerMetrics.kt index 02e86edf9f..c266ef0e4a 100644 --- a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/component/PowerMetrics.kt +++ b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/component/PowerMetrics.kt @@ -29,9 +29,9 @@ import org.meshtastic.core.ui.icon.Voltage import org.meshtastic.feature.node.model.VectorMetricInfo /** - * Displays power metrics for a node: for every channel reporting a non-zero voltage, its voltage and — when the channel - * reports one — its current are stacked in a single vertical column so the pair stays visually grouped and the columns - * flow side by side. + * Displays power metrics for a node: for every channel reporting a voltage, its voltage and — when the channel reports + * one — its current are stacked in a single vertical column so the pair stays visually grouped and the columns flow + * side by side. */ @Composable internal fun PowerMetrics(node: Node) { @@ -43,15 +43,21 @@ internal fun PowerMetrics(node: Node) { Triple(Res.string.channel_3, ch3_voltage, ch3_current), ) } - .filter { (_, voltage, _) -> (voltage ?: 0f) != 0f } - .map { (label, voltage, current) -> - // A reported current of 0mA is a real reading and is shown; only an absent one is hidden. - listOfNotNull( - VectorMetricInfo(label, "${NumberFormatter.format(voltage ?: 0f, 2)}V", MeshtasticIcons.Voltage), - current?.let { - VectorMetricInfo(label, "${NumberFormatter.format(it, 1)}mA", MeshtasticIcons.PowerSupply) - }, - ) + .mapNotNull { (label, voltage, current) -> + // Voltage and current both carry presence: an absent reading is hidden, but a reported 0V / 0mA is a + // real measurement and is shown. + voltage?.let { volts -> + listOfNotNull( + VectorMetricInfo(label, "${NumberFormatter.format(volts, 2)}V", MeshtasticIcons.Voltage), + current?.let { milliAmps -> + VectorMetricInfo( + label, + "${NumberFormatter.format(milliAmps, 1)}mA", + MeshtasticIcons.PowerSupply, + ) + }, + ) + } } MetricCardFlow(groups = channels) diff --git a/feature/node/src/jvmTest/kotlin/org/meshtastic/feature/node/component/PowerMetricsZeroVoltageTest.kt b/feature/node/src/jvmTest/kotlin/org/meshtastic/feature/node/component/PowerMetricsZeroVoltageTest.kt new file mode 100644 index 0000000000..f591a6780e --- /dev/null +++ b/feature/node/src/jvmTest/kotlin/org/meshtastic/feature/node/component/PowerMetricsZeroVoltageTest.kt @@ -0,0 +1,60 @@ +/* + * Copyright (c) 2026 Meshtastic LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.meshtastic.feature.node.component + +import androidx.compose.material3.MaterialTheme +import androidx.compose.ui.test.ComposeUiTest +import androidx.compose.ui.test.ExperimentalTestApi +import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.v2.runComposeUiTest +import org.meshtastic.core.model.Node +import kotlin.test.Test +import org.meshtastic.proto.PowerMetrics as PowerMetricsProto + +/** + * `PowerMetrics` channel readings are Wire-generated and nullable, so `null` is the only "no channel" signal. Both the + * absent and the measured-zero case are pinned — either alone lets the two states collapse back into one. + */ +@OptIn(ExperimentalTestApi::class) +class PowerMetricsZeroVoltageTest { + + @Test + fun zeroVoltageChannelIsShown() = runComposeUiTest { + setPowerMetrics(PowerMetricsProto(ch1_voltage = 0f, ch1_current = 0f)) + onNodeWithText("0.00V").assertIsDisplayed() + onNodeWithText("0.0mA").assertIsDisplayed() + } + + @Test + fun absentChannelIsHidden() = runComposeUiTest { + setPowerMetrics(PowerMetricsProto()) + onNodeWithText("0.00V").assertDoesNotExist() + onNodeWithText("0.0mA").assertDoesNotExist() + } + + @Test + fun reportedVoltageWithoutCurrentShowsVoltageOnly() = runComposeUiTest { + setPowerMetrics(PowerMetricsProto(ch1_voltage = 3.7f)) + onNodeWithText("3.70V").assertIsDisplayed() + onNodeWithText("0.0mA").assertDoesNotExist() + } + + private fun ComposeUiTest.setPowerMetrics(metrics: PowerMetricsProto) = setContent { + MaterialTheme { PowerMetrics(node = Node(num = 1, powerMetrics = metrics)) } + } +} From c6d20c1e65ac3b0db56ba7071041ecb2dcf4043c Mon Sep 17 00:00:00 2001 From: James Rich Date: Tue, 28 Jul 2026 19:11:57 -0500 Subject: [PATCH 2/2] =?UTF-8?q?fix(ui):=20address=20review=20=E2=80=94=20r?= =?UTF-8?q?ange-check=20soil=20moisture,=20anonymize=20BLE=20address?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - NodeItem soil moisture now range-checks 0..100 and formats via MetricFormatter, matching Node.getTelemetryStrings. Previously a sensor fault reporting 101% rendered in the UI but not in the telemetry strings. - Anonymize the address in the BleRadioTransport connection-confirmed log. - ast-grep float rule now covers the 0F / 0.0f / 0.0F literal spellings so a reformat cannot bypass it. - Document why the RSSI rule stops at `?: 0` and deliberately does not match `== 0` against stored rows (Reaction.kt reads pre-schema-51 data where 0 really is indistinguishable from "no reading"). Co-Authored-By: Claude Opus 5 --- .../ast-grep-rules/no-float-metric-zero-sentinel.yml | 7 +++++++ .coderabbit/ast-grep-rules/no-rssi-zero-default.yml | 5 +++++ .../core/network/radio/BleRadioTransport.kt | 5 ++++- .../org/meshtastic/core/ui/component/NodeItem.kt | 12 +++++++++--- .../core/ui/component/NodeItemZeroMetricsTest.kt | 7 +++++++ 5 files changed, 32 insertions(+), 4 deletions(-) diff --git a/.coderabbit/ast-grep-rules/no-float-metric-zero-sentinel.yml b/.coderabbit/ast-grep-rules/no-float-metric-zero-sentinel.yml index 1070d309a2..04485c055a 100644 --- a/.coderabbit/ast-grep-rules/no-float-metric-zero-sentinel.yml +++ b/.coderabbit/ast-grep-rules/no-float-metric-zero-sentinel.yml @@ -11,10 +11,17 @@ id: no-float-metric-zero-sentinel language: kotlin severity: error message: "0 is a real reading on this scale — use `?.let { }` instead of a zero-guard." +# Every spelling of the zero-float literal is listed: a reformat from `0f` to `0.0f` must not slip past the rule. rule: any: - pattern: "($X ?: 0f) != 0f" - pattern: "($X ?: 0f) == 0f" + - pattern: "($X ?: 0F) != 0F" + - pattern: "($X ?: 0F) == 0F" + - pattern: "($X ?: 0.0f) != 0.0f" + - pattern: "($X ?: 0.0f) == 0.0f" + - pattern: "($X ?: 0.0F) != 0.0F" + - pattern: "($X ?: 0.0F) == 0.0F" constraints: X: regex: "(temperature|voltage|current|soil_moisture)$" diff --git a/.coderabbit/ast-grep-rules/no-rssi-zero-default.yml b/.coderabbit/ast-grep-rules/no-rssi-zero-default.yml index c732a159b2..0d1f437f03 100644 --- a/.coderabbit/ast-grep-rules/no-rssi-zero-default.yml +++ b/.coderabbit/ast-grep-rules/no-rssi-zero-default.yml @@ -2,6 +2,11 @@ # # 0 dBm is the STRONGEST value on the RSSI scale, so defaulting a missing reading to 0 renders an unknown signal as an # excellent one. Keep the value nullable end-to-end and let `MetricFormatter.rssi(null)` render an em dash. +# +# Scope: this rule targets DEFAULTING a live reading (`?: 0`), not comparing a stored one. A `rssi == 0` test against +# persisted data can be legitimate migration handling — `Reaction.kt` reads pre-schema-51 rows that stored 0 where the +# column is now nullable, so there a 0 really is indistinguishable from "no reading". Broadening this rule to `== 0` +# would flag that documented exception and nothing else, so it deliberately stops at the `?: 0` form. id: no-rssi-zero-default language: kotlin severity: error diff --git a/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt b/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt index 9eac91d4c0..fe121a7f72 100644 --- a/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt +++ b/core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt @@ -506,7 +506,10 @@ class BleRadioTransport( try { bleConnection.deviceFlow.first()?.let { device -> val rssi = retryBleOperation(tag = address) { device.readRssi() } - Logger.d { "[$address] Connection confirmed. Initial RSSI: ${rssi?.let { "$it dBm" } ?: "unknown"}" } + Logger.d { + "[${address.anonymize()}] Connection confirmed. " + + "Initial RSSI: ${rssi?.let { "$it dBm" } ?: "unknown"}" + } } } catch (e: CancellationException) { throw e diff --git a/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItem.kt b/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItem.kt index 531c3cffae..5d0708cb2d 100644 --- a/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItem.kt +++ b/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItem.kt @@ -394,9 +394,15 @@ private fun gatherSensors(node: Node, tempInFahrenheit: Boolean, contentColor: C val temp = MetricFormatter.temperature(soilTemperature, tempInFahrenheit) items.add { SoilTemperatureInfo(temp = temp, contentColor = contentColor) } } - env.soil_moisture?.let { soilMoisture -> - items.add { SoilMoistureInfo(moisture = "$soilMoisture%", contentColor = contentColor) } - } + // Range-checked to match Node.getTelemetryStrings — a sensor fault reporting 101% is not a reading. + val soilMoistureRange = 0..100 + env.soil_moisture + ?.takeIf { it in soilMoistureRange } + ?.let { soilMoisture -> + items.add { + SoilMoistureInfo(moisture = MetricFormatter.percent(soilMoisture), contentColor = contentColor) + } + } env.voltage?.let { voltage -> items.add { PowerInfo( diff --git a/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/NodeItemZeroMetricsTest.kt b/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/NodeItemZeroMetricsTest.kt index 63c50e73da..68d6e91a46 100644 --- a/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/NodeItemZeroMetricsTest.kt +++ b/core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/NodeItemZeroMetricsTest.kt @@ -84,6 +84,13 @@ class NodeItemZeroMetricsTest { onNodeWithText("42%").assertIsDisplayed() } + @Test + fun nodeItem_hidesOutOfRangeSoilMoisture() = runComposeUiTest { + // A sensor fault reporting 101% is not a reading — matches Node.getTelemetryStrings. + setNodeItem(EnvironmentMetrics(soil_moisture = 101)) + onNodeWithText("101%").assertDoesNotExist() + } + @Test fun nodeItemCompact_showsZeroTemperature() = runComposeUiTest { setNodeItemCompact(EnvironmentMetrics(temperature = 0f))