Skip to content

feat: per-point timing support for PointCloud2 - #3103

Open
jeff-hykin wants to merge 12 commits into
mainfrom
jeff/feat/pointcloud_timing
Open

feat: per-point timing support for PointCloud2#3103
jeff-hykin wants to merge 12 commits into
mainfrom
jeff/feat/pointcloud_timing

Conversation

@jeff-hykin

@jeff-hykin jeff-hykin commented Jul 20, 2026

Copy link
Copy Markdown
Member

Overdue cleanup

TLDR: PointCloud2 now supports more of PointCloud2 spec, ex: x,y,z,intensity,offset_time,tag,line and x,y,z,timestamp instead of only supportingx,y,z,intensity

  • needed for fastlivo
  • Partly fixes the pr "TODO: work with full spectrum of pointcloud2".
  • good for cleaning up pointlio/fastlio

Test with

dimos run mid360

@jeff-hykin
jeff-hykin force-pushed the jeff/feat/pointcloud_timing branch from 1fa46bf to a61f4a4 Compare July 20, 2026 20:42
@greptile-apps

greptile-apps Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds per-point timing metadata to Livox PointCloud2 messages. The main changes are:

  • Publish offset_time, tag, and line fields from the Mid-360 driver.
  • Encode and decode these optional attributes in Python PointCloud2 messages.
  • Add an LCM round-trip test for timing metadata.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.

Important Files Changed

Filename Overview
dimos/hardware/sensors/lidar/livox/cpp/main.cpp Adds packed per-point timing and tag fields to the Mid-360 PointCloud2 publisher.
dimos/msgs/sensor_msgs/PointCloud2.py Adds optional timing, tag, and line attributes to PointCloud2 encoding and decoding.
dimos/msgs/sensor_msgs/test_PointCloud2.py Adds round-trip coverage for the new per-point metadata fields.

Reviews (2): Last reviewed commit: "review: vendor-neutral PointCloud2 docs/..." | Re-trigger Greptile

Comment thread dimos/hardware/sensors/lidar/livox/cpp/main.cpp Outdated
Comment thread dimos/hardware/sensors/lidar/livox/cpp/main.cpp Outdated
Comment thread dimos/msgs/sensor_msgs/PointCloud2.py
@jeff-hykin
jeff-hykin marked this pull request as draft July 20, 2026 20:47
…ng config (default on)

Consumers that never read offset_time/tag/line no longer pay for extracting
them (0.50 -> 0.35 ms/frame decode; plain layout 0.21). Mid360Config.per_point_timing=False
restores the legacy 16-byte wire layout entirely.
@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.04950% with 5 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
dimos/msgs/sensor_msgs/PointCloud2.py 93.15% 2 Missing and 3 partials ⚠️
@@            Coverage Diff             @@
##             main    #3103      +/-   ##
==========================================
+ Coverage   74.90%   74.91%   +0.01%     
==========================================
  Files        1114     1114              
  Lines      106086   106185      +99     
  Branches     9665    10223     +558     
==========================================
+ Hits        79459    79551      +92     
- Misses      23817    23822       +5     
- Partials     2810     2812       +2     
Flag Coverage Δ
OS-ubuntu-24.04-arm 68.53% <95.04%> (+0.01%) ⬆️
OS-ubuntu-latest 70.54% <95.04%> (+0.02%) ⬆️
Py-3.10 70.54% <95.04%> (+0.02%) ⬆️
Py-3.11 70.54% <95.04%> (+0.02%) ⬆️
Py-3.12 70.54% <95.04%> (+0.02%) ⬆️
Py-3.13 70.54% <95.04%> (+0.02%) ⬆️
Py-3.14 70.54% <95.04%> (+0.02%) ⬆️
Py-3.14t 70.53% <95.04%> (+0.02%) ⬆️
SelfHosted-Large 29.30% <5.05%> (-0.03%) ⬇️
SelfHosted-Linux 35.91% <20.20%> (-0.02%) ⬇️
SelfHosted-macOS 35.01% <20.20%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
dimos/hardware/sensors/lidar/livox/module.py 93.18% <100.00%> (+0.15%) ⬆️
dimos/msgs/sensor_msgs/test_PointCloud2.py 98.14% <100.00%> (+0.58%) ⬆️
dimos/msgs/sensor_msgs/PointCloud2.py 64.50% <93.15%> (+5.73%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jeff-hykin jeff-hykin added the backport:skip Skip creating a backport to any release branches label Jul 20, 2026
pc.fields[3] = make_field("intensity", 12, sensor_msgs::PointField::FLOAT32);
if (timing) {
pc.fields[4] =
make_field("offset_time", OFFSET_TIME_OFFSET, sensor_msgs::PointField::UINT32);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ROS version of livox follows the same general pattern

…fset_time) / legacy (x,y,z,intensity)

Replaces the per_point_timing bool. No LIO in the stack reads intensity
(passthrough for viz only), so minimal mode carries just geometry + the
per-point timing that scan undistortion needs, at the legacy 16 B/point.
@jeff-hykin jeff-hykin changed the title feat: per-point timing (offset_time/tag/line) in PointCloud2 + Mid-360 driver feat: per-point timing support for PointCloud2 Jul 20, 2026
…e wire default, full-format clouds imply a consumer that reads them
// minimal (default) x,y,z,offset_time — 16 B/point
// full x,y,z,intensity,offset_time,tag,line — 22 B/point
// legacy x,y,z,intensity — 16 B/point
// offset_time is uint32 ns since the header stamp; tag/line are the Livox

@jeff-hykin jeff-hykin Jul 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

main livox change: publish timing info

# Parse field offsets. The message is self-describing; a known field is
# honored only when its advertised datatype matches what we read it as,
# otherwise it is treated as absent rather than misread.
_expected_datatype = {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note: this is generic, PointField is a ROS struct, and Pointcloud2 supports all these fields

@jeff-hykin
jeff-hykin marked this pull request as ready for review July 27, 2026 18:15
@github-actions github-actions Bot added the ready-to-merge Required CI checks have passed on this PR label Jul 27, 2026
@github-actions github-actions Bot added ready-to-merge Required CI checks have passed on this PR and removed ready-to-merge Required CI checks have passed on this PR labels Jul 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip Skip creating a backport to any release branches PlzReview ready-to-merge Required CI checks have passed on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants