Add drafty human friendly current metrics#4433
Add drafty human friendly current metrics#4433NeimadTL wants to merge 13 commits intomeshtastic:mainfrom
Conversation
Currently, the metircs are always shown with the in milliampere (e.i, using the "mA" unit). A nicer way would be to have thoses metrics shown in more friendly way (e.g, 1A -> 1A, 1000A -> 1kA, 0.000001A -> 1μA...etc). At this point, this is just a draft of how it would be implemented.
The ranges were wrong and therefore gave wrong result so they've been fixed. Also, there's possiblity of the function using custom units instead of the default ones. This new parameter is optional as one may be just fine the default units.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4433 +/- ##
=======================================
Coverage 15.35% 15.35%
=======================================
Files 83 83
Lines 4344 4344
Branches 734 734
=======================================
Hits 667 667
Misses 3553 3553
Partials 124 124 ☔ View full report in Codecov by Sentry. |
Spotless has been apply to fix formatting and linting errors.
Both base and ampere units have been change into constants as gradle suggested it and also because it makes more sense.
|
@DaneEvans @jamesarich, could you please have look when you'll get some time please (couldn't assign it you for some reason). Also, I've got to do the same for the distance metric but couldn't find it in |
There was a problem hiding this comment.
Pull request overview
This pull request implements human-friendly formatting for electrical current metrics in the Environment Metrics display. Instead of always showing values in milliamperes (mA), it introduces intelligent unit scaling (μA, mA, A, kA, etc.) based on the magnitude of the value. This addresses issue #2944, which requests more readable display of large metric values (e.g., showing "18.2 A" instead of "18200 mA").
Changes:
- Added
numberToHuman()utility function to convert numeric values to human-readable format with appropriate SI prefixes - Added
BASE_UNITSandAMPERE_UNITSconstant maps to support unit conversion - Updated current display in
EnvironmentMetrics.ktto use the new formatting function
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| core/model/src/main/kotlin/org/meshtastic/core/model/util/UnitConversions.kt | Adds numberToHuman function and unit conversion constants for intelligent unit prefix selection |
| feature/node/src/main/kotlin/org/meshtastic/feature/node/metrics/EnvironmentMetrics.kt | Updates current value display to use numberToHuman instead of fixed mA formatting |
core/model/src/main/kotlin/org/meshtastic/core/model/util/UnitConversions.kt
Outdated
Show resolved
Hide resolved
core/model/src/main/kotlin/org/meshtastic/core/model/util/UnitConversions.kt
Outdated
Show resolved
Hide resolved
core/model/src/commonMain/kotlin/org/meshtastic/core/model/util/UnitConversions.kt
Show resolved
Hide resolved
core/model/src/main/kotlin/org/meshtastic/core/model/util/UnitConversions.kt
Outdated
Show resolved
Hide resolved
core/model/src/main/kotlin/org/meshtastic/core/model/util/UnitConversions.kt
Outdated
Show resolved
Hide resolved
core/model/src/main/kotlin/org/meshtastic/core/model/util/UnitConversions.kt
Outdated
Show resolved
Hide resolved
core/model/src/main/kotlin/org/meshtastic/core/model/util/UnitConversions.kt
Outdated
Show resolved
Hide resolved
core/model/src/main/kotlin/org/meshtastic/core/model/util/UnitConversions.kt
Outdated
Show resolved
Hide resolved
DaneEvans
left a comment
There was a problem hiding this comment.
Go through the copilot stuff, most of it seems sensible.
Remember we want this generalised for all units / areas eventually (windspeed, distance etc.)
The mappings should be unnecessary, especially to amps in particular.
A string insertion ala (python) f"{f0.2 value}{SIprefix}{ Unit}" should be adequate.
The original formatting was the number with two decimal places (e.i, "% 2f") so the returned value is formatted that way now. The exponent logic wasn't straightforward and complex? It's been replaced the following formulae `(exponent / 3) * 3` which will round down to the nearest multiple of 3 in the exponent calculation is not a multiple of 3. The base unit map was inverted a second time to assign the unit so it's been updated.
This makes sure that number less than or equal to 0 are handle before attempting calculations and "N/A" will be returned in that case. Metrics like current and distance are stored respectively in milliampere (mA) and millimeters (mm) so before doing calculations and determining the units, the metrics have to be converted in base unit (e.i, divided by 1000) first.
We eventually decided to display metrics in base unit to make things simpler so therefore mapping exponent to unit is no longer needed.
Tests cases have been added to make sure the fonction works as expected.
|
Thanks for your feedback @DaneEvans. Made the changes but looks like there other issue which I haven't figured out yet. Any suggestion please ? FYI, the same warning appears the last CI run:
|
Spotless have been applied sucessfully but not sure issues are resolved (attempt#1).
jamesarich
left a comment
There was a problem hiding this comment.
one thing you missed - you updated the EnvironmentMetrics chart to use base Amperes, but there are still several places hardcoded to display the raw telemetry values as mA.
for the app to be consistent, you need to wrap the raw values in your new convertToBaseUnit() and swap the string formatting over to A in these files:
- feature/node/src/main/kotlin/org/meshtastic/feature/node/metrics/PowerMetrics.kt (lines ~280 and ~386)
- feature/node/src/main/kotlin/org/meshtastic/feature/node/component/PowerMetrics.kt (line ~51, and the other channels)
- feature/node/src/main/kotlin/org/meshtastic/feature/node/component/EnvironmentMetrics.kt (line ~103)
- feature/node/src/main/kotlin/org/meshtastic/feature/node/component/NodeItem.kt (line ~365)
| * - 100 millimeters to 0.1 meters | ||
| */ | ||
| @Suppress("MagicNumber") | ||
| fun convertToBaseUnit(number: Float): Float = number / 1000f |
There was a problem hiding this comment.
Preferred in Kotlin to write this as an extension property or function directly on the Float (or Number) type.
val Float.milliToBase: Float get() = this / 1000f
This can then be used everywhere in the code:
envMetrics.current?.milliToBase
| } | ||
| if (hasCurrent) { | ||
| val current = envMetrics.current!! | ||
| val current = convertToBaseUnit(envMetrics.current!!) |
There was a problem hiding this comment.
Let's get rid of the !! here and wrap this with ?.let.
envMetrics.current?.let { currentMilli ->
val current = convertToBaseUnit(currentMilli)
Text(
text = "%s %.2f A".format(stringResource(Res.string.current), current),
// ...
)
}
| val current = convertToBaseUnit(envMetrics.current!!) | ||
| Text( | ||
| text = "%s %.2f mA".format(stringResource(Res.string.current), current), | ||
| text = "%s %.2f A".format(stringResource(Res.string.current), current), |
There was a problem hiding this comment.
Move String formatting into Compose Resources. Rather than doing "%s %.2f A".format(stringResource(Res.string.current), current) inside the Composable, it is best practice to move the format specifier directly into the strings.xml. e.g., <string name="current_value_format">%1$s %2$.2f A</string> And then in Compose: stringResource(Res.string.current_value_format, stringResource(Res.string.current), current) This is safer for localization and keeps your UI code much cleaner.
An Float extension was made out of the previous `convertToBaseUnit` method as it's more Kotlin-ish and the remaining files using ampere value have been updated.

Currently, the metrics are always shown with the in milliampere (e.i, using the "mA" unit). A nicer way would be to have those metrics shown in more friendly way (e.g, 1A -> 1A, 1000A -> 1kA, 0.000001A -> 1μA...etc). At this point, this is just a draft of how it would be implemented.
Related to #2944