Skip to content

Internal parameters api#19

Closed
esteve wants to merge 13 commits intomasterfrom
internal-parameters-api
Closed

Internal parameters api#19
esteve wants to merge 13 commits intomasterfrom
internal-parameters-api

Conversation

@esteve
Copy link
Copy Markdown
Member

@esteve esteve commented Apr 8, 2015

This PR adds the internal infrastructure for the parameters API.

Connects ros2/ros2#11

Connects ros2/ros2#28

@dirk-thomas @tfoote @wjwwood

@esteve esteve added the in progress Actively being worked on (Kanban column) label Apr 8, 2015
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Despite C++11 supporting unions with members with non-trivial constructors (e.g. std::string), I couldn't get the union play nice with std::map because of std::pair. Any feedback will be appreciated.

@dirk-thomas
Copy link
Copy Markdown
Member

Please make sure that the new code passes the unit tests, e.g. the code style checked by uncrustify.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This method could be const. Same for the get_params.

@dirk-thomas
Copy link
Copy Markdown
Member

The PR adds the API as well as the storage of the parameters to the node class directly. In the future a node will also have a service / topic interface to interact with them . Let me describe a few examples why I think the implementation should be separated from the node instance:

  • A node might want to use the parameter interface to store internal data (since it provides nice variant types) and wants to do that beside the "normal" parameters. Hence the node does not want to expose these internal parameters via the public service / topic interface. But without being implemented in a separate class the logic of parameters is not reusable.
  • Also the aspect of parameter storage and accessor functions should be separated from the service / topic interface. A node might want to expose a set of parameters via two different service / topic interfaces: e.g. via the services set_param and set_advanced_param. Again with an implementation in the node class a custom node does not have that freedom.
  • Writing a unit test for a "ParameterClass" is way cleaner and self contained then testing parameters inside of a node.

@esteve
Copy link
Copy Markdown
Member Author

esteve commented Apr 8, 2015

@dirk-thomas this PR does not implement the API that's exposed to developers, this just implements the internal storage for a node. This PR adds the internal structures and logic to support the public API.

@esteve esteve force-pushed the internal-parameters-api branch from 7387479 to 97dd346 Compare April 24, 2015 20:40
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