Repository navigation
Pass GPX elevation through to FIT record altitudes - #8
Conversation
The GPX reader already parsed <ele> into GeoPoint, and the course builder carried it through to records, but the FIT encoder dropped it, so converted courses had no elevation profile on device. Encode elevation as the record message's altitude field (uint16, scale 5, offset 500), matching what Garmin Connect and RWGPS write. The field is only defined for courses that have some elevation data, so files converted from GPX without <ele> are byte-for-byte unchanged. Records individually missing an elevation within such a course, and elevations outside the range the profile can represent, are encoded as the profile's invalid value. Adds integration tests covering altitude scaling, absent, partial, and out-of-range elevations via integration-stub, along with a CLI-level test that every elevation in cptr003.gpx survives the conversion. The stub's course spec now accepts an optional elevation per record, and cptr008.gpx is a new, small GPX track with no elevation at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
garmin-fit-sdk 21.178.0, which is what uv.lock pins, decodes an altitude set to the profile's invalid value as 12607.0 meters rather than reporting the field as unset, so the partial and out-of-range altitude tests failed in CI even though they passed against a newer SDK locally. Read those altitudes with fitdecode instead, which consistently reports invalid fields as None, and assert on the record definition message directly for the case where the field shouldn't be encoded at all. Also switch the GPX elevation helper to defusedxml, as devtools already does for parsing GPX. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mshroyer
left a comment
There was a problem hiding this comment.
This mostly looks good over all. I'll still need to manually test this with devices before approving for merging.
| ALTITUDE_ABS_TOLERANCE = 0.11 | ||
|
|
||
|
|
||
| def write_fit_records(tmpdir, integration_stub, records): |
There was a problem hiding this comment.
Please remove this helper and invoke integration_stub directly from within the new tests, as is done in the existing tests. I prefer keeping control flow as "flat" as possible in tests.
There was a problem hiding this comment.
Done in ac26e05 — the helper is gone and each test invokes integration_stub directly.
| - cptr003_connect.fit: cptr003.gpx imported into Garmin Connect and re-exported as FIT. | ||
| - cptr004.gpx: Export of [a Gaia GPS folder](https://www.gaiagps.com/map/?loc=13.6/-122.1094/37.2305&pubLink=b4sitlz7aVCkPFVDFaLm1bSi&folderId=16650ba9-3ecb-45cb-9c21-430e53c7d4c0) folder containing a hiking route and five waypoints, four of them located approximately on the route. | ||
| - cptr007.gpx: A GPX export from Gaia GPS containing two routes | ||
| - cptr008.gpx: A short, handwritten GPX track whose trackpoints have no elevation, along with one waypoint near the track. |
There was a problem hiding this comment.
Please note this as "AI generated" rather than "handwritten".
There was a problem hiding this comment.
Done — the README now says "AI generated", and I updated the file's own creator attribute to match.
| We read these with fitdecode rather than the Garmin SDK because the SDK's | ||
| treatment of the invalid value varies by version: 21.178.0 decodes it as an | ||
| altitude of 12607.0 meters instead of reporting the field as unset. |
There was a problem hiding this comment.
Please update the Garmin SDK dependency to the latest version if that simplifies the implementation (and doesn't break anything else).
There was a problem hiding this comment.
Done — bumped to 21.208.0, which reports fields set to the profile's invalid value as unset (21.178.0 decoded an invalid altitude as a literal 12607.0 m, which is why I'd reached for fitdecode here). That let me delete fitdecode_record_altitudes entirely. The record definition is still read with fitdecode, since the SDK can't distinguish a field that was never defined from one set to the invalid value, and that distinction is the point of the no-elevation test.
Addresses review feedback: - Drop the write_fit_records helper and invoke integration-stub directly from each test, keeping their control flow flat like the surrounding tests. - Update garmin-fit-sdk to 21.208.0, which reports a field set to the profile's invalid value as unset. That removes the need to read those altitudes with fitdecode, so fitdecode_record_altitudes goes away; the record definition is still read with fitdecode, since that's the only way to tell an undefined field from an invalid one. - Describe cptr008.gpx as AI generated rather than handwritten. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
I tested this on my Garmin Edge 1030 (software 31.33) and Fenix 7 (26.09) by exporting a GPX course and manually editing it to create copies with all, half, or none of its In the file containing only some elevation tags, while fitfileviewer.com correctly interpreted the u16 FFFFh values as invalid, both devices interpreted this as an absurdly high elevation. Please update this PR to:
|
Devices don't read an altitude set to the FIT profile's invalid value as missing data: an Edge 1030 and a fenix 7 both showed it as an absurdly high elevation. Write the record altitude field only when the whole course has elevations the field can represent, so that value never gets encoded, and treat an out-of-range elevation like a missing one. Also add a --no-elevation flag to the convert subcommand, backed by a new FitCourseOptions::with_elevation, for skipping the passthrough even when the input does have complete elevation data.
|
Both changes are in 84f8cbb.
Test coverage for the partial case moved with the behavior: the stub-level tests now assert the field is undefined for partial and out-of-range input (plus a new case at the exact range limits), and |
Summary
GPX elevations were parsed into
GeoPointand carried throughCourserecords, but the FIT encoder never wrote them, so converted courses showed no elevation profile on device. This encodes them as the record message'saltitudefield.altitude(field 2) is auint16with scale 5 and offset 500 m — the same field Garmin Connect and Ride with GPS write in their own course files (verified againstcptr003_connect.fitandcptr003.fit).convert --no-elevationskips the passthrough entirely, backed by a newFitCourseOptions::with_elevation.Testing
test_fit_encoding.pydrivingintegration-stub, which now accepts an optional per-record elevation in its course spec: altitudes round-trip through the Garmin SDK, including at the extremes of the field's range, and the field is left undefined for courses with no, partial, or out-of-range elevation.test_cli.py: every elevation incptr003.gpxcomes back out of the converted course in order; a newcptr008.gpxwith no elevation and a newcptr009.gpxwith elevation on only half its trackpoints both convert without an altitude field; and--no-elevationsuppresses altitudes for input that does have complete elevation data.cargo test -F cli, the full pytest suite (55 passed), andscripts/lint.shall pass locally.🤖 Generated with Claude Code