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

Is it possible to get the guid of the publisher from SampleInfo #179

Open
t0ny-peng opened this issue Nov 23, 2021 · 3 comments
Open

Is it possible to get the guid of the publisher from SampleInfo #179

t0ny-peng opened this issue Nov 23, 2021 · 3 comments

Comments

@t0ny-peng
Copy link
Contributor

Hi @eboasson. One quick question. I'm trying to implement a ignore_local_publications feature in our RMW. It's slightly different from your PR (#140) as we have one single DomainParticipant in one process, and by setting ignore_local_publications=true we want to disable communication in the same node, but allow communication among different nodes(in one process of course).

The way we want to go is to use dds::topic::ContentFilteredTopic. The signature of the filtering function is (bool)*(const dds::sub::SampleInfo & sample_info). To tell if a sample comes from the same node we need to check if the publisher is local.

Our old way is to maintain a list of instance_handle(a long data) locally and search the instance_handle gotten from SampleInfo to tell if it's local publisher. But as we moved to using the standard GUID, we need to get it from the SampleInfo object. Do you know if it's possible to do that? I didn't find any available functions from CycloneDDS-CXX API.

By contrast, rmw_cyclonedds_cpp uses DDS_IGNORELOCAL_PARTICIPANT(link)

Thanks!

@eboasson
Copy link
Contributor

Hi @t0ny-peng, there is dds_get_matched_publication_data in the C API which you can use to convert the publication_handle from the sample info to (among other things) a GUID. Going down that path gets into some trouble, however:

  • The C++ API implementation doesn't seem to contain a call to that function, so clearly some work is needed. This is perfectly doable, of course.
  • The implementation of dds_get_matched_publication_data is expensive. It returns more than just the GUID, that's a cost in itself, but it also goes about it in a fairly inefficient way because the underlying data structures aren't optimised for it. Doing it in the hot path of the code means you'd probably end up having to cache it locally and defeating the purpose.

We could rework things so the call gets to be less expensive (optionally adding a function to get just the GUID), but it would be interesting to see if there would be a good way to enhance "ignore_local_publications". One option could be that it gets a mode where the user_data QoS gets used as a tag and only readers/writers with the same tag get ignored. Just a random thought ...

@t0ny-peng
Copy link
Contributor Author

@eboasson Thanks for the info. After talking to Sumanth, we want to have a node-level filtering, so we cannot use the QoS(because it's DomainParticipant-level filtering). I did sone evaluation and seems that it's more efficient to maintain a local cache of instance_handle (since it's just a long type) than calling dds_get_matched_publication_data

But that being said, I completely second your idea about adding a less expensive way to get the guid from the instance handle. Could you elaborate the reason of having two unique identifiers for an entity?

@eboasson
Copy link
Contributor

The (original) DDS specification hides the global entity identifiers completely from the application, you only get an partially implementation-defined BuiltinTopicKey — curiously it is spec'd as an array of 3 elements of a non-reference types chosen by the implementation — and if you want to do anything with discovered entities you have to use the InstanceHandles instead. So it is in part a historical choice.

For most things it is fine; given that model, it makes sense to have this publication_handle in the sample info, and given that, I'm not so sure it would be a good idea to have the GUID in there as well. (Though you end up having to keep the GUID around in the reader history cache anyway ...)

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

No branches or pull requests

2 participants