Skip to content

Conversation

@zhexuany
Copy link
Contributor

@zhexuany zhexuany commented Feb 8, 2026

Summary

When decoding ROS1 bag messages through the dynamic CDR codec path (DynCodec), the code was using decode() which expects standard CDR format with alignment padding. However, ROS1 messages are stored:

  1. Without a CDR encapsulation header - just raw field data
  2. With packed serialization - no alignment padding between fields

This mismatch caused decode errors for ROS1 bag messages, particularly those with string arrays or complex nested types.

Root Cause

The DynCodec::decode() method was unconditionally calling self.decoder.decode() for all message schemas. This works for standard CDR but fails for ROS1 serialized data.

Changes

  • Added detection for ROS1 encoding by checking schema_encoding.contains("ros1")
  • Route ROS1 messages to decode_headerless_ros1() instead of decode()
  • Standard CDR messages continue using decode() as before

Fixes

Resolves decode errors in roboflow_sources when reading ROS1 bag files with messages like:

  • kuavo_msgs/lejuClawState (contains string arrays)
  • Other complex ROS1 message types with nested structures

Test

  • Added examples/test_leju_state.rs for manual verification
  • Added test fixture robocodec_test_24_leju_claw.bag

@codecov
Copy link

codecov bot commented Feb 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@zhexuany zhexuany force-pushed the fix/cdr-string-array-alignment branch from 64de397 to 54ec4b0 Compare February 8, 2026 20:53
@greptile-apps
Copy link

greptile-apps bot commented Feb 8, 2026

Greptile Overview

Greptile Summary

This PR fixes ROS1 decoding through the dynamic CDR codec path by routing CDR schemas whose schema_encoding indicates ROS1 to CdrDecoder::decode_headerless_ros1() instead of decode(), avoiding the 4-byte CDR header + alignment expectations that don’t match ROS1 bag serialization.

It also adds a ROS1 bag fixture and a new test suite intended to cover the streaming/S3-like path (StreamingBagParser + CodecFactory + decode_dynamic()), plus a manual example for local verification.

Main issues to address before merge are in the new tests/example: the tests appear to map record.conn_id to channels via as u16, which can cause records to be skipped and the regression to become ineffective, and one test asserts all channels have schema_encoding == "ros1msg", which is brittle and fixture-dependent. The example also hardcodes an absolute local filesystem path, making it non-portable.

Confidence Score: 3/5

  • This PR is likely correct functionally, but the added tests/example need fixes to be reliable and portable.
  • Core change in CdrCodec::decode_dynamic() is small and aligns with documented ROS1 serialization differences. However, the new streaming regression tests have a probable channel-id mapping issue (conn_id as u16) that can silently skip decoding, and a schema_encoding assertion that is overly strict and may break on fixture changes. The manual example hardcodes an absolute path, which will not work for other environments.
  • tests/ros1_decode_dynamic_tests.rs, examples/test_leju_state.rs

Important Files Changed

Filename Overview
src/encoding/cdr/codec.rs Routes ROS1-encoded CDR schemas to decode_headerless_ros1() in decode_dynamic() based on schema_encoding containing "ros1"; otherwise uses standard decode().
tests/ros1_decode_dynamic_tests.rs Adds regression tests using a ROS1 bag fixture to exercise streaming decode_dynamic(); includes assertions about schema_encoding and allows partial decode success, but has a likely channel-id mapping bug (conn_id as u16) and a brittle assumption that all channels have schema_encoding == "ros1msg".
examples/test_leju_state.rs Adds a manual example that hardcodes an absolute local file path and is not portable/reproducible as checked into the repo.
tests/fixtures/robocodec_test_24_leju_claw.bag Adds a ~1.8MB ROS1 bag fixture used by new tests; review needed for repo policies on committing binary fixtures and test determinism.

Sequence Diagram

sequenceDiagram
    participant Caller as Client/Consumer
    participant Factory as CodecFactory
    participant Codec as CdrCodec (DynCodec)
    participant Schema as SchemaMetadata::Cdr
    participant Parser as schema parser
    participant Decoder as CdrDecoder

    Caller->>Factory: get_codec(Encoding::Cdr)
    Factory-->>Caller: &dyn DynCodec (CdrCodec)
    Caller->>Codec: decode_dynamic(data, schema)
    Codec->>Schema: match SchemaMetadata::Cdr
    Codec->>Parser: parse_schema_with_encoding_str(type_name, schema_text, schema_encoding)
    Parser-->>Codec: MessageSchema
    Codec->>Codec: is_ros1 = schema_encoding contains "ros1"
    alt ROS1 schema_encoding
        Codec->>Decoder: decode_headerless_ros1(parsed_schema, data, Some(type_name))
        Decoder-->>Codec: DecodedMessage
    else Standard CDR
        Codec->>Decoder: decode(parsed_schema, data, Some(type_name))
        Decoder-->>Codec: DecodedMessage
    end
    Codec-->>Caller: Result<DecodedMessage>
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

4 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

@zhexuany zhexuany force-pushed the fix/cdr-string-array-alignment branch from 54ec4b0 to a7021bf Compare February 8, 2026 20:54
When decoding ROS1 bag messages through the dynamic CDR codec path,
we need to use decode_headerless_ros1() instead of decode() because:
1. ROS1 messages are stored without a CDR encapsulation header
2. ROS1 uses packed serialization (no alignment padding)

The decode() method expects standard CDR format with alignment,
which causes misalignment errors when reading ROS1 bag data.

This change detects ROS1 encoding from the schema_encoding field
and routes to the appropriate decoder method.

Fixes roboflow_sources decode errors for ROS1 bag messages with
string arrays and complex nested types.
@zhexuany zhexuany force-pushed the fix/cdr-string-array-alignment branch from a7021bf to de560fa Compare February 8, 2026 20:57
@zhexuany zhexuany merged commit 617fe72 into main Feb 8, 2026
15 checks passed
@zhexuany zhexuany deleted the fix/cdr-string-array-alignment branch February 8, 2026 21:12
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.

1 participant