Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try to reduce time taken to get a trackable online #481

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented Dec 1, 2022

I noticed that on a stationary device, or on a simulator set to simulate a static location, the first trackable added would come online almost immediately, but subsequent ones would not come online or sometimes would come online after a long delay.

This is because a trackable will not come online until, after the trackable is added, we receive a location update from LocationService. The frequency of location updates from PassiveLocationManager is determined by that of CLLocationManager, which usually only emits location updates when the device has moved a sufficient distance (where “sufficient” is determined by the publisher’s resolution policy).

So, we provide a mechanism for the publisher to request a location update from CLLocationManager independently of the device’s motion, and we request a location update when any trackable (except for the first) is added.

It’s probably worth also explaining how Android behaves in this scenario and why. My notes here are based on the codebase at commit ably/ably-asset-tracking-android@3a0d833.

There are two different things that happen in the Android asset tracking SDK, either of which has the side effect of causing the publisher to receive a location update when a new trackable is added:

  1. The publisher calls DefaultMapbox.changeResolution, which in turn ends up removing and re-adding all of (Google SDK) FusedLocationProviderClient’s location observers (via its requestLocationUpdates method). The documentation for FusedLocationProviderClient.requestLocationUpdates) implies that observers will always receive a location update after this method is called (although it might be an old location).

  2. (Only in the case where the added trackable is set as the active trackable – i.e. when it is added using .track and not .add)

    The publisher calls DefaultMapbox.clearRoute, which calls (Mapbox SDK) MapboxNavigation.setNavigationRoutes. Through some process that I haven’t looked into, this ends up calling (Mapbox SDK) MapboxTripSession.kt’s navigatorObserver’s onStatus, which makes an explicit call to updateLocationMatcherResult, which emits a location update.

I think that the solution I’ve implemented for iOS turns Android side effect 1 into an explicit behaviour.

Closes #425.

@lawrence-forooghian lawrence-forooghian force-pushed the 425-only-first-trackable-added-comes-online-when-stationary branch from 31986d9 to 0d913bf Compare December 1, 2022 13:34
@github-actions github-actions bot temporarily deployed to staging/pull/481/jazzy December 1, 2022 13:53 Inactive
@lawrence-forooghian lawrence-forooghian marked this pull request as ready for review December 1, 2022 14:10
Copy link
Contributor

@KacperKluka KacperKluka left a comment

Choose a reason for hiding this comment

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

Overall I like the idea and it's great that you've spotted and fixed it 🙇 I just have a few questions about the stopping and starting of location updates 😉

Comment on lines +80 to +83
locationManager.systemLocationManager.stopUpdatingLocation()
locationManager.systemLocationManager.startUpdatingLocation()
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder how this affects the Mapbox SDK 🤔

  1. Have you checked whether this doesn't break things like location history recording?
  2. Will there be a big delay in location updates between the "stop" and "start" operations?
  3. Additionally, do we know what happens when you stop and start location updates? Will it consume a lot of resources to do that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are all good questions, I'll look into it and get back to you 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added a script for analysing the logs from the publisher example app, to find out if the frequencies of location updates and recorded locations is affected. My brief investigation using the iOS simulator suggests that they are not. However, I want to test on a device too, and for that we'll probably need to add the ability to log to a file. Once I've done that and gathered some results from a device, I'll post my findings here in detail.

As for resources (which I'm mainly taking to mean battery life), I do not know what the impact would be. I could have a think about how to investigate it, but I don't have any ideas off the top of my head.

@lawrence-forooghian lawrence-forooghian force-pushed the 425-only-first-trackable-added-comes-online-when-stationary branch from 0d913bf to 056cc03 Compare December 14, 2022 17:32
@github-actions github-actions bot temporarily deployed to staging/pull/481/jazzy December 14, 2022 17:51 Inactive
Copy link
Contributor

@ikbalkaya ikbalkaya left a comment

Choose a reason for hiding this comment

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

The code looks good to me. Like Kacper I also would be interested to know whether stopping / starting location service might have side effects on other areas

I noticed that on a stationary device, or on a simulator set to simulate
a static location, the first trackable added would come online almost
immediately, but subsequent ones would not come online or sometimes
would come online after a long delay.

This is because a trackable will not come online until, after the
trackable is added, we receive a location update from LocationService.
The frequency of location updates from PassiveLocationManager is
determined by that of CLLocationManager, which usually only emits
location updates when the device has moved a sufficient distance (where
“sufficient” is determined by the publisher’s resolution policy).

So, we provide a mechanism for the publisher to request a location
update from CLLocationManager independently of the device’s motion, and
we request a location update when any trackable (except for the first)
is added.

It’s probably worth also explaining how Android behaves in this scenario
and why. My notes here are based on the codebase at commit 3a0d833.

There are two different things that happen in the Android asset tracking
SDK, either of which has the side effect of causing the publisher to
receive a location update when a new trackable is added:

1. The publisher calls DefaultMapbox.changeResolution, which in turn
   ends up removing and re-adding all of (Google SDK)
   FusedLocationProviderClient’s location observers (via its
   requestLocationUpdates method). The documentation for
   FusedLocationProviderClient.requestLocationUpdates implies that
   observers will always receive a location update after this method is
   called (although it might be an old location).

2. (Only in the case where the added trackable is set as the active
   trackable – i.e. when it is added using .track and not .add)

   The publisher calls DefaultMapbox.clearRoute, which calls (Mapbox
   SDK) MapboxNavigation.setNavigationRoutes. Through some process that I
   haven’t looked into, this ends up calling (Mapbox SDK)
   MapboxTripSession.kt’s navigatorObserver’s onStatus, which makes an
   explicit call to updateLocationMatcherResult, which emits a location
   update.

I think that the solution I’ve implemented for iOS turns Android side
effect 1 into an explicit behaviour.

Closes #425.
I need this logging to help me analyse the impact of the change in
5429b8d.
This is intended to help me analyse the impact of the change in 5429b8d.
@lawrence-forooghian lawrence-forooghian force-pushed the 425-only-first-trackable-added-comes-online-when-stationary branch from 056cc03 to 8463c90 Compare December 19, 2022 12:11
@lawrence-forooghian
Copy link
Collaborator Author

Thanks for the review, @ikbalkaya. I've just posted a reply to Kacper's questions, about the current state of my findings: #481 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Only the first trackable added to the publisher emits a didChangeConnectionState:forTrackable: callback
3 participants