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 support for NN startNavigationSession/stopNavigationSession methods #4435

Merged
merged 2 commits into from Apr 27, 2023

Conversation

kried
Copy link
Contributor

@kried kried commented Apr 25, 2023

Description

Closes NAVIOS-983

Implemented logic is:

  • iOS SDK calls startNavigationSession if the session was not started yet, and either an instance of PassiveLocationManager is created (free drive) or the callback of Navigator.setRoutesData was called (active guidance)
  • it calls stopNavigationSession when no more session running (could be deinitialization of PassiveLocationManager or Navigator.setRoutesData(null))
  • it calls storeNavigationSession before switching to offline or switching back to online. And SDK calls restoreNavigationSessionForState when switching finished

@kried kried requested a review from a team April 25, 2023 22:39
@kried kried self-assigned this Apr 25, 2023
@github-actions github-actions bot requested review from S2Ler and Udumft April 25, 2023 22:39
Sources/MapboxCoreNavigation/CoreNavigationNavigator.swift Outdated Show resolved Hide resolved
@@ -363,6 +365,7 @@ final class Navigator: CoreNavigator {

deinit {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we hook NavigationSessionManager to the Navigator lifecycle? It uses weak references for shared instance and thus we can detect when it is created and deallocated. Since start/stop calls seems to directly match NN Navigator lifetime it seems correct. Also, this will also save us from spreading this start/stop logic across Navigator consumers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree It would be much easier.
But it could lead to incorrect Telemetry reports. If start is called before the route is set to the NN Navigator, NN would first report fake navigation.freeDrive.start and navigation.freeDrive.stop events. Because NN doesn't know that navigator was created for active navigation until it finishes setRoutesData

So we later won't be able to differentiate actual free drive events from these fake events.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can detect this logic by probing Navigator.updateLocation(_:completion:) and Navigator.setRoutes. If location is updated before route is set - that's a free drive. If routes are set - we are entering AG and should end FD if any.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed that with the latest NN version, the NN stores the last reported location and reports free drive after Navigator.setRoutes(nil) even without the next call of Navigator.updateLocation(_:completion:).
So we need to stop the session if there are no free drive running in order to avoid fake free drive events.

@kried kried merged commit 0f96d3e into main Apr 27, 2023
16 checks passed
@kried kried deleted the NAVIOS-983-start-NN-telemetry branch April 27, 2023 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants