⬆️ [phone] Upgrade to the latest everything + some RFC 🎉

shankari commented 1 year ago

At long last, after clearing the decks with the label changes

I'm ready to start the annual ritual of upgrading to the most recent version of everything. I will also plan to make the experimental trip start feature the default https://github.com/e-mission/e-mission-docs/issues/785 and add silent push notifications to android to see if it can address background operation issues https://github.com/e-mission/e-mission-docs/issues/677

@TTalex @asiripanich @lgharib

shankari commented 1 year ago

I looked into the android 12 changes during the last upgrade. The relevant changes that we know are:

hibernation changes: already handled with setting "unused app restrictions". Need to test on android 12
- we should also make sure that the text for the setting is correct
foreground service launch restrictions:
- We should then be able to start the app when we do need the fine grained location access. But what happens if the service is killed while we are on the trip? That is not a geofence transition, and we are in the background. Or do they argue that this never happens.
- Although to be fair, even now, if the foreground service is killed during a trip, we won't really restart the foreground service until the trip ends, unless the trip is longer than an hour. Current locations for TripDiaryStateMachineForegroundService.startProperly are:
  - setNewState
  - `onServiceDisconnected
  - TripDiaryStateMachineReceiver
- There is also an option to turn off battery optimizations for the app. We may have to fallback to that if the foreground service can indeed be killed during a trip (and I don't see any guarantees that it won't be).
- "high priority FCM notifications" can also start foreground services from the background, although the system can lower the priority of the FCM notification if it does not
If your app requests the ACCESS_FINE_LOCATION runtime permission, you should also request the ACCESS_COARSE_LOCATION permission to handle the case where the user grants approximate location access to your app. You should include both permissions in a single runtime request.
Specifying android:allowBackup="false" does disable backups to Google Drive, but doesn’t disable D2D transfers for the app. (should we allow backup?)
~changes to the handling of third-party cookies to provide more security and privacy and offer users more transparency and control in WebView (we don't use third party cookies)~

Changes to investigate further:

data handling attribution access: detects whether private data app access happens in your app or in an SDK. We don't use any closed source SDKs, so we should not care about this. Looks like if we want to use attribution tags, they have to be defined in the manifest. But do we need to use attribution tags?
safer component exporting: If your app targets Android 12 or higher and contains activities, services, or broadcast receivers that use intent filters, you must explicitly declare the android:exported attribute for these app components.
pending intent mutability: all our intents should be defined as immutable so that "other apps cannot modify the intent to adjust the result of invoking the intent."
motion sensor rate limiting: does this affect the motion activity API and the motion activity transition latency?

shankari commented 1 year ago

for iOS, apple has approved our app, but has raised some questions on other apps on the same platform.

Potential changes:

switch to provisional notifications?: Not sure what the desired user flow should be - we ask for provisional notifications; they say "no", what should we do?
- verify that this does not affect silent push notifications
- we generate two kinds of visible notifications
  - push notifications
  - local notifications if the settings are incorrect
- looks like if the user says that they don't want to see the push notifications, we will also not be able to display local notifications. Should check whether the setting does affect all app notifications.
- if it does, we may want to have an onboarding question around whether the user wants to see daily notifications and only show them if they say yes. If they say "no", we can still send the local notifications, which are more important for baseline background data collection.
- we still always have the risk that the user originally accepts the notifications but then turns them off, and I am not sure how to deal with that or if there is any way to do so
Also, it looks like with iOS 14+, we don't have to get people to turn on the "always" permission in the app, we can prompt right after the "while in use". But is that really the best option from a UX perspective? wouldn't it be easier to have a single popup than two separate popups? https://medium.com/swlh/location-permission-in-ios-13-f9e10917c05e

shankari commented 1 year ago

@ttalex @lgharib @asiripanich @sebastianbarry @jGreenlee @jf87 @ericafenyo: I have some notes here on the changes required to support the new APIs and would like feedback on the user interface for the new changes:

android: allow backups to the cloud?
android: turn off app battery optimizations, or just see whether the foreground service is not killed during the trip
iOS: switch to provisional notifications? what do we do if the user says "no" when the provisional notification is displayed? How can we separate the local notifications to alert that settings are wrong from the notifications to remind users to label apps?
iOS: should we change the way we prompt for "always" location on iOS4?

shankari commented 1 year ago

Most recent versions of cordova seem to be:

cordova-android: android@11.0.0 https://cordova.apache.org/announcements/2022/07/12/cordova-android-release-11.0.0.html
cordova-ios: ios@6.2.0 (unchanged since last upgrade)
cordova-cli: cordova-cli@11

Template has been updated; let's see if we need to change anything to match it https://cordova.apache.org/news/2021/10/31/template-release.html

shankari commented 1 year ago

@asiripanich @ttalex @lgharib @ericafenyo @jf87 I plan to remove the following plugins, which are (to my knowledge) currently unused, and are getting more complex to maintain

cordova-jwt-auth: all projects that I am aware of login with a long "token"/opcode/secret key and not email/OpenID etc
cordova-plugin-ionic:
- since we are not using the ionic-deploy functionality any more. It was always unclear whether that constituted "code download"
- it is not currently authorized by NREL cyber
- having separate branches without a lot of oversight just led to a lot of divergent, throw-away code
- NREL cyber will not allow us to deploy code written by others without review and merge anyway
- so we are going to switch to a small number of features, checked into master as modules, and enabled/disabled via a simple static config file
e-mission-transition-notify: again, currently unused (nobody likes being interrupted at various times of the day/night) and sometimes causes crashes on android

JGreenlee commented 1 year ago

@shankari By the nature of the application, I think there is going to be an inherent cost for background location processing.

Android: I've seen that other apps with similar needs (Life360 is a good example) do take the route of disabling battery optimization. It might just have to be that way. The trickiest part of the issue, I think, is that battery optimization can vary a bit from vendor to vendor (ie Samsung handles differently than Pixel) so we can't predict if/when a process is killed

For android I see a few location priority options in the API - not sure what we currently use but if PRIORITY_BALANCED_POWER_ACCURACY is good enough, then we may be considered more 'compliant' with Android's recommendations. Fewer times of the OS annoying users with warnings about the battery drain

iOS: provisional notifications whenever possible, seems worth implementing. I can't comment much else bc I'm still learning what the app pushes and when

Agreed that having two popups to get location permission is annoying - but either way we can't reduce the number of clicks to less than 2. All things considered I think the best route from a UX perspective is to just comply with apple's recommended flow. It will be what iOS users become used to as other apps adopt it. If they deny, we just send them back and and ask them to allow from the system prefs

shankari commented 1 year ago

@JGreenlee I agree that there is an inherent cost of background location processing. Chapter 4 of my PhD thesis (https://www2.eecs.berkeley.edu/Pubs/TechRpts/2019/EECS-2019-180.pdf) explores various background sensing options in detail, and section 7.2 of my thesis performs a rigorous evaluation of of the power/accuracy tradeoff by using various different sensing regimes on identical phones. I would encourage you to read them first.

Note that for stock android phones, we currently do not require users to disable background optimizations. For non-stock android phones, we may need to have users disable optimizations (e.g. https://dontkillmyapp.com/) but the instructions are different for each make and model.

The question here is: "do we need to have users disable optimizations on stock android as well?". I would like to avoid doing that, because the fewer settings we ask users to modify, the better. If the foreground service (the persistent notification) is only rarely killed while the user is on a trip, then we don't need to ask users to change. If it is killed often, or if the foreground service is killed when the user removes the notification in android 13 (https://developer.android.com/guide/components/foreground-services#user-dismiss-notification), then we will have to ask users to change the setting.

So to reiterate: Should we err on the side of better data collection by asking the user to change this additional setting, or should we err on the side of usability by not asking them to change the setting and hoping that the service is not killed very often?

TTalex commented 1 year ago

On the subject of plugins: No issue from our side with removing the three unused ones. Especially if it frees some maintainability time.

By the way, one overall feedback I've got from the team is to try to reduce the interdependencies between plugins. And to try to isolate native parts as much as possible. This is linked to a specific need of "modulability", where only some parts of the app are used.

On the subject of battery optimizations: I tend to agree with usability being more important than data collection. As long as it doesn't completely break data collection. Something only data will tell. So as far as default values goes, I would rather go with not asking users to change the settings. Maybe keep it as an option for power users.

On the subject of Android cloud backups: I'm not sure that I fully understand this part. What is / can be part of a backup ? If it's user data, we try to avoid using google services when we can, so no issues with disabling backups.

I hope that helps.

shankari commented 1 year ago

@ttalex

wrt plugins: I agree that the plugins are not as modular as one would like.
- However many/most of the plugins are actually libraries that the other plugins depend on (e.g. logging/usercache), so the app will not really function without them. At this point, it is probably best to assume that all e-mission plugins (except for the trip-end-detection) are required and to make plans to include their functionality. If the team has concrete suggestions on which plugin functionality they would like to remove, I'm happy to discuss whether it is possible.
- At least from an NREL perspective, it is not a very high priority to modularize the plugins any further. Our funding is focused on providing an easy to use platform for deployment partners. So our priorities are around improving app performance, usability and related tools. When we do tackle modularity, we will do so in the order of (1) app UI, (2) server and (3) native code. If we are able to get TCF funding to work on "commercialization" with industry partners, we will be able to prioritize developer, as opposed to deployer, concerns. Of course, we are happy to accept community contributions wrt modularity in any order 😄
Android cloud backups: I would also prefer to not backup apps to the cloud. On iOS, I have turned this off for many versions (https://github.com/e-mission/cordova-usercache/blob/master/src/ios/BEMBuiltinUserCache.m#L137). I can now turn it off on android as well. But then, if the user gets a new phone, and chooses to "restore apps" on the new phone, this app will not be restored. I am OK with that, but wanted to check with the rest of you on user expectations.....

TTalex commented 1 year ago

Sounds good to me, thanks for the details.

From my side, starting from scratch with new phones is what we're currently doing in our use cases. This has not been an issue for users as far as I know (note: the sample size is limited).

shankari commented 1 year ago

After finishing up

two posters for TRB and two papers for journals, and
a bunch of server-side fixes related to exporting data and dealing with "big jumps" and
some new spatial analysis that we hope to integrate into the public dashboard and
lots of design on the new survey additions + diary/trip unification I am finally ready to pick this up again

shankari commented 1 year ago

Fixed some pending issues with the native + fixed CI issues. The next step is to make it so that we can install CocoaPods correctly. CocoaPods are designed to be installed with "the default version of ruby on OSX" However, in practice, it is "the default version of ruby on the most recent version of OSX" If we have an older version of OSX, the default version of ruby is too old I've filed an issue in the CocoaPods repo to update the documentation. https://github.com/CocoaPods/CocoaPods/issues/11763

In the meanwhile, seeing if we can also lock down the version of ruby

shankari commented 1 year ago

Next step, let's upgrade all the dependencies in setup/export_shared_dep_versions.sh New versions are:

export NVM_VERSION=0.39.3
export NODE_VERSION=19.5.0
export NPM_VERSION=9.3.1
# make sure that this is a stable version from
# so that https://github.com/postmodern/ruby-versions
# ideally, this would be the same version as the CI
# Looks like brew supports only major and minor, not patch version
export RUBY_VERSION=3.0
export COCOAPODS_VERSION=1.11.3
export GRADLE_VERSION=7.6
export OSX_EXP_VERSION=12

shankari commented 1 year ago

Upgrade to latest versions of the dev toolchain:

xcode 14.2
android CLI 9477386
android SDK packages to their latest versions
cordova-android 11.0.0
other cordova packages to their latest versions

shankari commented 1 year ago

Tried to bump up the android dependencies to

com.google.code.gson:gson: 2.8.8 -> 2.10.1 androidx.core:core: 1.7.0 -> 1.9.0 androidx.work:work-runtime: 2.7.1 -> 2.8.0

Unfortunately, these upgrades depend on version 33

       1.  Dependency 'androidx.work:work-runtime:2.8.0' requires libraries and applications that
           depend on it to compile against version 33 or later of the
           Android APIs.

           :app is currently compiled against android-32.

           Also, the maximum recommended compile SDK version for Android Gradle
           plugin 7.2.1 is 32.

           Recommended action: Update this project's version of the Android Gradle
           plugin to one that supports 33, then update this project to use
           compileSdkVerion of at least 33.

and the current version of cordova-android only supports API 32 and gradle plugin 7.2.1 https://cordova.apache.org/announcements/2022/07/12/cordova-android-release-11.0.0.html

Target SDK (targetSdk): 32
SDK Build Tool: 32.0.0
Gradle: 7.4.2
Kotlin: 1.5.21
Android Gradle Plugin (AGP): 7.2.1
Google Services Gradle Plugin: 4.3.10
AndroidX App Compat Library: 1.4.2
AndroidX WebKit Library: 1.4.0
AndroidX SplashScreen Core Library: 1.0.0-rc01

Trying a slightly older version...

shankari commented 1 year ago

Older versions do work, updated in https://github.com/e-mission/e-mission-data-collection/pull/202/commits/8043d6213e8a741170062770c1abc2c3001aa278

-    <framework src="com.google.code.gson:gson:2.8.8"/>
+    <framework src="com.google.code.gson:gson:2.10.1"/>
     <framework src="com.google.android.gms:play-services-location:$LOCATION_VERSION"/>
-    <preference name="LOCATION_VERSION" default="18.0.0"/>
+    <preference name="LOCATION_VERSION" default="21.0.1"/>
     <framework src="androidx.core:core:$ANDROIDX_CORE_VERSION"/>
-    <preference name="ANDROIDX_CORE_VERSION" default="1.7.0"/>
+    <preference name="ANDROIDX_CORE_VERSION" default="1.8.0"/>

shankari commented 1 year ago

Now handling

If your app requests the ACCESS_FINE_LOCATION runtime permission, you should also request the ACCESS_COARSE_LOCATION permission to handle the case where the user grants approximate location access to your app. You should include both permissions in a single runtime request.

This is actually not an issue for us because we require FINE_LOCATION and BACKGROUND_LOCATION, so on Android 12+, we just open the settings page directly.

So the changes are:

on the phone UI, for android 12, tell people that they need to provide fine location access
test on android 10 to see if we get the warning
test on older versions of android to see if we get the warning

shankari commented 1 year ago

Testing on android 10, we don't see the behavior outlined in https://developer.android.com/training/location/permissions#approximate-request

no coarse location access in the settings	Even after requesting both FINE and COARSE, we see the same prompt

No warning message

$ ~/Library/Android/sdk/platform-tools/adb logcat | grep FINE_LOCATION
03-12 10:26:15.025  8922  9062 D SensorPermissionsAndSettingsForegroundDelegate: For permission android.permission.ACCESS_FINE_LOCATION shouldShowRequest = false
03-12 10:26:27.684  7712  7712 V GrantPermissionsActivity: Permission grant result requestId=978032682888837297 callingUid=10157 callingPackage=edu.berkeley.eecs.emission permission=android.permission.ACCESS_FINE_LOCATION isImplicit=false result=4
03-12 10:26:27.716  8922  8922 I SensorPermissionsAndSettingsForegroundDelegate: permissions are [android.permission.ACCESS_BACKGROUND_LOCATION, android.permission.ACCESS_FINE_LOCATION]

So let's just update the instructions in the UI

shankari commented 1 year ago

After the upgrade to the new version of xcode, even without any of the other plugin changes, I get this error while uploading to the app store.

shankari commented 1 year ago

Searching through the binary for the transform: string, this seems to be related to to the GoogleDataTransport pod

$ grep -r transform: platforms/ios//Pods/GoogleDataTransport/GoogleDataTransport
platforms/ios//Pods/GoogleDataTransport/GoogleDataTransport/GDTCORLibrary/Public/GoogleDataTransport/GDTCORConsoleLogger.h:  /** For error messages concerning transform: not being implemented by an event transformer. */
platforms/ios//Pods/GoogleDataTransport/GoogleDataTransport/GDTCORLibrary/Public/GoogleDataTransport/GDTCOREventTransformer.h:- (nullable GDTCOREvent *)transform:(GDTCOREvent *)event;
platforms/ios//Pods/GoogleDataTransport/GoogleDataTransport/GDTCORLibrary/GDTCORTransformer.m:      if ([transformer respondsToSelector:@selector(transform:)]) {
platforms/ios//Pods/GoogleDataTransport/GoogleDataTransport/GDTCORLibrary/GDTCORTransformer.m:        transformedEvent = [transformer transform:transformedEvent];
platforms/ios//Pods/GoogleDataTransport/GoogleDataTransport/GDTCORLibrary/GDTCORTransformer.m:                       @"Transformer doesn't implement transform: %@", transformer);

which is included from

  - FirebaseCoreDiagnostics (1.7.0):
    - GoogleDataTransport (~> 7.4)
    - GoogleUtilities/Environment (~> 6.7)
    - GoogleUtilities/Logger (~> 6.7)
    - nanopb (~> 1.30906.0)

Checking the versions after the update, we have the same dependencies

  - FirebaseCoreDiagnostics (1.7.0):
    - GoogleDataTransport (~> 7.4)
    - GoogleUtilities/Environment (~> 6.7)
    - GoogleUtilities/Logger (~> 6.7)
    - nanopb (~> 1.30906.0)

shankari commented 1 year ago

Workarounds appear to be:

Use cordova plugin add https://github.com/Salesflare/cordova-plugin-push
The fix has been merged to master (https://github.com/havesource/cordova-plugin-push/pull/152) but on Apr 11 so we need to upgrade to 4.0.0-dev0

shankari commented 1 year ago

Looks like the warning doesn't actually result in a rejection, so we could just wait to see if this is released. But it's already been almost a year, so not sure if it will be soon

Looks also like 4.0.0-dev.0 has a bunch of android fixes for API 31, including the trampoline fixes. https://github.com/havesource/cordova-plugin-push/commit/c3fb5b894afe17a05e21be135961f5831bafb1e0

So we might want to just go with that for now.

shankari commented 1 year ago

Ok, so we are finally going to start the most complex set of changes for this upgrade, which is the restriction on launching foreground services.

There are a couple of potential fixes:

ask the user to turn off battery optimizations
only display the foreground service when the user is in motion

(2) has the benefit that it won't annoy users as much - got feedback from at least a couple of test users that having it on all the time was annoying. It has the disadvantage that we won't always show a "status ribbon" indicating whether the app is working or not so people may not know that the app is not working

would be worse to have status ribbon that is not updated properly/killed

@TTalex @JGreenlee @sebastianbarry @lgharib @allenmichael099 any thoughts about option (1) vs (2)? Will you miss the "status notification" on android if it only shows up when we are actually on a trip, or will that actually be better? We have to show the notification while on a trip; otherwise, we will get data at very low frequencies.

shankari commented 1 year ago

I am just a bit reluctant to make the foreground service only run during the trip because I spent a lot of time setting up the connection between the finite state machine service and the foreground service so that the state machine service could send messages to the foreground service periodically without having to restart it every time...

shankari commented 1 year ago

Well, we can always go back to the old implementation by reverting the changes. Let's try and follow the spirit of the change, which is that we should only have a foreground service when we are reading the location. Hopefully we can also discuss this in the meeting at the end of March.

shankari commented 1 year ago

We can still start foreground services if

Your app receives an event that's related to geofencing or activity recognition transition.

Does this mean that we have to start the foreground service within the geofence/activity transition callback, or can we do so in the service that handles the event?

Let's try to start with the service since it will make the implementation easier.

shankari commented 1 year ago

Note that we were always potentially subject to background service limits https://developer.android.com/about/versions/oreo/background

While an app is in the foreground, it can create and run both foreground and background services freely. When an app goes into the background, it has a window of several minutes in which it is still allowed to create and use services. At the end of that window, the app is considered to be idle. At this time, the system stops the app's background services, just as if the app had called the services' [Service.stopSelf()](https://developer.android.com/reference/android/app/Service#stopSelf()) methods.

We did not have to test this until now because we had a foreground service, so we were considered to be in the foreground. We now need to see whether the TripDiaryStateMachineService.java is launched properly although the app is in the background.

shankari commented 1 year ago

We originally set up the broadcast receiver + launched service method because we thought that we might be structured as a service/library and there might be other apps that listen to the broadcasts from our app and work accordingly. However, except for potentially TraceMob + Cozy, that has not been the design pattern that we have encountered so far.

Instead the design pattern has been an integrated app that tracks, stores and pushes data, and displays the results.

We could significantly simplify the codebase as long as we are OK with only listening to the broadcasts internally.

We can do this by using a bound service, which is not affected by background restrictions. The bound service would have a public method handleAction, which would essentially be the same as the current onStartCommand(intent.getAction()). We could also have a handlePeriodicSync interface that can be called from the server sync code. The entire broadcast receiver can then be removed.

Pretty big change, but hopefully not too complicated.

shankari commented 1 year ago

First try, with just the obvious service creation is at https://github.com/e-mission/e-mission-data-collection/pull/202/commits/d8828a2f60c88847f7e3fc716b1267f53bc23bd6

This generates the foreground service while testing in the emulator, but does not do so while testing on a real phone. Both NREL OpenPATH and Ma Mobilitie do work, so this is not an issue with the geofence exit.

shankari commented 1 year ago

it looks like we never get the geofence exit

We should see trips:

9:06 -> 9:10
10:10 -> 10:14

Installed new version

34464,1678851547.989,2023-03-14T20:39:07.989000-07:00,TripDiaryStateMachineRcvr : Comparing installed version 48 with new version 49
34465,1678851547.99,2023-03-14T20:39:07.990000-07:00,"TripDiaryStateMachineRcvr : Setup not complete, sending initialize"

Created geofence

34482,1678851548.232,2023-03-14T20:39:08.232000-07:00,"CreateGeofenceAction : creating geofence at location ........................"
34483,1678851548.236,2023-03-14T20:39:08.236000-07:00,"OPGeofenceExitActivityActions : Starting listening to activity transitions for list = [ActivityTransition [mActivityType=7, mTransitionType=0], ActivityTransition [mActivityType=1, mTransitionType=0], ActivityTransition [mActivityType=8, mTransitionType=0], ActivityTransition [mActivityType=0, mTransitionType=0], ActivityTransition [mActivityType=3, mTransitionType=0]]"
34489,1678851548.28,2023-03-14T20:39:08.280000-07:00,TripDiaryStateMachineService : newState after handling action is local.state.waiting_for_trip_start

Stopping the (non-existent) foreground service

34491,1678851548.281,2023-03-14T20:39:08.281000-07:00,TripDiaryStateMachineForegroundService : stopProperly called with context = edu.berkeley.eecs.emission.cordova.tracker.location.TripDiaryStateMachineService@cbd6f86

No logs between

34821,1678851632.899,2023-03-14T20:40:32.899000-07:00,"js : local and native values match, already synced"

34822,1678857137.295,2023-03-14T22:12:17.295000-07:00,BuiltinUserCache : Added value for key stats/client_nav_event at time 1.67885713729E9

Both the other apps got an exited_geofence transition at the right time, but we didn't

  #225: act=local.transition.exited_geofence flg=0x10 pkg=gov.nrel.cims.openpath
    +111ms dispatch +9ms finish
    enq=2023-03-14 21:05:43.523 disp=2023-03-14 21:05:43.634 fin=2023-03-14 21:05:43.643
  #226: act=local.transition.exited_geofence flg=0x10 pkg=com.fabmoqc.mamobilite
    0 dispatch +115ms finish
    enq=2023-03-14 21:05:43.519 disp=2023-03-14 21:05:43.519 fin=2023-03-14 21:05:43.634

Per stackoverflow, https://stackoverflow.com/questions/54348093/geofencing-in-background-on-android-8-or-9-does-not-work even the intent service (aka GeofenceIntentService) is subject to the restrictions on background limits https://github.com/e-mission/e-mission-docs/issues/838#issuecomment-1469152654

We need to handle this by switching to a JobIntentService.

Note: IntentService is a service, and is therefore subject to the new restrictions on background services. As a result, many apps that rely on IntentService do not work properly when targeting Android 8.0 or higher. For this reason, Android Support Library 26.0.0 introduces a new JobIntentService class, which provides the same functionality as IntentService but uses jobs instead of services when running on Android 8.0 or higher.

shankari commented 1 year ago

The android support library is deprecated, https://developer.android.com/reference/android/support/v4/app/JobIntentService and we need to use androidx instead.

There is an androidx.core.app.JobIntentService instead https://developer.android.com/jetpack/androidx/migrate/class-mappings

However, that is also now deprecated (https://developer.android.com/jetpack/androidx/releases/core#1.6.0-rc01) in favor of WorkManager.

Fortunately, we are already using WorkManager in the new OPGeofenceExit. But I am not quite sure how to use the work manager with the existing Geofence API.

Let's see if we can do the migration step by step.

shankari commented 1 year ago

The standard geofence API tutorial has now switched to using a BroadcastReceiver to receive the intents https://developer.android.com/training/location/geofencing

So I guess the desired pattern is: BroadcastReceiver -> WorkManager -> bound service?? -> foreground Not sure we even need the WorkManager -> bound service; we can just immediately talk to the bound service since it is already a longer-running task.

This seems like a lot of change, and I'm getting a bit of anxiety about the resulting accuracy etc. On the other hand, I don't want to just kick the can down the road further...

shankari commented 1 year ago

How about we first make the change to just turn off the background optimizations so that we have a working solution by the end of the month?

We can then make these changes in a separate branch

shankari commented 1 year ago

ok, I am going to revert my changes so far, and just turn off background optimizations for now. That is a simple fix that I can implement in a day and will get us over this hump.

Then, we should implement these changes and potentially some other changes to the native code (like iOS trip end detection) and do another round of MobilityNet data collection that includes the OPGeofence and the new iOS trip end detection.

shankari commented 1 year ago

Turning off battery optimizations:

The user turns off battery optimizations for your app. You can help users find this option by sending them to your app's App info page in system settings. To do so, invoke an intent that contains the ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS intent action.

You can use PowerManager.isIgnoringBatteryOptimizations() to determine if an application is already ignoring optimizations. You can use ACTION_REQUEST_IGNORE_BATTERY_OPTIMIZATIONS to ask the user to put you on this list.

Should we use ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS in the AppInfo page, or ACTION_REQUEST_IGNORE_BATTERY_OPTIMIZATIONS?

shankari commented 1 year ago

Difference between them:

Most apps should invoke an intent that contains the ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS.

Apps that satisfy an acceptable use case can instead invoke an intent that contains the ACTION_REQUEST_IGNORE_BATTERY_OPTIMIZATIONS intent action to let the user add the app to the exemption list directly, without going to system settings.

Acceptable use cases include: https://developer.android.com/training/monitoring-device-state/doze-standby#exemption-cases

Instant messaging, chat, or calling app; enterprise VOIP apps., if it can't use FCM because of technical dependency on another messaging service or Doze and App Standby break the core function of the app. <----> We don't qualify
Safety app: Apps that keep their users and their families safe. <-----> I guess we could rebrand, but that is not really the core
Task automation app: App's core function is scheduling automated actions, such as for instant messaging, voice calling, or new photo management. <------> We don't qualify
Peripheral device companion app. <---------> definitely don't qualify

Going with ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS

shankari commented 1 year ago

Note that ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS was added in API 23, so it's good that we are bumping up our min SDK in this release 😄 https://developer.android.com/reference/android/provider/Settings#ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS

shankari commented 1 year ago

While trying to launch the optimization settings, running into this infinite loop

03-15 16:46:58.941 32041 32325 W System.err:    at org.apache.cordova.CordovaInterfaceImpl.setActivityResultCa
llback(CordovaInterfaceImpl.java:80)
03-15 16:46:58.941 32041 32325 W System.err:    at edu.berkeley.eecs.emission.cordova.tracker.verification.Sen
sorControlForegroundDelegate.onActivityResult(SensorControlForegroundDelegate.java:517)
03-15 16:46:58.941 32041 32325 W System.err:    at edu.berkeley.eecs.emission.cordova.tracker.DataCollectionPl
ugin.onActivityResult(DataCollectionPlugin.java:228)
03-15 16:46:58.941 32041 32325 W System.err:    at org.apache.cordova.CordovaInterfaceImpl.setActivityResultCa
llback(CordovaInterfaceImpl.java:80)
03-15 16:46:58.942 32041 32325 W System.err:    at edu.berkeley.eecs.emission.cordova.tracker.verification.Sen
sorControlForegroundDelegate.onActivityResult(SensorControlForegroundDelegate.java:517)
03-15 16:46:58.942 32041 32325 W System.err:    at edu.berkeley.eecs.emission.cordova.tracker.DataCollectionPl
ugin.onActivityResult(DataCollectionPlugin.java:228)
03-15 16:46:58.942 32041 32325 W System.err:    at org.apache.cordova.CordovaInterfaceImpl.setActivityResultCa
llback(CordovaInterfaceImpl.java:80)
03-15 16:46:58.942 32041 32325 W System.err:    at edu.berkeley.eecs.emission.cordova.tracker.verification.Sen
sorControlForegroundDelegate.checkAndPromptIgnoreBatteryOptimizations(SensorControlForegroundDelegate.java:439
)
03-15 16:46:58.942 32041 32325 W System.err:    at edu.berkeley.eecs.emission.cordova.tracker.DataCollectionPl
ugin.execute(DataCollectionPlugin.java:124)
03-15 16:46:58.942 32041 32325 W System.err:    at org.apache.cordova.CordovaPlugin.execute(CordovaPlugin.java
:98)
03-15 16:46:58.942 32041 32325 W System.err:    at org.apache.cordova.PluginManager.exec(PluginManager.java:14
6)
03-15 16:46:58.942 32041 32325 W System.err:    at org.apache.cordova.CordovaBridge.jsExec(CordovaBridge.java:
59)
03-15 16:46:58.942 32041 32325 W System.err:    at org.apache.cordova.engine.SystemExposedJsApi.exec(SystemExp

shankari commented 1 year ago

The chain is

        // This will be a NOP if we are not handling the correct activity intent
                mControlDelegate.onActivityResult(requestCode, resultCode, data);

      Log.i(cordova.getActivity(), TAG, "Battery optimizations enforced, asking user to ignore");
      this.cordovaCallback = cordovaCallback;
      cordova.setActivityResultCallback(plugin);
      openBatteryOptimizationScreen(cordovaCallback);

    @Override
    public void setActivityResultCallback(CordovaPlugin plugin) {
        // Cancel any previously pending activity.
        if (activityResultCallback != null) {
            activityResultCallback.onActivityResult(activityResultRequestCode, AppCompatActivity.RESULT_CANCELED, null);
        }
        activityResultCallback = plugin;
    }

  public void onActivityResult(int requestCode, int resultCode, Intent data) {
    Activity mAct = cordova.getActivity();
    cordova.setActivityResultCallback(null);

So setActivityResultCallBack calls onActivityResult which calls setActivityResultCallback which calls onActivityResult and so on.

I'm pretty sure that the issue is that I am setting the callback twice while setting it up - once in checkAndPromptIgnoreBatteryOptimizations and once in openBatteryOptimizationScreen. Since we only call openBatteryOptimizationScreen in one place, inlining the call...

shankari commented 1 year ago

That seems to work. The main test now is to ensure that, with this fix, if the foreground service is killed, it is automatically restarted.

To kill the foreground service, we use

adb shell am force-stop edu.berkeley.eecs.emission

That kills the entire app, including the foreground service

Before killing	After killing

Now, the foreground service should automatically restart in the next periodic sync

03-14 07:58:03.849 10615 10765 I TripDiaryStateMachineRcvr: START PERIODIC ACTIVITY
03-14 07:58:03.880 10615 10765 D TripDiaryStateMachineForegroundService: In checkForegroundNotification, found
 0 active notifications
03-14 07:58:03.895 10615 10765 D TripDiaryStateMachineForegroundService: Did not find foreground notification
with ID 6646464 in list []
03-14 07:58:03.981 10615 10765 D TripDiaryStateMachineForegroundService: startProperly called with context = a
03-14 07:58:04.055 10615 10615 D TripDiaryStateMachineForegroundService: onStartCommand called with intent = I
ntent { cmp=edu.berkeley.eecs.emission/.cordova.tracker.location.TripDiaryStateMachineForegroundService } flag
03-14 07:58:04.099 10615 10615 D TripDiaryStateMachineForegroundService: onStartCommand called on oreo+, with
msg Ready for your next trip starting foreground service

03-15 18:13:11.812 19490 19533 I TripDiaryStateMachineRcvr: START PERIODIC ACTIVITY
03-15 18:13:11.822 19490 19533 D TripDiaryStateMachineForegroundService: In checkForegroundNotification, found 0 active notifications
03-15 18:13:11.840 19490 19533 D TripDiaryStateMachineForegroundService: Did not find foreground notification with ID 6646464 in list []
...

I do see this warning. Not sure if this matters given that the location access is from a separate intent service. Will need to kill the foreground service on a regular device and try it, or see if we can generate a geofence exit via adb

03-15 18:13:12.303  1416  3422 W ActivityManager: Foreground service started from background can not have location/camera/microphone access: service edu.berkeley.eecs.emission/.cordova.tracker.location.TripDiaryStateMach
ineForegroundService

shankari commented 1 year ago

I can also use ADB to directly generate broadcasts to services/broadcast receiver. Note that this requires root access and will only work on images with google_apis ONLY. It will not work on google_apis_playstore

$ adb root
restarting adbd as root

Calling initialize

$ ~/Library/Android/sdk/platform-tools/adb shell am broadcast -a local.transition.initialize -n edu.berkeley.eecs.emission/.cordova.tracker.location.TripDiaryStateMachineReceiver
Broadcasting: Intent { act=local.transition.initialize flg=0x400000 cmp=edu.berkeley.eecs.emission/.cordova.tracker.location.TripDiaryStateMachineReceiver }
Broadcast completed: result=0

03-15 19:55:35.305 13700 13700 I TripDiaryStateMachineRcvr: TripDiaryStateMachineReciever onReceive(android.app.ReceiverRestrictedContext@a4427a7, Intent { act=local.transition.initialize flg=0x400010 cmp=edu.berkeley.eecs.emission/.cordova.tracker.location.TripDiaryStateMachineReceiver }) called

03-15 19:55:35.329 13700 13700 D TripDiaryStateMachineForegroundService: startProperly called with context = android.app.ReceiverRestrictedContext@a4427a7
03-15 19:55:35.330   601  3004 I ActivityManager: Background started FGS: Allowed [callingPackage: edu.berkeley.eecs.emission; callingUid: 10148; uidState: RCVR; intent: Intent { cmp=edu.berkeley.eecs.emission/.cordova.tracker.location.TripDiaryStateMachineForegroundService }; code:SYSTEM_ALLOW_LISTED; tempAllowListReason <,reasonCode:SYSTEM_ALLOW_LISTED,duration:9223372036854775807,callingUid:-1>; targetSdkVersion:32; callerTargetSdkVersion:32; startForegroundCount:0; bindFromPackage:null]

03-15 19:55:35.425   601  3000 W ActivityManager: Foreground service started from background can not have location/camera/microphone access: service edu.berkeley.eecs.emission/.cordova.tracker.location.TripDiaryStateMachineForegroundService

03-15 19:55:35.533 13700 13737 D CreateGeofenceAction: Last location would have been Location[fused 37.722760, -122.164390 hAcc=8.423 et=+6m59s198ms alt=0.0 vAcc=0.5] if we hadn't reset it
03-15 19:55:35.537 13700 13737 I CreateGeofenceAction: isValidLocation = true. Yay!
03-15 19:55:35.542 13700 13737 D CreateGeofenceAction: Last location is Location[fused 37.722760,-122.164390 h
Acc=8.423 et=+6m59s198ms alt=0.0 vAcc=0.5] using it

We get that message about the foreground service started from the background, but it doesn't seem to affect the geofencing creation at least.

Let's now exit the geofence and start location tracking

03-15 20:03:04.766 17616 17616 D TripDiaryStateMachineService: TripDiaryStateMachineReceiver handleTripStart(local.transition.exited_geofence) called
03-15 20:03:04.769 17616 17616 D CreateGeofenceAction: Removing geofence with ID = DYNAMIC_EXIT_GEOFENCE
03-15 20:03:04.775 17616 17616 D ActivityRecognitionActions: Starting activity recognition with interval = 30000

03-15 20:03:04.784 17616 17616 D LocationTrackingAction: default request is Request[@1h BALANCED_POWER_ACCURACY, minUpdateInterval=10m, waitForAccurateLocation]
03-15 20:03:04.789 17616 17616 D LocationTrackingAction: after applying config, value is Request[@30s HIGH_ACCURACY, minUpdateInterval=5s, waitForAccurateLocation]
03-15 20:03:04.794 17616 17616 D LocationTrackingAction: requesting location updatesRequest[@30s HIGH_ACCURACY, minUpdateInterval=5s, waitForAccurateLocation]
03-15 20:03:04.798 17616 17616 D LocationTrackingAction: default request is Request[@1h BALANCED_POWER_ACCURACY, minUpdateInterval=10m, waitForAccurateLocation]
03-15 20:03:04.802 17616 17616 D LocationTrackingAction: after applying config, value is Request[@30s HIGH_ACCURACY, minUpdateInterval=5s, waitForAccurateLocation]
03-15 20:03:04.807 17616 17616 D TripDiaryStateMachineService: handleAction(local.state.waiting_for_trip_start, local.transition.exited_geofence) completed, waiting for async operations to complete

And we do get the locations read correctly

03-15 20:03:38.364 17616 17616 D LocationChangeIntentService: onCreate called
03-15 20:03:38.378 17616 18569 D LocationChangeIntentService: FINALLY! Got location update, intent is Intent { cmp=edu.berkeley.eecs.emission/.cordova.tracker.location.LocationChangeIntentService (has extras) }
03-15 20:03:38.393 17616 18569 D LocationChangeIntentService: Read locations [Location[fused 37.719410,-122.178100 hAcc=5.0 et=+19m3s88ms alt=0.0 vAcc=0.5 bear=348.0 bAcc=30.0]] from intent

03-15 20:06:47.129 17616 17616 D LocationChangeIntentService: onCreate called
03-15 20:06:47.988 17616 20255 D LocationChangeIntentService: Read locations [Location[fused 37.591778,-122.056140 hAcc=74.051 et=+22m11s891ms alt=0.0 vAcc=0.5]] from intent
...

So it looks like it all works fine on Android 12 (the "Foreground service started from background" does not seem to be significant)

Will kill the app on the physical device and verify

shankari commented 1 year ago

It's great that we can send broadcast notifications to test, but it seems like that might be a problem if we refactor to a bound service.

I have not been able to figure out how to make calls to a bound service using adb, but we can call an intent service like so on a rooted shell (using the googleapis without the play services)

$ adb shell am startservice -a 2 edu.berkeley.eecs.emission/.cordova.tracker.location.GeofenceExitIntentService

We do have to figure out the exact format of the various intent services, though

03-15 20:58:41.100 17616 17616 D GeofenceExitIntentService: onCreate called
03-15 20:58:41.114 17616 17616 D GeofenceExitIntentService: onStartCommand called with intent Intent { act=2 cmp=edu.berkeley.eecs.emission/.cordova.tracker.location.GeofenceExitIntentService } flags 0 startId 1
03-15 20:58:41.120 17616 17616 D GeofenceExitIntentService: onStart called with startId 1
03-15 20:58:41.125 17616 15089 D GeofenceExitIntentService: geofence exit intent action = 2
03-15 20:58:41.130 17616 15089 D GeofenceExitIntentService: parsedEvent = null
03-15 20:58:41.133 17616 15089 E AndroidRuntime: FATAL EXCEPTION: IntentService[GeofenceExitIntentService]
03-15 20:58:41.133 17616 15089 E AndroidRuntime: Process: edu.berkeley.eecs.emission, PID: 17616
03-15 20:58:41.133 17616 15089 E AndroidRuntime: java.lang.NullPointerException: Attempt to invoke virtual method 'int com.google.android.gms.location.GeofencingEvent.getGeofenceTransition()' on a null object reference
03-15 20:58:41.133 17616 15089 E AndroidRuntime:        at edu.berkeley.eecs.emission.cordova.tracker.location.GeofenceExitIntentService.onHandleIntent(GeofenceExitIntentService.java:57)
03-15 20:58:41.133 17616 15089 E AndroidRuntime:        at android.app.IntentService$ServiceHandler.handleMessage(IntentService.java:78)
03-15 20:58:41.133 17616 15089 E AndroidRuntime:        at android.os.Handler.dispatchMessage(Handler.java:106)
03-15 20:58:41.133 17616 15089 E AndroidRuntime:        at android.os.Looper.loopOnce(Looper.java:201)
03-15 20:58:41.133 17616 15089 E AndroidRuntime:        at android.os.Looper.loop(Looper.java:288)
03-15 20:58:41.133 17616 15089 E AndroidRuntime:        at android.os.HandlerThread.run(HandlerThread.java:67)

shankari commented 1 year ago

Force killed the app on the physical phone
Foreground service was restarted in a couple of hours
Took two trips today without starting app activity; both were recorded

Everything seems to be working properly

shankari commented 1 year ago

Final check for an issue that was failing yesterday. In the background checks, we treat the optimization issue as a location failure and go to the start state. So when the user does turn on optimization, we are need to restart the FSM. Made that change and verified that it works.

https://user-images.githubusercontent.com/2423263/225798404-abf6f8b8-ef79-48ee-aee6-5b81dd192856.mov

shankari commented 1 year ago

At this point, I think that most of the high level changes are done. The pending issues are:

data handling attribution access: detects whether private data app access happens in your app or in an SDK. We don't use any closed source SDKs, so we should not care about this. Looks like if we want to use attribution tags, they have to be defined in the manifest. But do we need to use attribution tags?
Specifying android:allowBackup="false" does disable backups to Google Drive, but doesn’t disable D2D transfers for the app. (should we allow backup?)

Neither of these sounds like a showstopper. I am going to push out a staging version with all the changes so far.

shankari commented 1 year ago

I can confirm that the iOS warning no longer appears during upload I am also able to upload the android version to the play store, so the new API version is recognized

Pending tasks:

adapt to the new QR code library
check whether push notifications are still working

shankari commented 1 year ago

Ok, so this is turning out to be more complicated than I had thought. Adapting to the new QR code library is fairly easy, but it doesn't seem to work. There are no instructions, and on calling scan, the little notification that we are using the phone shows up, but we can't actually see any way to scan.

Checking the documentation on the original plugin; otherwise will have to fall back to the phonegap plugin

shankari commented 1 year ago

I looked at the documentation on the original plugin, and I needed to call show to show the video preview. But even showing the preview doesn't work. This may be because this codebase is super fancy, and opens up the video preview behind the webview. And our HTML elements are not transparent, so they cover the video.

// Make the webview transparent so the video preview is visible behind it.
QRScanner.show();
// Be sure to make any opaque HTML elements transparent here to avoid
// covering the video.

Configures the native webview to have a transparent background, then sets the background of the and DOM elements to transparent, allowing the webview to re-render with the transparent background.

I'm not going to spend time figuring out which element is not transparent and then making it be transparent (and then restoring when we stop scanning).

Switching back to the old phonegap plugin instead

e-mission / e-mission-docs

⬆️ [phone] Upgrade to the latest everything + some RFC 🎉 #838