-
-
Notifications
You must be signed in to change notification settings - Fork 566
[pro forma]:Add support for the Shearwater Swift GPS #4668
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Artifacts: |
|
Artifacts: |
|
Artifacts: |
dirkhh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why a sane person would want this to be a float and not an integer is beyond me, but whatever. Appoved
|
Thanks for opening the PR. I installed the Subsurface version created from this commit and imported a dive from my Perdix 2. However, i cannot see the coordinates anywhere. They are not in the location textbox nor under "Extra Info." This is my first computer with GPS capabilities, so it might be that I just don't know where to look. |
|
This pull request has merge conflicts, please resolve them in order to make this pull request ready for review / merge. |
Thanks for the feedback - as it turns out Subsurface uses a format for location reporting that predates location support in libdivecomputer. This will need some changes in Subsurface to make location support work for dive computers other than Garmin. |
|
I would rather fix the libdc code and match the way we get gps days from the Garmin This has been an ongoing debate and fundamental disagreement for a decade with many other examples. |
If I got a dollar each time I saw floats used for lat / lon in 'commercial grade' software I'd be filthy rich by now... |
|
If I got a cent (US$ or NZ$, doesn't matter), for every time I saw a stupid choice of data type and data transfer protocol or data encapsulation protocol, I could buy myself quite a few nice toys... |
Piggybacking on an 'extra info' filed with a specific name seems to be somewhat of a workaround - to me using a dedicated field for location seems to a more robust approach. And doing our own thing here also comes with the risk that whenever libdivecomputer adds support for a dive computer, this will not push through into Subsurface unless somebody picks up on it and adds our own way - as has already happened for the DiveSystem, Divesoft, and Halcyon dive computers... |
|
@dirkhh: Not sure where we want to take this from here? I can see the following options:
The way I see it, 1. is not particularly sustainable as this will get us to play catch up whenever a new dive computer has got location support. And I really don't think that 'leverage special names for Extra Info to transmit data' is an approach that should be turned into best practice and used more widely. 2. gets us to swallow the frog of float lat / lons, but at least new dive computers will work out of the box. 3. is most work, but then it will fail fast and get us to update the backend for new dive computers if they support location, in order to make them build in Subsurface. |
|
I am frustrated with the choices that Jef makes. They are inconsistent and weird - but that hasn't changed in 15 years and is unlikely to change in the future. Let's go with the lowest effort approach of (2) and just move on. |
in other words... this response is technically correct and something I still believe, but the effort isn't worth it |
Pro forma pull request to generate test artifacts for the changes in the libdivecomputer submodule. Signed-off-by: Michael Keller <[email protected]>
2fb56f1 to
10817fe
Compare
|
Thank you for resolving the merge conflicts. |
|
Artifacts: |
|
Artifacts: |
|
Artifacts: |
|
@chaehni Can you please try importing again, with the latest artifacts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
|
Hi @mikeller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| bool got_location = false; | ||
| if (rc == DC_STATUS_SUCCESS) { | ||
| location_t location; |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The location variable is uninitialized before this check. The code should first populate location from dc_location (e.g., location.lat.udeg = dc_location.latitude; location.lon.udeg = dc_location.longitude;) before checking and using its values.
| location_t location; | |
| location_t location{}; | |
| // Populate the location from the dc_location obtained above. | |
| location.lat.udeg = dc_location.latitude; | |
| location.lon.udeg = dc_location.longitude; |
core/libdivecomputer.cpp
Outdated
|
|
||
| if (location.lat.udeg && location.lon.udeg) { | ||
| char location_string[64]; | ||
| snprintf(location_string, sizeof(location_string), "%.6f, %.6f", location.lat.udeg / 1000000.0, location.lon.udeg / 1000000.0); |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The magic number 1000000.0 is used for coordinate conversion. Consider defining a named constant (e.g., MICRO_DEGREES_DIVISOR) to improve code clarity and maintainability.
Add support for importing the location of a dive from libdivecomputer's DC_FIELD_LOCATION. Note that the backends that currently support this are limited to only reporting one location (presumably the start location of the dive), so any other information, like the end location, is not imported. Signed-off-by: Michael Keller <[email protected]>
10817fe to
62aaebd
Compare
|
Thanks @chaehni.
My bad, I removed the code that populates the location when I did the final cleanup of my test code before pushing. Fixed now.
That's expected. It's a consequence of GitHub running the CICD build on a speculative merge of the pull request content into the target branch - so there's an extra commit in the version that is built. |
|
Artifacts: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| location.lon.udeg = lrint(dc_location.longitude * 1000000); | ||
|
|
||
| char location_string[64]; | ||
| snprintf(location_string, sizeof(location_string), "%.6f, %.6f", location.lat.udeg / 1000000.0, location.lon.udeg / 1000000.0); |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conversion from udeg (micro-degrees) to decimal degrees is duplicated here and in the subsequent create() call. Consider extracting this formatting logic into a helper function to avoid repetition and potential inconsistencies.
|
|
||
| unregister_dive_from_dive_site(dive); | ||
| devdata->log->sites.create(location_string, location)->add_dive(dive); | ||
|
|
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The label 'Start location' differs from the GPS string field labels (e.g., 'GPS', 'GPS1') used elsewhere in the code. Consider using a consistent naming convention such as 'GPS' or documenting why 'Start location' is used for DC_FIELD_LOCATION data.
| // Store the structured libdivecomputer DC_FIELD_LOCATION as "Start location" to | |
| // distinguish it from GPS string fields (e.g., "GPS", "GPS1") parsed via DC_FIELD_STRING. |
|
Artifacts: |
|
Artifacts: |
|
Now it works! |
Add location information on test files after libdivecomputer location support has been added. Signed-off-by: Michael Keller <[email protected]>
|
Artifacts: |
|
Artifacts: |
|
Artifacts: |
Describe the pull request:
Pull request long description:
Pro forma pull request to generate test artifacts for the changes in the
libdivecomputer submodule.
Changes made:
Related issues:
Implements libdivecomputer/libdivecomputer#53.
Fixes subsurface/libdc#97.
Additional information:
Documentation change:
Mentions: