Skip to content

Conversation

@jacobperron
Copy link
Contributor

@esteve If this looks good to you, I can fast-forward merge dashing into master from the command line so that the head of each branch is the same.

I'll follow-up with any necessary updates to CI and documentation, as well as PRs for compatibility with Eloquent.

@esteve esteve self-requested a review June 30, 2020 21:35
README.md Outdated
mkdir -p ${ROS2_ANDROID_WORKSPACE}/src
cd ${ROS2_ANDROID_WORKSPACE}
wget https://raw.githubusercontent.com/esteve/ros2_java/master/ros2_java_android.repos
wget https://raw.githubusercontent.com/ros2-java/ros2_java/dashing/ros2_java_android.repos
Copy link
Member

Choose a reason for hiding this comment

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

I know this was not introduced in this branch, but given that it touches it, could you replace the wget call with curl to make it consistent with https://github.com/ros2-java/ros2_java/pull/117/files#diff-04c6e90faac2675aa89e2176d2eec7d8R102 ? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been meaning to update the README anyways. #119 updates the README for dashing. I'll add it to this fast-forward to master after it is merged.

type: git
url: https://github.com/ros2/ament_cmake_ros.git
version: 0.5.0
version: dashing
Copy link
Member

Choose a reason for hiding this comment

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

This could be master now that we intend to merge dashing into it.

Copy link
Contributor Author

@jacobperron jacobperron Jul 1, 2020

Choose a reason for hiding this comment

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

That's correct. Though, I'm planning to make this change after the fast-forward. This way the commits on dashing and master will be even up to the point where we start making changes for Eloquent and beyond.

