ably / ably-asset-tracking-android Goto Github PK

The last known location is used in the Publisher SDK as a starting point of a trip when setting tracking destination.
We are currently setting the location as the raw location. However, it might turn out that the enhanced location is a better choice. Or we can try to use both locations but this will require us to handle asynchronous updates of the same field.

┆Issue is synchronized with this Jira Task by Unito

We could think about reducing the number of fields (particularly maps and sets) that are currently in the DefaultPublisher
#97 (comment)
One solution could be to add Resolution to the Subscriber which should make some fields unnecessary.

┆Issue is synchronized with this Jira Task by Unito

Currently, we're using @Synchronized when starting and stopping location updates but this can lead to some deadlocks. This should be improved.

As discussed in Slack.

┆Issue is synchronized with this Jira Task by Unito

Prompted by: #114 (comment)

┆Issue is synchronized with this Jira Task by Unito

This issue centres around the method called (at the time of me writing this) performStopPublisher() in the DefaultPublisher internal class.

The first task, before we writing any code, is to to define what a correct shutdown procedure should be for a publisher instance.

It may be preferable to go as far as changing the signature for the stop() method on Publisher so that it takes callback routes for success and error.

This is as much a story around how we ensure integration testing can be predictable (reliable).

┆Issue is synchronized with this Jira Task by Unito

They will be needed by the majority of app developers, in particular connection state change monitoring.

There is no advantage in hanging them off an intermediary interface - they can all be supplied directly to the builder.

┆Issue is synchronized with this Jira Task by Unito

Overview

This issue proposes updates to the API for the initial release of these SDKs. Its focus is the publisher API; the subscriber API is simpler, and should follow naturally once the publisher API concepts are finalised.

API concepts

AssetPublisher

This is the entity that is constructed and typically survives for the lifetime of a publisher app. It maintains the Ably connection, and makes use of NavSDK resources as required. Its principal attributes, fixed for the lifetime of the AssetPublisher instance, include the AblyConfig (which in turn includes clientId) plus any other fixed metadata about the publisher.

Asset

This naming is provisional - we might want this to be Delivery or Track or whatever. The idea is that this represents a period in which a specific asset is tracked and its position broadcast. But given the other terminology already adopted I think Asset is probably the best fit.

Typically an Asset will be a single delivery; but could also include a rider: for example, in the use case of tracking rider locations when not on a delivery, for the purposes of assigning deliveries.

An Asset has a unique ID and fixed metadata.

An Asset can have multiple subscribers, which can vary over the lifetime of the Asset. The rights that permit an individual subscriber to view an in-progress Asset are valid for the lifetime of the Asset.

An Asset has a destination location, but this can be varied during the active time of the Asset.

An Asset corresponds to a single Ably channel, and the Ably channel name is canonically derived from the Asset ID.

An Asset has a lifecycle, in that publishing on the channel begins when the Asset is created, and stops when the Asset ends.

An AssetPublisher, at any given time, may have multiple Assets active, and the present location of the AssetPublisher may be published on the channel for each Asset.

The Assets associated with an AssetPublisher are ordered naturally as a result of the order of their creation. Assets may be ended out of order (eg in the case that a delivery is cancelled, for example).

Asset location updates: determining location resolution

Tracking is performed against a specification of the tracking Resolution - this captures the positional accuracy and update frequency. A Resolution captures the following parameters:

• priority: loosely analogous to Android’s LocationRequest.priority (https://developers.google.com/android/reference/com/google/android/gms/location/LocationRequest#setPriority(int)); this captures the requirement for positional accuracy - or the expense to which the publisher is willing to go to obtain accuracy - and will usually determine the sensor(s) used to obtain the location.

• maxDelay: this is an indication of the target currency or staleness of a location update, expressed in milliseconds. It is used to govern the frequency of updates requested from the underlying location provider, and the frequency of messages broadcast to subscribers.

• minDisplacement: this is an indication of the positional granularity required. Location updates whose position differs from the last known position by a distance smaller than the minDisplacement will not be broadcast. The minDisplacement is used to configure the underlying location provider and also to filter the broadcast of updates.

The publisher obtains location updates from the local platform, and publishes those to the Asset, based on a current targetResolution. The targetResolution governs the accuracy of location updates (for example by determining the sensors used) and the frequency of location updates being published. The publisher specifies the targetResolution for each Asset; it can be specified on construction of the Asset, and can vary it during the active lifetime of a Asset.

The aim in most practical situations is for targetResolution to vary dynamically based on circumstances; this might be based on parameters known by the publisher (such as proximity to destination, or the current power state of the device) but may also be based on the existence, or preferences, of the subscribers to the track. The tracking SDKs therefore incorporate the following additional concepts:

• subscriber requestedResolution: this captures the preference of a subscriber for update resolution. A subscriber’s preference might be a consequence of the subscriber simply existing, but might also depend on the subscriber’s circumstances (eg the zoom level of the map that the subscriber is using the display updates).

• ResolutionPolicy: this captures the strategy by which the various Resolution requests/preferences are translated into a targetResolution. By providing a ResolutionPolicy a publisher specifies the rules that govern how the targetResolution should be updated in response to changing circumstances. A ResolutionPolicy may be specified on construction of an Asset.

If a subscriber wishes to indicate a requestedResolution, then this is done via the Subscriber API; the effect of this is to entering presence on the Asset channel and providing the requestedResolution in its presence data. The publisher’s ResolutionPolicy can obtain occupancy, requestedResolutions, and use any other strategy, to update the targetResolution as required.

Impact on current API

The most obvious changes that would be required to the existing API under this proposal are:

• changes to terminology;

• creation of an Asset as a distinct interface, with a Builder; moving some of the current AssetPublisher.Builder options into Asset.Builder; addition of APIs on the AssetPublisher to add and remove Assets.

• the current BatteryConfiguration, associated with an AssetPublisher, will become ResolutionPolicy and be associated with an Asset.

• assetMetadataJson / tripMetadataJson: I think we will need freely structured metadata associated with the AssetPublisher, and freely structured metadata associated with the Asset.

• delivery: these will become associated with an Asset. destination should be structured as lat/long. There is no need for destination.trackingId - this will be the Asset.id - and other details of the delivery (eg a transaction Id) would be part of the Asset metadata.

Scope of preview releases

I propose that the Asset and Asset.Builder API are created now. For Preview 1 then perhaps we do not include the addAsset / removeAsset APIs in AssetPublisher but instead allow the AssetPublisher.Builder to be provided with an Asset.Builder. Preview 2 can include the ability to support multiple concurrently-active Assets.

Implementation of Resolution and some simple ResolutionPolicys need to be included in Preview 2.

┆Issue is synchronized with this Jira Task by Unito

At a minimum we should be using checkstyle, and perhaps consider creating a central Ably ruleset so we can carry it over to use in other Ably repositories.

#123 (comment)

Eventually we will need a brand new project for test utilities as neither core-sdk (is api for SDKs) or the forthcoming common-sdk (will be implementation for SDKs) are appropriate for testImplementation or androidTestImplementation dependency purposes

┆Issue is synchronized with this Jira Task by Unito

See this comment.

┆Issue is synchronized with this Jira Task by Unito

We have two fields for locations lastSentRaw and lastSentEnhanced and they're holding the value of the last location that was received from the location engine. Those values are updated each time a new location is received. Then those values are used to check if we should send location updates via Ably depending on the Trackable's resolution. This is wrong and could lead to no updates being sent for certain Trackables depending on their resolution.

We should create a collection of last sent locations for each of the Trackables, so we will be able to tell if the new location should be sent to Trackable subscribers.

We should keep in mind the issue #103 when solving this issue.

┆Issue is synchronized with this Jira Task by Unito

Prompted by @paddybyers' questions here.

My gut feeling is that if we have a correct and clear error path back to the app developer then our SDK should not also report errors via Timber.

However, I am sympathetic to the view that repetition is also helpful in terms of in-the-field debugging.

We need to debate. And then once we've decided conform our implementation for consistency and document this as part of the SDK contract.

┆Issue is synchronized with this Jira Task by Unito

I can't recall why I chose JUnit 4 initially for this project. I've just read: Best Practices for Unit Testing in Kotlin

• it seems to make many good suggestions, but is a few years old now, so my own musings as a result of reading that article are:
• JUnit 5 is still a Java-first testing framework and I wonder if it's just shoehorned in "for old time's sake"
• Kotest might be something to consider instead of JUnit
• kotlin.test should also, perhaps, be part of the picture here? How does that mix with or replace the other options?

There is no compelling reason I am aware of for us to replace JUnit 4 beyond the fact that it is old and we'll be writing a lot of tests tied to its APIs. However...

It certainly seems that other test frameworks will be Kotlin-first by design, allowing tests to be written more idiomatically.

The line of Kotlin test code that drove me to create this issue looked like this:

Assert.assertEquals(30F, batteryPercentage)

It is using assertEquals(Object, Object) which is presumably because batteryPercentage is a Kotlin Float and still presented as an object for test compilation purposes, despite being non-optional. My expectation would have been that assertEquals(double, double, doube) would have been the more appropriate assertion to use for floats, as we should be specifying a delta. We're not getting any warning or other indication that this would be preferable.

Implementation detail but it also needs documenting.

As raised by @kavalerov here.

┆Issue is synchronized with this Jira Task by Unito

To match the interfaces in the publishing-sdk (i.e. the subscribing-sdk currently, incorrectly, has AssetSubscriber and DefaultAssetSubscriber, where the publishing-sdk has dropped the Asset suffix for equivalents Publisher and DefaultPublisher.

┆Issue is synchronized with this Jira Task by Unito

In order to shrink the final SDK file and remove unused methods from it, we should prepare a Proguard configuration and use it when creating the release version of the SDK.

┆Issue is synchronized with this Jira Task by Unito

As referenced by @paddybyers here.

We need to decide on a new name for this.

┆Issue is synchronized with this Jira Task by Unito

We should decide which fields are required to build a Publisher and a Subscriber, and which can be optional.

┆Issue is synchronized with this Jira Task by Unito

e.g. Ably configuration and log configuration, but there will also be other fundamental primitives.

┆Issue is synchronized with this Jira Task by Unito

A good example here of using the aapt tool to dump badging, all wrapped up in a lovely Gradle task. Nice! 😁

Looking at the build-tools folder I can see there is an aapt2 which should probably be our preferred choice, and appears to do the same thing with the same arguments, just vending more information. e.g.:

ably-asset-tracking-android % $ANDROID_HOME/build-tools/30.0.2/aapt2 dump badging subscribing-example-app/build/outputs/apk/release/subscribing-example-app-release-unsigned.apk | grep '^package:'
package: name='com.ably.tracking.example.subscriber' versionCode='1' versionName='1.0.0-preview.1' platformBuildVersionName='11' platformBuildVersionCode='30' compileSdkVersion='30' compileSdkVersionCodename='11'

I've only had a quick play with this and proven it against one of our apk files.
I'm yet to find the equivalent tool and/or incantation to do it for aar files.

Currently, we're enabling the library user to simulate the phone location and listen for debug specific events. We should decide whether this should be kept in the final version of the SDK.

┆Issue is synchronized with this Jira Task by Unito

I've discussed this with @paddybyers and we've agreed to start asset tracking SDK error codes from 100,000, therefore part of completing this issue needs to involve opening a PR against ably-common/protocol/errors.json.

In terms of how this is implemented as an interface, perhaps the ErrorInformation data class we've introduced in #114 can have a new method or getter which returns a member of a local enum which contains all the possible local errors which can be generated by this SDK (or the SDKs, perhaps). If the code is not known to the local implementation then that enum could have a member called something like EXTERNAL or perhaps SERVICE to indicate that the code needs to be inspected based on Ably codes external to asset tracking SDKs.

Seems to be going to a plain text file which then has to be separately opened. Whereas the Android lint is emitting to console so it's more accessible, especially in CI.

e.g.:

ably-asset-tracking-android % ./gradlew clean check

> Task :publishing-sdk:ktlintMainSourceSetCheck FAILED
/Users/quintinwillison/code/ably/ably-asset-tracking-android/publishing-sdk/src/main/java/com/ably/tracking/publisher/DefaultAssetPublisher.kt:5:1: Unused import (no-unused-imports)
"plain" report written to /Users/quintinwillison/code/ably/ably-asset-tracking-android/publishing-sdk/build/reports/ktlint/ktlintMainSourceSetCheck/ktlintMainSourceSetCheck.txt

> Task :publishing-sdk:lint
Ran lint on variant debug: 2 issues found
Ran lint on variant release: 2 issues found
/Users/quintinwillison/code/ably/ably-asset-tracking-android/publishing-sdk/src/main/java/com/ably/tracking/publisher/DefaultAssetPublisher.kt:73: Information: TODO call found; points to code which must be fixed prior to release [StopShip]
        TODO()
        ~~~~~~
/Users/quintinwillison/code/ably/ably-asset-tracking-android/publishing-sdk/build.gradle:10: Information: A newer version of com.mapbox.mapboxsdk:mapbox-android-sdk than 9.5.0 is available: 9.6.0 [GradleDependency]
    implementation 'com.mapbox.mapboxsdk:mapbox-android-sdk:9.5.0'
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
0 errors, 0 warnings

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':publishing-sdk:ktlintMainSourceSetCheck'.
> A failure occurred while executing org.jlleitschuh.gradle.ktlint.KtLintWorkAction
   > Process 'command '/Users/quintinwillison/.asdf/installs/java/adoptopenjdk-8.0.272+10/bin/java'' finished with non-zero exit value 1

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.7.1/userguide/command_line_interface.html#sec:command_line_warnings

BUILD FAILED in 9s
92 actionable tasks: 91 executed, 1 up-to-date

┆Issue is synchronized with this Jira Task by Unito

Prompted by this commit via #60 (comment).

There are a lot more fields on ErrorInfo that we could encapsulate. Perhaps somewhat opaquely - e.g. a map with string keys which could change over time without changing the asset tracking SDK APIs.

┆Issue is synchronized with this Jira Task by Unito

Our implicit default should remain to automatically post events to the Looper running on the main / UI thread.

However, we need to add an override point in the API which allows app developers to supply an alternative Looper if they are concerned that our inbound events might make their main loop / queue too busy.

┆Issue is synchronized with this Jira Task by Unito

see #93 (comment)

┆Issue is synchronized with this Jira Task by Unito

At the moment only AssetPublisher and AssetSubscriber builders and implementations are marked as internal and aren't visible outside of the SDK. We should decide which objects should be public and which should be internal (i.e. GeoJsonMessage).

┆Issue is synchronized with this Jira Task by Unito

New home: https://github.com/ably/ably-asset-tracking-common

In response to: #123 (review)

• Geo test assets should be in a portable format (GeoJSON).
• Once moved to common they should be validated for correctness there.

Currently I'm looking at a line that reads:

showToast("Insert tracking ID")

Which is executed when the user has clicked the button in the app to start tracking but hasn't filled in required information.

A dialog that remains on screen until a button acknowledging the user has read it would be better. i.e. a classic alert.

┆Issue is synchronized with this Jira Task by Unito

Triggered by the observation that we currently import interfaces from the io.ably.lib and com.mapbox packages into the publishing SDK's ConfigurationModels.kt file, which is all public API from our user's perspective. However, Ably Realtime and MapBox are implementation details.

┆Issue is synchronized with this Jira Task by Unito

see #68 (comment)

┆Issue is synchronized with this Jira Task by Unito

spawned by: #97 (comment)

Deferred to this issue so we can unblock that PR.

┆Issue is synchronized with this Jira Task by Unito

I've not had time yet to investigate this properly, so I am creating this issue so we don't forget to discuss it.

My concern relates to the type aliases defined in both SDK APIs:

ably-asset-tracking-android % git grep '^typealias.*Listener\s*=\s*'
publishing-sdk/src/main/java/com/ably/tracking/publisher/Publisher.kt:typealias LocationUpdatedListener = (Location) -> Unit
subscribing-sdk/src/main/java/com/ably/tracking/subscriber/Subscriber.kt:typealias LocationUpdatedListener = (Location) -> Unit
subscribing-sdk/src/main/java/com/ably/tracking/subscriber/Subscriber.kt:typealias StatusListener = (Boolean) -> Unit

At a minimum we need some Java code that exercises these interfaces by implementing them.

┆Issue is synchronized with this Jira Task by Unito

On iOS, we're subscribing and asking for location updates as soon as we start tracking any Trackable (https://github.com/ably/ably-asset-tracking-cocoa/blob/main/Publisher/Sources/DefaultPublisher.swift#L41)
but on Android, it's started directly in Publisher's constructor (https://github.com/ably/ably-asset-tracking-android/blob/main/publishing-sdk/src/main/java/com/ably/tracking/publisher/DefaultPublisher.kt#L137)

Behavior should be unified, but I also think that we should use location updates only when we have any trackable available. It sounds like a minor thing but would be a nice battery optimization.

┆Issue is synchronized with this Jira Task by Unito

Starting location tracking requires us to be on the main thread. Currently, we're only handling the happy path, but we should also handle the scenario in which the AssetPublisher is called from a background thread.

┆Issue is synchronized with this Jira Task by Unito

See #43 (comment).

┆Issue is synchronized with this Jira Task by Unito

override fun resolve(request: TrackableResolutionRequest): Resolution

If the intention was to implement the logic from here: https://github.com/ably/ably-asset-tracking-android/pull/84/files

then this method is currently not doing that.

More specifically, this method right now is doing very simplistic fallback logic, where essentially if there is no resolution provided for given state, it will just disregard other resolution constraints. This is also related to the fact that I believe that DefaultResolutionSet class members should be optionals - user may provide some of them, may provide all of them, etc.

I think this is something we can address in the future, not neceserally in this PR

Originally posted by @kavalerov in #97 (comment)

┆Issue is synchronized with this Jira Task by Unito

See this comment.

┆Issue is synchronized with this Jira Task by Unito

We're most of the way there, in that our APIs have documentary commentary already.

I want to surface that commentary as visible in a browser (HTML) setting.

Needs to come out of a workflow (either existing or new) as an artifact at a minimum, but ideally to be hosted somewhere also - in such a way that we can view API docs output for any given PR before it lands to main.

See: Documenting Kotlin Code with KDoc and Dokka.

┆Issue is synchronized with this Jira Task by Unito

Currently, there are duplicated classes in subscribing and publishing SDKs. Since we'll have a core module that will be shared between both SDKs (#53) those duplicate elements should be moved to that core module.

┆Issue is synchronized with this Jira Task by Unito

Trigger: #49 (comment)

┆Issue is synchronized with this Jira Task by Unito

Mainly relates to the time when this repository opens up for public access.

We need to commit to which ecosystems we're going to release to as this will spawn work to make sure we have the infrastructure in place to automate those releases.

This MUST include working out how the MapBox SDK dependency fits into the picture, bearing in mind that it currently requires a downloads token. Presumably we have to push that requirement, in terms of pre-configuration of any "user repository", out to our SDK users? Is that a barrier in terms of developer experience for adoption? (FYI @Ugbot @paddybyers @kavalerov)

┆Issue is synchronized with this Jira Task by Unito

Repository Name: ably-asset-tracking-android

┆Issue is synchronized with this Jira Bug by Unito

Currently, we're only handling the happy path. We should also properly handle other paths and think about how to notify the SDK user about those errors.

┆Issue is synchronized with this Jira Task by Unito

As pointed out by @KacperKluk, this is required to make releases from the GitHub workflow where the S3 functionality works.

┆Issue is synchronized with this Jira Task by Unito

Currently, we're using @Synchronized when starting and stopping location updates but this can lead to some deadlocks. This should be improved.

As discussed in Slack.

┆Issue is synchronized with this Jira Task by Unito

@paddybyers and I had a discussion around this today so I wanted to capture what Paddy described to me so we don't lose the requirement. We need to allow app developers to be aware of changes in channel state (i.e. the Ably channel that is being used for their tracked asset to publish to). Specifically:

• It's possible that the key being used to connect to the Ably service does not have permission for this channel - which means the channel would move into a failed state.
• It's possible for channels to spontaneously detach.

┆Issue is synchronized with this Jira Task by Unito

Currently, all code that's inside the core module is being public for the SDK user. Some of the files aren't meant to be exposed to the SDK user. In order to hide them, we'll create another gradle module called common that will be then added to SDKs via the implementation, so it will be visible only inside the SDKs. All code currently present in the core module's common package should be moved to that new private module.

┆Issue is synchronized with this Jira Task by Unito

Not within the scope of this PR, as we need to get it landed, but perhaps @KacperKluka you could consider reflecting this somewhere in the user interface?

These example apps need to present best practice given the tools we're giving app developers. We're effectively giving them the ability to reflect that a Trackable item has been "queued" for tracking (pending) and then has transitioned to being "actively tracked". Perhaps orange dot to green dot in LED terms. 😁

Originally posted by @QuintinWillison in #60 (comment)

┆Issue is synchronized with this Jira Task by Unito

This came out of a call @paddybyers and I had, where he pointed out that the subclass of ResolutionConstraints, and its supporting data structures, more logically belong to policies (and, obviously, the DefaultResolutionPolicyFactory which is reliant upon them and to which they are somewhat tied).

So, at a minimum they could perhaps move out of ConfigurationModels.kt.

┆Issue is synchronized with this Jira Task by Unito