search

eclipse-cyclonedds / cyclonedds

Eclipse Cyclone DDS project
https://projects.eclipse.org/projects/iot.cyclonedds
Other
888 stars 363 forks source link

Interpartition crosstalk when using iceoryx #1589

Open thorstenSchroeteler opened 1 year ago

thorstenSchroeteler commented 1 year ago

When sending data between two pairs of publisher / subscribers with each pair using a different partition the subscribers receive also the messages of the publishers with the wrong partition. This happens only when cyclonedds is configured to use iceoryx.

Here is a modified helloworld example including a screenshot of the usage (I have omitted the cyclonedds iceoryx configuration): cyclone_helloworld_crosstalk.zip

eboasson commented 1 year ago

Thanks for discovering that one!

I can understand how that came to be (everybody being focussed on ROS 2 when working on the Iceoryx integration, and ROS 2 not using partitions), but it is pretty bad.

The solution is fortunately quite straightforward if we're willing to limit things to a single partition name without wildcards — Iceoryx doesn't do wildcards or complex matching in topic names as far as I know (@MatthiasKillat please correct me if I'm wrong!).

If we were to check the partition QoS here and only allow a single non-wildcarded name (if there are no partitions, it is equivalent to a empty string):

https://github.com/eclipse-cyclonedds/cyclonedds/blob/f663cb17460c1202b8b069173c8fb786346623d9/src/core/ddsc/src/dds_reader.c#L475

and then include it in the topic name:

https://github.com/eclipse-cyclonedds/cyclonedds/blob/f663cb17460c1202b8b069173c8fb786346623d9/src/core/ddsc/src/dds_reader.c#L709

(and the same on the writer side) the problem should disappear.

Would you be willing to try that out? That would be very helpful to me ☺️

thorstenSchroeteler commented 1 year ago

I am willing to help, but I fear that I do not know what to do. To patch cyclonedds directly is probably beyond my expertise ;)

eboasson commented 1 year ago

I'm sure your being overly modest — but please give this a try: https://github.com/eclipse-cyclonedds/cyclonedds/pull/1591

I turned it into a draft PR because this really needs a bit of cleaning up before I feel it is of sufficient elegance to merge it, but it should do the job.

thorstenSchroeteler commented 1 year ago

I tried your iceoryx-partition-xtalk branch with the modified helloworld test and it works fine, there is no more cross-talk.