Skip to content

shearwater: give Petrel 2 a distinct model number (15) - #124

Closed
mikeller wants to merge 1 commit into
subsurface:Subsurface-DS9from
mikeller:feat/shearwater-petrel2-model-#22
Closed

shearwater: give Petrel 2 a distinct model number (15)#124
mikeller wants to merge 1 commit into
subsurface:Subsurface-DS9from
mikeller:feat/shearwater-petrel2-model-#22

Conversation

@mikeller

@mikeller mikeller commented Sep 2, 2026

Copy link
Copy Markdown
Member

PETREL2 was aliased to PETREL (3), making it impossible to distinguish
a Shearwater Petrel 2 from a Petrel by model number alone.

Assign PETREL2 the distinct value 15 and update the 'Shearwater Petrel 2'
descriptor accordingly. PERDIX3 (14) was already present on Subsurface-DS9
from a prior merge; no change needed for that constant.

Hardware ID mappings in shearwater_common_get_model() are unchanged:

  • 0x0404 / 0x0909 -> PETREL (3)
  • 0x0505 / 0x0808 / 0x0838 / 0x08A5 / 0x0B0B / 0x7828 / 0x7B2C / 0x8838
    -> PETREL2 (now 15, previously aliased to 3)

shearwater_predator_parser.c defines its own local PREDATOR/PETREL/TERIC
constants and does not reference PETREL2; no change needed there.

Transport flags, protocol selection, parser behaviour, and all other
hardware-family boundaries are unchanged.

PETREL2 was aliased to PETREL (3), making it impossible to distinguish
a Shearwater Petrel 2 from a Petrel by model number alone.

Assign PETREL2 the distinct value 15 and update the 'Shearwater Petrel 2'
descriptor accordingly. PERDIX3 (14) was already present on Subsurface-DS9
from a prior merge; no change needed for that constant.

Hardware ID mappings in shearwater_common_get_model() are unchanged:
- 0x0404 / 0x0909 -> PETREL (3)
- 0x0505 / 0x0808 / 0x0838 / 0x08A5 / 0x0B0B / 0x7828 / 0x7B2C / 0x8838
  -> PETREL2 (now 15, previously aliased to 3)

shearwater_predator_parser.c defines its own local PREDATOR/PETREL/TERIC
constants and does not reference PETREL2; no change needed there.

Transport flags, protocol selection, parser behaviour, and all other
hardware-family boundaries are unchanged.

Signed-off-by: Michael Keller <github@ike.ch>
Copilot AI lite review requested due to automatic review settings September 2, 2026 01:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The changes consistently update the model constant and corresponding descriptor without impacting the existing hardware ID mapping logic.

Pull request overview

This PR updates the Shearwater device model numbering to uniquely identify the Petrel 2 by assigning it a distinct model number (15) instead of aliasing it to the Petrel (3), enabling unambiguous model-based identification across the library.

Changes:

  • Assign PETREL2 a distinct numeric model value (15) instead of aliasing it to PETREL.
  • Update the Petrel 2 descriptor entry to use model number 15.
File summaries
File Description
src/shearwater_common.h Changes PETREL2 from an alias of PETREL to a distinct model constant (15).
src/descriptor.c Updates the “Shearwater Petrel 2” descriptor to report model number 15 (previously 3).
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@jefdriesen

Copy link
Copy Markdown
Contributor

I think this change is a bad idea. The new value (15) will very likely result in a conflict with the next Shearwater model. Also in the parser, the value will get replaced again with the true model number (3), because that is the value that is stored in the final block.

The mapping of the hardware id to model number has always been an ugly hack that only existed because we didn't know there was also an RDBI command available to read the model number directly. The ID_MODEL command will always be correct, even for not yet released models, while the mapping with the ID_HARDWARE command will always be behind because there is no official list available and we can only update after new models appear.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The changes are narrowly scoped to model-number identity, and call-site context indicates the updated model value flows safely through existing descriptor/device logic without requiring additional behavioral changes.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@mikeller

mikeller commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

Thanks Jef, both points are correct.

You're right that model 15 risks colliding with the next upstream assignment,
and right that the parser overwrites the model with the Product-Version byte from
the final block anyway, so the change never achieved its goal.

For context: Shearwater did supply us with a hardware-id list, which is what
prompted this attempt. But working through the implications confirmed your point
rather than refuting it — the list is a snapshot, and as you note upstream
commit 75ecac9 shows the FWID can even change when firmware is updated, so it
can never be kept reliably current.

I've reworked the approach entirely in PR #125:

  • ID_MODEL (0x8060) is now the sole authoritative model source, read directly as
    upstream does, with shearwater_common_get_model() removed from the
    model-resolution path.
  • The raw FWID (ID_HARDWARE, 0x8050) is still read on live download and exposed
    to consumers via a new generic field on dc_event_devinfo_t (devinfo_hw_id,
    0 = unknown). This lets the consumer distinguish e.g. Petrel 1 vs Petrel 2 on
    a live download without libdc mapping the FWID into a synthetic model number.
  • The FWID-to-descriptor mapping lives in descriptor.c alongside the descriptor
    entries, exposed via a new generic dc_descriptor_find_by_hw_id() function.
    The consumer calls this with the reported family and devinfo_hw_id to get a
    refined descriptor, with no device-specific knowledge required on the consumer
    side. If the FWID is absent or unknown it falls back cleanly to the
    coarse model.

This aligns the fork with upstream's direction in f351434 while preserving the
Petrel 1/2 distinction on live downloads as best-effort.

@mikeller mikeller closed this Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants