Skip to content

Conversation

@jacobperron
Copy link
Contributor

Contains significant changes/additions related to Actions, QoS, ROS time, and graph APIs. This brings in all changes we've developed on our fork that are compatible with Galactic: https://github.com/osrf/ros2_java/tree/galactic-devel

For posterity, each commit references the pull request to the OSRF fork where review and any discussion occurred.

ivanpauno and others added 30 commits May 17, 2022 15:16
Add macros to handle java exceptions
Add macros to throw a java exception from an rcl error
Always use `base_message` instead of `message` in docblocks

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
* Improve QoSProfile constructors
* Add documentation to QoSProfile class
* Add fromRCL method to QoSProfile
* Use fromRCL to create the built-in qos profiles.

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
* Add NameAndTypes class
* Add getTopicNamesAndTypes method to Node

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
** Add EndpointInfo class.
* Add getPublishersInfo method to Node.

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
* Add NodeNameInfo class
* Implement getNodeNames

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
In these cases, we are likely leaking memory.

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
* Start revamping Time and Clock classes.

Signed-off-by: Chris Lalancette <[email protected]>

* Fixes from review.

Signed-off-by: Chris Lalancette <[email protected]>

* Add in logging to the TimeSource class.

Signed-off-by: Chris Lalancette <[email protected]>
* Document TimeSource class

Signed-off-by: Jacob Perron <[email protected]>

* Address feedback

* Empty -> Default

* Add more detail to class docblock

Signed-off-by: Jacob Perron <[email protected]>
* Notify clocks when use_sim_time parameter changes

Signed-off-by: Jacob Perron <[email protected]>

* Add unit tests for TimeSource class

Introduce Mockito dependency to help with creating mock objects.

Signed-off-by: Jacob Perron <[email protected]>

* Add test dependency on libmockito-java

Depends on ros/rosdistro#26308

Signed-off-by: Jacob Perron <[email protected]>

* Minor refactor

- Switch to using @mock annotation.
- Cleanup

Signed-off-by: Jacob Perron <[email protected]>

* Depend on mockito_vendor package

Signed-off-by: Jacob Perron <[email protected]>

* Add mockito_vendor to repos file

This should make CI green.

Signed-off-by: Jacob Perron <[email protected]>

* Update branch for mockito_vendor

Signed-off-by: Jacob Perron <[email protected]>
* Support wall-time and ROS-time timers

Add a new method to Node for creating ROS timers.

Refactor timer implementation to account for the two different types of timers.
Deprecate WallTimer interface and WallTimerImpl and replace with unified Timer interface and TimerImpl.

Signed-off-by: Jacob Perron <[email protected]>

* Add test

Signed-off-by: Jacob Perron <[email protected]>

* Log error if failed to dispose timer

Signed-off-by: Jacob Perron <[email protected]>
Useful for getting the current time from the Clock instance.

Signed-off-by: Jacob Perron <[email protected]>
This fixes two issues:

1. The default constructor for ParameterAndDescriptor initializes it's members to null.
  This can cause a segfault when trying to access a parameters descriptor if undeclared
  parameters are allowed and it has been set without a descriptor
  (e.g. using setParameters()).

  This change defines the constructor for ParameterAndDescriptor so that we don't hit
  any null values.

2. Parameters that are not declared and set with setParameters() do not have their
  descriptors set to the correct value.
  This change makes sure that the descriptor is initialized with the values of the
   parameter being set.

Signed-off-by: Jacob Perron <[email protected]>
The workAvailable flag was never being set to true.
If I understand the original intention, I think that the flag should be set as long as there are things being executed.

Signed-off-by: Jacob Perron <[email protected]>
Now we're actually running the tests.

This also fixes a bug where we were comparing the wrong objects in one of the tests.

Signed-off-by: Jacob Perron <[email protected]>
* change logic(list -> array) in msg.java.em

* change logic(list -> array) in msg.cpp.em

* Support getting and setting as Lists in addition to arrays

Signed-off-by: Jacob Perron <[email protected]>

* Update rcljava to work with new methods for list types

Signed-off-by: Jacob Perron <[email protected]>

* Update tests to work with new methods for list types

Signed-off-by: Jacob Perron <[email protected]>

* Fixes to template files

* Avoid nullptr access when converting from Java arrays to C arrays
* Make sure Java arrays are initialized

Signed-off-by: Jacob Perron <[email protected]>

* Minor refactor

Signed-off-by: Jacob Perron <[email protected]>

* A note about performance to *AsList docblocks

Signed-off-by: Jacob Perron <[email protected]>

Co-authored-by: pluris <[email protected]>
* Add interfaces for action goal, result, and feedback

