Skip to content

feat: prefer CR5L BLE characteristics on macOS#207

Open
KalenJosifovski wants to merge 2 commits into
submersion-app:mainfrom
KalenJosifovski:cr5l-support
Open

feat: prefer CR5L BLE characteristics on macOS#207
KalenJosifovski wants to merge 2 commits into
submersion-app:mainfrom
KalenJosifovski:cr5l-support

Conversation

@KalenJosifovski
Copy link
Copy Markdown

Summary

Prefer the CREST CR5L Nordic UART Service characteristics in the macOS BLE bridge.

This is the Submersion-side companion to pending libdivecomputer CR5L support. On its own, this PR does not add CR5L parsing or transport to Submersion. It adjusts the Darwin BLE bridge so that once the vendored libdivecomputer is updated to a CR5L-capable revision, Submersion selects the correct CoreBluetooth service/characteristics for the watch.

This PR should not be merged before the corresponding libdivecomputer CR5L support is available and vendored into Submersion.

Changes

  • Add CR5L-specific BLE characteristic preferences in the Darwin bridge.
  • Prefer the CR5L Nordic UART Service UUIDs when the device is CREST-CR5L.
  • Subscribe correctly to the CR5L response characteristics so downstream libdivecomputer transport can receive control responses, list packets, and bulk data packets.

Why

The CR5L uses a Nordic UART Service layout with multiple response paths:

  • Service: 6E400001-B5A3-F393-E0A9-E50E24DCCA9E
  • Write: 6E400002-B5A3-F393-E0A9-E50E24DCCA9E
  • Response / notify-indicate paths:
    • 6E400002-B5A3-F393-E0A9-E50E24DCCA9E
    • 6E400003-B5A3-F393-E0A9-E50E24DCCA9E
    • 6E400004-B5A3-F393-E0A9-E50E24DCCA9E

Without this preference logic, the macOS bridge can connect to the watch but route packets incorrectly, which prevents the downstream CR5L transport from completing a download.

Scope

Included in this PR:

  • packages/libdivecomputer_plugin/darwin/Sources/LibDCDarwin/BleIoStream.swift

Explicitly not included in this PR:

  • local development override files such as LIBDIVECOMPUTER_FORK_PATH.txt
  • local libdivecomputer fork build wiring used only for development
  • vendored third_party/libdivecomputer updates
  • unrelated macOS build workaround changes

Dependency

This PR depends on the corresponding libdivecomputer CR5L support PR. During local development, Submersion was tested against an external local libdivecomputer fork via an untracked pointer file and build override. That local development mechanism is intentionally not part of this PR.

The intended merge sequence is:

  1. Merge libdivecomputer CR5L support upstream.
  2. Vendor/update that libdivecomputer revision into Submersion.
  3. Merge this Submersion BLE bridge change alongside the vendor update if still needed.

Background Material

Optional background material for reviewers who want the reverse-engineering and validation context:

These are intentionally not required to review this PR. The code change here is narrow and only handles the macOS BLE bridge side.

Test Plan

  • flutter test passes
  • flutter analyze passes (failed only because of the unrelated local-only packages/google_sign_in_ios_patched package)
  • Manual macOS testing against a real CREST-CR5L watch using a local CR5L-enabled libdivecomputer fork

Manual testing performed locally:

  • Submersion discovered CREST-CR5L
  • macOS CoreBluetooth selected the intended CR5L NUS characteristics
  • end-to-end download succeeded downstream once paired with the local CR5L-enabled libdivecomputer

Notes For Reviewers

  • This PR is intentionally narrow.
  • The actual CR5L protocol implementation lives in the companion libdivecomputer PR.
  • If reviewers prefer, this PR can also wait until the Submersion vendored libdivecomputer subtree is updated in a follow-up PR.

Teach the Darwin BLE bridge to prefer the CREST CR5L Nordic UART Service characteristics and subscribe to the CR5L response paths. This keeps the Submersion-side change narrow while allowing a downstream CR5L-capable libdivecomputer to communicate with the watch correctly on macOS.
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Member

@ericgriffin ericgriffin left a comment

Choose a reason for hiding this comment

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

Thanks for the thorough writeup and the gist links.

