Read cylinder end pressure at surfacing, not at the end of the recording - #1290
Conversation
Dive computers keep recording after the diver reaches the surface. On a rebreather whose oxygen cylinder feeds a constant mass flow orifice, closing the valve topside leaves the hose bleeding down through it, and the tail of the recording sheds most of the cylinder's apparent contents. The reported dive read 41 bar of oxygen at 1.2 m and 4 bar two minutes later on the surface, so the import logged 4 bar. libdivecomputer builds DC_FIELD_TANK's end pressure by overwriting it with every pressure sample as it walks the log, with no surfacing check, so it lands on the last sample in the tail. The new rule finds the last sample deeper than 0.75 m (Subsurface's SURFACE_THRESHOLD) and reads each cylinder's most recent pressure at or before that moment. It corrects a reported end pressure only when that value matches the last post-surfacing reading, which proves the source simply took the last sample it had. A parser that reads end pressure from a log header, a transmitter that dropped out before surfacing, and an exporting app that computed its own value are all left alone. The correction only ever raises a value. Applied at two seams: resolveParsedTanks, shared by dive computer download and reparse, and a post-parse payload normalizer covering every file format at their common shape. Normalizing after the parsers run keeps FIT's cylinder-volume derivation on the figures Garmin reported. Governed by diver_settings.trim_tank_pressure_at_surfacing (schema v163, default on), surfaced in Settings > Data beneath Site Matching.
There was a problem hiding this comment.
Pull request overview
Implements a “tank pressure at surfacing” correction so end cylinder pressure is taken from the last in-water reading (deeper than 0.75 m) instead of the final post-surfacing tail sample, addressing dive computers that keep recording topside and can dramatically understate remaining pressure (Issue #1092). The change is applied across both dive-computer downloads/reparse and universal file imports, gated by a new persisted diver setting (default on) and exposed in Settings > Data.
Changes:
- Add core surfacing-pressure logic (
surfacingTankReadings+trimEndPressureBar) and apply it to dive-computer tank resolution and universal-import payload normalization. - Introduce
trim_tank_pressure_at_surfacing(schema v163) with UI toggle + localization across all supported locales. - Add targeted unit/integration tests covering the core rule, persistence/migration, download/reparse behavior, and universal import end-to-end.
Reviewed changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/helpers/mock_providers.dart | Adds mock SettingsNotifier support for the new toggle setter. |
| test/features/universal_import/presentation/providers/universal_import_surfacing_pressure_test.dart | End-to-end test proving the rule is reached during real file import and respects the setting. |
| test/features/universal_import/data/services/surfacing_pressure_normalizer_test.dart | Unit tests for payload-level end-pressure normalization behavior and immutability. |
| test/features/statistics/presentation/pages/records_page_test.dart | Updates local mock settings notifier for the new setter. |
| test/features/settings/presentation/pages/settings_page_test.dart | Updates local mock settings notifier for the new setter. |
| test/features/settings/presentation/pages/settings_page_surfacing_pressure_test.dart | Widget tests for the Settings > Data toggle default and persistence via notifier. |
| test/features/settings/presentation/pages/settings_page_shared_data_test.dart | Updates local mock settings notifier for the new setter. |
| test/features/settings/data/repositories/diver_settings_repository_surfacing_pressure_test.dart | Verifies new setting defaults to true and round-trips through repository updates. |
| test/features/dive_computer/presentation/providers/download_notifier_surfacing_test.dart | Ensures live download applies the setting at dive-arrival time. |
| test/features/dive_computer/data/services/reparse_service_surfacing_test.dart | Ensures reparse honors the setting and stores the corrected end pressure. |
| test/features/dive_computer/data/services/parsed_tank_resolver_surfacing_test.dart | Unit tests for libdivecomputer tank resolution with surfacing trimming and edge cases. |
| test/features/dive_computer/data/services/parsed_dive_mapper_test.dart | Adds coverage that downloaded mapping trims by default and supports opt-out. |
| test/core/profile/surfacing_pressure_test.dart | Comprehensive unit tests for surfacing-point selection and end-pressure trimming semantics. |
| test/core/database/migration_v163_surfacing_pressure_test.dart | Tests migration adds the new column with default=on and is present on fresh DBs. |
| lib/l10n/arb/app_en.arb | Adds title/subtitle strings for the new settings toggle (English). |
| lib/l10n/arb/app_de.arb | Adds German strings for the new settings toggle. |
| lib/l10n/arb/app_es.arb | Adds Spanish strings for the new settings toggle. |
| lib/l10n/arb/app_fr.arb | Adds French strings for the new settings toggle. |
| lib/l10n/arb/app_it.arb | Adds Italian strings for the new settings toggle. |
| lib/l10n/arb/app_pt.arb | Adds Portuguese strings for the new settings toggle. |
| lib/l10n/arb/app_nl.arb | Adds Dutch strings for the new settings toggle. |
| lib/l10n/arb/app_hu.arb | Adds Hungarian strings for the new settings toggle. |
| lib/l10n/arb/app_he.arb | Adds Hebrew strings for the new settings toggle. |
| lib/l10n/arb/app_ar.arb | Adds Arabic strings for the new settings toggle. |
| lib/l10n/arb/app_zh.arb | Adds Chinese strings for the new settings toggle. |
| lib/l10n/arb/app_localizations.dart | Generated localization API additions for new keys. |
| lib/l10n/arb/app_localizations_en.dart | Generated English localization implementation updates. |
| lib/l10n/arb/app_localizations_de.dart | Generated German localization implementation updates. |
| lib/l10n/arb/app_localizations_es.dart | Generated Spanish localization implementation updates. |
| lib/l10n/arb/app_localizations_fr.dart | Generated French localization implementation updates. |
| lib/l10n/arb/app_localizations_it.dart | Generated Italian localization implementation updates. |
| lib/l10n/arb/app_localizations_pt.dart | Generated Portuguese localization implementation updates. |
| lib/l10n/arb/app_localizations_nl.dart | Generated Dutch localization implementation updates. |
| lib/l10n/arb/app_localizations_hu.dart | Generated Hungarian localization implementation updates. |
| lib/l10n/arb/app_localizations_he.dart | Generated Hebrew localization implementation updates. |
| lib/l10n/arb/app_localizations_ar.dart | Generated Arabic localization implementation updates. |
| lib/l10n/arb/app_localizations_zh.dart | Generated Chinese localization implementation updates. |
| lib/features/universal_import/presentation/providers/universal_import_providers.dart | Applies surfacing-pressure normalization to completed import payloads, gated by setting. |
| lib/features/universal_import/data/services/surfacing_pressure_normalizer.dart | Implements payload-level end-pressure rewrite using the core surfacing rule. |
| lib/features/settings/presentation/providers/settings_providers.dart | Adds trimTankPressureAtSurfacing to AppSettings + notifier setter + default true. |
| lib/features/settings/presentation/pages/settings_page.dart | Adds Settings > Data switch for “Tank pressure at surfacing”. |
| lib/features/settings/data/repositories/diver_settings_repository.dart | Persists/loads the new setting in DiverSettingsRepository. |
| lib/features/dive_computer/presentation/providers/reparse_providers.dart | Wires current setting into ReparseService provider so reparses respect updates. |
| lib/features/dive_computer/presentation/providers/download_providers.dart | Reads setting at dive-arrival time to avoid tearing down active downloads. |
| lib/features/dive_computer/data/services/reparse_service.dart | Propagates the setting to parsed tank resolution during reparse updates. |
| lib/features/dive_computer/data/services/parsed_tank_resolver.dart | Adds surfacing trimming to tank resolution while keeping gas-switch derivation unchanged. |
| lib/features/dive_computer/data/services/parsed_dive_mapper.dart | Adds opt-out parameter and defaults to trimming for downloaded dive mapping. |
| lib/core/profile/surfacing_pressure.dart | New core surfacing-pressure rule implementation (threshold + matching tolerance + trim). |
| lib/core/database/database.dart | Adds diver_settings column + v163 migration/backstop and bumps schema version. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
📦 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. |
This branch and main both wrote schema v163 for different columns: main's is diver_settings.default_show_estimated_tank_pressure (#731), this branch's is diver_settings.trim_tank_pressure_at_surfacing (#1092). The scalar itself carried no conflict marker, because both sides wrote the identical line. This branch renumbers to v165: - currentSchemaVersion 163 -> 165; the ladder keeps main's 163 entry and appends 165. v164 is skipped: PR #1287 (#1090) holds it on an open branch. - git had fused _assertSurfacingPressureColumn and _assertEstimatedTankPressureDefaultColumn into a single hybrid method by matching their shared PRAGMA-guard body; both are restored as separate methods. - The onUpgrade guard and beforeOpen backstop for the surfacing column move to 'from < 165' and sit after main's v163 block. - migration_v163_surfacing_pressure_test.dart is renamed to migration_v165_...
Three conflicts, all from work that landed on main while this branch was open (#731's schema v163 and #1299's narrow site write-backs). Schema: this branch held v162, which #731 landed past. It renumbers to v166 (v164 is held by PR #1287, v165 by PR #1290, both still open): - currentSchemaVersion 162 -> 166; the ladder keeps main's 163 entry and appends 166. - git had fused _assertPlaceNameLanguageColumn and _assertEstimatedTankPressureDefaultColumn into one hybrid method by matching their shared PRAGMA-guard body; both are restored separately. - The onUpgrade guard moves to 'from < 166'. - migration_v162_place_name_language_test.dart is renamed to migration_v166_..., and the sync_data_serializer seed comment follows. site_repository_impl.dart: main's updateSiteAltitude and updateSiteCoordinates and this branch's fillMissingLocationDetails were added at the same place. All three are kept; each is a narrow column write that goes through applyImportedMetadata rather than updateSite (#1187). sync_diver_settings_fallback_test.dart: both sides appended a same-shaped pre-migration payload test and git spliced them into one. Both are restored as separate tests, one for defaultShowEstimatedTankPressure and one for placeNameLanguage. The plan doc anticipated the renumber and is left as the historical record.
Resolves the migration-ladder collision in lib/core/database/database.dart. Main landed v164 (media.manual_elapsed_seconds, issue submersion-app#1090) while this branch also held v164 for dive_types.short_name, so the branch renumbers to v167. v165 and v166 are claimed by the open PRs submersion-app#1290 and submersion-app#1300. All six sites of the claim moved together: - currentSchemaVersion 164 -> 167 (it auto-merged with no conflict marker because both sides wrote the literal 164) - the migrationVersions ladder entry - the shortName column docstring and the _assertDiveTypeShortNameColumn helper docstring - the onUpgrade `if (from < N)` guard and its reportProgress twin - the beforeOpen backstop comment - migration_v164_dive_type_short_name_test.dart renamed to migration_v167_..., with its greaterThanOrEqualTo and contains assertions minimumCompatibleSchemaVersion stays at 160: a new nullable column does not raise the compatibility floor.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 49 changed files in this pull request and generated no new comments.
Suppressed comments (1)
lib/core/database/database.dart:3189
- PR description says the diver_settings setting is schema v163 (with v162 claimed by #1269), but the migration ladder and currentSchemaVersion here are v165. Please update the PR description (and any linked docs) to match the actual schema version to avoid confusion when backporting or tracking migrations.
/// The current schema version as a static constant so that pre-open checks
/// (e.g. version-mismatch guard) can reference it without an instance.
static const int currentSchemaVersion = 165;
/// The oldest schema whose reader can apply this build's sync payloads
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 49 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
lib/core/database/database.dart:3188
- PR description says the diver setting is schema v163, but this change bumps the DB schema to v165 (including migration ladder and tests). Please update the PR description (or any linked docs/release notes) to reflect v165 so schema references stay consistent.
/// The current schema version as a static constant so that pre-open checks
/// (e.g. version-mismatch guard) can reference it without an instance.
static const int currentSchemaVersion = 165;
main took v161 for diver_settings.default_show_o2_cell_mv (issue submersion-app#1235) while this branch was open, so buddies.is_favorite moves to the next free rung. v165, v166 and v167 are claimed by PRs submersion-app#1290, submersion-app#1300 and submersion-app#1276.
v168 was already claimed and pushed by PR #1237 (issue #638, buddies.is_favorite). That claim was resolved locally and not yet pushed when this branch picked its number, so the open-PR diff scan this plan prescribes could not see it. Two branches writing the same scalar auto-merge with no conflict marker, so the collision would have surfaced only as a database silently skipping a rung. Moves all six coupled sites together plus the test filename: the scalar, the migrationVersions entry, the _assertTripDayWeatherSchema docstring, the onUpgrade guard, its reportProgress twin, and the beforeOpen backstop comment. The migration test is renamed to migration_v171_trip_day_weather_test.dart with its greaterThanOrEqualTo and contains assertions updated. Its stranded-database fixture moves from 168 to 171: 168 is now a real rung owned by #1237, so a database stamped there upgrades normally and no longer exercises the backstop. The design doc and plan are updated too, including the reasoning passages that were wrong rather than merely stale: the plan recorded #1237 as a stale v161 claim when it was a live v168 one. Both now say to scan every worktree's working-tree scalar alongside open PR diffs, and to re-run both immediately before pushing rather than only when picking the number. Ladder is left non-contiguous by design: 165 #1290, 166 #1300, 167 #1276, 168 #1237, 169 the dive-computer gear-twin branch, 170 #1322. Verified: ladder monotonic, unique, scalar == max, 168 absent; helper defined exactly once and referenced three times; guard and twin both at 171; flutter analyze clean; flutter test test/core/database/ 471 passed.
Main reserved v165 for this branch while it was open, so the ladder needs no renumbering: 165 keeps its place and currentSchemaVersion rises to main's 168. The two diver_settings helpers and their onUpgrade steps are both kept, in ascending order.
The payload adapter read a missing `timestamp` as second zero. The rule takes surfacing to be the latest sample deeper than the threshold, so a fabricated zero ranks an unstamped sample ahead of the whole dive: a profile that stamped only its tail placed surfacing at the start and promoted a mid-dive pressure into the end pressure. No parser emits a depth without a timestamp today (UDDF filters its waypoints on both keys, and the rest always write both), so this is a latent case rather than a live one. Skipping the unstamped samples, the way the depthless ones are already skipped, leaves such a dive uncorrected, which is what the rule does whenever it cannot tell where the dive ended.
Resolves the schema-ladder conflict in database.dart: main landed v166 (#1187 place_name_language) and v168 (#638 buddies.is_favorite) while this branch held v171, so both ladder entries, both onUpgrade steps and both beforeOpen backstops are kept in ascending order and currentSchemaVersion stays at 171. The v171 comment now names the gaps that are still open (165 #1290, 169 #1320, 170 #1322) and records that 167 is permanently skipped, since main landed 168 past PR #1276 and that PR moves up.
Resolves the schema-ladder conflict in database.dart: main landed v170 (submersion-app#1322, gas_consumption_display) while this branch held v173, so main's ladder entry, onUpgrade step and beforeOpen backstop are kept ahead of this branch's and currentSchemaVersion stays at 173, which is still above main. The v173 comment is refreshed for the new ladder: main has now also taken 170, 165 is still claimed by PR submersion-app#1290, 171 by PR submersion-app#1319 and 172 by PR submersion-app#1328, and 169 has joined 167 as permanently skipped, since main landed 170 past PR submersion-app#1320 and that branch moved up to 175. analyze clean; 3,719 tests pass across core/database, dive_log and architecture.
|
Flagging something found while clearing the merge conflicts across the other open PRs. This PR has no conflict, but its migration rung is now dead.
// v165: trim_tank_pressure_at_surfacing on diver_settings (#1092).
if (from < 165) {
await _assertSurfacingPressureColumn();
}Because only The Suggested fix, matching what the other overtaken PRs were moved to in this pass: renumber v165 to v176 (171 #1319, 172 #1328, 173 #1276, 174 #603, 175 #1320 are all claimed), leaving 165 permanently skipped, and update any I have not touched this branch, since it has no conflict to resolve. Happy to push the renumber if you want it. For reference, the ladder after this pass: main 170; 171 #1319; 172 #1328; 173 #1276; 174 #603; 175 #1320; next free 176. 138, 162, 167 and 169 are permanently skipped. |
Dive computers keep recording for a few minutes after the diver reaches the
surface. On a rebreather whose oxygen cylinder feeds a constant mass flow
orifice, closing the valve topside leaves the hose bleeding down through that
orifice, and the tail of the recording sheds most of the cylinder's apparent
contents. The reporter's dive read 41 bar of oxygen at 1.2 m and 4 bar two
minutes later on the surface, so the import logged 4 bar and overstated his
oxygen consumption roughly tenfold.
The cause is upstream of us and structural. libdivecomputer's
shearwater_predator_parser.cbuildsDC_FIELD_TANK's end pressure byoverwriting it with every pressure sample as it walks the log, with no
surfacing check, so it lands on the last sample in the tail. Both it and
sample.pressure.valueuse the identicalpressure * 2 * PSI / BARexpressionon the same raw value, so the reported end pressure is bit-identical to the
last sample emitted.
The rule
lib/core/profile/surfacing_pressure.dartfinds the last sample deeper than0.75 m (Subsurface's
SURFACE_THRESHOLD, so both apps agree on where adive ended) and reads each cylinder's most recent pressure at or before that
moment. A diver who drops back down after a surface break is measured from the
final descent, and each cylinder carries its own last value forward because
transmitters report on their own cadence.
It corrects a reported end pressure only when that value matches the last
post-surfacing reading. That match is the evidence the source simply took the
last sample it had. Three things follow from it:
stream (Suunto, Uwatec) is left alone, because its value has an origin this
rule knows nothing about.
correction, so a stale in-water reading cannot be promoted over a real end
pressure.
value, rather than being silently overridden by ours.
Beyond that the correction only ever raises a value, and a source that reported
no end pressure keeps reporting none rather than gaining a fabricated one.
Where it applies
resolveParsedTanks, so bothpaths get it from one seam. Reparse matters here: an already-imported dive is
corrected with Reparse raw data on the dive detail menu, with no need to
re-download from the computer.
tankswithendPressure, profile points withallTankPressures), so a singlepost-parse normalizer covers FIT, UDDF, DL7, Subsurface, Ratio, MacDive and
Shearwater Cloud. It runs after the parsers, which keeps
FitTankExtractor's cylinder-volume derivation on the start/end pair Garminactually reported.
Setting
diver_settings.trim_tank_pressure_at_surfacing, schema v165, defaulton. Main reserved that number while this branch was open, so the ladder
needed no renumbering when main went on to v168. It appears in Settings > Data beneath Site
Matching, the other import-interpretation preference, and is translated across
all 11 locales.
Not doing
The profile itself is untouched: the post-surfacing tail stays on the chart,
honest about what the computer recorded. Start pressure is unchanged, the
threshold is not configurable, and already-imported file dives are not
retroactively repaired (only dive computer dives, via reparse).
Closes #1092