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

Namespace harmonization to private node handles and debug display improvement #103

Open
wants to merge 10 commits into
base: indigo_dev
Choose a base branch
from

Conversation

ipa-rmb
Copy link
Contributor

@ipa-rmb ipa-rmb commented Feb 21, 2018

I did not encounter any differences in usage with my applications, please test if your applications run likewise as before.

@ipa-rmb
Copy link
Contributor Author

ipa-rmb commented Feb 21, 2018

The change with the namespaces is a proposal, by the way. I prefer it like that, but if the majority does not like it, we can stay with what we have.

Copy link
Member

@fmessmer fmessmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the duplicate /ns/node_name is quite ugly

also - again - do not provide features from indigo_dev...use a proper feature branch

std::string polygon_service_name = "/map_accessibility_analysis/map_polygon_accessibility_check";
std::string points_service_name = "/map_accessibility_analysis/map_accessibility_analysis/map_points_accessibility_check";
std::string perimeter_service_name = "/map_accessibility_analysis/map_accessibility_analysis/map_perimeter_accessibility_check";
std::string polygon_service_name = "/map_accessibility_analysis/map_accessibility_analysis/map_polygon_accessibility_check";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could simply remove the ns="map_accessibility_analysis" in the launch file to achieve backwards-compatibility wrt the resulting namespace...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was with double namespace before. But that's fine, I will change that.

Copy link
Member

@fmessmer fmessmer Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

advertising the topic under the - now private - nodehandle changed the topic/service names advertised by the server (because the nodename itself is now prepended to the topic)...see also that you had to add the extra namespace to the client...!

so for compatibility with external applications you need to keep topic/service namespace as they were before....
if you like the parameters in the private namespace you have to remove the external ns of the node...

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