Implementing these interfaces in the code generation template makes it easier to pass around these types in a generic way.
Note, the 'final' modifier had to be removed from generated message types in order to extend goal, result, and feedback types in action definitions.

Signed-off-by: Jacob Perron <[email protected]>

* Add new definitions for action goal response and request

Signed-off-by: Jacob Perron <[email protected]>

* Add getter for UUID to SendGoalRequest

Also make inner classes static.

Signed-off-by: Jacob Perron <[email protected]>

* Add getStamp method to GoalResponseDefinition

Signed-off-by: Jacob Perron <[email protected]>

* Partially revert "Add interfaces for action goal, result, and feedback"

Partially revert commit dd04614.

I don't think we need to aliases for the message types, but I'll add them back if they turn out to be useful.

* Parameterize goal request and response interfaces on action type

Signed-off-by: Jacob Perron <[email protected]>

* Fix getGoalUuid implementation

It should return a List, since it is hashable.

Signed-off-by: Jacob Perron <[email protected]>
* Add interfaces for action goal, result, and feedback

Implementing these interfaces in the code generation template makes it easier to pass around these types in a generic way.
Note, the 'final' modifier had to be removed from generated message types in order to extend goal, result, and feedback types in action definitions.

Signed-off-by: Jacob Perron <[email protected]>

* Add new definitions for action goal response and request

Signed-off-by: Jacob Perron <[email protected]>

* Add getter for UUID to SendGoalRequest

Also make inner classes static.

Signed-off-by: Jacob Perron <[email protected]>

* Add getStamp method to GoalResponseDefinition

Signed-off-by: Jacob Perron <[email protected]>

* Partially revert "Add interfaces for action goal, result, and feedback"

Partially revert commit dd04614.

I don't think we need to aliases for the message types, but I'll add them back if they turn out to be useful.

* Parameterize goal request and response interfaces on action type

Signed-off-by: Jacob Perron <[email protected]>

* Fix getGoalUuid implementation

It should return a List, since it is hashable.

Signed-off-by: Jacob Perron <[email protected]>

* List<Byte> -> byte[]

Signed-off-by: Jacob Perron <[email protected]>

* Add ActionServer skeleton

* Add new action module with classes/interfaces related to the ActionServer
* Add methods for creating and removing ActionServers from a Node
* Implement dispose method for ActionServer.

Signed-off-by: Jacob Perron <[email protected]>

* Add ActionServer creation logic and unit tests

Signed-off-by: Jacob Perron <[email protected]>

* Add action server logic for accepting and canceling goals

This changeset includes the following:
  * an implementation of the goal handle for action servers
  * action server integration with the base executor
  * action server integration with ROS nodes
  * unit tests for the action server, receiving goal and cancel requests

Signed-off-by: Jacob Perron <[email protected]>

* cleanup

Signed-off-by: Jacob Perron <[email protected]>

* more cleanup

Signed-off-by: Jacob Perron <[email protected]>

* Minor refactor: move response enums into callback interfaces

Signed-off-by: Jacob Perron <[email protected]>

* Remove TODO

The goal request already contains a getter for the goal ID.

Signed-off-by: Jacob Perron <[email protected]>

* Fix accident

Signed-off-by: Jacob Perron <[email protected]>

* Alphabetize

Signed-off-by: Jacob Perron <[email protected]>

* Refactor: replace 'getNumberOf*()' JNI functions with single 'getNumberOfEntites()' function

Signed-off-by: Jacob Perron <[email protected]>

* Avoid string copy

Signed-off-by: Jacob Perron <[email protected]>

* Remove extern C

Replace with 'namespace rcljava'

Signed-off-by: Jacob Perron <[email protected]>

* Remove unnecessary synchronization

Signed-off-by: Jacob Perron <[email protected]>

* Refactor access to goalCallback

Signed-off-by: Jacob Perron <[email protected]>

* typesafe request and response definitions

Signed-off-by: Jacob Perron <[email protected]>

* Be more specific about template type for GoalCallback

Signed-off-by: Jacob Perron <[email protected]>

* Minor refactor

Signed-off-by: Jacob Perron <[email protected]>

* Make GoalHandleImpl inner class instead of static

Signed-off-by: Jacob Perron <[email protected]>

* Remove synchronized from getHandle

Signed-off-by: Jacob Perron <[email protected]>

* Add TODO for Waitable interface

Signed-off-by: Jacob Perron <[email protected]>

* Fix style

Signed-off-by: Jacob Perron <[email protected]>

* Minor refactor

Signed-off-by: Jacob Perron <[email protected]>

