Resolve per-sample GPS after the profile walk, not during it - #951
Merged
Conversation
Code review found three defects that all trace to deciding entry/exit while the samples were still streaming, before the profile's length was known. Collect the first and last usable fix instead, then resolve once the walk is finished. A fix sequence of A, B, A -- a receiver reacquiring its original position after an intermediate lock -- left the mid-dive position B as the exit, because each fix was compared against the entry rather than deduplicated at the end. B is neither the entry nor the exit. The dive started and ended at A, so A is the one known position and there is no distinct exit. The first usable fix was unconditionally the entry, wherever it occurred. That is wrong for a receiver that only gets a lock after surfacing, and it is now reachable: OSTC 4 and Divesoft Freedom log fixes anywhere in the profile, and both reach this code since neither implements DC_FIELD_LOCATION. A single fix past the profile's midpoint is now stored as the exit. Nothing is stranded by that -- getUnmatchedDives accepts entry OR exit, and site_matching_service falls back to the exit when the entry is absent -- so the dive still resolves to a site, just pinned at the correct end. has_field_location was one flag set when either index succeeded, and it suppressed the sample fallback for both sides. Track the two sides separately so a parser supplying only the entry via the field cannot have its per-sample exit silently discarded. No parser does that today; the single flag encoded an assumption the name did not admit to. Two tests added for the newly covered behaviour, and the single-fix case now asserts both coordinates rather than only latitude. The DC_FIELD_LOCATION precedence gate remains untested: reaching it needs a parser implementing both APIs, which is Shearwater alone, and no Shearwater fixture exists. Its two paths read the same records behind the same GNSS_FIX and logversion checks, so they cannot disagree in practice.
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR fixes GPS entry/exit resolution by deferring per-sample fix interpretation until after the full profile is walked, preventing incorrect assignment when the profile length (and fix timing) matters.
Changes:
- Collect first/last usable per-sample GPS fixes during parsing and resolve them afterward based on profile length.
- Track
DC_FIELD_LOCATIONprecedence per-side (entry vs exit) instead of with a single combined flag. - Add/strengthen native C tests covering “A,B,A” return-to-entry and “late single fix is exit” scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/libdivecomputer_plugin/test/native/test_parse_raw_dive.c | Adds native tests for return-to-entry and late-fix-as-exit cases; tightens single-fix assertions. |
| packages/libdivecomputer_plugin/macos/Classes/libdc_download.c | Refactors per-sample GPS handling to collect fixes during walk and resolve entry/exit afterward; tracks field GPS per side. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
|
📦 Build artifacts for this PR · commit
Artifacts expire in 7 days. Downloading requires being signed in to GitHub. macOS needs two extractions: unzip the downloaded artifact, then unzip the Updated automatically on each push. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Review flagged the exact double comparison as fragile against GNSS jitter. It is deliberate: every source quantizes to an integer before we see it (int32/1e7 for Ratio, /1e6 for Halcyon and Divesoft, an exactly-widened float32 for OSTC 4) and this file only copies the value, so identical device bytes always yield bit-identical doubles. Equality means the device replayed the same stored record, which is the only case worth collapsing. A tolerance would discard real data instead: a shore dive exits a few metres from where it entered, and those are two genuine fixes. Comment only, no behaviour change.
github-project-automation
Bot
moved this from In review
to Done
in Submersion Release Tracker
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #945, which merged before these review findings were applied.
A code-review pass over #945 found three defects, all tracing to the same
mistake: entry/exit were decided while the samples were still streaming,
before the profile's length was known. This collects the first and last usable
fix during the walk and resolves them once, afterwards, in a new
resolve_sample_locations().Changes
1.
A, B, Aleft a mid-dive position as the exit. Each fix was comparedagainst the entry rather than deduplicated at the end, so a receiver that
reacquired its original position after an intermediate lock left
B— neitherthe entry nor the exit — as the exit point. The dive started and ended at
A,so
Ais the one known position and there is no distinct exit.2. The first usable fix became the entry wherever it occurred. Wrong for a
receiver that only gets a lock after surfacing, and now reachable:
hw_ostc(OSTC 4, GNSS on event bit
0x0400) anddivesoft_freedomlog fixes anywherein the profile, and both reach this code because neither implements
DC_FIELD_LOCATION. A lone fix past the profile's midpoint is now stored as theexit.
Verified this strands nothing before changing it — the concern being that
demoting a position out of
entrycould leave a dive with no matchablecoordinate at all:
dive_repository_impl.dart:502— the unmatched-dives query acceptsentry IS NOT NULLORexit IS NOT NULLsite_matching_service.dart:123—_pointFor(dive) => dive.entryLocation ?? dive.exitLocationSo the dive still resolves to a site; it is simply pinned at the correct end.
3.
has_field_locationwas one flag for two sides. It was set when eitherDC_FIELD_LOCATIONindex succeeded and suppressed the sample fallback forboth, so a parser supplying only the entry via the field would have had its
per-sample exit silently discarded. Now tracked per side. No parser does this
today — the single flag encoded an assumption its name did not admit to.
Test Plan
ctest), up from 7flutter analyzeclean,dart formatcleanTwo new cases, both of which fail against the merged #945 logic:
test_parse_ratio_ix3m_returns_to_entryA, B, A→ entryA, exit unset (was: exitB)test_parse_ratio_ix3m_late_fix_is_exittest_parse_ratio_ix3m_single_fixnow asserts both coordinates rather thanlatitude alone.
Known gap
The
DC_FIELD_LOCATIONprecedence gate is still untested. Reaching it needs aparser implementing both APIs, which is
shearwater_predatoralone, and noShearwater fixture exists in the repo. Its field and sample paths read the same
records behind the same
GNSS_FIX_2D/3Dandlogversion >= 17checks, so theycannot disagree in practice — but that is an argument from inspection, not from
a test. Adding a Shearwater GNSS fixture would close it.