ros2 / rmw_zenoh

RMW for ROS 2 using Zenoh as the middleware
Apache License 2.0
144 stars 29 forks source link

FastCDR version bump #141

Closed AlexDayCRL closed 3 months ago

AlexDayCRL commented 3 months ago

the ros2 rolling repo recently bumped FastCDR from version 1.1.x to 2.2.x. This moves around some of the constants for FastBuffers. Specifically it renames getSerializedDataLength to get_serialized_data_length and moves where the DDS_CDR serialization flag lives.

References: Rolling repos update: https://github.com/ros2/ros2/pull/1530/files FastCDR Include Dirs @ 1.1.x: https://github.com/eProsima/Fast-CDR/tree/1.1.x/include/fastcdr FastCDR Include Dirs @ 2.2.x: https://github.com/eProsima/Fast-CDR/tree/2.2.x/include/fastcdr

AlexDayCRL commented 3 months ago

this is also stacked on top of yadu/noble and rebased to rolling.

Yadunund commented 3 months ago

Thanks for spear heading this initiative. One thing to keep in mind is that we need the code to compile for Iron on Jammy with FastCDR 1.x.x and Rolling on Noble with FastCDR 2.x.x. We can achieve this in two ways

  1. Branch off rolling to create an iron branch for rmw_zenoh. Then merge this into rolling. All future PRs merged into rolling will need to be carefully backported to iron.
  2. We implement this change by
    • Checking version of FastCDR found a find_package() call within rmw_zenoh_cpp/CMakeLists.txt and then injecting a compile definition into the codebase, say -DFAST_CDR_2 if 2.x.x is found.
    • Updating rmw_zenoh.cpp such that it relies on #ifdef FAST_CDR_2 blocks to invoke the 2.x.x API or else the 1.x.x ones.

Personally I'm in favor on 2 since rmw_zenoh is still under heavy development and I rely on Iron binaries for Nav2, moveit, open-rmf, etc to test functionality. @clalancette what do you think?

clalancette commented 3 months ago

So I rebased this onto the latest, and I also went with strategy 2 as laid out by @Yadunund . For now, we can keep all of our development on the rolling branch, though eventually we are going to want to branch off for iron (and soon jazzy). In any case, what I did in 8dee384442cf0fef700ef97f08e2a15ed560170d should paper over those differences, and in my testing, it makes this build for both Fast-CDR v1 and v2.

Please take a look.

clalancette commented 3 months ago

I'm surprised we didn't need to include a add_compile_definitions(-DFASTCDR_VERSION_MAJOR ${FASTCDR_MAJOR_VERSION}) in the CMakeLists.txt file.

Fast-CDR already generates this during its own installation phase and puts it in config.h file, which we can then reuse. So no need for our own definition.

Yadunund commented 3 months ago

I'm surprised we didn't need to include a add_compile_definitions(-DFASTCDR_VERSION_MAJOR ${FASTCDR_MAJOR_VERSION}) in the CMakeLists.txt file.

Fast-CDR already generates this during its own installation phase and puts it in config.h file, which we can then reuse. So no need for our own definition.

Ah that explains it.