Examples updated in catkin workspace #84

ivanpauno · 2019-01-08T17:23:16Z

Building ok and tested:

image_transport_sample_app
move_base_app
pluginlib_sample_app
hello_world_example_app (already updated in Updating examples #83)

Building fine, not tested:

move_base_app

I think we can already merge this with #83 in kinetic.

Solves #78

ivanpauno · 2019-01-08T17:28:03Z

This is the error output when building move_base_app:

In file included from /opt/ros_android/example_workspace/src/move_base_app/jni/src/main/cpp/test_move_base.cpp:2:
In file included from /opt/ros_android/output/target/include/move_base/move_base.h:48:
In file included from /opt/ros_android/output/target/include/nav_core/base_local_planner.h:42:
In file included from /opt/ros_android/output/target/include/costmap_2d/costmap_2d_ros.h:49:
In file included from /opt/ros_android/output/target/include/pluginlib/class_loader.h:35:
In file included from /opt/ros_android/output/target/include/pluginlib/./class_loader.hpp:350:
/opt/ros_android/output/target/include/pluginlib/./class_loader_imp.hpp:73:26: error: function 'catkinFindLib' is not needed and will not be emitted [-Werror,-Wunneeded-internal-declaration]
std::vectorstd::string catkinFindLib()
^
In file included from /opt/ros_android/example_workspace/src/move_base_app/jni/src/main/cpp/test_move_base.cpp:2:
In file included from /opt/ros_android/output/target/include/move_base/move_base.h:48:
In file included from /opt/ros_android/output/target/include/nav_core/base_local_planner.h:42:
In file included from /opt/ros_android/output/target/include/costmap_2d/costmap_2d_ros.h:41:
In file included from /opt/ros_android/output/target/include/costmap_2d/layered_costmap.h:42:
In file included from /opt/ros_android/output/target/include/costmap_2d/layer.h:40:
/opt/ros_android/output/target/include/costmap_2d/costmap_2d.h:463:26: error: private field 'char_map_' is not used [-Werror,-Wunused-private-field]
const unsigned char* char_map_;

jubeira

I did a quick review and overall it looks good.
The errors are probably because of the CMAKE_CXX_FLAGS we are using; we don't need to use -Wall -Werror for these examples.

I just left a few questions. Please save the apk files for later; I'd like to include them in a release.

jubeira · 2019-01-08T18:06:08Z

example_workspace/src/move_base_app/CMakeLists.txt

+
+# Set the gradle targets you want catkin's make to run by default, e.g.
+# catkin_rosjava_setup(installDist)
+# catkin_android_setup(assembleDebug)


Hmm don't we need this?

Yes, it was commented out because move_base example had problems to build before.
Now it is corrected.

...le_workspace/src/image_transport_sample_app/jni/src/main/cpp/image_transport_plugin_test.cpp

meyerj

The example look good and seem to be useful, but in my opinion requires some more tweaking:

Avoid using $ENV{TARGET_DIR} in the inner CMakeLists.txt files. Setting CMAKE_FIND_ROOT_PATH once via build.gradle should be sufficient.
Fix busy waiting (?) in some examples and use an ros::AsyncSpinner instead?
Lots or repetitive code, but there is nothing to do about it, I guess:
- I stopped copying or referencing comments at some points. Many comments also apply to similar cases in other packages.
- For plugin linking we could add a cmake helper to the patched pluginlib package to avoid having to use the same tedious and error-prone cmake code in each package.
- Fix copied package descriptions in package.xml files...
Nice-to-have: Replace the hard-coded __ip and __master strings with a minimal settings UI or a configuration file.

meyerj · 2019-01-23T20:21:07Z

example_workspace/src/image_transport_sample_app/CMakeLists.txt

+
+# Set the gradle targets you want catkin's make to run by default, e.g.
+# catkin_rosjava_setup(installDist)
+catkin_android_setup(assembleDebug)


See #83 (comment).

meyerj · 2019-01-23T20:21:53Z

example_workspace/src/image_transport_sample_app/jni/CMakeLists.txt

+include_directories(${catkin_INCLUDE_DIRS})
+
+# build native_app_glue as a static lib
+set(${CMAKE_C_FLAGS}, "${CMAKE_C_FLAGS}")


See #83 (comment).

meyerj · 2019-01-23T20:25:53Z

example_workspace/src/image_transport_sample_app/jni/build.gradle

+            cmake {
+                arguments   '-DANDROID_STL=c++_shared',
+                            '-DCMAKE_FIND_ROOT_PATH=' + System.getenv('TARGET_DIR'),
+                            '-DCMAKE_PREFIX_PATH=' + System.getenv('TARGET_DIR'),


See #83 (comment) and #83 (comment).

meyerj · 2019-01-23T20:26:52Z

example_workspace/src/image_transport_sample_app/package.xml

+  <!-- One license tag required, multiple allowed, one license per tag -->
+  <!-- Commonly used license strings: -->
+  <!--   BSD, MIT, Boost Software License, GPLv2, GPLv3, LGPLv2.1, LGPLv3 -->
+  <license>TODO</license>


Add a license?

meyerj · 2019-01-23T20:32:16Z

example_workspace/src/image_transport_sample_app/jni/CMakeLists.txt

+    native_app_glue
+    log
+    ${WHOLE_ARCHIVE_LIBRARIES}
+    ${catkin_LIBRARIES}


Important to note that the exact order matters here: Because most libraries in ${catkin_LIBRARIES} are static and we do not know which other libraries ${WHOLE_ARCHIVE_LIBRARIES} depend on, the linker would not necessarily add the symbols required by one of the plugins while going over ${catkin_LIBRARIES} if it would be linked in before ${WHOLE_ARCHIVE_LIBRARIES}.

Yes, here link order is important. Maybe another solutions is to patch all the plugins to export also the flags (-Wl,--whole-static library_name -Wl,--no-whole-archive) for android in catkin_package LIBRARIES option (I think this is possible with #68). Maybe is more difficult to maintain (I don't think it will be accepted upstream), but easier to use for the user.
@meyerj WDYT?

Patching plugin packages only works for the packages in the catkin_ws, but is not a general solution for packages providing plugins in other workspace(s) listed in --extends-workspace.

catkin_LIBRARIES deduplicates libraries found in all packages and does not preserve the order of listed libraries: catkinConfig.cmake.in:97-102

What might work is to add the string "-Wl,--whole-static library_name -Wl,--no-whole-archive" to catkin_LIBRARIES, with spaces and not as three separate list entries. Should check whether target_link_libraries(foo ${catkin_LIBRARIES}) would accept this.

We were thinking about adding a CMake macro pluginlib_export_plugins() in a catkin CFG_EXTRAS script provided by pluginlib, or a separate package. The macro needs to be called before catkin_package() in packages providing plugins and generates another cmake script which appends to a list of plugins and sets another variable, e.g. pluginlib_EXPORTED_PLUGIN_LINKER_FLAGS, which then in turn is added to ${PROJECT_NAME}_CFG_EXTRAS again. Like that, each package can statically link to all plugins provided by all its dependencies by adding those flags to target_link_libraries(...), without being affected by catkin internals. It is similar to the mechanics of genmsg and friends.

On the downside, linking to all plugins with --whole-archive might blow up binary sizes.

Maybe it is not even necessary to patch packages that provide macros if the cmake script provided by pluginlib already parses package.xml and the exported plugin declarations and finds plugins on it own.

On the long term exporting plugins like this might even replace the pluginlib_helper in ros_android, because the same set of CMake scripts could also generate a source file with the descriptions of all listed plugins on the fly - similar to pluginlib_helper.cpp - and compile and link it to the target with a single pluginlib_link_plugins(target [plugin1 [plugin2 ...]) call that needs to be added by the user in the Android-specific CMakeLists.txt for his application.

meyerj · 2019-01-23T20:51:04Z

example_workspace/src/nodelet_sample_app/CMakeLists.txt

+
+# Set the gradle targets you want catkin's make to run by default, e.g.
+# catkin_rosjava_setup(installDist)
+catkin_android_setup(assembleDebug)


See #83 (comment).

meyerj · 2019-01-23T21:57:33Z

example_workspace/src/move_base_app/jni/src/main/cpp/test_move_base.cpp

+            if (ros::ok()) {
+                ros::spinOnce();
+                loop_rate.sleep();
+            }


Shouldn't all native activities invoke ros::spinOnce() or launch an ros::AsyncSpinner?

Probably an AsyncSpinner is better because now sleeping with loop_rate.sleep() blocks Looper events from being processed.

meyerj · 2019-01-23T21:57:52Z

example_workspace/src/move_base_app/jni/src/main/cpp/test_move_base.cpp

+                return;
+            }
+
+            if (ros::ok()) {


Exit loop and terminate activity if ros::ok() == false?

meyerj · 2019-01-23T22:00:10Z

example_workspace/src/pluginlib_sample_app/jni/src/main/cpp/polygon_loader.cpp

+            LOGD("APP DESTROYED BYE BYE");
+            return;
+        }
+    }


No timeout and no ros::Rate. Busy-wait?

Yes. I will change for a locking looper version (ALooper_pollAll(-1, ...)), as nothing more is done.

meyerj · 2019-01-23T22:00:24Z

...le_workspace/src/image_transport_sample_app/jni/src/main/cpp/image_transport_plugin_test.cpp

+    struct android_poll_source* source;
+
+    // Poll android events, without locking
+    while (ALooper_pollAll(0, NULL, &events, (void**)&source) >= 0) {


ivanpauno · 2019-01-24T19:20:35Z

@meyerj I'm merging this with #83 in order to unify and solve conflicts there with new repo structure. Further comments can be done there. Minimal settings UI is proposed in #101 .

ivanpauno added 7 commits January 8, 2019 09:45

Nodelet math example adapted

5039c64

Image transport sample app working

12e995d

Pluginlib sample app working fine

1453cef

Corrected app name in nodelet sample

477baa8

Updated move base example. Still failing when building

2b78107

Added whole-archive libraries to move base app cmake file

19b2139

Disabled move base app building for now

20b4e32

ivanpauno self-assigned this Jan 8, 2019

ivanpauno requested a review from jubeira January 8, 2019 17:23

jubeira reviewed Jan 8, 2019

View reviewed changes

Move base app building fine

75e997e

jubeira mentioned this pull request Jan 23, 2019

Updating ROS kinetic packages to latest versions. #98

Merged

meyerj suggested changes Jan 23, 2019

View reviewed changes

Addressed comments on catkinized examples PR

d16b88d

ivanpauno merged commit 4de0cd7 into updating-examples Jan 24, 2019

Examples updated in catkin workspace #84

Examples updated in catkin workspace #84

Uh oh!

Conversation

ivanpauno commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivanpauno commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jubeira left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

meyerj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanpauno commented Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ivanpauno commented Jan 8, 2019 •

edited

Loading

ivanpauno commented Jan 8, 2019 •

edited

Loading

ivanpauno commented Jan 24, 2019 •

edited

Loading