Support extended signature for message_type_support_callbacks_t::max_serialized_size() from rosidl_typesupport_fastrtps_cpp

[MiguelCompany]

This PR adapts the code to the API break introduced by ros2/rosidl_typesupport_fastrtps#67

[asorbini]

Hi @MiguelCompany, thank you for your submission. The changes are small and there are no problems in merging it (as long as we time it with ros2/rosidl_typesupport_fastrtps#67).

I have only one request, and I'll quote myself from another PR.

Basically, at the moment a single branch (master) is supporting multiple ROS2 releases. It was the easiest way to handle a quick evolving code base. I plan on creating maintenance branches now that the RMW is more stable, but until then I'd like to keep the branch buildable with multiple versions.

For now, can you add a new "feature flag" in static_config.hpp, something like:

#ifndef RMW_CONNEXT_HAVE_EXTENDED_TYPE_SUPPORT
#define RMW_CONNEXT_HAVE_EXTENDED_TYPE_SUPPORT \
  (RMW_CONNEXT_RELEASE > RMW_CONNEXT_RELEASE_FOXY)
#endif /* RMW_CONNEXT_HAVE_EXTENDED_TYPE_SUPPORT */

And wrap the call to max_serialized_size() with it?

(the fact that this is a non-negligible inconvenience for a very small change is definitely a point in favor of introducing branches and getting rid of these macros)

EDIT: grammar

[asorbini]

I've actually started the work to get rid of feature flags, and we can merge this PR after I've cleaned up master to only track Rolling/Galactic. Please disregard my comment above.

[MiguelCompany]

@asorbini I do like feature defines, so what I've done is to add one to ros2/rosidl_typesupport_fastrtps#67 and modified this PR to depend on it.

[asorbini]

What?! I get rid of them and you introduce them... 😄

Jokes aside, I'm also a fan of this strategy in general, but it was getting out of hand for the RMW and it doesn't look like it's much of a common practice in ROS 2 code.

Anyway, I'm ok with it if you think it's useful for rosidl_typesupport_fastrtps (e.g., I'm guessing, to build it with different versions of fastrtps fastcdr). I just left a minor comment on ros2/rosidl_typesupport_fastrtps#67.

EDIT: actually, my typo lead me to a question: would it be possible to remove the dependency on fastrtps from rosidl_typesupport_c[pp]? e.g..

I think the packages only depend on fastcdr, and it would be useful to avoid having rmw_connextdds unnecessarily depend on fastrtps (it's not a problem in a typical ROS 2 installation, but it might have some impact when trying to create a minimal build/runtime environment).

EDIT 2: for context, removing the dependency would simplify things in other situations too, see for example this exception we had to add to ros2/ci to make sure fastrtps is not blacklisted if rmw_connextdds is in use.

I can open a PR on ros2/rosidl_typesupport_fastrtps if you think it would make sense to make this change.

[MiguelCompany]

@asorbini I do think it can be removed. Please go ahead with the PR

[asorbini]

I created ros2/rosidl_typesupport_fastrtps#68 to remove the dependency.

Re: this PR, if you think the feature define will stay in, then it is probably ready to merge.

Btw, thank you for catching the bug in detecting empty messages. I will need to run some tests to validate that everything is fine. The fact that all CI tests are green now makes me think that this might be an undertested scenario (or maybe we just got lucky).

[asorbini]

@MiguelCompany do you think this PR will be included in Galactic?

@clalancette thoughts?

[MiguelCompany]

@asorbini I hope so.

I suppose someone has to approve it first. And then it should be tested along with ros2/rosidl_typesupport_fastrtps#67 and ros2/rmw_fastrtps#516

[clalancette]

Sorry, this one is going to have to wait for H-Turtle. It is both an API break and a new feature, so doesn't meet the criteria for Galactic anymore.

[MiguelCompany]

@clalancette This PR does not have an API break. It just gets ready for a future API break (the one in ros2/rosidl_typesupport_fastrtps#67). It also fixes a bug here, so I think this could be merged.

We could say the same for ros2/rmw_fastrtps#516

[asorbini]

@MiguelCompany good point about the bug fix. It seems to be an undertested situation, since there are no unit tests that I know of failing because of it, but if this PR doesn't make it into Galactic, then I would split that change in a separate fix.

I'll leave it up to @clalancette and others to make a decision, but in theory this PR could also be merged even if ros2/rosidl_typesupport_fastrtps#67 is not merged, since it uses a guard.

[asorbini]

I created #33 to merge the bug fix separately.

[MiguelCompany]

@clalancette I refactored ros2/rosidl_typesupport_fastrtps#67 to make the change binary compatible

@asorbini I removed the bugfix so that #33 can be merged independently, and updated the code to support the new interface. Would you mind reviewing those changes?

[clalancette]

CI for this is in ros2/rosidl_typesupport_fastrtps#67 (comment)