Updating hello-ros example with basic ui #101

ivanpauno · 2019-01-24T20:52:11Z

This is base on the previous hello-ros sample app. I moved from NativeActivity to a normal java Activity, as I think it is better for this. We can also apply this changes to the other examples.

Note: Taking in account the comments here https://stackoverflow.com/questions/7815368/how-to-set-content-view-of-nativeactivity-to-component-created-in-java, NativeActivity can still be used.

Supersede #100

meyerj

I like the implementation using a Java activity which offloads the ROS work via JNI. Looks much cleaner like that. Indeed there is no need to use a native activity as long as we can trigger the ROS initialization and shutdown via JNI.

What I am missing is that the master URI and eventually ROS_IP is stored persistently, maybe using SharedPreferences.

Is it possible to place the settings controls in a separate dialog? Seems there is a built-in mechanism for that in Android, which configures SharedPreferences. For a simple test app having the settings in the main activity might be fine, but for real-world apps you might want to hide the settings and start the application with the defaults loaded from preferences. If the master URI gets changed, most likely the application has to be restarted anyway because roscpp cannot switch ROS master.

Last but not least: How complex is it to integrate the ROS master implementation of rosjava, in order to offer the option to run a local master? This is just a nice-to-have.

Please also add some license headers to the added files.

meyerj · 2019-01-25T15:27:25Z

example_workspace/src/hello_world_example_app/app/build.gradle

