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

MapboxNavigationNative v18.0.1 #2477

Merged
merged 13 commits into from
Aug 5, 2020
Merged

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Jul 20, 2020

Upgraded to MapboxNavigationNative v16.0.0 v17.0.1 v18.0.0 v18.0.1. Notable changes in this upgrade compared to what’s in the release-v1.0-pre-registry branch:

  • MapboxCoreNavigation depends on builds of MapboxNavigationNative and MapboxCommon that require authentication. Before CocoaPods or Carthage can download Mapbox.framework, MapboxNavigationNative.framework, and MapboxCommon.framework, you need to create a special-purpose access token. See the updated installation instructions in the readme for more details.
  • If you install this SDK using Carthage, you need to also add MapboxCommon.framework to your application target’s Embed Frameworks build phase.
  • NavigationDirectionsCompletionHandler now accepts the original tile directory URL passed into NavigationDirections.configureRouter(tilesURL:completionHandler:); the number of tiles added is no longer passed in.
  • Fixed an issue where RouteController sometimes failed to acknowledge that the user arrived at the destination.
  • Fixed a crash when getting RouteController.snappedLocation while multiple RouteControllers are active.
  • Fixed an issue where RouteController takes too long to detect that the user has gone off-route.
  • RouteController now map-matches raw locations within a 100-meter search radius, up from 50 meters, to account for noisy GPS readings.
  • Fixed a crash during offline routing caused by a side street that loops back to a main road. (Flat Loops Shouldn't Break Opposing Edge Index valhalla/valhalla#2385)
  • Fixed a crash during offline routing caused by a turn channel leading away from a roundabout. (Fix segfault: Do not combine last turn channel maneuver valhalla/valhalla#2463)
  • Added Dutch and Japanese spoken instructions during offline routing, for consistency with the Mapbox Directions API. (Add Dutch locale valhalla/valhalla#2464, Add Japanese locale, update German valhalla/valhalla#2432)
  • Suppressed more extraneous maneuvers during offline routing. (Obvious maneuvers valhalla/valhalla#2436)
  • Visual and spoken instructions for ManeuverType.turn and ManeuverType.reachFork now indicate the name of the intersection, if available, during offline routing. (Update the turn and continue phrases to include junction names and guide signs valhalla/valhalla#2386)
  • Fixed an issue where visual instructions sometimes included duplicate content in VisualInstructionBanner.primaryInstruction and VisualInstructionBanner.secondaryInstruction during offline routing.

/cc @mapbox/navigation-ios @mapbox/navnative @avi-c @d-prukop

@1ec5 1ec5 added - chore backwards incompatible changes that break backwards compatibility of public API labels Jul 20, 2020
@1ec5 1ec5 added this to the v1.0.0 milestone Jul 20, 2020
@1ec5 1ec5 self-assigned this Jul 20, 2020
Copy link
Contributor

@MaximAlien MaximAlien left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@1ec5

This comment has been minimized.

@1ec5
Copy link
Contributor Author

1ec5 commented Jul 28, 2020

I’ve upgraded to v17.0.1. So far, I’ve only ensured that the targets compile (other than #2499) and the example and CarPlay example applications launch without crashing. I’ve also updated the documentation. Other than that, tests are still crashing.

@1ec5 1ec5 changed the title MapboxNavigationNative v16.0.0 MapboxNavigationNative v17.0.1 Jul 28, 2020
@MaximAlien
Copy link
Contributor

MaximAlien commented Jul 28, 2020

I've tried to run Navigation SDK example and was able to compile it successfully. Though getting such behavior right away:
output
After going through changelist I don't see any obvious reasons which might case this, it's also reproducible on iOS device (iPhone SE).

Actually we're getting .complete routeState right after starting navigation:

Navigator status : <MBNavigationStatus: 0x6000085a6580>
Navigator status state message : source: main
Navigator status routeState : complete
Navigator status coordinate : CLLocationCoordinate2D(latitude: 37.337657, longitude: -122.02398)
Navigator status voice instruction : You have arrived at Dropped Pin #1
Navigator status leg index : 0
Navigator status route index : 0
Navigator status intersection index : 4294967295
Navigator status banner instruction : Dropped Pin #1
Remaining status leg distance : 1.1754944e-38
Remaining status leg duration : 0.0
Current voice instruction: Head north, then turn right
Current banner instruction: Turn right

History:
history_iphone_5s_17.0.1.txt

@SiarheiFedartsou, @mskurydin, can you please take a look at this one? Is there any additional info I can provide which can be useful for you?

@MaximAlien MaximAlien self-requested a review July 28, 2020 23:15
@SiarheiFedartsou
Copy link
Contributor

@MaximAlien
Please see this note in our docs:
If this value is greater than zero getStatus(u64) must be used instead of getStatus(timestamp).
I.e. if we have non-zero monotonicTimestampNanoseconds it means we must be using new getStatus implementation(getStatus(u64)).
So as a temporary workaround you can just always pass zero there and use old getStatus, but we want all SDKs to use getStatus(u64) and monotonicTimestampNanoseconds in the future, but they should use the same time source as well.

Also I see one more thing in the provided trace:

        {
            "type": "updateLocation",
            "location": {
                "lat": 37.332331,
                "lon": -122.031181,
                "time": 1595975881.167951,
                "monotonicTimestampNanoseconds": 1595,
                "speed": 0.0,
                "altitude": -1.0
            },
            "result": true,
            "event_timestamp": 1595975881.168658,
            "delta_us": 9
        },

"monotonicTimestampNanoseconds": 1595 looks weird.

UPDATE
After changing monotonicTimestampNanoseconds to 1595975881167951000(what we probably expected here) everything works as expected.

@@ -11,7 +11,7 @@ extension FixLocation {

// In practice, “submillisecond precision” is 10 nanosecond precision at best, but convert the timestamp to nanoseconds anyways.
// Unlike on Android, we aren’t concerned about the timestamps’ monotonicity.
let timestamp = location.timestamp.timeIntervalSince1970 * 1e-6
let timestamp = location.timestamp.timeIntervalSince1970 * 1e6
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"monotonicTimestampNanoseconds": 1595 looks weird.

Turns out I was passing in megaseconds instead of nanoseconds. 🤦

@1ec5
Copy link
Contributor Author

1ec5 commented Jul 29, 2020

We’re down to the following test failures:

Test Case '-[MapboxCoreNavigationTests.NavigationServiceTests testSnappedAtEndOfStepLocationWhenCourseIsSimilar]' started.
2020-07-29 15:35:25.692019+0000 Example[79512:131516] Task <1F4AF0F9-A630-4B76-89F8-2B4968A48A7B>.<1> finished with error - code: -1002
2020-07-29 15:35:25.693043+0000 Example[79512:131516] Task <40AEA5C3-3F14-4689-AC6C-F303685100AF>.<2> finished with error - code: -1002
/Users/distiller/project/MapboxCoreNavigationTests/NavigationServiceTests.swift:220: error: -[MapboxCoreNavigationTests.NavigationServiceTests testSnappedAtEndOfStepLocationWhenCourseIsSimilar] : XCTAssertEqualWithAccuracy failed: ("37.79162899999999") is not equal to ("37.791000000000004") +/- ("0.0005") - Latitudes should be almost equal
/Users/distiller/project/MapboxCoreNavigationTests/NavigationServiceTests.swift:221: error: -[MapboxCoreNavigationTests.NavigationServiceTests testSnappedAtEndOfStepLocationWhenCourseIsSimilar] : XCTAssertEqualWithAccuracy failed: ("-122.41248399999999") is not equal to ("-122.417411") +/- ("0.0005") - Longitudes should be almost equal
/Users/distiller/project/MapboxCoreNavigationTests/NavigationServiceTests.swift:223: error: -[MapboxCoreNavigationTests.NavigationServiceTests testSnappedAtEndOfStepLocationWhenCourseIsSimilar] : XCTAssertLessThan failed: ("439.5321754200767") is not less than ("1.0")
Test Case '-[MapboxCoreNavigationTests.NavigationServiceTests testSnappedAtEndOfStepLocationWhenCourseIsSimilar]' failed (0.034 seconds).

@@ -208,7 +208,7 @@ class NavigationServiceTests: XCTestCase {
altitude: 0,
horizontalAccuracy: 30,
verticalAccuracy: 10,
course: -finalHeading,
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’m surprised we got away with this until now, but negating a CLLocationDegrees makes it invalid rather than pointing it in the other direction.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, when I read docs here it states A negative value indicates that the course information is invalid.

@@ -208,7 +208,7 @@ class NavigationServiceTests: XCTestCase {
altitude: 0,
horizontalAccuracy: 30,
verticalAccuracy: 10,
course: -finalHeading,
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, when I read docs here it states A negative value indicates that the course information is invalid.

@1ec5
Copy link
Contributor Author

1ec5 commented Jul 30, 2020

SKU token tests have suddenly started failing:

Test Case '-[MapboxNavigationTests.SKUTests testDirectionsSKU]' started.
/Users/distiller/project/MapboxNavigationTests/SKUTests.swift:15: error: -[MapboxNavigationTests.SKUTests testDirectionsSKU] : XCTAssertNotNil failed
/Users/distiller/project/MapboxNavigationTests/SKUTests.swift:17: error: -[MapboxNavigationTests.SKUTests testDirectionsSKU] : XCTAssertEqual failed: ("nil") is not equal to ("Optional("08")")
Test Case '-[MapboxNavigationTests.SKUTests testDirectionsSKU]' failed (0.004 seconds).
Test Case '-[MapboxNavigationTests.SKUTests testSKUTokensMatch]' started.
2020-07-30 00:57:36.483353+0000 Example[83302:145268] Unbalanced calls to begin/end appearance transitions for <UIViewController: 0x7fad72d52da0>.
/Users/distiller/project/MapboxNavigationTests/SKUTests.swift:38: error: -[MapboxNavigationTests.SKUTests testSKUTokensMatch] : XCTAssertEqual failed: ("Optional("00")") is not equal to ("Optional("08")")
/Users/distiller/project/MapboxNavigationTests/SKUTests.swift:39: error: -[MapboxNavigationTests.SKUTests testSKUTokensMatch] : XCTAssertEqual failed: ("Optional("1003KRI30hR21df505553e16ee2c66ac74450b7d5aa")") is not equal to ("nil")
/Users/distiller/project/MapboxNavigationTests/SKUTests.swift:40: error: -[MapboxNavigationTests.SKUTests testSKUTokensMatch] : XCTAssertEqual failed: ("Optional("1003KRI30hR21df505553e16ee2c66ac74450b7d5aa")") is not equal to ("nil")
Test Case '-[MapboxNavigationTests.SKUTests testSKUTokensMatch]' failed (0.337 seconds).
Test Case '-[MapboxNavigationTests.SKUTests testSpeechSynthesizerSKU]' started.
/Users/distiller/project/MapboxNavigationTests/SKUTests.swift:23: error: -[MapboxNavigationTests.SKUTests testSpeechSynthesizerSKU] : XCTAssertNotNil failed
/Users/distiller/project/MapboxNavigationTests/SKUTests.swift:25: error: -[MapboxNavigationTests.SKUTests testSpeechSynthesizerSKU] : XCTAssertEqual failed: ("nil") is not equal to ("Optional("08")")
Test Case '-[MapboxNavigationTests.SKUTests testSpeechSynthesizerSKU]' failed (0.007 seconds).

Locally, I’m seeing these warnings in the console when running the example application in this repository:

objc[64674]: Class MBXAccounts is implemented in both /path/to/Library/Developer/CoreSimulator/Devices/6A42FE84-6F38-46A0-8880-1A4AF27C039A/data/Containers/Bundle/Application/5B74B76D-8744-4BEA-A58D-FA97422954FB/Example.app/Frameworks/MapboxCommon.framework/MapboxCommon (0x10fb0a7d0) and /path/to/Library/Developer/CoreSimulator/Devices/6A42FE84-6F38-46A0-8880-1A4AF27C039A/data/Containers/Bundle/Application/5B74B76D-8744-4BEA-A58D-FA97422954FB/Example.app/Frameworks/MapboxAccounts.framework/MapboxAccounts (0x10f79f610). One of the two will be used. Which one is undefined.
objc[64674]: Class MBXExpected is implemented in both /path/to/Library/Developer/CoreSimulator/Devices/6A42FE84-6F38-46A0-8880-1A4AF27C039A/data/Containers/Bundle/Application/5B74B76D-8744-4BEA-A58D-FA97422954FB/Example.app/Frameworks/MapboxCommon.framework/MapboxCommon (0x10fb0b130) and /path/to/Library/Developer/CoreSimulator/Devices/6A42FE84-6F38-46A0-8880-1A4AF27C039A/data/Containers/Bundle/Application/5B74B76D-8744-4BEA-A58D-FA97422954FB/Example.app/Frameworks/MapboxNavigationNative.framework/MapboxNavigationNative (0x10f09c408). One of the two will be used. Which one is undefined.
objc[64674]: Class MBXFeature is implemented in both /path/to/Library/Developer/CoreSimulator/Devices/6A42FE84-6F38-46A0-8880-1A4AF27C039A/data/Containers/Bundle/Application/5B74B76D-8744-4BEA-A58D-FA97422954FB/Example.app/Frameworks/MapboxCommon.framework/MapboxCommon (0x10fb0b180) and /path/to/Library/Developer/CoreSimulator/Devices/6A42FE84-6F38-46A0-8880-1A4AF27C039A/data/Containers/Bundle/Application/5B74B76D-8744-4BEA-A58D-FA97422954FB/Example.app/Frameworks/MapboxNavigationNative.framework/MapboxNavigationNative (0x10f09c458). One of the two will be used. Which one is undefined.
objc[64674]: Class MBXGeometry is implemented in both /path/to/Library/Developer/CoreSimulator/Devices/6A42FE84-6F38-46A0-8880-1A4AF27C039A/data/Containers/Bundle/Application/5B74B76D-8744-4BEA-A58D-FA97422954FB/Example.app/Frameworks/MapboxCommon.framework/MapboxCommon (0x10fb0b1d0) and /path/to/Library/Developer/CoreSimulator/Devices/6A42FE84-6F38-46A0-8880-1A4AF27C039A/data/Containers/Bundle/Application/5B74B76D-8744-4BEA-A58D-FA97422954FB/Example.app/Frameworks/MapboxNavigationNative.framework/MapboxNavigationNative (0x10f09c4a8). One of the two will be used. Which one is undefined.
objc[64674]: Class MBXPeerWrapper is implemented in both /path/to/Library/Developer/CoreSimulator/Devices/6A42FE84-6F38-46A0-8880-1A4AF27C039A/data/Containers/Bundle/Application/5B74B76D-8744-4BEA-A58D-FA97422954FB/Example.app/Frameworks/MapboxCommon.framework/MapboxCommon (0x10fb0b220) and /path/to/Library/Developer/CoreSimulator/Devices/6A42FE84-6F38-46A0-8880-1A4AF27C039A/data/Containers/Bundle/Application/5B74B76D-8744-4BEA-A58D-FA97422954FB/Example.app/Frameworks/MapboxNavigationNative.framework/MapboxNavigationNative (0x10f09c4f8). One of the two will be used. Which one is undefined.

This is a serious issue: it means nondeterministic behavior at runtime and likely conflicting, unrelated implementations for MBXAccounts.

@MaximAlien MaximAlien self-requested a review July 30, 2020 04:08
@MaximAlien
Copy link
Contributor

I do not have enough context yet, but probably one of the ways to fix duplicated symbols warning is to build MapboxNavigationNative with symbols visibility which are duplicated set to hidden, currently default is used (I don't see that they're really used on iOS). On the other hand MBXAccounts will probably have to be renamed. It's also worth noting that we also have Swift symbols called Feature (struct) and Geometry (enum) which are coming from Turf.

@1ec5
Copy link
Contributor Author

1ec5 commented Jul 30, 2020

It's also worth noting that we also have Swift symbols called Feature (struct) and Geometry (enum) which are coming from Turf.

Those symbols are implemented in Swift and don’t bridge to Objective-C, so their runtime names are already prefixed with the Turf module name and mangled, avoiding any potential for collision.

@1ec5 1ec5 mentioned this pull request Jul 30, 2020
@zugaldia zugaldia changed the title MapboxNavigationNative v17.0.1 MapboxNavigationNative v18.0.0 Jul 31, 2020
@1ec5 1ec5 requested review from d-prukop and avi-c August 4, 2020 01:02
@1ec5
Copy link
Contributor Author

1ec5 commented Aug 4, 2020

Not sure what’s up with codecov, but we definitely didn’t just drop this much in test coverage.

@1ec5 1ec5 mentioned this pull request Aug 5, 2020
@1ec5 1ec5 changed the title MapboxNavigationNative v18.0.0 MapboxNavigationNative v18.0.1 Aug 5, 2020
1ec5 and others added 12 commits August 4, 2020 19:09
The navigator needs nanoseconds, not megaseconds.
Previously, the test attempted to simulate making a wrong turn by negating the expected final heading and using that as the course of the location update. But a negative course is an invalid course that we avoid passing along to the navigator. The correct way to turn in the opposite direction is to add 180°.
… contains updated MapboxCommon version 4.0.0.
The idea behind introducing the monotonicTimestampNanoseconds parameter to FixLocation was apparently that the navigator could rely on a guaranteed monotonic value to sort locations and detect and discard any jumps in time that may be caused by changes to or unreliability in the system time. The timestamp in each location update serves a somewhat different purpose than a monotonic counter, since it corresponds specifically to the time at which the system thinks it received the location update. A significant amount of time may have elapsed since the location update, and that gap may not be consistent between location updates, a fact that would be obscured by relying on the monotonic counter instead of the location timestamp. To avoid misunderstandings about the origin and behavior of the timestamp, this change goes back to specifying only a conventional timestamp and leaving the monotonic counter unspecified.
@1ec5
Copy link
Contributor Author

1ec5 commented Aug 5, 2020

Still getting this error intermittently:

Test Case '-[MapboxNavigationTests.SKUTests testSKUTokensMatch]' started.
2020-08-05 02:40:09.824612+0000 Example[85173:149220] Unbalanced calls to begin/end appearance transitions for <UIViewController: 0x7fd1b54440c0>.
/Users/distiller/project/MapboxNavigationTests/SKUTests.swift:39: error: -[MapboxNavigationTests.SKUTests testSKUTokensMatch] : XCTAssertEqual failed: ("Optional("108ZzxqEcEI55dbf76a9baf130703f7121877a87c69")") is not equal to ("Optional("108e7RftWGd55dbf76a9baf130703f7121877a87c69")")
/Users/distiller/project/MapboxNavigationTests/SKUTests.swift:40: error: -[MapboxNavigationTests.SKUTests testSKUTokensMatch] : XCTAssertEqual failed: ("Optional("108ZzxqEcEI55dbf76a9baf130703f7121877a87c69")") is not equal to ("Optional("108e7RftWGd55dbf76a9baf130703f7121877a87c69")")
Test Case '-[MapboxNavigationTests.SKUTests testSKUTokensMatch]' failed (0.403 seconds).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards incompatible changes that break backwards compatibility of public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants