mares: refine Mares Icon HD display label using version-packet product name by mikeller · Pull Request #143 · subsurface/libdc

mikeller · 2026-10-03T22:42:55Z

Several marketed Mares Icon HD products share a single coarse numeric model
used for parser layout selection. For example, Puck Pro and Puck Pro + both
report model 0x18, and Puck 4, Puck Lite, Puck Pro EZ, and Puck Pro Ultra
all report model 0x35. As a result, Subsurface previously labelled dives
from these devices with whichever descriptor happened to match the model
number first, regardless of which specific variant was connected.

The mares_iconhd driver already reads a 140-byte version packet on open and
extracts a product-name string at offset 0x46 to select the correct coarse
model internally. That string was not previously reported to consumers.

Changes:

dc_event_devinfo_t (include/libdivecomputer/device.h):
Add product_name[DC_DEVINFO_PRODUCT_NAME_SIZE] (17 bytes: 16 + NUL) to
the end of dc_event_devinfo_t. The field is populated only on live
download; it is always an empty string when not applicable. The numeric
model field remains the authoritative parser-layout selector.

dc_descriptor_find_by_product_name() (include/libdivecomputer/descriptor.h,
src/descriptor.c, src/libdivecomputer.symbols):
New resolver function that maps a raw firmware product-name string to the
best-matching descriptor for a given family and coarse model. The mapping
table is evidence-based and limited to confirmed product-name strings from
the mares_iconhd driver's existing matching table:
model 0x18: "Puck Pro" → "Puck Pro"
model 0x35: "Puck4" → "Puck 4"
model 0x35: "Puck Lite" → "Puck Lite"
model 0x35: "Puck Pro U" → "Puck Pro Ultra"
Returns NULL for empty, NULL, or unrecognised names; callers fall back to
the coarse-model descriptor.

mares_iconhd (src/mares_iconhd.c):
Add mares_iconhd_fill_product_name() (placed after mares_iconhd_get_model()
to avoid the frequently-extended layout-struct block). Call it at all three
DC_EVENT_DEVINFO emit sites. Also zero-initialise dc_event_devinfo_t at
each site so that newer fields are always clean when not explicitly set.

Tests (test/test_descriptor_product_name.c, test/Makefile.am):
Unit test for dc_descriptor_find_by_product_name() covering the shared-
model Puck Pro pair, all three unambiguous Puck 4 family variants, and the
coarse-model fallback cases (ambiguous name, unknown name, NULL, empty,
wrong family). Wired into autotools via test/Makefile.am to keep the
top-level Makefile.am and configure.ac changes minimal.

Known limitations:

"Puck Pro +" has no confirmed firmware product-name string. It falls back
to the "Puck Pro" descriptor (the first match for model 0x18), which is
the same result as before this change.
"Puck" (model 0x35) is a BLE advertisement prefix used by multiple Puck
variants and is not mapped; it falls back to the "Puck 4" descriptor (the
first match for model 0x35).
product_name is populated only on live download. Import from saved dumps
or offline sources is unaffected and continues to use the coarse model.

Some device families (e.g. Mares Icon HD) distinguish multiple marketed products through a product-name string in the device version packet, but share a single coarse numeric model. The numeric model remains the authoritative parser-layout selector; the product_name field provides supplementary information for consumer-side label refinement. - Add DC_DEVINFO_PRODUCT_NAME_SIZE (17: 16 bytes + NUL terminator). - Add product_name[DC_DEVINFO_PRODUCT_NAME_SIZE] to dc_event_devinfo_t, documented as live-download-only, empty string when not applicable. - Consumers that do not use this field are unaffected; the struct is zero-initialised by callers before population. Signed-off-by: Michael Keller <github@ike.ch>

Some device families share a coarse numeric model across multiple marketed products. For the Mares Icon HD family, the device version packet contains a product-name string (at offset 0x46) that uniquely identifies several marketed variants sharing the same parser model. Add a product-name-to-descriptor mapping table (g_product_name_map) containing only confirmed product-name strings from the mares_iconhd firmware matching table, and a resolver function: dc_descriptor_t *dc_descriptor_find_by_product_name( dc_family_t family, unsigned int model, const char *product_name); Mapped strings (all DC_FAMILY_MARES_ICONHD, evidence: mares_iconhd.c): - model 0x18: "Puck Pro" → "Puck Pro" - model 0x35: "Puck4" → "Puck 4" - model 0x35: "Puck Lite" → "Puck Lite" - model 0x35: "Puck Pro U" → "Puck Pro Ultra" Strings that are ambiguous ("Puck", which is a BLE prefix matching multiple variants) are intentionally omitted; callers fall back to the coarse-model descriptor for those. The coarse model field remains the authoritative parser-layout selector. NULL / empty product_name returns NULL. Exported from libdivecomputer.symbols. Signed-off-by: Michael Keller <github@ike.ch>

The mares_iconhd driver reads a 140-byte version packet during device open; the product-name string lives at offset 0x46 (up to 16 bytes). This string is already used internally by mares_iconhd_get_model() to select the coarse numeric model, but was not previously reported to consumers. Add mares_iconhd_fill_product_name() that copies the version-packet name into devinfo.product_name (NUL-terminated, at most DC_DEVINFO_PRODUCT_NAME_SIZE-1 bytes). Place the helper after mares_iconhd_get_model() to avoid wedging into the layout-struct block that upstream frequently extends. Call it at all three DC_EVENT_DEVINFO emit sites: - mares_iconhd_device_dump() - mares_iconhd_device_foreach_raw() - mares_iconhd_device_foreach_object() Also initialise the dc_event_devinfo_t struct to zero at each site so that fields added after the original code (hw_id, product_name) are always zero/empty when not explicitly set. Signed-off-by: Michael Keller <github@ike.ch>

Add test/test_descriptor_product_name.c and wire it into the autotools build via a dedicated test/Makefile.am subdirectory, avoiding the need for subdir-objects in the top-level AM_INIT_AUTOMAKE. Build wiring: - test/Makefile.am: check_PROGRAMS / TESTS for the C test binary. - Makefile.am: unconditional 'SUBDIRS += test' so 'make check' from the root runs the test subdirectory. - configure.ac: add test/Makefile to AC_CONFIG_FILES. Test coverage: - Mares Puck Pro (model 0x18): "Puck Pro" firmware name resolves to the "Puck Pro" descriptor; "Puck Pro +" (no confirmed firmware name) returns NULL, so the caller falls back to the coarse model. - Mares Puck 4 family (model 0x35): "Puck4" → "Puck 4", "Puck Lite" → "Puck Lite", "Puck Pro U" → "Puck Pro Ultra". - Coarse-model fallback: ambiguous "Puck", unknown string, NULL, and empty product_name all return NULL. - Wrong family always returns NULL. Signed-off-by: Michael Keller <github@ike.ch>

Copilot

Copilot review overview

🟡 Changes recommended

The Puck Pro Ultra mapping is unreachable, and emitted product names are not propagated through the common event cache.

Review effort: Lite
Findings: 1 · 1

Open (2)

Match Puck Pro U before the shorter Puck Pro prefix · New
Preserve product_name when caching and emitting device info · New

What changed in this PR

Adds Mares Icon HD product-name reporting and descriptor refinement for devices sharing coarse model numbers.

Changes:

Extends device-info events with product_name.
Adds and exports product-name descriptor resolution.
Populates names from version packets and adds Autotools unit tests.

File	Description
`test/test_descriptor_product_name.c`	Tests product-name resolution and fallback behavior.
`test/Makefile.am`	Registers the new test.
`src/mares_iconhd.c`	Extracts and emits product names; cache propagation remains unresolved.
`src/libdivecomputer.symbols`	Exports the resolver API.
`src/descriptor.c`	Implements product mappings; the `Puck Pro U` mapping is unreachable due to model matching order.
`Makefile.am`	Includes test sources and distribution files.
`include/libdivecomputer/device.h`	Adds the product-name event field.
`include/libdivecomputer/descriptor.h`	Declares the resolver API.
`configure.ac`	Configures the test subdirectory.

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

+	 * matching table and map to specific descriptor product names. */
+	{DC_FAMILY_MARES_ICONHD, 0x35, "Puck4",      "Puck 4"},
+	{DC_FAMILY_MARES_ICONHD, 0x35, "Puck Lite",  "Puck Lite"},
+	{DC_FAMILY_MARES_ICONHD, 0x35, "Puck Pro U", "Puck Pro Ultra"},


 	devinfo.model = device->model;
 	devinfo.firmware = 0;
 	devinfo.serial = array_uint32_le (data + 0x0C);
+	mares_iconhd_fill_product_name (device, &devinfo);


The prefix-match loop in mares_iconhd_get_model() compared the firmware product-name field against each table entry using memcmp of the entry length. 'Puck Pro' (length 8) was listed before 'Puck Pro U' (length 10), so a device reporting 'Puck Pro U...' matched the shorter prefix first and was assigned PUCKPRO (0x18) instead of PUCK4 (0x35). Move 'Puck Pro U' before 'Puck Pro' so the more specific prefix is tested first. All other entries are unaffected. Signed-off-by: Michael Keller <github@ike.ch>

device_event_emit() cached model, firmware, and serial from the driver's local devinfo into device->devinfo, then passed &device->devinfo to the callback. The product_name field was not copied, so every callback received the zero-initialised cached value rather than the string the driver had populated. Add a memcpy() of the full product_name array to the cache-update block so consumers see the populated field. Signed-off-by: Michael Keller <github@ike.ch>

mikeller · 2026-10-03T23:22:39Z

Thanks for the review. Both points are substantiated — fixes follow.

1. "Puck Pro U" unreachable (high)

Confirmed. mares_iconhd_get_model() uses memcmp against the entry's string length, so the 8-byte "Puck Pro" prefix matched first for any version-packet name starting with "Puck Pro". The "Puck Pro U" entry at the end of the table was never reached, causing Puck Pro Ultra devices to be assigned model 0x18 (PUCKPRO) instead of 0x35 (PUCK4).

Fix: move "Puck Pro U" before "Puck Pro" in the table so the longer prefix is tested first.

2. product_name not propagated through device_event_emit cache (medium)

Confirmed. device_event_emit() for DC_EVENT_DEVINFO copied model, firmware, and serial into device->devinfo, then passed &device->devinfo to the callback. The product_name field was not copied, so every callback received an empty string regardless of what the driver had populated.

Fix: add memcpy(device->devinfo.product_name, devinfo->product_name, sizeof(device->devinfo.product_name)) to the cache-update block.

Both fixes are on the branch. All 16 unit tests in test_descriptor_product_name pass.

Copilot

Copilot review overview

🟡 Changes recommended

Other device drivers may copy uninitialized product-name data, exposing garbage instead of an empty string.

Review effort: Lite
Findings: 1 · 2

Open (3)

Match Puck Pro U before the shorter Puck Pro prefix
Initialize product_name before copying device info events · New
Preserve product_name when caching and emitting device info

Other device drivers declare dc_event_devinfo_t on the stack without zero-initialising it, so product_name is indeterminate for those emitters. Reset the cached field to all-zeros before copying from the incoming struct, and copy only DC_DEVINFO_PRODUCT_NAME_SIZE-1 bytes, leaving the final byte as the NUL set by the preceding memset. This guarantees the cached product_name is always a valid NUL-terminated string regardless of whether the emitting driver initialised the field. Signed-off-by: Michael Keller <github@ike.ch>

automake's parallel-tests mode (the default since automake 1.13) installs test-driver at the top level of the source tree via 'autoreconf -fi'. The other automake-generated auxiliary files at the same level (ar-lib, depcomp, install-sh, ltmain.sh, missing) are already listed in .gitignore; test-driver was omitted because the test/ subdirectory with TESTS was only added in this branch. Add /test-driver alongside /ar-lib to close the gap. Signed-off-by: Michael Keller <github@ike.ch>

Copilot

Copilot review overview

🔵 Needs a closer look

Existing device-info producers must initialize the new field before events are emitted.

Review effort: Lite
Findings: 1 · 1

Open (2)

Match Puck Pro U before the shorter Puck Pro prefix
Preserve product_name when caching and emitting device info

Resolved since last review (1)

Initialize product_name before copying device info events

mikeller · 2026-10-04T00:45:34Z

Initialize product_name before copying device info events

Substantiated — fixed.

The memcpy added in the previous round copied product_name unconditionally from the incoming dc_event_devinfo_t, but every driver other than mares_iconhd declares that struct on the stack without = {0}, leaving product_name as indeterminate bytes. Those bytes were then copied into the cache and propagated to every callback, potentially supplying a non-empty, non-NUL-terminated string to consumers.

Fix: before the memcpy, memset the destination product_name field to all-zeros; copy only DC_DEVINFO_PRODUCT_NAME_SIZE - 1 bytes from the source, leaving the final byte as the NUL established by the memset. This guarantees the cached field is always a valid NUL-terminated string. For mares_iconhd (which correctly uses = {0} and NUL-terminates the field), the product name is preserved unchanged. For all other drivers, the field is "" unless stack garbage happens to produce a non-NUL first byte, in which case the string is still bounded and NUL-terminated at position 16.

Touching all ~40 call sites to add = {0} would also be correct but is out of scope for this PR.

mikeller added 4 commits October 4, 2026 10:13

Copilot AI lite review requested due to automatic review settings October 3, 2026 22:42

Copilot started reviewing on behalf of mikeller October 3, 2026 22:43 View session

mikeller mentioned this pull request Oct 3, 2026

mares: refine Mares Icon HD display label using version-packet product name subsurface/subsurface#5001

Open

1 of 6 tasks

Copilot AI reviewed Oct 3, 2026

View reviewed changes

mikeller added 2 commits October 4, 2026 12:15

Copilot AI lite review requested due to automatic review settings October 3, 2026 23:20

Copilot started reviewing on behalf of mikeller October 3, 2026 23:21 View session

Copilot AI reviewed Oct 3, 2026

View reviewed changes

Comment thread src/device.c Outdated

mikeller added 2 commits October 4, 2026 13:06

Copilot AI lite review requested due to automatic review settings October 4, 2026 00:42

Copilot started reviewing on behalf of mikeller October 4, 2026 00:43 View session

Copilot AI reviewed Oct 4, 2026

View reviewed changes

Navigation Menu

mares: refine Mares Icon HD display label using version-packet product name - #143

mares: refine Mares Icon HD display label using version-packet product name#143
mikeller wants to merge 8 commits into
subsurface:Subsurface-DS9from
mikeller:feat/mares-iconhd-label-refinement-122

mikeller commented Oct 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

mikeller commented Oct 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

mikeller commented Oct 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mikeller commented Oct 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

mikeller commented Oct 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

mikeller commented Oct 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants