Upgrade to MapboxNavigationNative 14.1.5 by MaximAlien · Pull Request #2417 · mapbox/mapbox-navigation-ios

MaximAlien · 2020-06-23T21:38:38Z

Due to availability changes in CLLocation API had to drop support of Xcode lower than 11.4.1. Please let me know if that's okay. Other than that there is workaround for architectures handling in nav. native when running pod lint (added in branch for update to 12.0.3).

/ref #2416

avi-c · 2020-06-25T20:26:06Z

As part of this update we will need to add retrieving and providing a SKU Token from the SDK to Nav Native.

dersim-davaod

Consider some questions/comments:

dersim-davaod · 2020-06-26T14:53:30Z

-        XCTAssertEqual(navigation.router.location!.coordinate.latitude, firstCoordinateOnUpcomingStep.latitude, accuracy: 0.0005, "User is snapped to upcoming step when moving")
-        XCTAssertEqual(navigation.router.location!.coordinate.longitude, firstCoordinateOnUpcomingStep.longitude, accuracy: 0.0005, "User is snapped to upcoming step when moving")
+        // User is snapped to upcoming step when moving
+        XCTAssertEqual(navigation.router.location!.coordinate.latitude, firstCoordinateOnUpcomingStep.latitude, accuracy: 0.005, "Latitudes should be almost equal")


First, I thought the rerouting algorithm threats legs and steps with new rules, but eventually I had to make the similar changes as coordinates within the route have "almost equality" now.

The question is: is that ok?
Did these tests work fine earlier? If so, I'm not sure why the threshold for accuracy is appeared. Want to make sure the introducing the check against specific accuracy is applicable here. @1ec5 probably you can shed some light?

The other question that comes from the question above: should we make accuracy threshold lower?

Based on this table, I think a difference of 0.005° longitude is more than 100 meters off at the latitudes in this test fixture, whereas 0.0005° is within a dozen meters or so. So I think the accuracy threshold does need to be lower, maybe nearly as low as it was previously.

In previous versions of nav. native right after starting navigation initialized routeState was returned by RouteController. This meant that rawLocation was used, which is last location, as a result no accuracy checks were needed at that time. Example with routeState values with 3 locations:

1. Current route state: initialized 2. Current route state: initialized 3. Current route state: tracking

As per discussion with @SiarheiFedartsou in nav. native 14.1.0 active guidance logic was changed and now initialized state is only returned in setRouteForRouteResponse(_:route:leg:) (I've checked to make sure and that's correct). This means that now route state has such sequence:

1. Current route state: tracking 2. Current route state: tracking 3. Current route state: tracking

This means that instead of rawLocation we'll be using snappedLocation because of such logic in RouteController:

var snappedLocation: CLLocation? { let status = navigator.getStatusForTimestamp(Date()) guard status.routeState == .tracking || status.routeState == .complete else { return nil } return CLLocation(status.location) } ... public var location: CLLocation? { return snappedLocation ?? rawLocation }

Since snappedLocation is returned from MBNavigator it explains the fact that last location and snappedLocation are not the same (that is the reason accuracy was introduced). I can see two solutions for now:

Leaving accuracy argument, but slightly change it to contain more 'realistic' value, e.g. distance between snappedLocation and last location?

Changing logic in RouteController to apply new flow in nav. native. E.g. somehow detecting that we're in initialized state even though we're currently already in tracking as per status returned from nav. native.

I might still miss some pitfalls, so if you have any ideas please let me know.

@1ec5, I'd really appreciate your thoughts regarding this.

Thanks for the comprehensive analysis!

Seems the first option is quicker, but it looks more like cutting the corner.
Should we spend time on trying to update the RouteController?

I've updated tests to use original accuracy of 0.0005° for coordinates and added distance check between coordinates (set to default accuracy of 1 meter). Had to update several tests to reflect current behavior of logic in nav. native 14.1.2. Please check updated comments in changed tests.

dersim-davaod · 2020-06-26T14:54:59Z

            let fromLocation = notification.userInfo![RouteController.NotificationUserInfoKey.locationKey] as? CLLocation
-            return fromLocation == testLocation
+
+            XCTAssertTrue(fromLocation == testLocation)


Should we make it as a different test?
So one test checks for notification, the other one checks the notification is bearing expected payload. WDYT?

Probably not the scope for the current PR.

I've added XCTAssertTrue instead for plain return value to see where test fails more easily. Previously it wasn't really obvious since in case if false is returned expectation would fail with timeout.

dersim-davaod · 2020-06-26T14:55:21Z

            let _ = notification.userInfo![RouteController.NotificationUserInfoKey.routeProgressKey] as! RouteProgress

