[follesoe]

Messages for a SLAM on the drone: it builds a 3D map from a mapping sensor (the Water Linked Sonar 3D-15 first) and tracks the drone in it.

• Telemetry on the SLAM's own stream: the pose estimate with its covariance (SlamPoseTel), map updates and the whole map (SlamMapUpdateTel, SlamMapTel), the latest scan (SlamScanTel) and the SLAM's state, sensors and recording (SlamStatusTel).
• Requests to start and stop it, change the sensor's settings, reset the map, fetch the map and record (SetSlamActiveReq, SetSlamSensorSettingsReq, ResetSlamReq, GetSlamMapReq, SetSlamRecordingReq).
• Loop closures (SlamLoopClosure) on the map update whose correction they make, and all of them in the whole map, so a viewer can show what a loop corrected.
• Sensor-neutral: poses are the platform's (the drone's), and each sensor has an id and its mount on the drone, so a stereo camera can map alongside the sonar later. The map frame says how it lies in the world (gravity, north, origin depth).
• Fixes the frame in the DvlVelocity comment: y is right, not left.
• Not SLAM: ResetPubFrequencyReq/Rep and ResetLogFrequencyReq/Rep reset a message's publishing or logging rate to the drone's default (empty: every message), so a client that raised a rate (as the SLAM's recording does for the dive log) can put it back.

Related: BluEye-Robotics/libblunux#527
Related: BluEye-Robotics/p2_drone#1216
Related: BluEye-Robotics/p2-django#732
Related: BluEye-Robotics/meta-blueye#535

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

The pose documentation mislabels the platform pose as a sensor pose, which could cause consumers to omit the mount transform for tilted or offset sensors.

1 open issue

• Clarify that the estimated pose is the platform pose · New

What changed in this PR

Adds sensor-neutral SLAM protocol definitions for mapping sensors on Blueye drones, covering map data, platform poses, telemetry, and control.

Changes:

• Defines SLAM poses, covariance, point clouds, map updates, sensor metadata, and coordinate frames.
• Adds SLAM telemetry and requests for activation, settings, map retrieval/reset, and recording.
• Adds publishing/logging frequency reset requests and corrects the DVL velocity frame comment from “y left” to “y right.”

File	Description
protobuf_definitions/​telemetry.proto	Adds SLAM pose, map, scan, and status telemetry.
protobuf_definitions/​req_rep.proto	Adds SLAM control and frequency reset request/response messages.
protobuf_definitions/​message_formats.proto	Defines shared SLAM data types and corrects the DVL frame comment.

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

The branch versioning can generate invalid SemVer for all-digit SHAs with leading zeros, causing pnpm version to fail and blocking publication.

2 open issues

• Prefix the short SHA to ensure a valid SemVer prerelease · New
• Clarify that the estimated pose is the platform pose

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

The shared SLAM wire contract depends on four inaccessible implementation changes, requiring domain-owner review of producer/consumer agreement on transforms, covariance conventions, map sequencing, and recording semantics.

No open issues 🎉

2 resolved since last review

• Prefix the short SHA to ensure a valid SemVer prerelease
• Clarify that the estimated pose is the platform pose

_{Review effort: Balanced}

[follesoe]

Thanks Juan. All three main points are in: protocol 69e4406, be-slam 402b47da (p2_drone#1216), and viewer c7dc324d (p2-django#732, on the 3.4.0-sonar3d-slam.g69e44062 prerelease).

1. Submaps: SlamSubmap now has oneof content { PointCloud points = 3; }.
   • I kept the existing numbers (points 3, sensor_id 4) rather than renumbering, so it stays wire-compatible with the recordings we have.
   • removed_submap_ids and the map-part rule no longer mention points. Every repeated field is concatenated in part order, and every other field is taken from the part that sets it. The viewer now joins parts generically by that rule.
2. Catch-up: GetSlamMapReq/Rep as you proposed.
   • be-slam keeps its latest 256 updates (at most 8 MB) and republishes the missed ones; otherwise it sends the whole map, with part_count in the reply.
   • The viewer drives its sync from the reply and falls back to the whole map if a catch-up stalls. It treats a reply with neither field (an older be-slam) as the whole map.
3. Frame in the status: SlamStatusTel.frame = 7, sent every second; the viewer uses it until it holds the map. SlamPoseTel now says the pose is the platform's (SlamMapFrame.platform).

Smaller notes:

• Recordings: the SlamRecordingState and SlamMapTel comments now say a recording can hold several maps, each ended with the whole map (in parts).
• Covariance: reworded as you suggested.
• ResetSlamReq.map_id 0: now resets only when there is no map yet, in be-slam and the viewer (which already sends the id it holds).
• PointCloud.count: positions are exactly count × 12 bytes, and intensities empty or count bytes. be-slam and the viewer now drop a cloud that disagrees instead of truncating it.
• replace / removed_submap_ids: be-slam doesn't send them yet, and nothing is planned short term. I kept them because the viewer handles them and raster content will need a replace rule anyway. I'm happy to drop them until they're used if you'd rather.
• SlamMap vs SlamMapUpdate: I left them separate. A snapshot would carry update-only fields (moved_poses, removed_submap_ids) that mean nothing there. With the part rule the viewer's join is already generic, and both feed the same store.
• slam.proto: done. The Tel messages stay in telemetry.proto, the Req/Rep pairs in req_rep.proto, and Quaternion and PointCloud in message_formats.proto. CLAUDE.md lists the new file.
• CI trigger: noted. I'll drop it before merging, and re-pin p2-django to a master build after.

[jp-pino]

Thanks, this looks good. I went through 69e4406 and be-slam's 402b47da: the submap oneof, the catch-up (the reply logic in map_server.cpp follows the request/reply comments exactly), the part rule, the frame in the status, slam.proto and the smaller notes are all as discussed. Keeping SlamMap and SlamMapUpdate separate and keeping replace/removed_submap_ids are fine by me.

One small comment, not blocking: SlamMap.map_id and GetSlamMapRep.map_id say "0 before the first scan", but be-slam gives a new map its id at reset, before its first scan (Slam::reset), so after a reset the id is non-zero with no keyframes yet. Maybe "0 when the SLAM has no map", which also matches the new ResetSlamReq wording.

Approving on the understanding that the sonar3d-slam trigger in ci-typescript.yaml comes out before merging.

[sindrehan]

I generated blueye.protocol from this branch (69e4406) and built the SDK docs against it. It generates cleanly, the docs build with --strict, and the SDK unit tests pass. Some notes from the Python side:

1. SlamSensor.type becomes type_ in Python

type is reserved by proto-plus, so the field gets a trailing underscore. There is precedent: we have type_ three times and range_ twice already. Since nothing is released yet, would sensor_type work? The rename is wire-compatible, though it changes the TS and JSON names.

2. Where is the SLAM publisher?

The Slam*Tel comments say "on the SLAM publisher, not the main telemetry socket", but the proto never says where that is. As I understand the control system side, it is PUB 5561 (ws 9989) and REP 5562 (ws 9990), with requests forwarded by the drone's request server. Could the slam.proto header say so? SDK users read these comments as the reference docs. Without it they will expect the normal telemetry subscription to deliver the messages.

3. Can a submap be split across SlamMap parts?

The joining rule says repeated fields are concatenated in part order. If one submap's points can span two parts, the client gets two submaps entries with the same submap_id and has to merge them. Can that happen? Either answer belongs in the comment.

4. ResetPubFrequencyReq/Rep and ResetLogFrequencyReq/Rep

These are in the diff but missing from the description, and they have nothing to do with SLAM. Please add a line about them so they make it into the changelog.

5. Small things

• SlamSensor.id says "when no sensor of that model is connected". Should that be "no other sensor of that model"?
• ResetSlamRep has available as field 2. The other four replies have it as field 1, so the attribute order differs in the docs. Cheap to align now.
• mount_tilt, depth, magnetic_declination and origin_depth are the first optional fields in the protocol. No change needed here; the Python docs for them need a template fix on our side.

Docs rendering (no change needed here)

SlamPose, Quaternion and SlamMapUpdateTel lose the blank line after the summary, and 13 of the new summaries are cut mid-sentence in the class tables. The generator mishandles a long first line. Many existing messages have the same problem, so I'll fix it in the blueye.protocol templates and leave the comments here as they are. The SDK also needs a reference page for slam.proto. I'll add that when the protocol is bumped.

[follesoe]

Thanks Sindre, very useful. Changes are in 32827e4, with be-slam d9e2866f (p2_drone#1216) and the viewer eb76d477 (p2-django#732, on the 3.4.0-sonar3d-slam.g32827e44 prerelease).

1. type: renamed to sensor_type. It keeps the same field number, so existing recordings are unaffected.
2. Where the SLAM publishes: the slam.proto header now says the SLAM has its own publisher on TCP 5561 (WebSocket 9989). Its requests go to the drone's request server like any other, which forwards them to the SLAM on 5562 (WebSocket 9990).
3. Submaps across parts: no. be-slam never splits a submap, and the SlamMap comment now says so: each submap comes whole, in one part.
4. Frequency messages: added a line about ResetPubFrequencyReq/Rep and ResetLogFrequencyReq/Rep to the description.
5. Small things:
   • "no other sensor of that model": fixed.
   • ResetSlamRep now has available = 1 and map_id = 2, like the other replies. It's renumbered while it's unreleased, and be-slam and the viewer are updated.

Thanks for taking the docs rendering and the slam.proto reference page on your side.

[follesoe]

Thanks Juan. Fixed in 7eed67c: SlamMap.map_id, GetSlamMapRep.map_id and SlamStatusTel.map_id now say 0 when the SLAM has no map. The sonar3d-slam trigger comes out before merging.

[Copilot]

🔵 Needs a closer look

SLAM wire-contract compatibility requires maintainer validation because the four linked producer and client repositories were inaccessible, preventing checks of endpoint routing, coordinate frames, and map synchronization.

No open issues 🎉

1 resolved since last review

• Keep sensor IDs stable when same-model sensors connect

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Human assessment is needed to evaluate ambiguous sensor-selection behavior when an empty sensor ID matches multiple sensors.

No open issues 🎉

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Human assessment is needed because critical map catch-up and snapshot-retry edge cases can double-apply pose corrections or silently lose keyframes.

No open issues 🎉

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

Allowing later parts to omit snapshot identity fields can cause clients to combine different snapshots and apply subsequent updates to the wrong baseline.

1 open issue

• Require snapshot identity on every map part · New

_{Review effort: Balanced}