fix/wind-speed-shows-wrong-unit by bruschill · Pull Request #1628 · meshtastic/Meshtastic-Apple

bruschill · 2026-03-17T01:34:05Z

What changed?

Now conditionally choosing units for windspeed based on initial value, based on my assumption that the persisted/fetched value is m/s.

Why did it change?

Assuming the unit of the persisted value is m/s, the display would show km/s regardless w/o conversion. Fixes #1617.

How is this tested?

Written tests.

Screenshots/Videos (when applicable)

n/a

Checklist

My code adheres to the project's coding and style guidelines.
I have conducted a self-review of my code.
I have commented my code, particularly in complex areas.
I have verified whether these changes require an update to existing documentation or if new documentation is needed, and created an issue in the docs repo if applicable.
I have tested the change to ensure that it works as intended.

- conditionally handle windspeed unit display

garthvh · 2026-04-02T15:01:59Z

This needs to be fixed in the firmware if there are different values, the apps should not have to determine the unit. It looks like the firmware values are all meters per second.

Copilot

Pull request overview

This PR aims to fix incorrect wind speed units in the Environment Metrics table (Issue #1617) by adjusting how wind speed measurements are formatted/displayed, and adds a new test file to cover the behavior.

Changes:

Updated the Environment Metrics “Wind Speed” table column formatting logic.
Added WindSpeedColumnTests.swift and wired it into the MeshtasticTests target.
Updated the Xcode project to include the new test source.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
Meshtastic/Views/Nodes/Helpers/Metrics Columns/EnvironmentDefaultColumns.swift	Changes how wind speed is turned into a `Measurement<UnitSpeed>` for table display.
Meshtastic/Views/Nodes/Helpers/Metrics Columns/WindSpeedColumnTests.swift	Adds XCTest coverage intended to validate wind speed formatting/units.
Meshtastic.xcodeproj/project.pbxproj	Adds the new test file reference and includes it in the test target sources.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-16T05:36:38Z

+		for speed in testCases {
+			let entity = createMockTelemetryEntity(windSpeed: speed)
+			let view = extractViewContent(from: entity)
+
+			// The view should contain m/s unit
+			XCTAssertNotNil(view, "View should be created for speed \(speed)")
+		}
+	}
+
+	func testHighSpeedUsesKilometersPerHour() {
+		// Test speeds >= 10 m/s should be displayed in km/h
+		let testCases: [Float] = [10.0, 15.5, 25.0, 50.0]
+
+		for speed in testCases {
+			let entity = createMockTelemetryEntity(windSpeed: speed)
+			let view = extractViewContent(from: entity)
+
+			// The view should contain km/h unit
+			XCTAssertNotNil(view, "View should be created for speed \(speed)")
+		}


These assertions (XCTAssertNotNil(view, ...)) don’t validate the wind-speed formatting/unit behavior: MetricsTableColumn.body(_:) always returns an AnyView (the initializer wraps the optional view into AnyView), so view will be non-nil regardless of whether the unit/value is correct. Consider asserting the formatted string/measurement output instead (e.g., by extracting wind-speed formatting into a pure function and unit-testing that), or using a SwiftUI view inspection approach that can verify the rendered text includes the expected unit and converted value.

Copilot · 2026-04-16T05:36:39Z

+		entity.windSpeed = windSpeed ?? 0
+
+		// If windSpeed is nil, we need to set it as nil in the entity
+		if windSpeed == nil {
+			entity.windSpeed = 0  // TelemetryEntity might not support optional Float directly
+		}
+


TelemetryEntity.windSpeed is an optional (Float?), so the test helper can set entity.windSpeed = windSpeed directly. As written, the helper coerces nil to 0, so testNilSpeedShowsIndicator never exercises the nil-path in the tableBody and can’t catch regressions around Constants.nilValueIndicator.

Suggested change

entity.windSpeed = windSpeed ?? 0

// If windSpeed is nil, we need to set it as nil in the entity

if windSpeed == nil {

entity.windSpeed = 0 // TelemetryEntity might not support optional Float directly

}

entity.windSpeed = windSpeed

Copilot · 2026-04-16T05:36:39Z

+		// When displayed as km/h, the value should be the same number
+		// but the unit is km/h (the actual conversion happens in formatting)
+		let measurement = Measurement(value: Double(speedInMPS), unit: UnitSpeed.kilometersPerHour)
+		XCTAssertEqual(measurement.value, 15.0)


testKilometersPerHourConversion constructs Measurement(value: speedInMPS, unit: .kilometersPerHour) using a value that the test names as meters/second, and then asserts the value is unchanged. If the persisted telemetry value is m/s (as the PR description assumes), the correct behavior when displaying km/h is to convert the measurement (m/s → km/h) before formatting, so this test currently encodes the same bug you’re trying to fix.

Suggested change

// When displayed as km/h, the value should be the same number

// but the unit is km/h (the actual conversion happens in formatting)

let measurement = Measurement(value: Double(speedInMPS), unit: UnitSpeed.kilometersPerHour)

XCTAssertEqual(measurement.value, 15.0)

// Persisted telemetry wind speed is stored in m/s and must be

// converted before being displayed as km/h.

let measurement = Measurement(value: Double(speedInMPS), unit: UnitSpeed.metersPerSecond)

.converted(to: UnitSpeed.kilometersPerHour)

XCTAssertEqual(measurement.value, 54.0, accuracy: 0.0001)

Copilot · 2026-04-16T05:36:39Z

+		// Create a mock TelemetryEntity with the specified wind speed
+		let context = PersistenceController.preview.container.viewContext
+		let entity = TelemetryEntity(context: context)
+		entity.windSpeed = windSpeed ?? 0


The tests use PersistenceController.preview, which is configured with inMemory: false and will load a real persistent store. This can make unit tests slow/flaky and can leak state across runs. Prefer creating a dedicated in-memory PersistenceController(inMemory: true) (or a test-only NSPersistentContainer) for these tests.

Copilot · 2026-04-16T05:36:39Z

+
+						let windSpeed: Measurement<UnitSpeed>
+						if speedInMetersPerSecond < 10 {
+							windSpeed = Measurement(value: speedInMetersPerSecond, unit: UnitSpeed.metersPerSecond)
+						} else {
+							windSpeed = Measurement(value: speedInMetersPerSecond, unit: UnitSpeed.kilometersPerHour)
+						}
+


The wind speed value appears to be persisted as meters/second (see MeshPackets assigning telemetry.windSpeed = ...windSpeed). This code switches the unit label to km/h for values >= 10 but keeps the same numeric value, which will display an incorrect speed (e.g., 15 m/s will be shown as 15 km/h instead of ~54 km/h). Use a Measurement created in .metersPerSecond and either let formatting choose the display unit, or explicitly convert to .kilometersPerHour when you want km/h.

Suggested change

let windSpeed: Measurement<UnitSpeed>

if speedInMetersPerSecond < 10 {

windSpeed = Measurement(value: speedInMetersPerSecond, unit: UnitSpeed.metersPerSecond)

} else {

windSpeed = Measurement(value: speedInMetersPerSecond, unit: UnitSpeed.kilometersPerHour)

}

let baseWindSpeed = Measurement(

value: speedInMetersPerSecond,

unit: UnitSpeed.metersPerSecond

)

let windSpeed: Measurement<UnitSpeed>

if speedInMetersPerSecond < 10 {

windSpeed = baseWindSpeed

} else {

windSpeed = baseWindSpeed.converted(to: .kilometersPerHour)

}

garthvh · 2026-05-05T06:06:02Z

@copilot resolve the merge conflicts in this pull request

garthvh · 2026-05-17T05:13:12Z

Thanks for the report and the PR — the underlying bug was real. However, `main` already has the correct fix in place.

The protobuf definition is authoritative and unambiguous:

// telemetry.pb.swift
/// Wind speed in m/s
public var windSpeed: Float

Both display paths in `main` now unconditionally construct a `Measurement` with `UnitSpeed.metersPerSecond` and let the system format it into the user's preferred locale units:

`Meshtastic/Views/Nodes/Helpers/NodeDetail.swift` (line 322):

  let windSpeedMeasurement = Measurement(value: Double(windSpeed), unit: UnitSpeed.metersPerSecond)

`Meshtastic/Views/Nodes/Helpers/Metrics Columns/EnvironmentDefaultColumns.swift` (line 202):

  let windSpeed = Measurement(value: Double(\$0), unit: UnitSpeed.metersPerSecond)

This approach is preferable to conditionally guessing the unit from the value magnitude — the proto contract guarantees m/s, so no guessing is needed.

Closing as superseded. Thanks again for surfacing this! Closing as superseded by the existing fix in main — see comment above.

fix/wind-speed-shows-wrong-unit

75e8b15

- conditionally handle windspeed unit display

garthvh requested review from Copilot and garthvh April 16, 2026 05:31

Copilot started reviewing on behalf of garthvh April 16, 2026 05:32 View session

Copilot AI reviewed Apr 16, 2026

View reviewed changes

garthvh closed this May 17, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix/wind-speed-shows-wrong-unit#1628

fix/wind-speed-shows-wrong-unit#1628
bruschill wants to merge 1 commit into
meshtastic:mainfrom
bruschill:fix/wind-speed-shows-wrong-unit

bruschill commented Mar 17, 2026 •

edited

Loading

Uh oh!

garthvh commented Apr 2, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 16, 2026

Uh oh!

Copilot AI Apr 16, 2026

Uh oh!

Copilot AI Apr 16, 2026

Uh oh!

Copilot AI Apr 16, 2026

Uh oh!

Copilot AI Apr 16, 2026

Uh oh!

garthvh commented May 5, 2026

Uh oh!

garthvh commented May 17, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-		// When displayed as km/h, the value should be the same number
-		// but the unit is km/h (the actual conversion happens in formatting)
-		let measurement = Measurement(value: Double(speedInMPS), unit: UnitSpeed.kilometersPerHour)
-		XCTAssertEqual(measurement.value, 15.0)
+		// Persisted telemetry wind speed is stored in m/s and must be
+		// converted before being displayed as km/h.
+		let measurement = Measurement(value: Double(speedInMPS), unit: UnitSpeed.metersPerSecond)
+			.converted(to: UnitSpeed.kilometersPerHour)
+		XCTAssertEqual(measurement.value, 54.0, accuracy: 0.0001)

Uh oh!

Conversation

bruschill commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed?

Why did it change?

How is this tested?

Screenshots/Videos (when applicable)

Checklist

Uh oh!

garthvh commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

garthvh commented May 5, 2026

Uh oh!

garthvh commented May 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bruschill commented Mar 17, 2026 •

edited

Loading

garthvh commented Apr 2, 2026 •

edited

Loading

garthvh commented May 17, 2026 •

edited

Loading