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

Add route refresh #2570

Merged
merged 1 commit into from
Mar 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import com.mapbox.annotation.navigation.module.MapboxNavigationModule
import com.mapbox.annotation.navigation.module.MapboxNavigationModuleType
import com.mapbox.api.directions.v5.models.DirectionsRoute
import com.mapbox.api.directions.v5.models.RouteOptions
import com.mapbox.navigation.base.route.RouteRefreshCallback
import com.mapbox.navigation.base.route.Router
import com.mapbox.navigation.utils.network.NetworkStatusService
import com.mapbox.navigation.utils.thread.ThreadController
Expand Down Expand Up @@ -57,7 +58,8 @@ class MapboxHybridRouter(
* Private interface used with handler classes here to call the correct router
*/
private interface RouterDispatchInterface {
fun execute(routeOptions: RouteOptions, clientCallback: Router.Callback)
fun getRoute(routeOptions: RouteOptions, clientCallback: Router.Callback)
fun getRouteRefresh(route: DirectionsRoute, legIndex: Int, callback: RouteRefreshCallback)
}

private class RouterHandler(
Expand Down Expand Up @@ -100,19 +102,27 @@ class MapboxHybridRouter(
/**
* This method is equivalent to calling .getRoute() with the additional parameter capture
*/
override fun execute(routeOptions: RouteOptions, clientCallback: Router.Callback) {
override fun getRoute(routeOptions: RouteOptions, clientCallback: Router.Callback) {
reserveRouterCalled = false
options = routeOptions
callback = clientCallback
mainRouter.getRoute(routeOptions, this)
}

override fun getRouteRefresh(route: DirectionsRoute, legIndex: Int, callback: RouteRefreshCallback) {
mainRouter.getRouteRefresh(route, legIndex, callback)
}
}

override fun getRoute(
routeOptions: RouteOptions,
callback: Router.Callback
) {
routeDispatchHandler.get().execute(routeOptions, callback)
routeDispatchHandler.get().getRoute(routeOptions, callback)
}

override fun getRouteRefresh(route: DirectionsRoute, legIndex: Int, callback: RouteRefreshCallback) {
routeDispatchHandler.get().getRouteRefresh(route, legIndex, callback)
}

override fun cancel() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,18 @@ package com.mapbox.navigation.route.offboard
import android.content.Context
import com.mapbox.annotation.navigation.module.MapboxNavigationModule
import com.mapbox.annotation.navigation.module.MapboxNavigationModuleType
import com.mapbox.api.directions.v5.DirectionsCriteria
import com.mapbox.api.directions.v5.MapboxDirections
import com.mapbox.api.directions.v5.models.DirectionsResponse
import com.mapbox.api.directions.v5.models.DirectionsRoute
import com.mapbox.api.directions.v5.models.RouteOptions
import com.mapbox.api.directionsrefresh.v1.MapboxDirectionsRefresh
import com.mapbox.navigation.base.accounts.SkuTokenProvider
import com.mapbox.navigation.base.route.RouteRefreshCallback
import com.mapbox.navigation.base.route.RouteRefreshError
import com.mapbox.navigation.base.route.Router
import com.mapbox.navigation.route.offboard.router.routeOptions
import com.mapbox.navigation.route.offboard.routerefresh.RouteRefreshCallbackMapper
import com.mapbox.navigation.utils.exceptions.NavigationException
import retrofit2.Call
import retrofit2.Callback
Expand All @@ -32,13 +38,15 @@ class MapboxOffboardRouter(
}

private var mapboxDirections: MapboxDirections? = null
private var mapboxDirectionsRefresh: MapboxDirectionsRefresh? = null

override fun getRoute(
routeOptions: RouteOptions,
callback: Router.Callback
) {
mapboxDirections = RouteBuilderProvider.getBuilder(accessToken, context, skuTokenProvider)
.routeOptions(routeOptions)
.enableRefresh(routeOptions.profile() == DirectionsCriteria.PROFILE_DRIVING_TRAFFIC)
kmadsen marked this conversation as resolved.
Show resolved Hide resolved
.build()
mapboxDirections?.enqueueCall(object : Callback<DirectionsResponse> {

Expand Down Expand Up @@ -67,5 +75,24 @@ class MapboxOffboardRouter(
override fun cancel() {
mapboxDirections?.cancelCall()
mapboxDirections = null

mapboxDirectionsRefresh?.cancelCall()
mapboxDirectionsRefresh = null
}

override fun getRouteRefresh(route: DirectionsRoute, legIndex: Int, callback: RouteRefreshCallback) {
try {
val refreshBuilder = MapboxDirectionsRefresh.builder()
.accessToken(accessToken)
.requestId(route.routeOptions()?.requestUuid())
.legIndex(legIndex)

mapboxDirectionsRefresh = refreshBuilder.build()
mapboxDirectionsRefresh?.enqueueCall(RouteRefreshCallbackMapper(route, legIndex, callback))
} catch (throwable: Throwable) {
callback.onError(RouteRefreshError(
message = "Route refresh call failed",
throwable = throwable))
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ internal object RouteBuilderProvider {
.accessToken(accessToken)
.voiceInstructions(true)
.bannerInstructions(true)
.enableRefresh(false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a pain to find. I had to ask in #directions to figure it out
https://mapbox.slack.com/archives/C03KW1JQD/p1583881618015100

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating this feature to be enabled automatically with traffic profiles because that's the only place it's supported.
https://mapbox.slack.com/archives/C03KW1JQD/p1583883018020600?thread_ts=1583881618.015100&cid=C03KW1JQD

.voiceUnits(context.inferDeviceLocale().getUnitTypeForLocale())
.interceptor {
val httpUrl = it.request().url()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package com.mapbox.navigation.route.offboard.routerefresh

import com.mapbox.api.directions.v5.models.DirectionsRoute
import com.mapbox.api.directionsrefresh.v1.models.DirectionsRefreshResponse
import com.mapbox.navigation.base.route.RouteRefreshCallback
import com.mapbox.navigation.base.route.RouteRefreshError
import retrofit2.Call
import retrofit2.Callback
import retrofit2.Response

internal class RouteRefreshCallbackMapper(
private val originalRoute: DirectionsRoute,
private val currentLegIndex: Int,
private val callback: RouteRefreshCallback
) : Callback<DirectionsRefreshResponse> {

override fun onResponse(call: Call<DirectionsRefreshResponse>, response: Response<DirectionsRefreshResponse>) {
val routeAnnotations = response.body()?.route()
var errorThrowable: Throwable? = null
val refreshedDirectionsRoute = try {
mapToDirectionsRoute(routeAnnotations)
} catch (t: Throwable) {
errorThrowable = t
null
}
if (refreshedDirectionsRoute != null) {
callback.onRefresh(refreshedDirectionsRoute)
} else {
callback.onError(RouteRefreshError(
message = "Failed to read refresh response",
throwable = errorThrowable))
}
}

override fun onFailure(call: Call<DirectionsRefreshResponse>, t: Throwable) {
callback.onError(RouteRefreshError(throwable = t))
}

private fun mapToDirectionsRoute(routeAnnotations: DirectionsRoute?): DirectionsRoute? {
val validRouteAnnotations = routeAnnotations ?: return null
val refreshedRouteLegs = originalRoute.legs()?.let { oldRouteLegsList ->
val legs = oldRouteLegsList.toMutableList()
for (i in currentLegIndex until legs.size) {
validRouteAnnotations.legs()?.let { annotationHolderRouteLegsList ->
val updatedAnnotation = annotationHolderRouteLegsList[i - currentLegIndex].annotation()
legs[i] = legs[i].toBuilder().annotation(updatedAnnotation).build()
}
}
legs.toList()
}
return originalRoute.toBuilder().legs(refreshedRouteLegs).build()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class MapboxOffboardRouterTest : BaseTest() {
every { mockSkuTokenProvider.obtainUrlWithSkuToken("/mock", 1) } returns ("/mock&sku=102jaksdhfj")
every { RouteBuilderProvider.getBuilder(accessToken, context, mockSkuTokenProvider) } returns mapboxDirectionsBuilder
every { mapboxDirectionsBuilder.interceptor(any()) } returns mapboxDirectionsBuilder
every { mapboxDirectionsBuilder.enableRefresh(any()) } returns mapboxDirectionsBuilder
every { mapboxDirectionsBuilder.build() } returns mapboxDirections
every { mapboxDirections.enqueueCall(capture(listener)) } answers {
callback = listener.captured
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import com.mapbox.api.directions.v5.models.DirectionsResponse
import com.mapbox.api.directions.v5.models.DirectionsRoute
import com.mapbox.api.directions.v5.models.RouteOptions
import com.mapbox.navigation.base.options.MapboxOnboardRouterConfig
import com.mapbox.navigation.base.route.RouteRefreshCallback
import com.mapbox.navigation.base.route.Router
import com.mapbox.navigation.base.route.internal.RouteUrl
import com.mapbox.navigation.navigator.MapboxNativeNavigator
Expand Down Expand Up @@ -114,6 +115,10 @@ class MapboxOnboardRouter(
mainJobControl.job.cancelChildren()
}

override fun getRouteRefresh(route: DirectionsRoute, legIndex: Int, callback: RouteRefreshCallback) {
// Does nothing
}

private fun retrieveRoute(url: String, callback: Router.Callback) {
mainJobControl.scope.launch {
try {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package com.mapbox.navigation.base.route

import com.mapbox.api.directions.v5.models.DirectionsRoute

interface RouteRefreshCallback {
fun onRefresh(directionsRoute: DirectionsRoute)

fun onError(error: RouteRefreshError)
}

data class RouteRefreshError(
val message: String? = null,
val throwable: Throwable? = null
)
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,19 @@ interface Router {
*/
fun cancel()

/**
* Refresh the traffic annotations for a given [DirectionsRoute]
*
* @param route DirectionsRoute the direction route to refresh
* @param legIndex Int the index of the current leg in the route
* @param callback Callback that gets notified with the results of the request
*/
fun getRouteRefresh(
route: DirectionsRoute,
legIndex: Int,
callback: RouteRefreshCallback
)

/**
* Callback for Router fetching
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import com.mapbox.navigation.core.directions.session.RoutesRequestCallback
import com.mapbox.navigation.core.fasterroute.FasterRouteController
import com.mapbox.navigation.core.fasterroute.FasterRouteObserver
import com.mapbox.navigation.core.module.NavigationModuleProvider
import com.mapbox.navigation.core.routerefresh.RouteRefreshController
import com.mapbox.navigation.core.telemetry.MapboxNavigationTelemetry
import com.mapbox.navigation.core.telemetry.MapboxNavigationTelemetry.TAG
import com.mapbox.navigation.core.telemetry.events.TelemetryUserFeedback
Expand Down Expand Up @@ -136,6 +137,7 @@ constructor(
private val internalRoutesObserver = createInternalRoutesObserver()
private val internalOffRouteObserver = createInternalOffRouteObserver()
private val fasterRouteController: FasterRouteController
private val routeRefreshController: RouteRefreshController

private var notificationChannelField: Field? = null
private val MAPBOX_NAVIGATION_NOTIFICATION_PACKAGE_NAME =
Expand Down Expand Up @@ -192,6 +194,8 @@ constructor(
}

fasterRouteController = FasterRouteController(directionsSession, tripSession)
routeRefreshController = RouteRefreshController(directionsSession, tripSession)
routeRefreshController.start()
}

/**
Expand Down Expand Up @@ -289,6 +293,7 @@ constructor(
tripSession.unregisterAllBannerInstructionsObservers()
tripSession.unregisterAllVoiceInstructionsObservers()
fasterRouteController.stop()
routeRefreshController.stop()
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package com.mapbox.navigation.core.directions.session

import com.mapbox.api.directions.v5.models.DirectionsRoute
import com.mapbox.api.directions.v5.models.RouteOptions
import com.mapbox.navigation.base.route.RouteRefreshCallback
import com.mapbox.navigation.base.route.Router

internal interface DirectionsSession {
Expand All @@ -22,6 +23,8 @@ internal interface DirectionsSession {
*/
fun requestFasterRoute(adjustedRouteOptions: RouteOptions, routesRequestCallback: RoutesRequestCallback)

fun requestRouteRefresh(route: DirectionsRoute, legIndex: Int, callback: RouteRefreshCallback)

fun cancel()

fun registerRoutesObserver(routesObserver: RoutesObserver)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package com.mapbox.navigation.core.directions.session
import com.mapbox.api.directions.v5.models.DirectionsRoute
import com.mapbox.api.directions.v5.models.RouteOptions
import com.mapbox.navigation.base.extensions.ifNonNull
import com.mapbox.navigation.base.route.RouteRefreshCallback
import com.mapbox.navigation.base.route.Router
import java.util.concurrent.CopyOnWriteArrayList

Expand Down Expand Up @@ -33,6 +34,10 @@ class MapboxDirectionsSession(
router.cancel()
}

override fun requestRouteRefresh(route: DirectionsRoute, legIndex: Int, callback: RouteRefreshCallback) {
router.getRouteRefresh(route, legIndex, callback)
}

override fun requestRoutes(
routeOptions: RouteOptions,
routesRequestCallback: RoutesRequestCallback?
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package com.mapbox.navigation.core.routerefresh

import android.util.Log
import com.mapbox.api.directions.v5.DirectionsCriteria
import com.mapbox.api.directions.v5.models.DirectionsRoute
import com.mapbox.navigation.base.route.RouteRefreshCallback
import com.mapbox.navigation.base.route.RouteRefreshError
import com.mapbox.navigation.core.directions.session.DirectionsSession
import com.mapbox.navigation.core.trip.session.TripSession
import com.mapbox.navigation.utils.timer.MapboxTimer
import java.util.concurrent.TimeUnit
import kotlinx.coroutines.Job

/**
* This class is responsible for refreshing the current direction route's traffic.
* This does not support alternative routes.
*
* If the route is successfully refreshed, this class will update the [TripSession.route]
*
* [start] and [stop] are attached to the application lifecycle. Observing routes that
* can be refreshed are handled by this class. Calling [start] will restart the refresh timer.
*/
internal class RouteRefreshController(
private val directionsSession: DirectionsSession,
private val tripSession: TripSession
) {
private val routerRefreshTimer = MapboxTimer()

init {
routerRefreshTimer.restartAfterMillis = TimeUnit.MINUTES.toMillis(5)
}

fun start(): Job {
stop()
return routerRefreshTimer.startTimer {
val route = tripSession.route?.takeIf { supportsRefresh(it) }
route?.let {
val legIndex = tripSession.getRouteProgress()?.currentLegProgress()?.legIndex() ?: 0
directionsSession.requestRouteRefresh(
route,
legIndex,
routeRefreshCallback)
}
}
}

fun stop() {
routerRefreshTimer.stopJobs()
}

private fun supportsRefresh(route: DirectionsRoute?): Boolean {
val isTrafficProfile = route?.routeOptions()
?.profile()?.equals(DirectionsCriteria.PROFILE_DRIVING_TRAFFIC)
return isTrafficProfile == true
}

private val routeRefreshCallback = object : RouteRefreshCallback {

override fun onRefresh(directionsRoute: DirectionsRoute) {
Log.i("RouteRefresh", "Successful refresh")
tripSession.route = directionsRoute
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 a good idea? Wondering if we should implement something similar to faster route in which we expose the route to the client and it's up to them to use it or not, instead of us always doing it internally 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update the MapboxDirectionsSession#routes[0] too 🤔? Actually, updating the MapboxDirectionsSession#routes will end up updating MapboxTripSession#route 👀

private fun createInternalRoutesObserver() = object : RoutesObserver {
override fun onRoutesChanged(routes: List<DirectionsRoute>) {
if (routes.isNotEmpty()) {
tripSession.route = routes[0]
} else {
tripSession.route = null
}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Also noticed that we're updating the whole route in NN

instead of only the annotations as we do in the legacy 👀 RouteHandler. Do you know if this has a performance impact? Probably yes because it's not the same to serialize the whole route rather than only updating the annotations of the current route.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looking into update the annotations only now, i did try before but that was early on.

considered performance impacts and considered it minor since it's per minutes (opposed to per seconds)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, that's not correct 👀

} else {
route.legs()?.let { routeLegs ->
for (i in routeLegs.indices) {
routeLegs[i].annotation()?.toJson()?.let { annotationJson ->
mapboxNavigator.updateAnnotations(annotationJson, INDEX_FIRST_ROUTE, i)
}
}
}
}
if it's not a NEW_ROUTE we're only updating the annotations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah discussed offline, now see how fresh_route causes the condition in RouteHandler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When attempting to only update annotations in NN, it didn't update the the traffic on the map.

val directionsSessionRoutes = directionsSession.routes.toMutableList()
if (directionsSessionRoutes.isNotEmpty()) {
directionsSessionRoutes[0] = directionsRoute
directionsSession.routes = directionsSessionRoutes
}
}

override fun onError(error: RouteRefreshError) {
if (error.throwable != null) {
Log.e("RouteRefresh", error.message, error.throwable)
} else {
Log.e("RouteRefresh", error.message)
}
}
}
}