Skip to content

fromPgn: implement DECIMAL field type (fixes PGN 129808 DSC MMSI decode) - #452

Open
fakehec wants to merge 2 commits into
canboat:masterfrom
fakehec:fix/decimal-fieldtype-129808
Open

fromPgn: implement DECIMAL field type (fixes PGN 129808 DSC MMSI decode)#452
fakehec wants to merge 2 commits into
canboat:masterfrom
fakehec:fix/decimal-fieldtype-129808

Conversation

@fakehec

@fakehec fakehec commented Jul 16, 2026

Copy link
Copy Markdown

Implements the DECIMAL field type in readValue(), which was previously unhandled.

Problem

FieldType: "DECIMAL" had no branch in readValue(), so a 40-bit DECIMAL field fell through to the generic binary reader (bs.readBits) and was decoded as a little-endian integer. Per the canboat spec, DECIMAL means each byte holds two decimal digits (00–99).

This is used by PGN 129808 (DSC Call Information) for dscMessageAddress and mmsiOfShipInDistress, so DSC consumers such as signalk-dsc logged incorrect MMSIs for every call. Verified against a Raymarine MFD, which decodes the same on-wire PGN correctly.

DECIMAL is used only by PGN 129808 in the current spec, so blast radius is minimal.

Fix

  • Decode each byte as two decimal digits and concatenate.
  • Return a digit string to preserve leading zeros (e.g. coast-station identities 00MIDxxxx).
  • All-0xFF (or any byte > 99) → null (not available).
  • Bitstream advance is identical to the previous binary read, so downstream field alignment is unchanged.

Verified end-to-end with a real DSC position-response frame: the MMSI now decodes correctly (matching the MFD) instead of the previous bogus integer.

Closes #451

Summary by CodeRabbit

  • Bug Fixes
    • Improved decoding of decimal values, preserving leading zeros and correctly interpreting packed decimal digits.
    • Decimal values are now reported as unavailable when data is truncated, undefined, contains invalid bytes, or exceeds the supported range.
    • Improved handling of incomplete bit-aligned fields to prevent incorrect readings.

readValue() had no branch for FieldType DECIMAL, so 40-bit DECIMAL fields
fell through to the generic binary reader and were decoded as little-endian
integers. Per the canboat spec, DECIMAL means each byte holds two decimal
digits (00-99). This affects PGN 129808 dscMessageAddress and
mmsiOfShipInDistress, causing wrong MMSIs in DSC consumers (e.g. signalk-dsc).

Emit the field as a digit string to preserve leading zeros (coast-station
identities); all-0xFF or any byte > 99 => null. Bitstream advance is
unchanged, so downstream field alignment is preserved.

Fixes canboat#451
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 08734dfd-9a44-4963-acf4-eb2b1950c1c4

📥 Commits

Reviewing files that changed from the base of the PR and between 94fbae1 and abfeed4.

📒 Files selected for processing (1)
  • lib/fromPgn.ts

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

readValue now validates DECIMAL fields, detects invalid bytes across the full field, consumes non-byte-aligned remainder bits, and returns padded digit strings or null.

Changes

DECIMAL decoding

Layer / File(s) Summary
Packed decimal readValue path
lib/fromPgn.ts
The decoder rejects truncated fields, tracks invalid bytes above 99, consumes remaining bits, and returns null for invalid fields. Valid bytes remain zero-padded digit strings.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to abfee

The PR makes a localized DECIMAL decoding correction; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: sbender9

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the DECIMAL implementation and its PGN 129808 MMSI decoding fix.
Linked Issues check ✅ Passed The changes satisfy the DECIMAL decoding, validation, alignment, and PGN 129808 correction requirements in [#451].
Out of Scope Changes check ✅ Passed The changes are limited to DECIMAL decoding behavior and related bitstream handling described in [#451].
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

lib/fromPgn.ts

ESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/fromPgn.ts`:
- Around line 1278-1285: Update the byte-validation loop in the surrounding
parser to use a boolean invalid/unavailable flag that is set when any byte
exceeds 99 and never reset by later valid bytes. Return [null, undefined]
whenever that flag is triggered, including for isolated 0xFF bytes, while
preserving normal string construction for valid bytes.
- Around line 1274-1284: Update the DECIMAL handling block around nbytes and its
byte-reading loop to resolve bitLength safely, detect insufficient bs.bitsLeft
before reading, and handle truncated input without allowing an uncaught
bitstream exception. After processing the full-byte portion, consume any
remaining bitLength remainder so downstream fields stay aligned, and update the
return logic below the loop to use the existing isValid state.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5cc521ab-af6d-4272-8bbc-236be3dcd5e0

📥 Commits

Reviewing files that changed from the base of the PR and between c584d70 and 94fbae1.

📒 Files selected for processing (1)
  • lib/fromPgn.ts

Comment thread lib/fromPgn.ts Outdated
Comment thread lib/fromPgn.ts Outdated
- Latch invalid-byte detection so a valid byte after 0xFF/>99 cannot
  unmask it (e.g. [0xFF, 0x12] now yields null, not "25518").
- Guard against bitstream underflow on truncated packets: this block
  runs outside the surrounding try/catch, so an underflow would throw.
- Consume non-byte-aligned remainder bits so downstream fields stay
  aligned when BitLength is not a multiple of 8.

Addresses the CodeRabbit review comments on canboat#452.
@fakehec

fakehec commented Aug 21, 2026

Copy link
Copy Markdown
Author

Addressed the CodeRabbit review:

  • Invalid-byte detection now latches — a valid byte following an 0xFF/>99 byte can no longer unmask the field (e.g. [0xFF, 0x12] yields null instead of "25518").
  • Bitstream underflow on truncated packets is guarded before reading (this block runs outside the surrounding try/catch).
  • Non-byte-aligned remainder bits are now consumed so downstream fields stay aligned when BitLength isn't a multiple of 8.

Thanks for the review.

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.

PGN 129808 (DSC): FieldType DECIMAL not implemented — MMSI address fields decode as binary integers

1 participant