From 4e30fb5cd56c767cbafda6aac595eb53e495843b Mon Sep 17 00:00:00 2001 From: James Rich <2199651+jamesarich@users.noreply.github.com> Date: Tue, 11 Aug 2026 19:10:28 -0700 Subject: [PATCH] fix(node): chart environment metrics in the user's display units (#6634) Co-authored-by: Claude Fable 5 --- .../core/model/util/UnitConversions.kt | 3 + .../core/model/util/UnitConversionsTest.kt | 35 +++++++ .../feature/node/metrics/EnvironmentCharts.kt | 55 ++++++++++- .../node/metrics/EnvironmentMetrics.kt | 7 +- .../node/metrics/EnvironmentChartUnitsTest.kt | 92 +++++++++++++++++++ 5 files changed, 186 insertions(+), 6 deletions(-) create mode 100644 core/model/src/commonTest/kotlin/org/meshtastic/core/model/util/UnitConversionsTest.kt create mode 100644 feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/metrics/EnvironmentChartUnitsTest.kt diff --git a/core/model/src/commonMain/kotlin/org/meshtastic/core/model/util/UnitConversions.kt b/core/model/src/commonMain/kotlin/org/meshtastic/core/model/util/UnitConversions.kt index 9725bb8202..50bee2eb39 100644 --- a/core/model/src/commonMain/kotlin/org/meshtastic/core/model/util/UnitConversions.kt +++ b/core/model/src/commonMain/kotlin/org/meshtastic/core/model/util/UnitConversions.kt @@ -24,6 +24,9 @@ object UnitConversions { @Suppress("MagicNumber") fun celsiusToFahrenheit(celsius: Float): Float = (celsius * 1.8F) + 32 + @Suppress("MagicNumber") + fun metersPerSecondToMph(metersPerSecond: Float): Float = metersPerSecond * 2.23694f + /** Formats temperature as a string with the unit suffix. */ fun Float.toTempString(isFahrenheit: Boolean): String { if (this.isNaN()) return "--" diff --git a/core/model/src/commonTest/kotlin/org/meshtastic/core/model/util/UnitConversionsTest.kt b/core/model/src/commonTest/kotlin/org/meshtastic/core/model/util/UnitConversionsTest.kt new file mode 100644 index 0000000000..e1264b8e3e --- /dev/null +++ b/core/model/src/commonTest/kotlin/org/meshtastic/core/model/util/UnitConversionsTest.kt @@ -0,0 +1,35 @@ +/* + * 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.util + +import kotlin.test.Test +import kotlin.test.assertEquals + +class UnitConversionsTest { + + @Test + fun celsiusToFahrenheit() { + assertEquals(32f, UnitConversions.celsiusToFahrenheit(0f)) + assertEquals(212f, UnitConversions.celsiusToFahrenheit(100f)) + } + + @Test + fun metersPerSecondToMph() { + assertEquals(0f, UnitConversions.metersPerSecondToMph(0f)) + assertEquals(22.3694f, UnitConversions.metersPerSecondToMph(10f), absoluteTolerance = 0.001f) + } +} diff --git a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/EnvironmentCharts.kt b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/EnvironmentCharts.kt index 3b29ae3964..caf974c873 100644 --- a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/EnvironmentCharts.kt +++ b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/EnvironmentCharts.kt @@ -38,6 +38,7 @@ import com.patrykandpatrick.vico.compose.cartesian.layer.rememberLineCartesianLa import com.patrykandpatrick.vico.compose.common.data.ExtraStore import org.jetbrains.compose.resources.stringResource import org.meshtastic.core.common.util.formatString +import org.meshtastic.core.model.util.UnitConversions import org.meshtastic.core.resources.Res import org.meshtastic.core.resources.baro_pressure import org.meshtastic.core.resources.humidity @@ -154,12 +155,45 @@ internal fun pressureAxisRange(dataMin: Double, dataMax: Double): Pair PRESSURE_DEFAULT_MIN to PRESSURE_DEFAULT_MAX } +/** + * Wind speed arrives in m/s; imperial locales chart it in mph to match the cards (design §10.5). Temperatures are + * already converted upstream by the view model, so they pass through here unchanged. + */ +internal fun chartValue(metric: Environment, telemetry: Telemetry, isImperial: Boolean): Float? = + metric.getValue(telemetry)?.let { + if (metric == Environment.WIND_SPEED && isImperial) UnitConversions.metersPerSecondToMph(it) else it + } + +/** + * Unit suffix for a plotted metric's axis and marker labels, in the user's display units, or "" for metrics whose unit + * would be noise on a shared axis. Includes any leading space, so it appends directly to a formatted value. + */ +internal fun unitSuffix(metric: Environment, isFahrenheit: Boolean, isImperial: Boolean): String = when (metric) { + Environment.TEMPERATURE, + Environment.SOIL_TEMPERATURE, + Environment.ONE_WIRE_TEMP_1, + Environment.ONE_WIRE_TEMP_2, + Environment.ONE_WIRE_TEMP_3, + Environment.ONE_WIRE_TEMP_4, + Environment.ONE_WIRE_TEMP_5, + Environment.ONE_WIRE_TEMP_6, + Environment.ONE_WIRE_TEMP_7, + Environment.ONE_WIRE_TEMP_8, + -> if (isFahrenheit) "°F" else "°C" + + Environment.WIND_SPEED -> if (isImperial) " mph" else " m/s" + + else -> "" +} + @Suppress("LongMethod", "CyclomaticComplexMethod") @Composable fun EnvironmentMetricsChart( modifier: Modifier = Modifier, telemetries: List, graphData: EnvironmentGraphingData, + isFahrenheit: Boolean, + isImperial: Boolean, vicoScrollState: VicoScrollState, selectedX: Double?, onPointSelected: (Double) -> Unit, @@ -186,6 +220,11 @@ fun EnvironmentMetricsChart( } val colorToLabel = allLegendData.associate { it.color to (it.labelOverride ?: stringResource(it.nameRes)) } + val colorToUnit = + allLegendData.associate { legend -> + val metric = legend.metricKey as? Environment + legend.color to (metric?.let { unitSuffix(it, isFahrenheit, isImperial) } ?: "") + } val showPressure = shouldPlot[Environment.BAROMETRIC_PRESSURE.ordinal] && Environment.BAROMETRIC_PRESSURE !in hiddenMetrics @@ -221,7 +260,7 @@ fun EnvironmentMetricsChart( } } - LaunchedEffect(pressureData, otherMetricsData) { + LaunchedEffect(pressureData, otherMetricsData, isImperial) { modelProducer.runTransaction { /* Pressure on its own layer/axis */ if (showPressure && pressureData.isNotEmpty()) { @@ -237,7 +276,10 @@ fun EnvironmentMetricsChart( val metricData = otherMetricsData[metric] ?: emptyList() if (metricData.isNotEmpty()) { lineModel { - series(x = metricData.map { it.time }, y = metricData.map { metric.getValue(it)!! }) + series( + x = metricData.map { it.time }, + y = metricData.map { chartValue(metric, it, isImperial)!! }, + ) } } } @@ -249,7 +291,7 @@ fun EnvironmentMetricsChart( valueFormatter = ChartStyling.createColoredMarkerValueFormatter { value, color -> val label = colorToLabel[color] ?: "" - formatString("%s: %.1f", label, value) + formatString("%s: %.1f", label, value) + (colorToUnit[color] ?: "") }, ) @@ -322,9 +364,14 @@ fun EnvironmentMetricsChart( }, endAxis = if (otherMetrics.isNotEmpty()) { + // The end axis is shared, so it can only carry a unit when every metric on it uses the same + // one. + val endAxisUnit = + otherMetrics.map { unitSuffix(it, isFahrenheit, isImperial) }.distinct().singleOrNull() + ?: "" VerticalAxis.rememberEnd( label = ChartStyling.rememberAxisLabel(color = endAxisColor), - valueFormatter = { _, value, _ -> formatString("%.0f", value) }, + valueFormatter = { _, value, _ -> formatString("%.0f", value) + endAxisUnit }, ) } else { null diff --git a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/EnvironmentMetrics.kt b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/EnvironmentMetrics.kt index 8cf959ed7b..7a6d1a5a63 100644 --- a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/EnvironmentMetrics.kt +++ b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/EnvironmentMetrics.kt @@ -85,6 +85,8 @@ fun EnvironmentMetricsScreen(viewModel: MetricsViewModel, onNavigateUp: () -> Un viewModel.saveEnvironmentMetricsCSV(uri, filteredTelemetries) } + val isImperial = state.displayUnits == org.meshtastic.proto.Config.DisplayConfig.DisplayUnits.IMPERIAL + BaseMetricScreen( onNavigateUp = onNavigateUp, telemetryType = TelemetryType.ENVIRONMENT, @@ -108,6 +110,8 @@ fun EnvironmentMetricsScreen(viewModel: MetricsViewModel, onNavigateUp: () -> Un modifier = modifier, telemetries = filteredTelemetries.reversed(), graphData = graphData, + isFahrenheit = state.isFahrenheit, + isImperial = isImperial, vicoScrollState = vicoScrollState, selectedX = selectedX, onPointSelected = onPointSelected, @@ -123,8 +127,7 @@ fun EnvironmentMetricsScreen(viewModel: MetricsViewModel, onNavigateUp: () -> Un EnvironmentMetricsCard( telemetry = telemetry, environmentDisplayFahrenheit = state.isFahrenheit, - isImperial = - state.displayUnits == org.meshtastic.proto.Config.DisplayConfig.DisplayUnits.IMPERIAL, + isImperial = isImperial, isSelected = telemetry.time.toDouble() == selectedX, onClick = { onCardClick(telemetry.time.toDouble()) }, ) diff --git a/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/metrics/EnvironmentChartUnitsTest.kt b/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/metrics/EnvironmentChartUnitsTest.kt new file mode 100644 index 0000000000..2c631b3676 --- /dev/null +++ b/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/metrics/EnvironmentChartUnitsTest.kt @@ -0,0 +1,92 @@ +/* + * 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.metrics + +import org.meshtastic.core.common.util.nowSeconds +import org.meshtastic.proto.EnvironmentMetrics +import org.meshtastic.proto.Telemetry +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull + +@Suppress("MagicNumber") +class EnvironmentChartUnitsTest { + + private fun telemetry(env: EnvironmentMetrics) = Telemetry(time = nowSeconds.toInt(), environment_metrics = env) + + // ---- chartValue ---- + + @Test + fun windSpeedMetricStaysMetersPerSecond() { + val t = telemetry(EnvironmentMetrics(wind_speed = 10f)) + + assertEquals(10f, chartValue(Environment.WIND_SPEED, t, isImperial = false)!!, 0.001f) + } + + @Test + fun windSpeedImperialIsConvertedToMph() { + val t = telemetry(EnvironmentMetrics(wind_speed = 10f)) + + assertEquals(22.3694f, chartValue(Environment.WIND_SPEED, t, isImperial = true)!!, 0.001f) + } + + /** 0 m/s is a real reading (dead calm), so it must survive conversion rather than read as missing. */ + @Test + fun windSpeedZeroIsPreserved() { + val t = telemetry(EnvironmentMetrics(wind_speed = 0f)) + + assertEquals(0f, chartValue(Environment.WIND_SPEED, t, isImperial = true)!!, 0.001f) + } + + @Test + fun missingWindSpeedStaysNull() { + val t = telemetry(EnvironmentMetrics()) + + assertNull(chartValue(Environment.WIND_SPEED, t, isImperial = true)) + } + + /** Temperatures are converted upstream by the view model, so the chart must not convert them again. */ + @Test + fun temperatureIsNotConvertedHere() { + val t = telemetry(EnvironmentMetrics(temperature = 20f)) + + assertEquals(20f, chartValue(Environment.TEMPERATURE, t, isImperial = true)!!, 0.001f) + } + + // ---- unitSuffix ---- + + @Test + fun windSpeedSuffixFollowsDisplayUnits() { + assertEquals(" m/s", unitSuffix(Environment.WIND_SPEED, isFahrenheit = false, isImperial = false)) + assertEquals(" mph", unitSuffix(Environment.WIND_SPEED, isFahrenheit = false, isImperial = true)) + } + + @Test + fun temperatureSuffixFollowsFahrenheitSetting() { + assertEquals("°C", unitSuffix(Environment.TEMPERATURE, isFahrenheit = false, isImperial = false)) + assertEquals("°F", unitSuffix(Environment.TEMPERATURE, isFahrenheit = true, isImperial = false)) + assertEquals("°F", unitSuffix(Environment.SOIL_TEMPERATURE, isFahrenheit = true, isImperial = false)) + assertEquals("°F", unitSuffix(Environment.ONE_WIRE_TEMP_8, isFahrenheit = true, isImperial = false)) + } + + @Test + fun sharedAxisMetricsHaveNoSuffix() { + assertEquals("", unitSuffix(Environment.HUMIDITY, isFahrenheit = true, isImperial = true)) + assertEquals("", unitSuffix(Environment.IAQ, isFahrenheit = true, isImperial = true)) + assertEquals("", unitSuffix(Environment.LUX, isFahrenheit = true, isImperial = true)) + } +}