Revert "Implement inconsistent topic."

Crola1702 commented 1 year ago

Reverts ros2/rmw_cyclonedds#431

As mentioned in https://github.com/ros2/rmw_cyclonedds/pull/431#issuecomment-1478510006. It seems this PR somehow made CycloneDDS fail on Rolling branch.

As CycloneDDS is at Tier 1 support level. I want to the PR until further investigation is done, and the issue is solved.

FYI: @clalancette @claraberendsen @Blast545

clalancette commented 1 year ago

So we can't actually do this revert without breaking other things (we'd also have to revert the similar PR in rclcpp, rmw_fastrtps, and rmw_connextdds). So I'm going to convert this to a draft for now and try to find out why this is failing.

eboasson commented 1 year ago

What's happening is that this PR

adds an RMW_EVENT_SUBSCRIPTION_INCOMPATIBLE_TYPE event on the subscriber
this gets mapped to DDS_INCONSISTENT_TOPIC_STATUS
which gets passed to dds_set_status_mask in gather_event_entities (in rmw_cyclonedds_cpp) because is_event_supported (also rmw_cyclonedds_cpp) says "sure"
and then dds_set_status_mask refuses the mask because this status is not supported on the reader and leaves the status mask whatever it is (everything enabled, probably)
that error is not picked up (one of those "cannot happens"), and so everything proceeds happily
and then dds_wait returns immediately because some status is set (subscription matched, or so)
and then rclcpp::wait_for_message tries to take data, finding nothing is available and returns false
resulting in a test failure in test_wait_for_message.cpp:42

https://github.com/ros2/rmw_cyclonedds/pull/436 is relevant here, in that it follows the way Cyclone reports type incompatibilities. https://github.com/eclipse-cyclonedds/cyclonedds/issues/1523#issuecomment-1359605212 also goes into some detail.

I may well have reviewed something and declared it looked good because I overlooked this detail ... If so I apologise.

clalancette commented 1 year ago

Hey @eboasson

I may well have reviewed something and declared it looked good because I overlooked this detail ... If so I apologise.

It is definitely not your fault, it's mine. I didn't test enough on Cyclone because I thought this was more-or-less a no-op. My mistake. Anyway, I definitely appreciate your analysis, and I'll see what I can do to make it happier. Thanks!

clalancette commented 1 year ago

Given that we fixed this in #444, I'm going to close this out. Feel free to reopen if you think that was in error.

ros2 / rmw_cyclonedds

Revert "Implement inconsistent topic." #442