Overall the approach makes sense. A few things I'd like to flag before merge though:

  1. Scope of the read-buffer change. The switch from readBuffer: Data to readPackets: [Data] + partialReadBuffer affects every device, not just CR5L — a performRead call can no longer span packet boundaries. I think that's fine under libdivecomputer's partial-read contract, but it's worth calling out explicitly in the PR description since it touches Pelagic Gen1 and anything else on the existing path.

  2. "CR5L" behavior is actually keyed on the generic NUS RX UUID (6E400002-B5A3-F393-E0A9-E50E24DCCA9E), so any NUS-speaking peripheral would hit the multi-subscribe + write-with-response path. Might want to either narrow the gate (peripheral.name prefix) or rename/broaden the framing so it's honest about what's happening.

  3. A couple of correctness issues in the state machine and notify handling (detailed inline) - one is a genuine silent-drop path I'd want to fix before merging.

Specifics inline. Happy to pair on any of these.

Comment thread packages/libdivecomputer_plugin/darwin/Sources/LibDCDarwin/BleIoStream.swift Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the macOS (Darwin) BLE bridge in libdivecomputer_plugin to prefer CREST CR5L’s Nordic UART Service (NUS) UUIDs and to support subscribing to multiple notify/indicate characteristics so downstream libdivecomputer transport can receive CR5L response paths correctly.

Changes:

  • Add NUS service/write/notify UUIDs to the bridge’s preferred UUID scoring.
  • Introduce multi-characteristic subscription (subscribedCharacteristics) for CR5L-style services.
  • Refactor read buffering from a single byte buffer into queued notification packets.

Narrow the Nordic UART handling to likely CREST devices, harden the notify state machine, ignore empty notifications, add fallback diagnostics, and enforce the packet-based read contract expected by libdivecomputer transports.
@KalenJosifovski
Copy link
Copy Markdown
Author

Thanks. I addressed the requested changes in 02a1eaf.

Summary:

  • narrowed the NUS-specific behavior to likely CREST devices instead of keying only on the generic NUS RX UUID
  • kept notifyCharacteristic explicitly as log-only
  • added warning logs for ignored notifications and fallback paths
  • ignored empty notifications
  • fixed waitingForNotifyEnable on the error / !isNotifying paths
  • preserved the packet-based read behavior and updated the PR description accordingly

On the read-path change: I also aligned it with libdivecomputer's packet-transport expectations from jefdriesen's comment. In particular, if a single queued BLE packet is larger
than the requested read size, performRead now returns LIBDC_STATUS_IO instead of silently partial-reading it.

@KalenJosifovski
Copy link
Copy Markdown
Author

Two extra points for context:

  • I’ll leave the review threads for @ericgriffin to resolve as maintainer once he’s happy with the
    follow-up commit.
  • I don’t have the CR5L watch with me right now and won’t have it again until Monday, so I have not been
    able to re-run a live physical-device test after this review-response commit. I’m happy to hold off on
    asking for merge until I can retest against the watch if that’s preferred.

Copy link
Copy Markdown
Member

@ericgriffin ericgriffin left a comment

Choose a reason for hiding this comment

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

👍 Let me know once you're able to test again and then happy to merge. Thanks.

@KalenJosifovski
Copy link
Copy Markdown
Author

Quick update from additional live testing.

I hit and fixed a new robustness issue locally that is not in this PR branch yet:

  • CR5L scan alias matching (CREST-CR5L advertisement name variants)
  • known-device vendor/product normalization before descriptor lookup

Those fixes are currently in my local cr5l-libdc-log-wiring branch. I can port them into this PR as a small focused follow-up commit if you want.

Separately, I also wired explicit libdivecomputer internal log callback handling in that same branch for protocol debugging. That logging wiring is useful for diagnostics, but I’m treating it as optional and can keep it out of the main PR unless you want it included.

Related context: I’m currently working with Jef on the libdivecomputer PR to settle final CR5L protocol framing details using the raw capture. Since this Submersion support depends on that backend behaviour, it may be cleaner to let that resolve first and then finalise here. No rush on my side.

@ericgriffin
Copy link
Copy Markdown
Member

Thanks for the update. Feel free to bring all of the things you mentioned into this branch if you'd like to. Happy to merge it whenever you feel it's ready.

@KalenJosifovski
Copy link
Copy Markdown
Author

Thanks Eric, will do. I will try and get this done before I go on annual leave end of May but cant promise anything. Either way we will be waiting on the libdivecomp side so no rush I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

4 participants