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

Revert "Implement inconsistent topic." #442

Closed

Conversation

Crola1702
Copy link

Reverts #431

As mentioned in #431 (comment). 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
Copy link
Contributor

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.

@clalancette clalancette marked this pull request as draft March 21, 2023 20:12
@eboasson
Copy link
Collaborator

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

#436 is relevant here, in that it follows the way Cyclone reports type incompatibilities. eclipse-cyclonedds/cyclonedds#1523 (comment) 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
Copy link
Contributor

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
Copy link
Contributor

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.

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.

3 participants