Skip to content

Conversation

@graiola
Copy link

@graiola graiola commented Nov 20, 2022

Add a widget to publish the phone's coordinates to ROS.

It needs testing and a good code review because I am not an android developer :P

It would solve #98 and #79

@graiola graiola changed the title Gps2Ros Gps2Ros: a widget to publish phone's GPS coordinates to ROS Nov 20, 2022
@graiola graiola changed the title Gps2Ros: a widget to publish phone's GPS coordinates to ROS Location: a widget to publish phone's location to ROS Nov 21, 2022
@nicostudt nicostudt self-requested a review November 22, 2022 07:18
Copy link
Contributor

@nicostudt nicostudt left a comment

Choose a reason for hiding this comment

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

I left you some comments on the code. I'll try it in the next days.
So far - good work! Thank you for making this PR

<uses-permission android:name="android.permission.ACCESS_WIFI_STATE" />
<uses-permission android:name="android.permission.SYSTEM_ALERT_WINDOW"/>
<uses-permission android:name="android.permission.ACCESS_BACKGROUND_LOCATION" />

Copy link
Contributor

Choose a reason for hiding this comment

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

With a newer Android version you actually have to include the permission for coarse location as well

Copy link
Author

@graiola graiola Dec 9, 2022

Choose a reason for hiding this comment

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

I fixed that in the last commit

message.setLatitude(latitude);
message.setLongitude(longitude);
message.setAltitude(altitude);

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider to include the position_covariance as well to enable the user to use this widget in conjunction with fused odometry.

Copy link
Author

Choose a reason for hiding this comment

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

I have to check how to get these info

final boolean networkEnabled = locationManager.isProviderEnabled(LocationManager.NETWORK_PROVIDER);

if(gpsEnabled)
locationManager.requestLocationUpdates(LocationManager.GPS_PROVIDER, 0, 0, locationListenerGps);
Copy link
Contributor

Choose a reason for hiding this comment

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

Here it would be nice, if the user can control the minimum distance/time via widget parameters to reduce the drain on the device battery.

Copy link
Author

@graiola graiola Dec 9, 2022

Choose a reason for hiding this comment

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

With the fused provider it is slightly different. I will try to export these parameters in the widget if we decide to keep this version using the LocationManager.

//this.publishRate = 20f;
this.text = "Publish phone's location to ROS";
this.rotation = 0;
this.buttonPressed = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something the user should control via parameters from the config?

Copy link
Author

@graiola graiola Dec 9, 2022

Choose a reason for hiding this comment

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

Text and rotation can be changed by the user

@nicostudt
Copy link
Contributor

Thing to discuss:
Should the LocationProvider still be running, if the App isnt connected to any Ros Master?

@graiola
Copy link
Author

graiola commented Nov 22, 2022

I left you some comments on the code. I'll try it in the next days. So far - good work! Thank you for making this PR

Thank you! :)

Thing to discuss: Should the LocationProvider still be running, if the App isnt connected to any Ros Master?

Actually there is a problem with that: the publisher stops when the phone locks. It would be nice to have the publisher running in the background even if the phone locks. I was thinking to embed the locationManager into a Service but if you have a better idea please let me know.

I will work on the changes in the next days. Thank you again for your amazing work and suggestions.

@graiola
Copy link
Author

graiola commented Dec 9, 2022

I made other two branches to test the new google API for the fused location and to test foreground services with the LocationManager . I did not find a way to keep the publisher running when the phone screen gets locked, so I am forcing the screen to stay awake while using the app.

Here are the branches:

https://github.com/graiola/ROS-Mobile-Android/tree/fused

https://github.com/graiola/ROS-Mobile-Android/tree/service

I think the fused method is better because requires way less code to get the current location, moreover it has a way to define the publishing interval.

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.

2 participants