logger.info("Using RMW implementation: {}", RCLJava.getRMWIdentifier());
initialized = true;
}
public static synchronized void rclJavaInit() {
Copy link
Member

Choose a reason for hiding this comment

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

Locking at the method level would only prevent multiple calls to rclJavaInit, but globalExecutor needs to be protected as well, hence the use of synchronized at the class level and not at the method level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is true. My understanding is that synchronizing the static method acquires the same class-level lock as the one used in getGlobalExecutor. I.e. I believe the following two are equivalent:

public static synchronized void rclJavaInit() {
  ...
}

and

public static void rclJavaInit() {
  synchronized (RCLJava.class)  {
    ...
  }
}

Reference

@jacobperron jacobperron changed the base branch from master to main May 17, 2021 18:43
@jacobperron jacobperron changed the title Fast-forward merge dashing to master Fast-forward merge dashing to main May 17, 2021
@jacobperron
Copy link
Contributor Author

jacobperron commented May 17, 2021

I've rebased this PR and targeted main.
I've also updated the CI configuration to try and make it green (05c98dd).

@ivanpauno Let me know if this batch of changes will make porting things from our fork easier and we can merge it first.

@jacobperron
Copy link
Contributor Author

@esteve it would be great if you could take another look at the changes proposed in this PR. It will effectively make main compatible with Dashing. Then we can add updates for other distros from there.

@ivanpauno
Copy link
Contributor

@jacobperron is this a fast-forward from https://github.com/osrf/ros2_java/commits/dashing?

I see some changes in https://github.com/ros2_java/ros2_java/commits/dashing that aren't here.
I also see that there are conflicts between osrf:dashing and osrf:galactic_devel, I will check what happened there.


More info

$ git merge-base osrf/dashing origin/dashing

cf2ef07

$ git rev-list cf2ef0707f7420f9b83e6c053775c9d52752a9e2^..osrf/dashing

cf2ef07

# i.e. ros2_java:dashing has all commits that osrf:dashing has

$ git rev-list cf2ef0707f7420f9b83e6c053775c9d52752a9e2^..origin/dashing

d4163f8
4681ca3
a9244b8
e5dcec2
0a3df77
c3a2d7a
0e8e8cc
efe2df5
66d622c
0b48353
720cbed
7fb3dfe
95fefa6
cf2ef07

# i.e. There are 13 commits in ros2_java:dashing not in osrf:dashing


There are also 8 commits on osrf:dashing that aren't in osrf:galactic_devel, and there were some conflicts when rebasing 😕.

jacobperron and others added 15 commits May 17, 2021 13:36
* Update rosidl_generator_java for new IDL pipeline

Supporting ROS Dashing or later.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Fix name JNI name mangling

Accidentally broken during update.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Remove java compile flags

This remove some compile warnings.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Refactor to support services

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Wide string support

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Avoid duplicate includes

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Use test_interface_files

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Support for actions

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Add suffix for long literals

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Handle unsigned literals

Though Java supports unsigned values, it doesn't support unsigned literals (ie. literals that are larger than the max signed value).
As a workaround, we can convert the literal to it's negative equivalent.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Fix escape string function

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Use cast to workaround integer literals

Otherwise the compiler complains about potential loss of data.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Minor refactor

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Remove TODO

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
* Update API for getting rcl error string

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Add implementation for ROS Context

A context represents an init/shutdown cycle and is used in the creation of top level entities
like nodes and guard conditions.

For convenience, a default context is created when rcljava is initialized.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Add implementation for Clock

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Update for Dashing support

* Update wait set API calls
* Update entity creation API calls
* Use Context objects to check 'ok()' status
* Add Clock and Context members to NodeImpl
* Fix static member reference: 'this.defaultContext' -> 'RCLJava.defaultContext'

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Avoid hiding errors when cleaning up init options

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Disable tests

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Fix typos in JNI library files

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Fix native node method signature

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Populate missing QoS settings with defaults

Otherwise we run into a runtime error about Fast-RTPS not supporting liveliness.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Fix issues with Clock class

* Load with JNIUtils
* Rename native create method for consistency
* Fix bug in native implementation

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Enable linter tests

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Fix lint errors

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Enable RCLJava test and fix bugs

* Use Context.ok() and deprecate RCLJava.isInitialized().
* Move implementation loading to static initialization code.
  Otherwise, calls to getDefaultContext() fail if called before rclJavaInit().
  It wasn't clear to me why the implementation should be loaded in a separate function call.
  We can probably refactor the code to avoid the error if we want to move the loading back
  into rclJavaInit().
* Refactor test into one init/shutdown test. Previously, not calling RCLJava.shutdown()
  was leaving a context around between tests.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Fix NodeTest

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Enable most tests

Tests that involve services are broken due to issues related to interface generation.
There's a separate PR for a fix: #76.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
* Update rosidl_generator_java tests

Now using messages from test_interface_files.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Remove old interface files

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
The package does not exist.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
This fixes some fatal runtime errors during testing.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
* Fix service and action interface generation

Before, we were not generating code for the messages and services that make up service and action
interfaces.

Due to issues with duplicate definitions caused by instantiating the msg.cpp.em template
multiple times, I've opted to generate separate files for each service, action, and the interfaces
that they are made of. This is similar to what we are doing with the generated Java code.

I've added a test confirming that generated service code can be used. Adding a test for actions is
difficult at the moment due to a circular dependency with action_msgs.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Add missing header include

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Rename top level generated cpp file

This avoids name clashing with other generated files. Similar to what we do with generated Java files.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Fix JNI name mangling function so it works for service and action subtypes

For example, 'example_interfaces/srv/AddTwoInts_Request' should be mangled to 'example_1interfaces_srv_AddTwoInts_1Request'.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Remove vestigal references to jni_package_name

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Add comment about action and service headers

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Simplify include logic

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
* Fix service and action interface generation

Before, we were not generating code for the messages and services that make up service and action
interfaces.

Due to issues with duplicate definitions caused by instantiating the msg.cpp.em template
multiple times, I've opted to generate separate files for each service, action, and the interfaces
that they are made of. This is similar to what we are doing with the generated Java code.

I've added a test confirming that generated service code can be used. Adding a test for actions is
difficult at the moment due to a circular dependency with action_msgs.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Add missing header include

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Rename top level generated cpp file

This avoids name clashing with other generated files. Similar to what we do with generated Java files.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Fix JNI name mangling function so it works for service and action subtypes

For example, 'example_interfaces/srv/AddTwoInts_Request' should be mangled to 'example_1interfaces_srv_AddTwoInts_1Request'.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Remove vestigal references to jni_package_name

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Add comment about action and service headers

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Simplify include logic

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Rename cpp headers to have .hpp suffix

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Update files to reflect new header suffix

This resolves a cppcheck linter error complaining about "is invalid C code".
Changing the suffix causes cppcheck to treat the files as C++ code.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Co-authored-by: Esteve Fernandez <esteve@apache.org>
We don't need to build everything from source, only some of the ROS interface packages.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
* Add dashing workflow

Using the ros-tooling custom GitHub actions

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Update repos file URL

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
* Add example_interfaces to desktop repos file

This package is required by rcljava_examples.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Append .git to repository URLs for consistency

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
* Fix README with correct branch in the instructions.

* Update README instructions repository links.

Changed the repository links from a private github account to ROS2 Organisation repositories links.

Co-authored-by: Niels Tiben <nielstiben@outlook.com>
…85)

