Update the rcljava parameter APIs #1
Conversation
This implementation was missing BoolArray, IntegerArray, DoubleArray, and StringArray. Doing this required a change to the constructor for ParameterVariant, since List is a Generic and we can't have multiple constructors with the same signature. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
It gets rid of a warning when starting the tests that looks like: log4j:WARN No appenders could be found for logger (org.ros2.rcljava.common.JNIUtils). log4j:WARN Please initialize the log4j system properly. log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more info. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
It doesn't need to change, so it can just be a class variable. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
That way we have the information available when other methods want to retrieve it. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
The parameters are stored in a HashMap of name -> parameter, but we were unnecessarily iterating over the HashMap to find things. Instead, look up the items directly in the map which should be much faster. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Whether to allow undeclared parameters is a decision that is made during Node creation. Plumb through the necessary option so that the user can choose to allow undeclared parameters when they create the node. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
This one just takes a name and sets the type to NOT_SET. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
This interface is what users will have to implement in order to have their callback called when parameters are set. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
jacobperron
left a comment
jacobperron
left a comment
There was a problem hiding this comment.
Awesome stuff! Keeping logical changes per commit made it easy to review 👍
The only commit that seems completely unrelated is 5533b40, we could probably upstream that right away (e.g. to the dashing branch).
|
|
||
| void declareParameter(ParameterVariant parameter); | ||
|
|
||
| void declareParameter(ParameterVariant parameter, rcl_interfaces.msg.ParameterDescriptor descriptor); |
There was a problem hiding this comment.
Have you considered creating a new parameters interface instead of adding these methods to the Node interface?
We can then have the Node interface extend the parameters interface, which I think is a little nicer for extensibility.
Taking it a step further, we could move the implementation to a ParametersImpl class that NodeImpl uses. I think this is also similar to what we have in rclcpp.
There was a problem hiding this comment.
I followed the example of both rclcpp and rclpy in putting these methods directly into the main Node class. I think consistency is nice to have here, as it means looking across all of the rcl* layers is easier.
That being said, I don't feel really strongly about this, so if you still think we should refactor into a ParametersImpl class, I can look into doing that.
There was a problem hiding this comment.
I think this is what rclcpp is doing (I guess not rclpy); composition of interfaces. We'd still have these methods here, but the implementation would be in another object (e.g. for re-use in a future LifecycleNode or something).
There was a problem hiding this comment.
It it seems like too much overhead we postpone a refactor until we see a direct need for it.
The parameter API was significantly updated during the Dashing release cycle. Update the API in rcljava to provide similar functionality and make the API look a lot more like rclcpp and rclpy. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
clalancette
force-pushed
the
clalancette/parameter-api-update
branch
from
508cbd7 to
b2463b4
Compare
|
All right, I've now fixed up the remaining comments, and rebased the whole thing so it is a logical set of patches. @jacobperron I don't have permissions on this repo to merge, so please go ahead when you are ready. I'll suggest a "Rebase and Merge" rather than a "Squash and Merge" to keep the logical separation between patches. |
* Implement all parameter types. This implementation was missing BoolArray, IntegerArray, DoubleArray, and StringArray. Doing this required a change to the constructor for ParameterVariant, since List is a Generic and we can't have multiple constructors with the same signature. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Add log4j initialization to all tests. It gets rid of a warning when starting the tests that looks like: log4j:WARN No appenders could be found for logger (org.ros2.rcljava.common.JNIUtils). log4j:WARN Please initialize the log4j system properly. log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more info. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Make the parameter separator a class variable. It doesn't need to change, so it can just be a class variable. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Store the parameter and the descriptor. That way we have the information available when other methods want to retrieve it. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Switch parameters to using map lookups. The parameters are stored in a HashMap of name -> parameter, but we were unnecessarily iterating over the HashMap to find things. Instead, look up the items directly in the map which should be much faster. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Plumb undeclaredParameters through the Node constructor. Whether to allow undeclared parameters is a decision that is made during Node creation. Plumb through the necessary option so that the user can choose to allow undeclared parameters when they create the node. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Add in another ParameterVariant. This one just takes a name and sets the type to NOT_SET. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Add in a ParameterCallback interface. This interface is what users will have to implement in order to have their callback called when parameters are set. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Update the Node parameter methods to Dashing/Foxy equivalents. The parameter API was significantly updated during the Dashing release cycle. Update the API in rcljava to provide similar functionality and make the API look a lot more like rclcpp and rclpy. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Add tests for Node parameters. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Add node parameters test with undeclared parameters. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Fix rebasing error Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> * configure logger in an android compatible way Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> * configure logger in an android compatible way Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>
2 participants
The main goal of this PR is to bring feature parity to the parameter APIs inside of rcljava. The parameter APIs in rclcpp and rclpy were changed fairly significantly during the Dashing release cycle, and have been furthered refined in Foxy. This PR brings rcljava up to the same level of API compliance. There are also tests to confirm the behavior.
I know this is a very large PR, and hence may be difficult to review. I have made an attempt to split the changes up into logical commits, so my suggestion is to review commit-by-commit, rather than looking at the whole thing at once.
@jacobperron FYI