Continuing discussion from #536:

FYI, I think this change is going to cause a string of new test failures in ros2cli, as the warning appears when running various CLI tools (e.g. ros2 topic echo).

The warning that appeared is the one saying that Fast-RTPS doesn't support qos incompatibility callbacks, right?

I think the problem can be avoided in the following way:

• A warning is showed if the event type is not supported and the user explicitly required a callback.
• If the user doesn't specify a callback, the default one is registered. If the event isn't supported no warning is showed, and there's not default callback for that rmw vendor.

The approach commented here #536 (comment) will make this easier to implement IMO.

@mm318 let me know if this sounds reasonable to you.
In case that yes, I would delete the warning in rclcpp too: https://github.com/aws-ros-dev/rclcpp/blob/f6d0fc388fbf9baf3262aa158b32569228437de8/rclcpp/include/rclcpp/publisher.hpp#L100.

@mm318 The revert commit has to be singed too, to make the DCO check pass.

A warning is showed if the event type is not supported and the user explicitly required a callback.

Sorry for the mistake here. Currently we're ricing an exception in this case, and that approach is correct. Ignore this item.

Let's just remove the warning log message in the case of failing to register a default callback.

The approach commented here #536 (comment) will make this easier to implement IMO.

The structure shouldn't really change, because the logic is:

if (user provided callback) {  // the "use default callback" flag can still be true here
  register user provided callback
  if (failed to register) {
    throw exception
  }
} else if (use default callback) {  // the "user provided callback" was None
  register default callback
  if (failed to register) {
    log warning message  // will be removing this
    don't throw exception
  }
}

If we set the incompatible_qos field to a default callback beforehand (as suggested in #536 (comment)), it would be more difficult to determine if it was a user provided callback or a default callback.

The structure shouldn't really change, because the logic is:

Yes, sorry. I was convinced that the error was shown when creating the publisher.

@ivanpauno, @nuclearsandwich, and @dirk-thomas, I am not sure which packages should go through CI for this pull request, but here is the relevant gist: https://gist.githubusercontent.com/mm318/8bb18da56a1acff5cd6944f1e5ca2166/raw/c5d4d67220843da47b4591821a6fe4342baa9b00/ros2_incompatible_qos_callbacks.repos (it covers both ros2/rclcpp#1067 and #544).

Full Linux CI here:

• Linux

If the others agree, I will just run CI covering rclpy and rclcpp in the other platforms (which are the repos being modified).

@mm318 There are a few more test failures than in the nightlies https://ci.ros2.org/view/nightly/job/nightly_linux_release/1514/.

Can you take a look locally to them?
Unfortunately, the test output that CI shows in launch tests isn't really useful.

Sorry, there was something wrong with the .repos gist. Can you please re-run CI with this one: https://gist.githubusercontent.com/mm318/8bb18da56a1acff5cd6944f1e5ca2166/raw/5d2aa0956ef303b1583d39dbd0367668c8f39796/ros2_incompatible_qos_callbacks.repos? Thanks!

• Linux

• Linux

Hi @ivanpauno, I just rebased. Can you please re-run CI?

• Linux-aarch64
• macOS
• Windows

Ok, CI looks good, going finally in!

Great! Thanks!