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.

Sign up for GitHub

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

Jump to bottom

Redundant service call parameter "node_name" for system_modes services #24

Closed
chcorbato opened this issue Jul 17, 2020 · 5 comments · Fixed by #31
Closed

Redundant service call parameter "node_name" for system_modes services #24

chcorbato opened this issue Jul 17, 2020 · 5 comments · Fixed by #31
Assignees
@norro
Labels
enhancement New feature or request

Comments

@chcorbato
Copy link

Currently, system_modes/srv/GetAvailableModes, system_modes/srv/ChangeMode and system_modes/srv/GetMode require as parameter the node_name. However, the similar services from lifecycle_msgs/srv/* do not require to pass that parameter, since the service name/scope already includes it.
For example:

ros2 service call /actuation/get_available_modes system_modes/srv/GetAvailableModes "{node_name: actuation}"}"

Maybe that service parameter could be optional, also for system_modes to have a more similar interface to lifecycle?
@norro norro self-assigned this Jul 21, 2020
@norro norro added the enhancement New feature or request label Jul 21, 2020
@norro
Copy link
Collaborator

norro commented Jul 21, 2020

Thanks, @chcorbato, this is a very valuable observation. This redundancy was indeed also present in the according lifecycle_msgs/srv/ when we first started this project (before pushing it to github), but was since removed: ros2/rcl_interfaces#39 (October 2018).

So you are right, this should be removed from the system mode services as well.

norro added a commit that referenced this issue Jul 23, 2020
@norro
Removes redundant node name from mode services
fc11701 
* Node name is already encoded in the respective topic name of the service

#24

Signed-off-by: Arne Nordmann <arne.nordmann@de.bosch.com>
norro added a commit that referenced this issue Jul 23, 2020
@norro
Adapts callbacks to simplified mode services
6d105ac 
* Adapts mode callbacks for nodes and systems (get mode, change mode,
get available modes)
* part name (system or node) no longer cames from request object, but is
coded in the callback

#24

Signed-off-by: Arne Nordmann <arne.nordmann@de.bosch.com>
norro added a commit that referenced this issue Jul 23, 2020
@norro
Version bump to 0.3.0
b6d35bc 
* Version bump to 0.3.0 due to API change (mode services)
* Adapts system modes exaple documentation to new service format

#24

Signed-off-by: Arne Nordmann <arne.nordmann@de.bosch.com>
@norro norro linked a pull request Jul 23, 2020 that will close this issue
Removes redundancy from mode services #31
Merged
@norro
Copy link
Collaborator

norro commented Jul 23, 2020 •
edited
Loading

@chcorbato please have a look at PR #31

@norro norro mentioned this issue Aug 4, 2020
Merged
@norro norro closed this as completed Sep 15, 2020
@chcorbato chcorbato mentioned this issue Sep 30, 2020
Merged
@marioney marioney mentioned this issue Oct 15, 2020
Closed
@marioney
Copy link

Hi @norro

This issue is still present in the /feature/rules branch, I'm not sure if a new issue is needed, but It'll be good to apply the solution to that branch as well.

@norro
Copy link
Collaborator

norro commented Oct 15, 2020

Very good point, I will port it there as well.

@norro
Copy link
Collaborator

norro commented Oct 16, 2020

@marioney should be ported now. If it causes any issues, please don't hesitate to open further bug reports.

@norro norro mentioned this issue Oct 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@norro norro

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Removes redundancy from mode services micro-ROS/system_modes
3 participants
@norro @marioney @chcorbato