osrf / capabilities

Implements the concept of capabilities as part of the robots-in-concert system.
Other
8 stars 26 forks source link

Support for system default providers #6

Closed mikeferguson closed 10 years ago

mikeferguson commented 10 years ago

One feature that would be very helpful for deploying this on robots, and making it easier on applications developers to hop from robot to robot, is the notion of a set of system/platform defaults, so that a robot manufacturer may easily set the default set of capabilities that should be preferred when not overwritten by a particular preference passed by an app.

For instance, almost every robot will implement the Navigation capability, but will do so in their own package/launch file, setting important parameters and configurations for that robot. As an application developer, your application should be able to run on any of these robots, and simply ask for "Navigation" and get the proper config for the robot.

I'm not sure how we want to implement these -- should they be parameters to the capabilities server? That way, when the main robot launch file that brings up drivers/roscore is launched, you've set the defaults.

wjwwood commented 10 years ago

Are you talking about parameters?

My thinking on this would be that any configuration of the capabilities on the robot side would be done through an application developers code which used dynamic parameters. The dynamic_reconfigure parameters have defaults, descriptions and ranges defined already, I guess I was thinking app developers would lean on this for these kinds of configuration interactions with capabilities.

mikeferguson commented 10 years ago

No, I'm talking about which capability provider is the default one for the robot, i.e. if someone installs turtlebot_navigation on my robot, I still want the provider found in my_robot_navigation to be the default one used on my_robot

wjwwood commented 10 years ago

I see. Yeah, I am still working out how complex the provider preference system will be, and then how it will work.

I'll keep this in mind.

mikeferguson commented 10 years ago

I've update the title to make it a bit clearer what I was talking about. I think this really is an important aspect if we want this to be widely adopted as an easy way for applications to be able to support many different robots (and at the same time not have to completely lock down the system such that non-default providers ever get installed).

bit-pirate commented 10 years ago

:+1: Preferences for providers should definitely be added.

I see the robot developer in charge of setting up a reasonable config. I.e. roslaunch my_robot_bringup minimal.launch should set the default/preferred navigation capability provider to my_robot_navigation. If later someone decides to install additional navigation software, it would just add another capability provider for navigation. It would also be that person's job to update the minimal configuration accordingly - either deciding that the default/preferred provider is still my_robot_navigation or the new one.

wjwwood commented 10 years ago

I am considering using rosparams for setting the default providers, something like minimal.launch setting /capability_server/<interface_name> to <default_provider_name> will indicate to the capability_server which default provider to use.

Thoughts? Is there a need for more sophisticated configuration?

bit-pirate commented 10 years ago

For configuration that should be ok. On the capability server side we probably need to introduce some logic and checks for the case, that multiple providers are available but no default chosen.

E.g. pop up a warning, which contains which provider has been chosen, or just an error and shut down the server.

wjwwood commented 10 years ago

@bit-pirate what should the behavior be?

Should the check occur at startup or at runtime of a capability?

Should the check produce a warning (choosing some arbitrary default at runtime) or an error?

How should we handle capabilities without providers available?

For capabilities with only one provider, should we:

I would lean towards the simple and explicit policy:

At startup, any capability interfaces with providers available and no default provider configuration will produce an error and exit. Capabilities with no providers will be ignored.

bit-pirate commented 10 years ago

Should the check occur at startup or at runtime of a capability?

For now we don't target runtime reconfiguration of capabilities. Hence one check at startup is enough.

At startup, any capability interfaces with providers available and no default provider configuration will produce an error and exit. Capabilities with no providers will be ignored.

I like this. We could add, that if only one provider is available, we only show a warning. It's a bit redundant to setup a default provider, if only one is available/configured.

wjwwood commented 10 years ago

Ok, I'll try to implement this today.

wjwwood commented 10 years ago

Upgraded this to a pull request. Out for review @mikeferguson @bit-pirate

mikeferguson commented 10 years ago

LGTM -- one note is: could this cause any sort of namespace collision on the param server? Since the params are directly under capability_server, is there any possibility that you have a parameter for the server that is accidently interpreted as a default provider for a (potentially nonexisting) capability?

wjwwood commented 10 years ago

@mikeferguson Good point, maybe I should namespace the parameters under /capability_server/defaults/ just to be sure. Thoughts?

mikeferguson commented 10 years ago

@wjwwood That was exactly my thought.

wjwwood commented 10 years ago

Ok, I have improved the name spacing.

wjwwood commented 10 years ago

Any complaints for me merging this?

mikeferguson commented 10 years ago

not from me

wjwwood commented 10 years ago

I have one last fixup before I merge it.

wjwwood commented 10 years ago

Ok, I put a better message for when there are no capabilities, rebased against master, and squashed the commits.

wjwwood commented 10 years ago

Travis passed, but the feedback never made it here...

https://travis-ci.org/osrf/capabilities/builds/11299614

bit-pirate commented 10 years ago

Looking good.

We could shrink

[ERROR] [WallTime: 1379029882.343308] No default provider given for capability interface 'minimal_pkg/Minimal'. 
[WARN] [WallTime: 1379029882.343547] 'minimal_pkg/Minimal' has only one provider, 'minimal_pkg/minimal', using that as the default.

To just one warning, I'd say. But that is just cosmetics.

PS: IMO you can merge things without waiting for my approval, since we are not using this code yet on the robots. I'll just complain, if I see anything strange. :-)

wjwwood commented 10 years ago

I previously had that as one line, but it was getting really long.

bit-pirate commented 10 years ago

I see. Then maybe just two warnings? The error gives the impression something is really going wrong.

wjwwood commented 10 years ago

Yeah, I can change that.

wjwwood commented 10 years ago

Might not get to it today, so:

https://github.com/osrf/capabilities/issues/16