osrf / capabilities

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

Should interfaces be namespaced under their respective package? #4

Closed mikeferguson closed 10 years ago

mikeferguson commented 10 years ago

With messages, services, and actions, everything gets namespaced within the package it is defined within. For instance, trajectory_msgs/JointTrajectory is very clearly different than some my_package/JointTrajectory.

It appears right now that capabilities have no package namespace -- this means that once we define some "Navigation" capability, we can never define another -- and what happens if someone else defines "Navigation" in their own package and releases it. At least with packages, you'll quickly see a namespace collision on the buildfarm/wiki -- with the capability.yaml, you might not.

We also run the risk of really running out of interface names...

I'm imagining that this lets us create a "std_interfaces" package down the road, with the most common things like Navigation, RGBDCamera, 2DCamera....

wjwwood commented 10 years ago

You are right, the specification should probably be namespaced by the package's they are in.

@bit-pirate @stonier thoughts?

bit-pirate commented 10 years ago

I'm not opposed to this idea either. I was initially expecting the capability specifications to be a small and basic set and something people can easily agree on. Also the options of semantics and later on maybe inheritance would allow easy modification/extension.

However, I don't see the need to force people to use a small set of these interface specifications. The few capabilities, which turn out to be used frequently, can later be standardised through REPs - as Fergs noted: _stdinterfaces or _stdcapabilities.

mikeferguson commented 10 years ago

Namespacing doesn't force you to use a specific set of interfaces, however, there clearly are several that are already in REPs (for instance Navigation is defined by a REP, 3d RGBD cameras are also). The big advantage of using standard interfaces would of course be the large number of applications that already interface to them at some point.

bit-pirate commented 10 years ago

Sorry, my wording was confusing. I meant that my initial idea of using no namespaces would force people to use only a small set of available specifications and for that I don't see a need. Hence, I prefer your suggestion of using namespaces and later on use REPs to standardise the popular interfaces/capabilities.

mikeferguson commented 10 years ago

@bit-pirate Ah, ok, yes, I misread how you meant that.

wjwwood commented 10 years ago

I am working on a pull request to implement this.

wjwwood commented 10 years ago

I am still working on the refactoring of the code, but what I have finished is in this branch:

https://github.com/osrf/capabilities/tree/package_namespace

I will upgrade this to a pull request when I finish the changes.

wjwwood commented 10 years ago

So this is taking longer than expected, I wanted to make the API flexible so you could still use the non-fully qualified name when referring to specs, but I think that is making things too complicated. I am going to spin a new branch which requires you to always specify the fully qualified name (package/spec_name).

wjwwood commented 10 years ago

This would basically mean that you always have to use this form when referring to specs, which is a lot more verbose, but more explicit. Thoughts?

wjwwood commented 10 years ago

I'm doing the second attempt here:

https://github.com/osrf/capabilities/tree/package_namespace_simple

wjwwood commented 10 years ago

I upgraded this to a pull request. Could either of you review this? Specifically, look at the changes to the test data, which consist mostly of example packages and spec files, so that you can get a sense for how this changes things.

wjwwood commented 10 years ago

Since I don't have continuous integration running yet, here are the tests (without ROS, with coverage):

% nosetests --with-coverage --cover-package capabilities
Skipping test_server.py because ROS depenencies not imported: No module named srv
Skipping test_discovery.py because ROS depenencies not imported: No module named srv
....................
Name                                    Stmts   Miss  Cover   Missing
---------------------------------------------------------------------
capabilities                                0      0   100%
capabilities.discovery                    136      0   100%
capabilities.specs                          0      0   100%
capabilities.specs.common                  10      0   100%
capabilities.specs.interface              161      0   100%
capabilities.specs.provider                94      0   100%
capabilities.specs.remappings              25      0   100%
capabilities.specs.semantic_interface      60      0   100%
---------------------------------------------------------------------
TOTAL                                     486      0   100%
----------------------------------------------------------------------
Ran 20 tests in 0.864s

OK

And more (with ROS, without coverage):

% make run_tests                                                                                                                                                               ±
Built target clean_test_results
Built target tests
-- run_tests.py: execute commands
  /usr/local/Cellar/cmake/2.8.11.2/bin/cmake -E make_directory /Users/william/devel/capabilities/build/test_results/capabilities
  /usr/local/bin/nosetests -P --process-timeout=60 --where=/Users/william/devel/capabilities/test --with-xunit --xunit-file=/Users/william/devel/capabilities/build/test_results/capabilities/nosetests-test.xml
.......................
----------------------------------------------------------------------
Ran 23 tests in 0.398s

OK
-- run_tests.py: verify result "/Users/william/devel/capabilities/build/test_results/capabilities/nosetests-test.xml"
Built target _run_tests_capabilities_nosetests_test
Built target _run_tests_capabilities_nosetests
Built target _run_tests_capabilities
Built target run_tests
mikeferguson commented 10 years ago

@wjwwood RE: needing package names: I actually think we should require the package_name, it just follows typical ROS conventions.

mikeferguson commented 10 years ago

@wjwwood I meant to take a look over this over the weekend but didn't get time -- I'll take a look at it tomorrow morning/early afternoon.

wjwwood commented 10 years ago

I am going to merge this, so I can work on the documentation some. If you have feedback in the future we can deal with it here or in a new issue.

bit-pirate commented 10 years ago

Sorry for the late reply. Regarding the naming issue, I am OK with both. Avoiding verbosity is nice, but adding the package name should be fine as it follows the ROS convention as Fergs already mentioned.

stonier commented 10 years ago

Totally missed this one. Sorry.

Package name/spec is a good convention. Ive been following ken conleys lead on doing that (he calls them resources) for almost anything materialistic. The ros package name tends to help with ensuring it is unique enough without resorting to wierd naming tricks.