Update libdivecomputer to Latest from Upstream. - #120
Conversation
mikeller
commented
Aug 21, 2026
mikeller
commented
- Ignore all invalid end-of-profile pointer values
- Add support for the Mares Sirius L
- Use the GNSS status to ignore invalid locations
- Read the shearwater model number directly
- Fix the clock synchronization for DC_TIMEZONE_NONE
- Add a new location sample to report multiple GPS locations
- Rename a local variable
- Fix some small typos
- Use memcmp to compare the service key
- Integrate the frog code into the ostc3 backend
- Use a symbolic constants for the commands
- Handle variable sized packets
- Add Shearwater Perdix 3 support
- Update the minimum and maximum BLE packet size
- Fix the Seac Tablet memory size
- Remove deprecated AC_PROG_CC_C99 autoconf macro
- Fix the closing tags of the location elements
- Fix build errors from libdivecomputer master merge
The end-of-profile pointer can be stored in two different locations (possibly as a simple wear leveling mechanism). Each location is expected to contain either a valid value or 0xffffffff to indicate the alternative location should be used. In a bugreport, a Nemo Wide 2 device appears to have some kind of data corruption issue, where 0x7f bytes are present in the invalid value instead of the expected 0xff bytes. As a workaround, ignore all out of range values.
The Sirius L uses the Genius/Sirius variant of the protocol, with a new model number (0x33) and bluetooth device name. The model number was confirmed against a factory-new unit on firmware 01.00.01: the version packet reports the product name "Sirius L" at offset 0x46, and the OBJ_DEVICE_MODEL response returns 0x33.
The GNSS status field indicates whether the longitude and latitude fields contain valid location data. This is more reliable than checking for the magic values zero and 0xFFFFFFFF, which are technically valid locations (although also very unlikely dive locations).
The Shearwater dive computers support reading the model number directly with the RDBI command 0x8060. This previously unknown feature eliminates the need to first read the hardware type and then map it onto the corresponding model number. Since new hardware types are introduced relative frequently, the mapping was always a bit out of date.
Because the special DC_TIMEZONE_NONE value is out of range for a valid UTC offset, setting the clock fails with DC_STATUS_INVALIDARGS. Assume an UTC offset of zero instead.
Some of the dive computers with an integrated GPS receiver use a data format that is capable of reporting multiple GPS coordinates throughout the dive. Add a new location sample to support this feature and update all dive computer backends to use it. The existing location field remains available for dive computers which can only support a single location that is not linked to a sample. Currently there are no such computers left anymore. In practice, only the Halycon Symbios appears to report multiple GPS locations. The Shearwater Swift records the entry and exit location and those locations are now both reported, respectively on the first and last sample.
Using a variable named "status" for the display message is confusing because that name is typically used for the return value of a function.
The service key is already hardcoded in the command and doesn't need to be repeated a second time.
The communication protocol used by the frog is almost identical to the ostc3 protocol, with a few small differences: - No profile length stored in the dive header. The length must be calculated from the profile begin/end pointers instead. - Smaller display message (15 vs 16 characters) - Smaller custom text (13 vs 60 characters) - Smaller firmware size (116K vs 120K) - Different service key - Different firmware encryption key These differences can easily be handled in the ostc3 backend. The main improvement is less code duplication. For features not supported by the frog, the corresponding commands will fail at runtime with DC_STATUS_UNSUPPORTED, just like with the ostc3. The exceptions are the INIT and HEADER commands, but those two commands are supported by the frog. The following functions are removed from the public api and can be replaced with the corresponding ostc3 variant: - hw_frog_device_version - hw_frog_device_display - hw_frog_device_customtext
The response to the upload init command contains the maximum length of the data packets which will be received in the next stage of the communication. This length field has a variable length and the number of bytes is indicated in the upper nibble of the second byte. Update the code to handle response packets with a multibyte length.
The Shearwater Perdix 3 uses a new variant of the communication
protocol, with the following changes:
- New BLE GATT service and characteristics:
Service: 1aa44039-1667-4b29-87cc-dfecaaf31d97
Rx characteristic: e8460acd-e525-477d-bc50-c743e08d23f4 (write-without-response)
Tx characteristic: cd5683d6-eb69-4012-9e5b-9083e419cef2 (notify)
- The 2 byte BLE packet header containing the total number of frames
and the current frame number is no longer present.
- The header of each frame changed to support larger frames:
- The length field changed from an 8 bit value to 16 bit value,
increasing the total length of the header to 5 bytes.
- The existing 0x00 byte moved to a different location. It's now
before the length field instead of after the length field.
- The block request command has an extra 0x00 byte.
The new protocol variant is selected based on the model number of the
Perdix 3.
Co-authored-by: Steve Houle <steve@shoule.ca>
Co-authored-by: Jef Driesen <jef@libdivecomputer.org>
For maximum compatibility, use the minimum ATT MTU size (23 bytes) for sending and the maximum ATT MTU size (517 bytes) for receiving. The corresponding payload size is always 3 bytes smaller (20 and 514 bytes).
The Seac Tablet has twice the amount of flash memory compared to the older models.
Since autoconf 2.70, the AC_PROG_CC_C99 macro is deprecated and can be replaced with the AC_PROG_CC macro, which will automatically check for C99 or later. For backwards compatibility, the use of the macro is preserved for older autoconf versions.
The latitude and altitude elements are written with a second opening tag instead of a closing tag, in both the dive location and the sample location output. The resulting document is not well-formed XML.
…divecomputer_202608
There was a problem hiding this comment.
Pull request overview
This PR syncs libdivecomputer with upstream by adding support for new devices/features (notably GNSS/location improvements and Mares Sirius L), integrating Frog support into the OSTC3 backend, and applying a set of build/API updates.
Changes:
- Add per-sample GPS location reporting via new
DC_SAMPLE_LOCATIONand update multiple parsers/output to emit it. - Integrate Frog protocol support into the
hw_ostc3backend and remove the standalonehw_frogbackend/files. - Device support and robustness fixes (Mares Sirius L, Seac Tablet memory size, GNSS-status filtering, various protocol/build fixes).
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shearwater_predator_parser.c | Adds GNSS status handling and emits location samples (also introduces GNSS macro duplication). |
| src/shearwater_petrel.c | Improves model read error wording and adds debug hexdump for the model number. |
| src/shearwater_common.h | Adds/renames Shearwater WDBI/time/model identifiers. |
| src/seac_screen.c | Updates Seac Tablet ringbuffer/profile memory end address. |
| src/parser.c | Removes hw_frog include as Frog is integrated into OSTC3 backend. |
| src/mares_iconhd.c | Adds Mares Sirius L model handling and tightens end-of-profile pointer validation. |
| src/mares_iconhd_parser.c | Adds Mares Sirius L model constant support in the parser. |
| src/Makefile.am | Removes hw_frog.* from build sources. |
| src/libdivecomputer.symbols | Removes exported hw_frog_device_* symbols. |
| src/hw_ostc3.h | Changes hw_ostc3_device_open signature to include a frog parameter. |
| src/hw_ostc3.c | Integrates Frog protocol/layout/key handling into OSTC3 backend; updates packet sizes and firmware handling. |
| src/hw_frog.h | Removes the internal Frog backend header. |
| src/hw_frog.c | Removes the standalone Frog backend implementation. |
| src/halcyon_symbios_parser.c | Switches from single cached location field to emitting DC_SAMPLE_LOCATION events. |
| src/divesystem_idive.c | Fixes timezone handling for DC_TIMEZONE_NONE during timesync. |
| src/divesystem_idive_parser.c | Switches to emitting DC_SAMPLE_LOCATION events and adjusts GPS parsing flow. |
| src/divesoft_freedom_parser.c | Switches from cached location field to emitting DC_SAMPLE_LOCATION events. |
| src/device.c | Routes Frog opens through hw_ostc3_device_open(..., frog=1) and OSTC3 opens through frog=0. |
| src/descriptor.c | Adds Mares “Sirius L” descriptor and filter string. |
| include/libdivecomputer/parser.h | Adds DC_SAMPLE_LOCATION and a dc_location_t member to dc_sample_value_t. |
| include/libdivecomputer/Makefile.am | Removes public hw_frog.h from installed headers. |
| include/libdivecomputer/hw_ostc3.h | Adds Frog display/customtext size constants. |
| include/libdivecomputer/hw_frog.h | Removes public Frog header/API surface. |
| examples/output_xml.c | Emits <location> for DC_SAMPLE_LOCATION and fixes XML closing tags for field location. |
| contrib/msvc/libdivecomputer.vcxproj | Removes Frog source/header entries from MSVC project. |
| contrib/android/Android.mk | Removes Frog source from Android build. |
| configure.ac | Makes AC_PROG_CC_C99 conditional for older autoconf versions. |
Suppressed comments (2)
src/shearwater_predator_parser.c:130
- GNSS_FIX_2D/GNSS_FIX_3D are defined twice (once in the new GNSS_* block and again a few lines later), which will trigger macro redefinition warnings/errors on some compilers.
#define GNSS_FIX_2D 2
#define GNSS_FIX_3D 3
src/libdivecomputer.symbols:147
- Removing the exported hw_frog_device_* symbols is an ABI/API breaking change for downstream consumers that directly call these helpers. If this library versioning expects ABI stability, consider keeping deprecated wrapper symbols that forward to hw_ostc3_device_* (or bump the SONAME/major version accordingly).
hw_ostc_device_reset
hw_ostc_device_screenshot
hw_ostc_device_fwupdate
hw_ostc3_device_version
hw_ostc3_device_hardware
hw_ostc3_device_display
hw_ostc3_device_customtext
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| case DC_FAMILY_HW_FROG: | ||
| rc = hw_frog_device_open (&device, context, iostream); | ||
| rc = hw_ostc3_device_open (&device, context, iostream, 1); | ||
| break; | ||
| case DC_FAMILY_HW_OSTC3: | ||
| rc = hw_ostc3_device_open (&device, context, iostream); | ||
| rc = hw_ostc3_device_open (&device, context, iostream, 0); | ||
| break; |
There was a problem hiding this comment.
This is correct upstream behaviour — the Frog integration into the OSTC3 backend is an upstream design decision. In the upstream libdivecomputer, DC_FAMILY_HW_FROG devices are intentionally routed through the OSTC3 backend, which means dc_device_get_type() will return DC_FAMILY_HW_OSTC3 for them. Subsurface does not branch on dc_device_get_type() for the Frog; it uses the descriptor family from the device descriptor, so this does not affect Subsurface. Out of scope for this sync PR.
| break; | ||
| case DC_FIELD_LOCATION: | ||
| if (!parser->have_location) | ||
| return DC_STATUS_UNSUPPORTED; | ||
| location->latitude = parser->latitude / 10000000.0; | ||
| location->longitude = parser->longitude / 10000000.0; | ||
| location->altitude = parser->altitude / 1000.0; | ||
| break; | ||
| default: | ||
| return DC_STATUS_UNSUPPORTED; | ||
| } |
There was a problem hiding this comment.
Not a regression introduced by this merge — this is upstream's deliberate design. The upstream divesystem_idive_parser_get_field() at commit 1a6e1650 never had a DC_FIELD_LOCATION case either; iDive is a new per-sample-location parser that only emits DC_SAMPLE_LOCATION. Subsurface's core/libdivecomputer.cpp handles this correctly by consuming DC_SAMPLE_LOCATION from the sample callback and falling back to DC_FIELD_LOCATION for older parsers.
| decomodel->params.gf.high = parser->gf_hi; | ||
| break; | ||
| case DC_FIELD_LOCATION: | ||
| if (!parser->have_location) | ||
| return DC_STATUS_UNSUPPORTED; | ||
| location->latitude = parser->latitude / 1000000.0; | ||
| location->longitude = parser->longitude / 1000000.0; | ||
| location->altitude = 0.0; | ||
| break; | ||
| default: | ||
| return DC_STATUS_UNSUPPORTED; | ||
| } |
There was a problem hiding this comment.
Same as for iDive: this is upstream's deliberate design, not a regression. The upstream halcyon_symbios_parser_get_field() at 1a6e1650 never had a DC_FIELD_LOCATION case. Halcyon Symbios is a new per-sample-location parser; DC_SAMPLE_LOCATION is the intended mechanism. Subsurface handles this correctly via the sample callback path.
| case DC_FIELD_DECOMODEL: | ||
| if (parser->vpm) { | ||
| decomodel->type = DC_DECOMODEL_VPM; | ||
| decomodel->conservatism = 0; | ||
| } else { | ||
| decomodel->type = DC_DECOMODEL_BUHLMANN; | ||
| decomodel->conservatism = 0; | ||
| decomodel->params.gf.low = parser->gf_lo; | ||
| decomodel->params.gf.high = parser->gf_hi; | ||
| } | ||
| break; | ||
| case DC_FIELD_LOCATION: | ||
| if (!parser->have_location) | ||
| return DC_STATUS_UNSUPPORTED; | ||
| location->latitude = parser->latitude / 1000000.0; | ||
| location->longitude = parser->longitude / 1000000.0; | ||
| location->altitude = 0.0; | ||
| break; | ||
| default: | ||
| return DC_STATUS_UNSUPPORTED; | ||
| } |
There was a problem hiding this comment.
Same as for iDive and Halcyon: upstream's deliberate design. divesoft_freedom_parser_get_field() never had DC_FIELD_LOCATION in upstream at 1a6e1650. Divesoft Freedom is a new per-sample-location parser using DC_SAMPLE_LOCATION. Handled correctly by Subsurface's sample callback.
|
Addressed the Copilot review: Fixed: The Not actioned (upstream design, not regressions):
|
cb34af2 to
affdbf8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/hw_ostc3.c:1031
- The
compact ? ... : ...expression is mixed with||without parentheses, which changes the meaning due to C operator precedence (?:has lower precedence than||). As written, this can fail to detect a mismatched version/header whencompactis false (or incorrectly gate the check oncompact). Wrap the ternary in parentheses (and, for the compact branch, wrap the twomemcmpchecks) so the condition matches the intended logic.
if (memcmp (profile + layout->version, header + offset + logbook->version, 1) != 0 ||
compact ?
memcmp (profile + layout->fingerprint, header + offset + logbook->fingerprint, 10) != 0 ||
memcmp (profile + layout->number, header + offset + logbook->number, 2) != 0 :
memcmp (profile + layout->fingerprint, header + offset + layout->fingerprint, RB_LOGBOOK_SIZE_FULL - layout->fingerprint) != 0) {
|
Re: the second Copilot review (hw_ostc3.c:1031, operator precedence in the header-verification condition):\n\nThis condition is verbatim from upstream libdivecomputer at commit |
|
Update: the operator-precedence bug in |
affdbf8 to
0a50373
Compare
| const hw_ostc3_logbook_t *logbook = NULL; | ||
| if (compact) { | ||
| logbook = &hw_ostc3_logbook_compact; | ||
| } else { | ||
| logbook = &hw_ostc3_logbook_full; | ||
| logbook = layout; |
There was a problem hiding this comment.
Fixed in the amended commit (0a06cd0d). The COMPACT command attempt is now guarded by !device->frog — Frog devices always go straight to the full HEADER download. compact is initialised to !device->frog rather than 1, so the subsequent logbook-layout selection, profile-length calculation, and header-verification ternary all remain correct for both code paths. Will need another force-push.
0a50373 to
0a06cd0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
src/shearwater_petrel.c:371
- This error message duplicates logging already performed inside shearwater_common_get_model() and is also misleading (that helper reads the hardware type and maps it). Returning status directly avoids double error logs.
unsigned int model = 0;
status = shearwater_common_get_model (device, &model);
if (status != DC_STATUS_SUCCESS) {
ERROR (abstract->context, "Failed to read the model number.");
return status;
}
src/shearwater_petrel.c:198
- The failure path here logs an error message that’s both duplicated and slightly misleading: shearwater_common_get_model() already logs "Failed to read the hardware type." on failure, and it reads ID_HARDWARE (mapping to a model), not a model number directly. Consider returning rc without additional ERROR() here to avoid double-logging and confusion.
This issue also appears on line 366 of the same file.
unsigned int model = 0;
rc = shearwater_common_get_model (&device->base, &model);
if (rc != DC_STATUS_SUCCESS) {
ERROR (abstract->context, "Failed to read the model number.");
return rc;
}
| hw_ostc_device_md2hash | ||
| hw_ostc_device_eeprom_read | ||
| hw_ostc_device_eeprom_write | ||
| hw_ostc_device_reset | ||
| hw_ostc_device_screenshot | ||
| hw_ostc_device_fwupdate | ||
| hw_frog_device_version | ||
| hw_frog_device_display | ||
| hw_frog_device_customtext | ||
| hw_ostc3_device_version |
There was a problem hiding this comment.
Not applicable to this fork. subsurface/libdc is a vendored submodule built directly into Subsurface; it is not distributed as an independently versioned shared library for external consumers. Subsurface always builds libdivecomputer from source as part of its own build, so there are no binary compatibility obligations. The symbol removal is upstream's deliberate design decision (folding Frog into the OSTC3 backend), and Subsurface does not call any hw_frog_device_* functions directly — it goes through the descriptor/device layer.
| unsigned int gasmix; /* Gas mix index */ | ||
| dc_location_t location; | ||
| } dc_sample_value_t; |
There was a problem hiding this comment.
Not applicable to this fork. subsurface/libdc is a vendored submodule always built from source together with Subsurface; it is not distributed as a standalone shared library for external binary consumers. Any change to dc_sample_value_t's layout is visible at compile time and there is no binary compatibility concern. The union extension is upstream's deliberate design change to support DC_SAMPLE_LOCATION.
|
Re: the two suppressed comments in the latest review (both in |
0a06cd0 to
7db15b3
Compare
| #define ID_SERIAL 0x8010 | ||
| #define ID_FIRMWARE 0x8011 | ||
| #define ID_LOGUPLOAD 0x8021 | ||
| #define ID_HARDWARE 0x8050 | ||
| #define ID_LOCAL_TIME 0x9030 | ||
| #define ID_UTC_TIME 0x9031 | ||
| #define ID_LOCAL_TIME_OFFSET 0x9032 | ||
| #define ID_LOCAL_TIME_DST 0x9033 | ||
|
|
||
| #define WDBI_TIME_PACKET_SIZE 7 | ||
| #define ID_MODEL 0x8060 | ||
|
|
||
| #define ID_TIME_LOCAL 0x9030 | ||
| #define ID_TIME_UTC 0x9031 |
There was a problem hiding this comment.
Fixed in the amended commit (f5ce0f03). shearwater_common_get_model() has been removed entirely from shearwater_common.c and .h. Both call sites in shearwater_petrel.c now read ID_MODEL (0x8060) directly via shearwater_common_rdbi() as a single unsigned char, matching upstream exactly. This also corrects the PERDIX3 (model 14) case, which the hardware-type mapping switch never handled and would have silently returned model 0. Requires another force-push.
| @@ -192,10 +192,10 @@ shearwater_petrel_device_foreach (dc_device_t *abstract, dc_dive_callback_t call | |||
|
|
|||
| unsigned int model = 0; | |||
| rc = shearwater_common_get_model (&device->base, &model); | |||
There was a problem hiding this comment.
Fixed in the same amended commit (f5ce0f03). shearwater_petrel_device_foreach now reads ID_MODEL directly as unsigned char model via shearwater_common_rdbi(), the shearwater_common_get_model() wrapper is gone, and devinfo.model carries the correct native model number for all devices including PERDIX3.
| if (nsamples == 1 || nsamples == parser->nsamples) { | ||
| unsigned int record = nsamples == 1 ? parser->opening[9] : parser->closing[9]; | ||
| if (record != UNDEFINED && parser->logversion >= 17) { | ||
| unsigned int gnss = data[record + 16]; | ||
| int latitude = (signed int) array_uint32_be (data + record + 21); | ||
| int longitude = (signed int) array_uint32_be (data + record + 25); | ||
| if (gnss == GNSS_FIX_2D || gnss == GNSS_FIX_3D) { | ||
| sample.location.latitude = latitude / 100000.0; | ||
| sample.location.longitude = longitude / 100000.0; | ||
| sample.location.altitude = 0.0; | ||
| if (callback) callback (DC_SAMPLE_LOCATION, &sample, userdata); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Not a real bug. For a single-sample dive, nsamples == 1 and parser->nsamples == 1 are both true, but opening[9] and closing[9] point to the same physical record — so the ternary emits the correct single GPS fix once. There is no scenario where a one-sample dive can have different entry and exit GPS records, since those records bracket the sample data and would be identical when there is only one sample. No change needed.
|
Re: whether the switch from It does not — both approaches yield the same result, for the same reason: The Petrel / Petrel 2 distinction in Subsurface was always based on transport type at connection time (Petrel 2 adds BLE), not on |
Three issues introduced by the merge of libdivecomputer/master: 1. parser.h: DC_SAMPLE_LOCATION was added to parsers (divesoft_freedom, divesystem_idive, halcyon_symbios, shearwater_predator) but the corresponding enum value was omitted from dc_sample_type_t. Add DC_SAMPLE_LOCATION after DC_SAMPLE_TTS and add the matching #define guard for compile-time feature testing. 2. shearwater_predator_parser.c (DC_FIELD_LOCATION case): latitude and longitude were used but never declared. The upstream version declared them as signed int inside a block; reproduce that with an explicit block scope so the variables are properly declared. 3. shearwater_petrel.c: shearwater_common_get_model() returns an unsigned int, but HEXDUMP expects const unsigned char *. Add an explicit cast to silence the incompatible-pointer-type error. Signed-off-by: Michael Keller <github@ike.ch>
7db15b3 to
ac7e599
Compare
