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

Add service name to client and service #56

Merged
merged 1 commit into from
Mar 9, 2016
Merged

Conversation

jacquelinekay
Copy link
Contributor

No description provided.

@jacquelinekay jacquelinekay added the in progress Actively being worked on (Kanban column) label Feb 27, 2016
@jacquelinekay jacquelinekay mentioned this pull request Feb 27, 2016
5 tasks
@tfoote
Copy link

tfoote commented Feb 27, 2016

Before we do this do we want to revisit the named resource discussion?

@jacquelinekay
Copy link
Contributor Author

Sure. Can you link to any existing issues about it? I'm not familiar with that discussion off the top of my head.

I figured this was justified because topic_name is a field for both subscription and publisher right now:
https://github.com/ros2/rmw/blob/master/rmw/include/rmw/types.h#L46-L58

thought it's not fully implemented on master yet (I have a series of PRs to finish the implementation in review right now: ros2/rcl#29)

@wjwwood
Copy link
Member

wjwwood commented Feb 27, 2016

I'm not exactly clear on what @tfoote is talking about, it could be ROS 1 Names (http://wiki.ros.org/Names) or "named" services (ones that the user gives a name for and doesn't have to hold on to the handle to keep it alive). I'd guess the former.

Either way I agree that this should be done to bring it in line with the other rmw structures. I wouldn't block it on the other design considerations (we'll have to change lots of things anyways when we decide that stuff).

@jacquelinekay
Copy link
Contributor Author

I've accepted the possibility that at least 80% of the code I'm writing will be rewritten at some point once we've made more design decisions.

@tfoote
Copy link

tfoote commented Feb 27, 2016

@wjwwood more generally I was referring to supporting fully qualified resource addressing so as to remove ambiguity between topics, services, parameters and nodes. I don't necessarily want to hold this up but thought that it might be something we should bump up in priority.

@jacquelinekay jacquelinekay added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Mar 2, 2016
@jacquelinekay jacquelinekay self-assigned this Mar 2, 2016
@jacquelinekay
Copy link
Contributor Author

even though this branch is called c_services I've decided to start an identical branch called service_name and split the connext and opensplice changes into their own branches, to make this a separate change from C services (since it's actually orthogonal). I could close this PR and reopen one for the new branch if needed.

@jacquelinekay
Copy link
Contributor Author

@jacquelinekay
Copy link
Contributor Author

a test for this functionality will come in ros2/rcl#30

@jacquelinekay
Copy link
Contributor Author

need to address warnings on Windows

@jacquelinekay
Copy link
Contributor Author

I've changed strncpy back to memcpy. This pending CI job will hopefully be clean:

http://ci.ros2.org/job/ci_windows/1113

@wjwwood
Copy link
Member

wjwwood commented Mar 9, 2016

This and related pr's lgtm, if you resolved the warnings on Windows (it's hard to tell from the CI, but I think you did).

@jacquelinekay
Copy link
Contributor Author

I remember looking at that set of warnings and thinking that they were all present on master at the time.

jacquelinekay added a commit that referenced this pull request Mar 9, 2016
Add service name to client and service
@jacquelinekay jacquelinekay merged commit 8766457 into master Mar 9, 2016
@jacquelinekay jacquelinekay deleted the c_services branch March 9, 2016 23:25
@jacquelinekay jacquelinekay removed the in review Waiting for review (Kanban column) label Mar 9, 2016
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