* Re-enable tests related to services

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Add Client methods for checking and waiting for service availability

These methods are very useful for allowing a client to wait for a service to be available before making requests.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Refactor ClientTest to avoid repeatedly sending requests

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
* Android part pul command use the wrong reops

Signed-off-by: Jay Hou <hjjehovah@gmail.com>
nielstiben and others added 16 commits May 17, 2021 13:36
Generate action code for:

* `<goal_name>_SendGoal_Request`
* `<goal_name>_SendGoal_Response`
* `<goal_name>_GetResult_Request`
* `<goal_name>_GetResult_Response`
This fixes CI while we wait for an upstream issue to be resolved.
See ament/ament_lint#252

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
* Add methods to remove entities from Node

When an entity is disposed, make sure to also remove it from the Node.
This resolves an issue where invalid entities may be used by other classes or users.
For example, a disposed Subscription being accessed by the executor.

Fixes #105

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Add tests for disposing publishers, services, and clients

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Rename test

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
* clear node/context handle on RclJava.cleanup()

* clear publishers, subscriptions, clients, services and timers on Node.dispose()
* Remove redundant code generation

The request and response messages are already generated as part of the
srv template.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Strip action service suffixes from C include prefix

The generated C headers for actions are included in a single header named after the action.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Generate Java code for SendGoal and GetResult service definitions

Though not strictly necessary, it is nice to have definitions for these action-specific services for the purpose of writing unit tests.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Compile generated service definitions for actions

Previously, though we were generated service definitions for actions we were not compiling them.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
* Use curl for consistency

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Update build badges

Currently, we only have CI for Dashing on Linux.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Update URLs

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Minor formatting changes

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Update build instructions for desktop

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Use different header markdown syntax