@@ -4,7 +4,7 @@ android {
    compileSdkVersion 28

    defaultConfig {
-        applicationId = 'com.example.hello_ros'
+        applicationId = 'com.ros.example.hello_ros'


rosjava uses the org.ros prefix. If we go away from com.example, should we also use org instead of com following the registered internet domain? What is the convention here?

I'd propose org.ros.android as prefix and then .example.hello_ros.

example_workspace/src/hello_world_example_app/app/src/main/cpp/main_thread.cpp

meyerj · 2019-01-25T15:44:59Z

example_workspace/src/hello_world_example_app/app/src/main/cpp/main_thread.cpp

+    MainThread::MainThread(std::string name) : node_name(name) {}
+    MainThread::~MainThread() {}
+
+    bool MainThread::check_ros_master(std::string master_ip, std::string my_ip) {


Maybe my_ip should be optional? On devices with only one interface the automatic detection implemented in ros::network::determineHost() works well for us on Android.

Hmm now I moved it to remapping arguments so the IP address is also included there. I'm not sure if it's really needed in this implementation of ros::init using ros::VP_string.

In any case scanning the VP for the IP is a bit annoying (not worse than sending a map through JNI though, which is another option). In any case the Java side gets the IP for you, so from the user's point of view it's basically the same.

My comment was actually independent on how the IP address is passed down to the native code and the exact parameter name. What I meant is that it should be possible to simply not set a ROS_IP like for Linux and leave it to roscpp to choose a hostname or IP string wisely. At least for us that works well and would not require the user to manually enter the IP address of the device.

On the other hand ros::network::determineHost() prefers non-private IP addresses if there is more than one network interface, which is most likely not what we want on phones which have a connection to a cellular network.

I'll retest this tomorrow; if it works without setting the IP address I'd leave it in the activity as a text view (non-editable) for information purposes.

Indeed, the IP address is not necessary. I'll change it to a read-only field to provide that information to the user.

Is it possible to make it optional (leave it empty)? In some cases, for example if you have a 3G connection and a Wifi connection, it is necessary to select one of them by setting the ROS_IP (or ROS_HOSTNAME).

I don't see much added value in showing the IP address to the user. If so, it should be exactly the same as ros::network::determineHost() found. It is available using ros::network::getHost(). In fact, it does not matter whether the string is an IP address or a hostname, as long as potential subscribers can reach the Android device using that name.

A spinner listing all available interfaces and their IP addresses would be ideal.

that sounds good; let's make it an (optional) spinner: if the user doesn't specify any interface we shall send no remapping for the IP address and let ROS decide it.

example_workspace/src/hello_world_example_app/app/src/main/cpp/main_thread.cpp

...ce/src/hello_world_example_app/app/src/main/java/com/ros/example/hello_ros/MainActivity.java

meyerj · 2019-01-25T16:11:22Z

...ce/src/hello_world_example_app/app/src/main/java/com/ros/example/hello_ros/MainActivity.java

+                mainThread.stop();
+                status = Status.WAITING;
+                break;
+        }


As far as I know roscpp does not allow to initialize the node twice, so ros::init() cannot be called twice within the same process, even if you call ros::shutdown() in between. Given that this is true, does it make sense to allow to toggle the state between WAITING and RUNNING?

See #101 (comment); this seems to be working.

It cannot if g_initialized is not destroyed and reinitialized to its default value: It is only ever set to true in init.cpp:469 and never reset. It might seem to work, but the new settings won't take effect. This behavior might be trivial to fix in roscpp, but without a patch you cannot change the master URI or ROS_IP without terminating the app or unloading all shared libraries.

I see. So this would work only to start / stop the program using the same ROS master; in any case it still sounds somewhat useful to me. I'll add a note somewhere clarifying that the app should be restarted if the ROS master changes after the first connection.

I tested the app today and as expected changes of the ROS master IP have no effect until the app is terminated by swiping it up or down in the app view.

jubeira · 2019-01-28T21:33:34Z

Update: I addressed some comments, adding support for remapping arguments and shared preferences.
I'll check how much it takes to separate the settings from the main activity.

Last but not least: How complex is it to integrate the ROS master implementation of rosjava, in order to offer the option to run a local master? This is just a nice-to-have.

Hmm I wouldn't go that way in this example. Using rosjava and wrapping the native code in a NativeNodeMain is always an option (the MasterChooser already has that feature), but that's not exactly what we are doing here. To use rosjava's master I'd need to duplicate code that's already in rosjava, and I'd like to avoid that.

meyerj · 2019-01-29T00:06:10Z

Last but not least: How complex is it to integrate the ROS master implementation of rosjava, in order to offer the option to run a local master? This is just a nice-to-have.

Hmm I wouldn't go that way in this example. Using rosjava and wrapping the native code in a NativeNodeMain is always an option (the MasterChooser already has that feature), but that's not exactly what we are doing here. To use rosjava's master I'd need to duplicate code that's already in rosjava, and I'd like to avoid that.

Okay, understood.

meyerj · 2019-01-29T19:30:48Z

example_workspace/src/hello_world_example_app/app/src/main/cpp/main_thread.cpp

+        } else {
+            ROS_INFO("NO ROS MASTER.");
+        }
+        ros::start();


Only call ros::start() if ros::master::check() returns true?

Apparently ros::start() tries to contact the master, logs

ros.roscpp: [registerPublisher] Failed to contact master at [x.x.x.x:11311]. Retrying...

and waits forever. Android then shows the "RosHello isn't responding" dialog.

…tional.

jubeira · 2019-02-01T21:41:48Z

I added the spinner for the IP address and formatted the layout a bit:

Now forcing the IP address is optional and it can be controlled within the UI.
Note that I was using a phone with 3G but only one IP address appeared for me; I didn't look into that with much detail.

meyerj · 2019-02-12T19:13:01Z

I added the spinner for the IP address and formatted the layout a bit:

Thanks for updating the UI and addressing comments.

Now forcing the IP address is optional and it can be controlled within the UI.
Note that I was using a phone with 3G but only one IP address appeared for me; I didn't look into that with much detail.

Maybe the app requires some additional permissions to access network state? The documentation of getNetworkInterfaces() mentions that NetPermission is required, but the documentation of that class says "Legacy security code; do not use".

meyerj · 2019-02-12T20:08:33Z

LGTM, except for this nice-to-have comment has not been addressed yet. Currently the app freezes if the ROS master cannot be reached, e.g. due to a typo in the IP address of the ROS master. Instead, checkRosMaster() should return false and the UI shows the error in the statusText box.

Additionally, there is no reason to not allow alphanumeric host names instead of an IP address in the ROS_MASTER_URI, as long as the hostname can be resolved. At the moment the text box limits the entry to digits and dots and the names of the UI element and shared preferences imply that the host part of the URI is an IP address, but that is not enforced by ROS or roscpp (ROS_MASTER_URI).

Feel free to apply these two changes, or not.

Updating hello-ros example with basic ui

19b80dd

ivanpauno self-assigned this Jan 24, 2019

ivanpauno requested a review from jubeira January 24, 2019 20:52

This was referenced Jan 24, 2019

Basic android app with ros master selection #100

Closed

Examples updated in catkin workspace #84

Merged

meyerj reviewed Jan 25, 2019

View reviewed changes

jubeira changed the base branch from updating-examples to kinetic January 25, 2019 19:02

Juan Ignacio Ubeira added 3 commits January 28, 2019 16:49

Adding basic remapping arguments support to sample app.

c66f8d6

Adjusting gitignore for Android Studio files.

acb2e5b

Adding clarification about remapping arguments in README.

b49fa37

jubeira force-pushed the hello-ros-with-basic-ui-reorganized branch from a25652a to b49fa37 Compare January 28, 2019 19:52

Juan Ignacio Ubeira added 4 commits January 28, 2019 17:27

Addressing comments and nits.

fec07fd

Changing namespace to org.ros.android.

40f43ec

Adding shared preferences to remappings and ROS master IP.

efdb1c0

Adding ROS master port to preferences and shared preferences.

31bbcf4

Merge branch 'kinetic' into hello-ros-with-basic-ui-reorganized

a75edb9

meyerj reviewed Jan 29, 2019

View reviewed changes

Juan Ignacio Ubeira added 2 commits February 1, 2019 10:45

Merge branch 'kinetic' into hello-ros-with-basic-ui-reorganized

ac33e96

Moving device's IP address to a spinner with a checkbox to make it op…

719f8df

…tional.

Adding rosjava_build_tools as buildtool deps to samples.

10a292b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating hello-ros example with basic ui #101

Updating hello-ros example with basic ui #101

ivanpauno commented Jan 24, 2019

meyerj left a comment

meyerj Jan 25, 2019

jubeira Jan 28, 2019

meyerj Jan 25, 2019

jubeira Jan 28, 2019

meyerj Jan 29, 2019

jubeira Jan 29, 2019

jubeira Jan 30, 2019

meyerj Jan 31, 2019

jubeira Jan 31, 2019

meyerj Jan 25, 2019

jubeira Jan 28, 2019

meyerj Jan 29, 2019

jubeira Jan 29, 2019

meyerj Jan 29, 2019

jubeira commented Jan 28, 2019

meyerj commented Jan 29, 2019

meyerj Jan 29, 2019

jubeira commented Feb 1, 2019

meyerj commented Feb 12, 2019 •

edited

Loading

meyerj commented Feb 12, 2019

Updating hello-ros example with basic ui #101

Are you sure you want to change the base?

Updating hello-ros example with basic ui #101

Conversation

ivanpauno commented Jan 24, 2019

meyerj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubeira commented Jan 28, 2019

meyerj commented Jan 29, 2019

Choose a reason for hiding this comment

jubeira commented Feb 1, 2019

meyerj commented Feb 12, 2019 • edited Loading

meyerj commented Feb 12, 2019

meyerj commented Feb 12, 2019 •

edited

Loading