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

Support multiple participants for cyclone #90

Closed

Conversation

BMarchi
Copy link

@BMarchi BMarchi commented Sep 24, 2019

As the title says, when a CycloneDDSCommunicator is instantiated it tries to create a topic if it doesn't exist yet. But the topic is bound to the participant, so if multiple participants are allowed, then the global topic can't be used for different participants.


This change is Reviewable

@daggarwa
Copy link

As the title says, when a CycloneDDSCommunicator is instantiated it tries to create a topic if it doesn't exist yet. But the topic is bound to the participant, so if multiple participants are allowed, then the global topic can't be used for different participants.

This change is Reviewable

@BMarchi Can you please provide some more context on this ? I am not sure if I understand the purpose of this completely and what we are trying to achieve here? Thanks

@BMarchi
Copy link
Author

BMarchi commented Oct 18, 2019

You can see in the resource manager for cyclone that it checks if we are using a single participant and returns that result, otherwise we create a participant.

dds_entity_t ResourceManager::cyclonedds_participant() const
{
std::lock_guard<std::mutex> lock(m_global_mutex);
dds_entity_t result = 0;
if (!m_ec.use_single_participant()) {
result = dds_create_participant(DDS_DOMAIN_DEFAULT, nullptr, nullptr);
} else {
if (!m_cyclonedds_participant) {
result = dds_create_participant(DDS_DOMAIN_DEFAULT, nullptr, nullptr);
}
result = m_cyclonedds_participant;
}
return result;

Now in the constructor

void register_topic()
{
if (m_topic == 0) {
m_topic = dds_create_topic(m_participant, Topic::CycloneDDSDesc(),
Topic::topic_name().c_str(), nullptr, nullptr);
if (m_topic < 0) {
throw std::runtime_error("failed to create topic");
}
}
}

we create a topic (which it's just an ID) which is bound to the participant and cyclone gives us an ID for that. Then we save it as a global topic id and reuse it if we instantiate another communicator. The thing is that if we are not using a single participant, then there's no use of having a global topic id that is bound to a specific participant for a completely different one, so we need to create another topic for the new participant.
Cyclone complained here:

https://github.com/eclipse-cyclonedds/cyclonedds/blob/8ec68e1d7d65e3b1f13d29320680fd030698d5c6/src/core/ddsc/src/dds_reader.c#L371-L376

Cyclone rmw implementation is outdated in ros2's repository and is not compiling with the latest changes. I'll see if I can backtrack and put the steps to reproduce it.
Does it make sense to you? @daggarwa

Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
@pbaughman pbaughman force-pushed the support_multiple_cyclone_participants branch from 7ec5a64 to ef878b3 Compare October 18, 2019 20:21
@daggarwa
Copy link

You can see in the resource manager for cyclone that it checks if we are using a single participant and returns that result, otherwise we create a participant.

dds_entity_t ResourceManager::cyclonedds_participant() const
{
std::lock_guard<std::mutex> lock(m_global_mutex);
dds_entity_t result = 0;
if (!m_ec.use_single_participant()) {
result = dds_create_participant(DDS_DOMAIN_DEFAULT, nullptr, nullptr);
} else {
if (!m_cyclonedds_participant) {
result = dds_create_participant(DDS_DOMAIN_DEFAULT, nullptr, nullptr);
}
result = m_cyclonedds_participant;
}
return result;

Now in the constructor

void register_topic()
{
if (m_topic == 0) {
m_topic = dds_create_topic(m_participant, Topic::CycloneDDSDesc(),
Topic::topic_name().c_str(), nullptr, nullptr);
if (m_topic < 0) {
throw std::runtime_error("failed to create topic");
}
}
}

we create a topic (which it's just an ID) which is bound to the participant and cyclone gives us an ID for that. Then we save it as a global topic id and reuse it if we instantiate another communicator. The thing is that if we are not using a single participant, then there's no use of having a global topic id that is bound to a specific participant for a completely different one, so we need to create another topic for the new participant.
Cyclone complained here:
https://github.com/eclipse-cyclonedds/cyclonedds/blob/8ec68e1d7d65e3b1f13d29320680fd030698d5c6/src/core/ddsc/src/dds_reader.c#L371-L376

Cyclone rmw implementation is outdated in ros2's repository and is not compiling with the latest changes. I'll see if I can backtrack and put the steps to reproduce it.
Does it make sense to you? @daggarwa

@BMarchi Yes it does make a lot of sense to me now. Thanks for the explanation.

@daggarwa
Copy link

Cyclone rmw implementation is outdated in ros2's repository and is not compiling with the latest changes. I'll see if I can backtrack and put the steps to reproduce it.

@BMarchi I was able to successfully compile and run changes in this PR using https://github.com/eclipse-cyclonedds/cyclonedds and ROS dashing. Can you please elaborate what do you mean by the above?

@BMarchi
Copy link
Author

BMarchi commented Oct 23, 2019

Sorry, I'm dumb. I mixed this PR with the eloquent update and I kept on my head that I had to run everything in eloquent. I'll try to post the steps to reproduce it

@BMarchi
Copy link
Author

BMarchi commented Oct 23, 2019

So, I was checking this out this afternoon and I wasn't able to reproduce it again with the latest changes. Even though I think the change is reasonable, probably cyclone is doing something under the hood that I'm not seeing, so I'm closing this PR and if something strange happens again within Cyclone, we can take a peek at this change again!

@BMarchi BMarchi closed this Oct 23, 2019
@daggarwa
Copy link

So, I was checking this out this afternoon and I wasn't able to reproduce it again with the latest changes. Even though I think the change is reasonable, probably cyclone is doing something under the hood that I'm not seeing, so I'm closing this PR and if something strange happens again within Cyclone, we can take a peek at this change again!

Okay sounds good. Thank you :)

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.

2 participants