For more granularity in header levels.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Fix whitespace formatting

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Formatting

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Add ref to ticket for ROS time

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* 'merge install' for Windows

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Fix typo

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Update Android instructions with CI (#125)

* Update Android instructions

This is a work in progress, still troubleshooting build issues.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Update Android repos file

Now the repos file contains all packages in the upstream dashing repos file, without the Qt packages:

https://github.com/ros2/ros2/blob/dashing/ros2.repos
Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Added job for Android

* Only build the rcljava subset of dependencies

* Fix URL

* Replace ros-tooling scripts with plain commands

* Fix typo

* Install vcstool for vcs

* Install Android NDK

* Add ros2_java workspace to CMake find root path

* Disable building tests

* Do not build osrf_testing_tools_cpp as it uses execinfo.h, which is not available on Android

* Workaround for Android

* Reenable building tests

* Skip cyclonedds for now

* Install lark parser

* Install python3-dev

* Pass PYTHON_LIBRARY and PYTHON_INCLUDE_DIR to the CMake Python module

* Skip rcl_logging_log4cxx

* Use rmw fork for Android. Skip CycloneDDS packages

* Revert to 1.6

* Configure log4j dynamically to avoid errors when building for Android

* Configure log4j dynamically to avoid errors when building for Android

* Switch to jacob/instructions temporarily

* Fix compilation error

* Trim repos file for Android

* Readd googletest repo

* Fix missing import

* Build entire workspace

* Only build up to the Android examples

* Install colcon extensions for Gradle

* Revert changes for desktop

* Move ros2_android repo to ros2-java org

* Install Gradle

* Build rcljava_examples

* Install Gradle

* Do not build rcljava_examples temporarily

* Update README.md

* Do not use fork

* Use repos files from GitHub workspace

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Add checkout action for cloning the branch to test

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Use version of ros2_java in the branch being tested

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Switch back to dashing branch

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Co-authored-by: Esteve Fernandez <esteve@apache.org>

Co-authored-by: Esteve Fernandez <esteve@apache.org>
* Bump version of ros-tooling/setup-ros to 0.1.0

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Bump version of ros-tooling/action-ros-ci to 0.1.0

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
* Remove test dependency on rosidl_typesupport_connext_c

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Remove Connext packages from Android repos file

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
This also works around the problem of assigning to an 'incompatible type', but looks much cleaner.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Contributor Author

I see some changes in https://github.com/ros2_java/ros2_java/commits/dashing that aren't here.

Oops, you're right. I forgot to include changes since this PR was opened. I've reapplied changes from dashing.

@ivanpauno
Copy link
Contributor

Oops, you're right. I forgot to include changes since this PR was opened. I've reapplied changes from dashing.

I have checked locally, I can rebase osrf:galactic_devel on ros2_java:dashing more or less easily https://github.com/osrf/ros2_java/tree/ivanpauno/galactic-devel-rebase-origin-dashing.
So I think this fast-forward is a good idea 👍.


This isn't actually a fast-forward though, and rebasing again on this branch adds more conflicts.
Can we just make the dashing branch the exact new main?

There isn't anything interesting in main that isn't in dashing really:

$ git log $(git merge-base origin/main origin/dashing)^..origin/main
commit b4e64f71538a057b3a1dbb49315aabeeedab3c88 (origin/master, origin/main, origin/HEAD)
Author: Esteve Fernandez <esteve@apache.org>
Date:   Fri Dec 18 07:45:04 2020 +0100

    Pin version of rcl_interfaces

commit f861280c504d311991c4152635778edea711592f
Author: Esteve Fernandez <esteve@apache.org>
Date:   Fri Dec 18 07:44:25 2020 +0100

    Pin version of rcl_interfaces

commit 62918477f4fb8810d4df0ac9c9e67b885b635d01 (osrf/master)
Author: Martin Huber <mhubii@users.noreply.github.com>
Date:   Fri Jan 17 14:46:25 2020 +0100

    Update README.md (#72)

The changes on the readme were already done in the dashing branch and the pinning is irrelevant.

@jacobperron
Copy link
Contributor Author

Good point. I'm fine just making this the new main, pending approval.

@ivanpauno
Copy link
Contributor

Good point. I'm fine just making this the new main, pending approval.

To be more clear, the new main would be the dashing branch and not this branch (the diff between the two branches is empty, it only makes the process of cherry-picking changes from osrf:ros2_java a bit easier to me.)

@jacobperron
Copy link
Contributor Author

jacobperron commented May 17, 2021

To be more clear, the new main would be the dashing branch and not this branch

Right, that's what I meant 👍

@jacobperron
Copy link
Contributor Author

Alright, with all the comments addressed, I've created a new branch main based on the dashing branch and made that the default branch for the repository. We should start targeting new development there. I'll open up a PR documenting our procedure.

@jacobperron jacobperron deleted the jacob/ff-master branch May 20, 2021 18:23
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.

9 participants