Skip to content

Revert "Implement inconsistent topic."#442

Closed
Crola1702 wants to merge 1 commit intorollingfrom
revert-431-clalancette/inconsistent-topic-event
Closed

Revert "Implement inconsistent topic."#442
Crola1702 wants to merge 1 commit intorollingfrom
revert-431-clalancette/inconsistent-topic-event

Conversation

@Crola1702
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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