* Add docs for createActionServer

Signed-off-by: Jacob Perron <[email protected]>

* Switch from List to array

Signed-off-by: Jacob Perron <[email protected]>
* Add cmake function for bundling source files into a jar
* Create and install source jar for rcljava

Signed-off-by: Gonzalo de Pedro <[email protected]>

Co-authored-by: Jacob Perron <[email protected]>
ivanpauno and others added 20 commits May 17, 2022 15:36
* Add ACCEPT_AND_EXECUTE/ACCEPT_AND_DEFER to the result of an action goal callback.

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Automatically transition from accepted to executing if ACCEPT_AND_EXECUTE was returned.

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Expose an "execute()" method in goal callback.

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
… to the feedback topic (osrf#60)

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
* GoalHandle transitions should be able to set a result.
* Refactor GoalStatus enumeration.
* Add interface for ResultRequestDefinition, ResultResponseDefinition, FeedbackDefinition, GoalDefinition, ResultDefinition.
* Modify rcljava rosidl generator to implement those interfaces.
* Handle result requests.
* Handle goal handle terminal transitions.
* Cleanup goal handle status transition bindings.
* Take advantage of nested class.
* Publish status after terminal state transition.
* Handle expire goals.
* Add support for publishing feedback.

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Co-authored-by: Jacob Perron <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
* Cleanup service/client creation code

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Add methods to create a parameter client in Node

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Add overloads for spinOnce methods to include a timeout argument and call this when spinning in the future.

This fixes a bug where the future.get(...) call blocks forever, e.g. when waiting on a service response and the service becomes unavailable.

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@jacobperron jacobperron requested review from esteve and ivanpauno May 17, 2022 23:05
@jacobperron
Copy link
Contributor Author

There is an unexpected test failure on linux CI:

 5: 1) testAdd(org.ros2.rcljava.client.ClientTest)
  5: java.lang.AssertionError: expected:<1> but was:<7>
  5: 	at org.junit.Assert.fail(Assert.java:88)
  5: 	at org.junit.Assert.failNotEquals(Assert.java:834)
  5: 	at org.junit.Assert.assertEquals(Assert.java:645)
  5: 	at org.junit.Assert.assertEquals(Assert.java:631)
  5: 	at org.ros2.rcljava.client.ClientTest.testAdd(ClientTest.java:131)
  5: 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
  5: 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
  5: 	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
  5: 	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
  5: 	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
  5: 	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
  5: 	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
  5: 	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
  5: 	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
  5: 	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
  5: 	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
  5: 	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
  5: 	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
  5: 	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
  5: 	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
  5: 	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
  5: 	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
  5: 	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
  5: 	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
  5: 	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
  5: 	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
  5: 	at org.junit.runners.Suite.runChild(Suite.java:128)
  5: 	at org.junit.runners.Suite.runChild(Suite.java:27)
  5: 	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
  5: 	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
  5: 	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
  5: 	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
  5: 	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
  5: 	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
  5: 	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
  5: 	at org.junit.runner.JUnitCore.run(JUnitCore.java:115)
  5: 	at org.junit.runner.JUnitCore.runMain(JUnitCore.java:77)
  5: 	at org.junit.runner.JUnitCore.main(JUnitCore.java:36)

There are also, not-so-unexpected, build issues on android (since we're not running CI on our fork). I'll resolve these before merging any commits contained in this PR.

@esteve
Copy link
Member

esteve commented May 18, 2022

@jacobperron first of all, thanks for the PR. But in all honesty, reviewing a 8K+ line diff is a lot of work, is there any chance this could be broken into separate features? Given that you and @ivanpauno both have write access to this repo, could you in the future do the work directly here? We did a sync months ago, and I thought that from then on, all development would be done upstream.

@jacobperron
Copy link
Contributor Author

Technically, either Ivan or I have already reviewed the changes in this PR (though I understand if we want to break this into separate smaller PRs). We've done previous PRs (#199 and #206) to start porting changes over, but there are a number of outstanding changes to upstream. I collected all remaining changes from our fork that we tested with Galactic and put them in this PR. I am hoping after this, there will be one more PR that adds changes for Humble/Rolling and then we can archive our fork.

@jacobperron
Copy link
Contributor Author

Thanks for opening up smaller PRs, @ivanpauno. I'll close this one in favor of those.

@jacobperron jacobperron closed this Aug 9, 2022
@ivanpauno ivanpauno deleted the jacob/osrf_ports_2 branch August 10, 2022 15:51
@bluecamel
Copy link

I don't see that there was a separate PR to port the action server from the osrf fork. Is that still a WIP?

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.

8 participants