Shearwater: restore ID_MODEL as model source, expose FWID for consumer sub-model refinement - #125
Open
mikeller wants to merge 4 commits into
Conversation
…source
ID_MODEL (RDBI 0x8060, Product-Version byte) is now the sole authoritative
model source on live Shearwater download, aligning with upstream libdc.
The previous shearwater_common_get_model() FWID->model mapping is removed
from the model-resolution path.
The raw FWID (ID_HARDWARE, RDBI 0x8050) is read best-effort in
shearwater_petrel_device_foreach() and exposed to consumers through a new
generic field on dc_event_devinfo_t:
unsigned int devinfo_hw_id;
Value 0 means unknown or not available (all non-Shearwater drivers, Predator
memory-dump path, and any RDBI failure). This lets consumers distinguish
e.g. Petrel 1 vs Petrel 2 hardware on a live download without libdc mapping
the FWID to a synthetic model number.
shearwater_petrel_device_timesync() is also updated to read ID_MODEL
directly rather than calling the now-removed shearwater_common_get_model().
Only the two Shearwater drivers that use devinfo_hw_id (shearwater_petrel.c
and shearwater_predator.c) zero-initialise dc_event_devinfo_t; all other
drivers retain their existing plain declaration since they explicitly assign
all fields before use.
// AI-generated (Claude)
Signed-off-by: Michael Keller <github@ike.ch>
…el lookup
Some device families share a coarse model number across multiple marketed
products. The Shearwater Petrel family is one example: both Petrel 1 and
Petrel 2 report ID_MODEL=3, with the hardware type (FWID) read via RDBI
0x8050 being the only runtime distinguisher.
Add a generic public API to descriptor.c / descriptor.h:
dc_descriptor_t *
dc_descriptor_find_by_hw_id (dc_family_t family, unsigned int hw_id);
The implementation keeps a separate static table g_hw_id_map[] that maps
(family, hw_id) to a (family, model, product) triple, then resolves that
to a direct pointer into g_descriptors[] at call time. This avoids
touching g_descriptors[] and keeps the mapping table small and legible.
The returned pointer is a direct reference into the static table, so
dc_descriptor_free() is safe to call on it (it remains a no-op, consistent
with the iterator path). hw_id==0 returns NULL immediately.
The initial g_hw_id_map[] entries cover the Shearwater Petrel family:
Petrel 1 (model=3): FWIDs 0x0404, 0x0909
Petrel 2 (model=3): FWIDs 0x0505, 0x0808, 0x0838, 0x08A5,
0x0B0B, 0x7828, 0x7B2C, 0x8838
Remove shearwater_fwid_to_product_version() and its suppress-unused stub
from shearwater_common.c. That knowledge now lives in descriptor.c;
shearwater_petrel.c continues to populate devinfo.devinfo_hw_id from the
RDBI 0x8050 read, which is the source for the hw_id passed to the new API.
Consumers that receive DC_EVENT_DEVINFO can call dc_descriptor_find_by_hw_id()
with the reported family and devinfo_hw_id to obtain a refined descriptor
without any device-specific knowledge on their side. A NULL return means
the hw_id is not in the table and the caller should fall back to the coarse
model-based descriptor.
// AI-generated (Claude)
Signed-off-by: Michael Keller <github@ike.ch>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new dc_event_devinfo_t.devinfo_hw_id field is not guaranteed to be initialized to 0 for many existing DC_EVENT_DEVINFO emitters, which can expose undefined/garbage values to consumers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adjusts Shearwater model identification to treat ID_MODEL as the authoritative coarse model source (aligning with upstream libdc), while exposing the raw Shearwater hardware identifier (FWID / ID_HARDWARE) to consumers for best-effort sub-model refinement (e.g., Petrel 1 vs Petrel 2). It also adds a generic descriptor-lookup helper to let consumers map (family, hw_id) to a more specific dc_descriptor_t when available.
Changes:
- Shearwater Petrel live download now reads
ID_MODELdirectly; FWID is read best-effort and exposed viaDC_EVENT_DEVINFO. dc_event_devinfo_tgains a newdevinfo_hw_idfield (0 when unknown/not available).- Descriptor API gains
dc_descriptor_find_by_hw_id()and a hardware-id mapping table (initially for Shearwater Petrel family).
File summaries
| File | Description |
|---|---|
| src/shearwater_predator.c | Zero-initialize devinfo and explicitly emit devinfo_hw_id = 0 for the dump path. |
| src/shearwater_predator_parser.c | Clarifies that stored logs carry the coarse Product-Version, not FWID, so sub-model refinement isn’t possible on re-parse. |
| src/shearwater_petrel.c | Reads ID_MODEL directly; reads FWID best-effort and emits it via DC_EVENT_DEVINFO; updates timesync model check. |
| src/shearwater_common.h | Documents PETREL2 sharing the same ID_MODEL value as PETREL; removes shearwater_common_get_model declaration. |
| src/shearwater_common.c | Removes shearwater_common_get_model() implementation. |
| src/descriptor.c | Adds g_hw_id_map[] and implements dc_descriptor_find_by_hw_id(). |
| include/libdivecomputer/device.h | Adds devinfo_hw_id to dc_event_devinfo_t with documentation. |
| include/libdivecomputer/descriptor.h | Declares and documents dc_descriptor_find_by_hw_id(). |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+57
to
+61
| * with the device's hardware type identifier (e.g. Shearwater FWID | ||
| * from RDBI 0x8050). It supplements the coarse model field for | ||
| * consumer-side sub-model differentiation. Consumers that do not use | ||
| * this field are unaffected; the model field remains authoritative. */ | ||
| unsigned int devinfo_hw_id; |
…drivers Rename the new dc_event_devinfo_t field from devinfo_hw_id to hw_id for consistency with the existing model, firmware, and serial fields, which do not carry the struct name as a prefix. Update all references: struct definition, descriptor.h comment, shearwater_petrel.c (assignment), shearwater_predator.c (assignment and comment), shearwater_predator_parser.c (comment), shearwater_common.h (comment), and descriptor.c (comment). Add an explicit devinfo.hw_id = 0 assignment immediately before every DC_EVENT_DEVINFO emit in the 37 non-Shearwater drivers. These drivers declare dc_event_devinfo_t on the stack without an initialiser and only assign model, firmware, and serial, leaving hw_id as indeterminate stack memory. The Subsurface consumer guards the sub-model refinement path on hw_id != 0; indeterminate stack garbage could trigger a spurious dc_descriptor_find_by_hw_id() lookup. The explicit assignment is the minimal targeted fix: it adds one line per emit site and does not alter any existing line, avoiding merge conflicts if upstream adds to those blocks independently. Shearwater Petrel already sets devinfo.hw_id = fwid (RDBI 0x8050 read). Shearwater Predator already sets devinfo.hw_id = 0 explicitly. No behaviour change for any existing user: hw_id was new on this branch. Signed-off-by: AI-generated (Claude) Signed-off-by: Michael Keller <github@ike.ch>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new public API dc_descriptor_find_by_hw_id is not added to src/libdivecomputer.symbols, so it may not be exported on builds that rely on the export list.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 45/45 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+144
to
+145
| dc_descriptor_t * | ||
| dc_descriptor_find_by_hw_id (dc_family_t family, unsigned int hw_id); |
The function was added to the public API (declared in include/libdivecomputer/descriptor.h, implemented in src/descriptor.c) but was omitted from src/libdivecomputer.symbols. On Autotools builds the linker export map is generated from this file, so the symbol was unexported, causing a link failure for any consumer calling it. Add dc_descriptor_find_by_hw_id in the dc_descriptor_* group, immediately after dc_descriptor_filter. Signed-off-by: Michael Keller <github@ike.ch>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are consistent with the stated two-layer design, and the new hw_id field is safely initialized across drivers with a best-effort Shearwater FWID exposure path.
Review details
Suppressed comments (1)
include/libdivecomputer/device.h:61
- The PR description refers to this new field as
devinfo_hw_id, but the public API introduces it asdc_event_devinfo_t::hw_id. To avoid consumer confusion (and mismatched downstream patches), please align the PR description and any related docs/examples to the actual exported name (hw_id) or rename consistently across the API ifdevinfo_hw_idis intended.
/* Generic hardware identifier, 0 = unknown/not available.
* On live download from devices that support it, this is populated
* with the device's hardware type identifier (e.g. Shearwater FWID
* from RDBI 0x8050). It supplements the coarse model field for
* consumer-side sub-model differentiation. Consumers that do not use
* this field are unaffected; the model field remains authoritative. */
unsigned int hw_id;
- Files reviewed: 46/46 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.
The Shearwater Petrel 1 and Petrel 2 share the same ID_MODEL value (3), making
them indistinguishable by model number alone. The previous approach on this fork
mapped ID_HARDWARE (the FWID) to a synthetic model number, but that has two
problems raised in review: the parser overwrites the model with the
Product-Version byte from the dive log's final block regardless, and a
fork-local number immediately above the upstream range risks colliding with the
next Shearwater model assignment.
This replaces that approach with a two-layer design.
Layer 1 — ID_MODEL is authoritative (fccf5e2)
ID_MODEL (RDBI 0x8060) is now the sole authoritative model source on live
Shearwater download, matching upstream libdc. shearwater_common_get_model() is
removed from the model-resolution path. shearwater_petrel_device_timesync() is
also updated to read ID_MODEL directly.
The raw FWID (ID_HARDWARE, RDBI 0x8050) is still read best-effort and exposed
to consumers through a new generic field on dc_event_devinfo_t:
This is 0 for all non-Shearwater drivers, for the Predator memory-dump path,
and on any RDBI failure. Only the two Shearwater drivers that use the field
zero-initialise dc_event_devinfo_t; all other drivers retain their existing
plain declaration.
Layer 2 — generic descriptor lookup by hardware id (27c0477)
A new function is added to the descriptor API:
The FWID-to-product mapping lives in descriptor.c in a separate g_hw_id_map[]
table alongside the descriptor entries. g_descriptors[] itself is unchanged.
The initial table covers the Shearwater Petrel family, where both Petrel 1 and
Petrel 2 report model=3:
Petrel 1: 0x0404, 0x0909
Petrel 2: 0x0505, 0x0808, 0x0838, 0x08A5, 0x0B0B, 0x7828, 0x7B2C, 0x8838
A consumer can call dc_descriptor_find_by_hw_id() with the family and
devinfo_hw_id from DC_EVENT_DEVINFO to obtain a more specific descriptor without
any device-specific knowledge on its side. NULL means the FWID is unknown; the
caller falls back to the coarse model-based descriptor. This is best-effort: the
FWID is only available on live download and can change across firmware updates.
A companion Subsurface PR calls dc_descriptor_find_by_hw_id() generically in
the DC_EVENT_DEVINFO handler, so a Petrel 2 is correctly identified even if the
user selected "Petrel" in the drop-down, and incorrect information is not
returned when the selected and connected devices differ.
Unchanged
Transport flags, protocol selection (including the Perdix 3 V2 special-case in
shearwater_common_setup), parser sample decoding, Teric time sync, Teric
tank-serial handling, and Predator calibration scaling are all unchanged.