Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

uCDR and IDL: remove field ordering, causing improper serialization/deserializarion #12025

Merged
merged 1 commit into from
May 16, 2019

Conversation

TSC21
Copy link
Member

@TSC21 TSC21 commented May 15, 2019

Solves PX4/px4_ros_com#12.

Test data / coverage
Example output from listener vehicle_gps_position:

pxh> listener vehicle_gps_position

TOPIC: vehicle_gps_position instance 0 #1
vehicle_gps_position_s
        timestamp: 134492000 (0.112000 seconds ago)
        time_utc_usec: 0
        lat: 473977506
        lon: 85456072
        alt: 488102
        alt_ellipsoid: 0
        s_variance_m_s: 0.2500
        c_variance_rad: 0.0000
        eph: 1.0000
        epv: 1.0000
        hdop: 0.0000
        vdop: 0.0000
        noise_per_ms: 0
        jamming_indicator: 0
        vel_m_s: 0.0000
        vel_n_m_s: 0.0100
        vel_e_m_s: 0.0000
        vel_d_m_s: -0.0100
        cog_rad: 0.3826
        timestamp_time_relative: 0
        heading: nan
        heading_offset: nan
        fix_type: 3
        vel_ned_valid: False
        satellites_used: 10

Example output from a ROS 2 listener for the same message being propagated in a uRTPS UDP link: (still has problems in some enum fields, but at least the message is usable)

RECEIVED DATA ON VEHICLE GPS POSITION
================================
ts: 186892000
lat: 473977506
lon: 85456072
alt: 488089
alt_ellipsoid: 0
s_variance_m_s: 0.25
c_variance_rad: 0
fix_type:
eph: 1
epv: 1
hdop: 0
vdop: 0
noise_per_ms: 0
vel_m_s: 0
vel_n_m_s: 0
vel_e_m_s: 0.01
vel_d_m_s: 0.02
cog_rad: 1.70147
vel_ned_valid: 0
timestamp_time_relative: 0
time_utc_usec: 0
satellites_used:

heading: nan
heading_offset: nan

Describe problem solved by the proposed pull request
While running an instance of a Fast-CDR deserialisation in a micro-RTPS bridge, the following error was triggered:

terminate called after throwing an instance of 'eprosima::fastcdr::exception::BadParamException'
what(): Unexpected byte value in Cdr::deserialize(bool), expected 0 or 1
Aborted (core dumped)

Further debug using gdb allowed to verify which msgs where triggering this error, and which specific fields. It turns out that the message fields where not being properly initialised because they were sorted before being serialized but then the deserialization was not considering the sorting of the fields, which resulted in wrong deserialization of the fields.

Describe your preferred solution
There's not obvious advantage on sorting the fields before serialization and when building the IDL file, so that sorting was removed and the problem was solved.

Describe possible alternatives
We can consider sorting in both sides of serialization and deserialization (ROS2 agent side), but no obvious advantage on that.

@TSC21
Copy link
Member Author

TSC21 commented May 15, 2019

@jkflying, @thomasgubler FYI.

Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

There's not obvious advantage on sorting the fields before serialization and when building the IDL file, so that sorting was removed and the problem was solved.

There is, in terms of alignment: if padding bytes need to be added, not sorting will lead to larger messages and higher bandwidth requirements.
Do you know if fields are padded if needed for serialization?

Merging now since it's a fix, but needs to be looked at closer.

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.

2 participants