feat(dive-computer): rank sensorless CCR bailout gases against each other - #1752
ericgriffin merged 7 commits into
Conversation
…ther Without a transmitter, dive_tanks.role was inferred per gas in isolation from a single O2/He threshold, ignoring the dive-mode context. That misclassified programmed CCR diluent and bailout gases the computer never attaches to a tank record. libdivecomputer's stock dc_gasmix_t already carries a per-gas dc_usage_t (oxygen/diluent/sidemount), independent of any tank record, but the plugin never bridged it past the C layer. Thread GasMix.usage through pigeon and all four platform converters, then use it in the sensorless resolver: - usage reported by the device (oxygen/diluent/sidemount) is authoritative, regardless of dive mode. - On a CCR dive, the remaining gases (bailout candidates) are ranked against each other instead of scored alone: lowest O2 is bailout (ties broken by higher helium, a further tie gives bailout to all tied gases), O2 >= 41% is deco, everything else is stage. - Every other dive mode keeps the original single-threshold heuristic unchanged. Ref: submersion-app#1747
CI's dart format --set-exit-if-changed . flagged all three files touched by the previous commit. The pigeon-generated dive_computer_api.g.dart needs a format pass after regeneration too, not just the hand-written files -- no re-run of pigeon or build_runner needed, the generated content itself was already correct.
… not a sidemount role Copilot review on #61: the doc comment and test name implied sidemount usage kept a distinct "sidemount" role, but it actually maps to TankRole.backGas -- the device flag says the gas is on a sidemount cylinder, not which side, so it cannot pick between sidemountLeft and sidemountRight. Correct the comment and rename the test to match the real behavior; no logic change.
There was a problem hiding this comment.
🟡 Changes recommended
The new CCR bailout ranking logic relies on exact floating-point equality for tie detection, which can misclassify roles due to rounding differences from native-to-Dart percent conversions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves sensorless (tankless) dive imports by bridging libdivecomputer’s per-gas dc_usage_t into GasMix.usage across all platforms and using that data (plus CCR-specific relative ranking) to assign more accurate cylinder roles for CCR dives without transmitters/tank records.
Changes:
- Add
GasMix.usageto the Pigeon API and propagate it through the shared C layer plus Android/JNI, Darwin/Swift, Linux, and Windows converters. - Update sensorless role inference to respect device-reported gas usage (oxygen/diluent/sidemount) and to rank CCR bailout candidates relative to each other.
- Add unit tests covering CCR ranking and usage mapping behavior.
File summaries
| File | Description |
|---|---|
| lib/features/dive_computer/data/services/parsed_tank_resolver.dart | Uses GasMix.usage for sensorless role inference and adds CCR bailout ranking logic. |
| test/features/dive_computer/data/services/parsed_tank_resolver_test.dart | Adds coverage for CCR sensorless ranking and direct usage mapping. |
| packages/libdivecomputer_plugin/pigeons/dive_computer_api.dart | Extends the Pigeon GasMix model with optional usage. |
| packages/libdivecomputer_plugin/lib/src/generated/dive_computer_api.g.dart | Regenerates Dart bindings to encode/decode GasMix.usage. |
| packages/libdivecomputer_plugin/android/src/main/kotlin/com/submersion/libdivecomputer/DiveComputerApi.g.kt | Regenerates Kotlin bindings for GasMix.usage. |
| packages/libdivecomputer_plugin/android/src/main/kotlin/com/submersion/libdivecomputer/SerialDownloadRunner.kt | Reads bridged gasmix usage and populates GasMix.usage. |
| packages/libdivecomputer_plugin/android/src/main/kotlin/com/submersion/libdivecomputer/DiveComputerHostApiImpl.kt | Reads bridged gasmix usage and populates GasMix.usage. |
| packages/libdivecomputer_plugin/android/src/main/cpp/libdc_jni.cpp | Extends JNI gasmix array payload to include usage. |
| packages/libdivecomputer_plugin/darwin/Sources/LibDCDarwin/DiveComputerHostApiImpl.swift | Populates GasMix.usage from bridged gm.usage. |
| packages/libdivecomputer_plugin/ios/Classes/DiveComputerApi.g.swift | Regenerates Swift bindings for GasMix.usage. |
| packages/libdivecomputer_plugin/macos/Classes/libdc_wrapper.h | Adds usage to libdc_gasmix_t. |
| packages/libdivecomputer_plugin/macos/Classes/libdc_download.c | Copies dc_gasmix_t.usage into the shared parsed dive structure. |
| packages/libdivecomputer_plugin/linux/dive_converter.c | Passes gasmix usage through the Linux converter into the generated API object. |
| packages/libdivecomputer_plugin/linux/dive_computer_api.g.h | Regenerates Linux header bindings to include GasMix.usage. |
| packages/libdivecomputer_plugin/linux/dive_computer_api.g.cc | Regenerates Linux C++ bindings to store/serialize GasMix.usage. |
| packages/libdivecomputer_plugin/windows/dive_converter.cc | Passes gasmix usage through the Windows converter into the generated API object. |
| packages/libdivecomputer_plugin/windows/dive_computer_api.g.h | Regenerates Windows header bindings to include GasMix.usage. |
| packages/libdivecomputer_plugin/windows/dive_computer_api.g.cc | Regenerates Windows C++ bindings to store/serialize GasMix.usage. |
Review details
- Files reviewed: 17/18 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 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. |
…-break Copilot review on submersion-app#1752: the lowest-O2 and helium tie-break comparisons used exact double equality. Each of the four platform converters computes o2Percent/hePercent independently as fraction * 100.0 from the native dc_gasmix_t, so two mixes the diver set to the same nominal percentage can differ by a few ULPs instead of being bit-identical -- exact == would then miss a real tie and misassign bailout/deco/stage roles. Add a small epsilon comparison and a regression test simulating that floating-point noise.
There was a problem hiding this comment.
🟢 Approval recommended
The cross-platform GasMix.usage bridging and the updated CCR sensorless role inference are consistent with the PR’s stated behavior and are backed by targeted unit tests for the new ranking/tie-break logic.
Review details
- Files reviewed: 17/18 changed files
- Comments generated: 1
- Review effort level: Lite
… comment Copilot review on submersion-app#1752: [TankRole.sidemountLeft]/`Right` only linked sidemountLeft and left Right as a stray inline-code fragment. Link both roles properly.
… comment Copilot review on submersion-app#1752: [TankRole.sidemountLeft]/`Right` only linked sidemountLeft and left Right as a stray inline-code fragment. Link both roles properly.
There was a problem hiding this comment.
🔵 Needs a closer look
It touches cross-platform native bridging (C/JNI/Kotlin/Swift/C++/GLib) where subtle ABI/serialization issues can slip through despite unit tests, so it warrants final human review.
Review details
- Files reviewed: 17/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Broad cross-platform native changes and an uncovered native usage handoff merit final human verification.
Review details
Suppressed comments (1)
packages/libdivecomputer_plugin/macos/Classes/libdc_download.c:593
- The production handoff of
dc_gasmix_t.usageis not covered by the native tests:test_dive_converter.conly asserts the oxygen/helium fields onlibdc_gasmix_t, while the Dart resolver tests constructGasMixdirectly. A regression in this C extraction or the shared struct field could therefore leave usage null on every platform while the current tests still pass; add a native regression assertion for a nonzero usage value (and the zero/none case) through the wrapper/converter path.
dive->gasmixes[i].usage = gm.usage;
- Files reviewed: 17/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Hi @ericgriffin, just a friendly nudge on this one whenever you have a moment - all CI checks are green, the branch is up to date with main, and both points from the Copilot review are addressed. Happy to make any further adjustments if you'd like a different approach. Thanks! |
github-project-automation
Bot
moved this from In review
to Done
in Submersion Release Tracker
Summary
Without a transmitter,
_inferRole()inparsed_tank_resolver.dartscored every reportedgas in isolation against a single fixed threshold (no helium and O2 >= 41% -> Deco,
otherwise Back Gas), regardless of the recognized dive mode. On a CCR dive with several
programmed diluent and bailout gases, that produced wrong roles whenever none of them had
a transmitter/tank record.
libdivecomputer's stock
dc_gasmix_talready carries a per-gasdc_usage_t(oxygen/diluent/sidemount), independent of any tank record - the Shearwater parser sets it - but
the plugin never bridged that field past the C layer (
GasMixonly ever exposedo2Percent/hePercent).This threads
GasMix.usagethrough pigeon and all four platform converters, then uses itin the sensorless path:
dive mode. Sidemount maps to Back Gas (the flag doesn't say left or right, so it can't
pick between
TankRole.sidemountLeft/Right).against each other instead of scored alone: the lowest O2 percentage becomes Bailout
(tie -> higher helium wins; a further tie gives Bailout to every tied gas; a gas that
only loses the helium tie-break falls through to the rules below), O2 >= 41% becomes
Deco, everything else becomes Stage.
unchanged.
Related: #1365 (Transmitter Registry, resolved by #1677) sets role only for tanks with
a sensor, or via the registry; this PR is scoped to the sensorless case only, discussed in
#1747.
Native changes
GasMix.usagerequired bridgingdc_gasmix_t.usagethrough the shared C download engine(
macos/Classes/libdc_download.c,libdc_wrapper.h, used by all platforms) and eachplatform converter (Linux, Windows, macOS/iOS Swift, Android JNI + Kotlin), following the
exact pattern already used for
TankInfo.usage.Testing
flutter test test/features/dive_computer/data/services/parsed_tank_resolver_test.dartdiluent/sidemount), the bailout/deco/stage ranking, the helium tie-break (both the
win and the fall-through), the all-tied case, and that non-CCR dive modes are
unaffected.
dart analyzeclean onlib/features/dive_computer/andpackages/libdivecomputer_plugin/lib.dart format .clean.dart run pigeon --input pigeons/dive_computer_api.dart.Darwin Swift tests, code generation, analyze & format) passed on the equivalent PR in my
fork: feat(dive-computer): rank sensorless CCR bailout gases against each other alpheios-one/submersion#61.
Not done here
dc_gasmix_t.usagethe same way Shearwater does - only verified against the vendoredShearwater parser source.
Ref: #1747