-            return location!.distance(from: rawLocation!) <= 0.0005
+            XCTAssertTrue(location!.distance(from: rawLocation!) <= 0.5)


Extract 0.5 number (and the rest of constants) into separate consts?

Still in discussion about the best way how to fix this, when finished will address this.

dersim-davaod · 2020-06-26T14:59:45Z

                                  altitude: -1,
                                  horizontalAccuracy: -1,
-                                  verticalAccuracy: 0,
+                                  verticalAccuracy: 1,


Could you please describe what's the reason of the change? Is that important to use 1 value?

I’d be surprised if anything in MapboxCoreNavigation or MapboxNavigationNative cares about the vertical accuracy field at this point.

verticalAccuracy, courseAccuracy, speedAccuracy were just recently introduced. I've added values 1, 2, 3 respectively for those properties just for the sake of variety, to see that initializer assigns correct values.

dersim-davaod · 2020-06-26T15:07:38Z

+                                  speed: 18,
                                  timestamp: Date())

        XCTAssertEqual(location.timestamp, location.shifted(to: location.timestamp).timestamp)


Does that make sense to use different timestamp for the test? Otherwise looks like the test does not do the job.

Makes sense, updated.

dersim-davaod · 2020-06-26T15:10:38Z

-        XCTAssertEqual(location.altitude, -1)
-        XCTAssertEqual(location.horizontalAccuracy, -1)
+        if #available(iOS 13.4, *) {
+            XCTAssertEqual(fixLocation.bearingAccuracy, 2)


nit:
Extract values we check into const within "given" section:

Suggested change

XCTAssertEqual(fixLocation.bearingAccuracy, 2)

XCTAssertEqual(fixLocation.bearingAccuracy, expectedBearingAccuracy)

?

Good note, updated test.

dersim-davaod · 2020-06-26T15:12:54Z

-        let departEvent = events.filter { $0.event == MMEEventTypeNavigationDepart }.first!
-        let rerouteEvent = events.filter { $0.event == MMEEventTypeNavigationReroute }.first!
-        let arriveEvent = events.filter { $0.event == MMEEventTypeNavigationArrive }.first!
+        guard let departEvent = events.filter({ $0.event == MMEEventTypeNavigationDepart }).first else { XCTFail(); return }


nit: add description for XCTFail calls?

There is a possibility that this test might slightly change, will check afterwards.

1ec5 · 2020-06-26T18:42:12Z

+          name: "Xcode_11.4.1_iOS_12.2"
+          xcode: "11.4.1"
+          iOS: "12.2"


Various test fixtures are conditionalized by iOS version, based on their file names. So we’ll need to rename or delete and rerecord a slew of test fixtures, similar to what took place in #2427.

Thanks, fixtures were updated. I've also decided to remove iOS 13.4.1 build job since there is one for 13.5 already (this will allow to speed up build time and prevent adding additional fixtures for 13.4.1).

1ec5 · 2020-06-26T18:44:07Z

+          name: "Xcode_11.4.1_iOS_10.3.1"
+          xcode: "11.4.1"


We need to continue to test against iOS 10.x somehow, because the SDK still supports iOS 10.x. On the other hand, it’s fine to drop Xcode 10 support, but we should update the readme to make that clear.

I've added step to test against iOS 10.3.1. Do you think that's enough? Also another note is that since properties like courseAccuracy were introduced only in iOS 13.4 minimum usable version of Xcode is 11.4.1. Please let me know if that's okay.

Yes, that’s OK.

Udumft · 2020-06-30T12:02:54Z

 extension FixLocation {
    convenience init(_ location: CLLocation) {
+        var speedAccuracy: NSNumber? = nil
+        if #available(iOS 10.0, *) {


This check seems redundant. MapboxCoreNavigation minimum iOS version is 10.0

Thanks, check was removed.

Udumft · 2020-06-30T12:07:43Z

+            let navigator: Navigator = {
+                let settingsProfile = SettingsProfile(application: ProfileApplication.kMobile,
+                                                      platform: ProfilePlatform.KIOS)
+                return Navigator(profile: settingsProfile, customConfig: "")
+            }()


Can we use self.navigator here instead of initializing a new one?

In this place navigator is used in static context, probably better to leave it as it is.

MaximAlien · 2020-06-30T23:00:54Z

    }

-    func testAdvancingToFutureStepAndNotRerouting() {
+    func testNotReroutingForAllSteps() {


I see that when using nav. native 9.0.4 this test was working non deterministically. E.g. here check for advancing to future step is made only for steps 1 and 3 (why not for all?). When I check steps advancing for all steps I see that stepIndex is not increased for steps other than 1 and 3 (e.g. we remain on stepIndex 1 (instead of 2) for location with index 1 and on stepIndex 3 for locations 4 and 5). Explanation for this is that stepIndex is tied to value stored in nav. native class MBNavigationStatus.
I've modified test to check whether locations in list are on route, I think there is no reliable way to check in what case stepIndex will be increased.

MaximAlien · 2020-06-30T23:09:09Z


        let status = status ?? navigator.getStatusForTimestamp(location.timestamp)
-        let offRoute = status.routeState == .offRoute
+        let offRoute = status.routeState == .offRoute || status.routeState == .invalid


As per discussion with @SiarheiFedartsou in Slack, for now it's better to treat invalid state as offRoute.

MaximAlien · 2020-06-30T23:34:17Z

testDepartRerouteArrive() is failing due to unexpected route status values returned by nav. native. Details can be found here.

avi-c

The mechanics of this all look good to me. If Minh is happy with the test infrastructure then I think we should merge so we can move forward ASAP and stay more in-sync with Nav Native.

1ec5 · 2020-07-02T22:08:51Z

-            Settings.directions.configureRouter(tilesURL: tilesURL!) { [weak self] (numberOfTiles) in
-                let message = String.localizedStringWithFormat(NSLocalizedString("ROUTER_CONFIGURED_MSG", value: "Router configured with %ld tile(s).", comment: "Alert message when a router has been configured; 1 = number of map tiles"), numberOfTiles)
+            Settings.directions.configureRouter(tilesURL: tilesURL!) { [weak self] in
+                let message = String.localizedStringWithFormat(NSLocalizedString("ROUTER_CONFIGURED_MSG", value: "Router was configured.", comment: "Alert message when a router has been configured."))


This would’ve required rerunning scripts/extract_localizable.sh.

Thanks, for now I've reverted changes back after update to 14.1.4, had to slightly change tests again though.

1ec5 · 2020-07-02T22:09:21Z

+            Settings.directions.configureRouter(tilesURL: tilesURL!) { [weak self] in
+                let message = String.localizedStringWithFormat(NSLocalizedString("ROUTER_CONFIGURED_MSG", value: "Router was configured.", comment: "Alert message when a router has been configured."))


This backwards compatibility issue in MapboxNavigationNative was rolled back in v14.1.4, which I think we can upgrade to pretty smoothly from v14.1.3. I guess we might as well bring back the callback argument.

…dding accuracy parameter.

…lizer since iOS 13.4.

…hether user is on route for all steps and removing stepIndex check since it's not deterministic. Rename test to testNotReroutingForAllSteps.

…pboxNavigationTests.

…rror current logic in nav. native 14.1.2. Remove testSnappedLocation as it's duplicated in other tests multiple times.

…4.1.3 (unable to update locally).

This reverts commit 7c34a11.

…ld be revised).

…ive now contains both 32 and 64 bit architectures.

MaximAlien self-assigned this Jun 23, 2020

MaximAlien marked this pull request as draft June 23, 2020 21:39

MaximAlien changed the base branch from master to dersim-davaod-update-nav-native-to-12.0.3 June 23, 2020 21:39

MaximAlien requested review from 1ec5 and dersim-davaod June 24, 2020 02:26

1ec5 added the build Issues related to builds and dependency management. label Jun 24, 2020

1ec5 added this to the v1.0.0 milestone Jun 24, 2020

dersim-davaod force-pushed the dersim-davaod-update-nav-native-to-12.0.3 branch 2 times, most recently from 33d05bb to bfaddb0 Compare June 25, 2020 15:02

dersim-davaod force-pushed the dersim-davaod-update-nav-native-to-12.0.3 branch from a621a07 to eb0f909 Compare June 26, 2020 14:45

dersim-davaod reviewed Jun 26, 2020

View reviewed changes

1ec5 reviewed Jun 26, 2020

View reviewed changes

Udumft reviewed Jun 30, 2020

View reviewed changes

MaximAlien commented Jun 30, 2020

View reviewed changes

MaximAlien changed the title ~~Update mapbox-navigation-native to 14.1.0.~~ Update mapbox-navigation-native to 14.1.2. Jul 1, 2020

MaximAlien force-pushed the maxim-makhun-update-nav-native-to-14.1.0 branch from de1e0e3 to 5255591 Compare July 1, 2020 02:40

MaximAlien requested review from 1ec5, Udumft and dersim-davaod July 2, 2020 00:54

MaximAlien marked this pull request as ready for review July 2, 2020 00:56

avi-c reviewed Jul 2, 2020

View reviewed changes

1ec5 changed the base branch from dersim-davaod-update-nav-native-to-12.0.3 to release-v1.0-pre-registry July 2, 2020 21:56

1ec5 mentioned this pull request Jul 2, 2020

Update nav-native to 12.0.3 #2416

Closed

1ec5 changed the title ~~Update mapbox-navigation-native to 14.1.2.~~ Upgrade to MapboxNavigationNative 14.1.3 Jul 2, 2020

1ec5 mentioned this pull request Jul 2, 2020

Reenable 32-bit architectures #2431

Closed

1ec5 reviewed Jul 2, 2020

View reviewed changes

MaximAlien and others added 24 commits July 6, 2020 12:37

Attempt to fix testSnappedAtEndOfStepLocationWhenCourseIsSimilar by a…

556c6fc

…dding accuracy parameter.

Update testCLLocationToFixLocation to use common expected values.

9499890

Remove availability check for speedAccuracy.

76f05f6

Remove speedAccuracy verification since it's only available in initia…

4576761

…lizer since iOS 13.4.

Fix failing testUserIsOffRoute.

e1fd569

Update to nav. native 14.1.2.

f616d9d

Fix testAdvancingToFutureStepAndNotRerouting by adding verification w…

7967aba

…hether user is on route for all steps and removing stepIndex check since it's not deterministic. Rename test to testNotReroutingForAllSteps.

Temporarily mark testDepartRerouteArrive as skipped test to verify Ma…

449b7ee

…pboxNavigationTests.

Add missing fixtures.

ae65b54

Use non default shifted timestamp in testTimestampShiftForLocation.

57f3d2c

Rewrite tests related to snapping location to be more accurate and mi…

4ac4d35

…rror current logic in nav. native 14.1.2. Remove testSnappedLocation as it's duplicated in other tests multiple times.

Update to nav. native 14.1.3 for Carthage.

09973b6

Stop using numberOfTiles completionHandler argument.

f60cd12

Remove empty Fixtures and Templates project groups.

5103d03

Temporarily mark testOfflineDirections as skipped.

30ac0c0

Attempt to force CocoaPods to load new version of navigation native 1…

a76de59

…4.1.3 (unable to update locally).

Revert "Stop using numberOfTiles completionHandler argument."

71413ee

This reverts commit 7c34a11.

Upgrade to 14.1.4.

d4bc153

Update OfflineRoutingTests to mirror logic in navigation native (shou…

8c73594

…ld be revised).

Updated changelog

d244aaa

Upgrade Carthage nav. native dependency to 14.1.5.

40d9f0f

Remove ARCHS, VALID_ARCHS and ONLY_ACTIVE_ARCH flags because nav. nat…

179d8d5

…ive now contains both 32 and 64 bit architectures.

Upgrade CocoaPods to nav. native 14.1.5.

02d2f26

Add missing CocaPods upgrade after upgrade to nav. native 14.1.5.

694eae3

MaximAlien force-pushed the maxim-makhun-update-nav-native-to-14.1.0 branch from 27e081f to 694eae3 Compare July 6, 2020 19:51

MaximAlien merged commit a85e478 into release-v1.0-pre-registry Jul 6, 2020

MaximAlien deleted the maxim-makhun-update-nav-native-to-14.1.0 branch July 6, 2020 20:09

This was referenced Jul 7, 2020

CarPlayManagerTests.testManagerTellsDelegateWhenNavigationStartsAndEndsDueToArrival sporadically crashes on CI #2441

Closed

Require Xcode 11 to build SDK. #2281 #2282

Closed

Require Xcode 11.0 #2281

Closed

	XCTAssertEqual(fixLocation.bearingAccuracy, 2)
	XCTAssertEqual(fixLocation.bearingAccuracy, expectedBearingAccuracy)

		Settings.directions.configureRouter(tilesURL: tilesURL!) { [weak self] in
		let message = String.localizedStringWithFormat(NSLocalizedString("ROUTER_CONFIGURED_MSG", value: "Router was configured.", comment: "Alert message when a router has been configured."))

Conversation

MaximAlien commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avi-c commented Jun 25, 2020

Uh oh!

dersim-davaod left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaximAlien Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaximAlien Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Udumft Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaximAlien commented Jun 30, 2020

Uh oh!

avi-c left a comment

MaximAlien commented Jun 23, 2020 •

edited

Loading

1ec5 Jun 26, 2020 •

edited

Loading

MaximAlien Jun 29, 2020 •

edited

Loading

MaximAlien Jul 1, 2020 •

edited

Loading

Udumft Jun 30, 2020 •

edited

Loading