-
Notifications
You must be signed in to change notification settings - Fork 319
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
Adasis functionality #7538
Adasis functionality #7538
Conversation
ChangelogFeatures
Bug fixes and improvements
Known issues
|
Codecov ReportAttention:
@@ Coverage Diff @@
## main #7538 +/- ##
==========================================
Coverage 74.10% 74.11%
- Complexity 6165 6252 +87
==========================================
Files 835 852 +17
Lines 33175 33672 +497
Branches 3957 4011 +54
==========================================
+ Hits 24584 24955 +371
- Misses 7055 7167 +112
- Partials 1536 1550 +14
|
cb90434
to
4efd4f5
Compare
libnavigation-core/src/main/java/com/mapbox/navigation/core/sensor/SensorData.kt
Outdated
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/sensor/SensorData.kt
Outdated
Show resolved
Hide resolved
d3819b0
to
cb8e205
Compare
libnavigation-core/src/main/java/com/mapbox/navigation/core/adasis/ADASISv2Message.kt
Outdated
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/adasis/ADASISv2Message.kt
Outdated
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/adasis/AdasisConfig.kt
Outdated
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/adasis/AdasisConfigCycleTimes.kt
Outdated
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/adasis/AdasisConfigCycleTimes.kt
Outdated
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/adasis/Segment.kt
Outdated
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/adasis/ProfileShort.kt
Outdated
Show resolved
Hide resolved
...n-core/src/main/java/com/mapbox/navigation/core/adasis/AdasisConfigProfileLongTypeOptions.kt
Outdated
Show resolved
Hide resolved
...-core/src/main/java/com/mapbox/navigation/core/adasis/AdasisConfigProfileShortTypeOptions.kt
Outdated
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/adasis/ProfileLong.kt
Outdated
Show resolved
Hide resolved
examples/src/main/java/com/mapbox/navigation/examples/core/MapboxNavigationActivity.kt
Outdated
Show resolved
Hide resolved
examples/src/main/java/com/mapbox/navigation/examples/core/MapboxNavigationActivity.kt
Outdated
Show resolved
Hide resolved
Approved for snapshot, but before merging we still have some follow-up to do. |
59f2311
to
092542c
Compare
libnavigation-core/src/main/java/com/mapbox/navigation/core/adasis/SpeedLimitInfo.kt
Outdated
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/adasis/SpeedLimitRestriction.kt
Outdated
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/adasis/SpeedLimitRestriction.kt
Outdated
Show resolved
Hide resolved
5a06d28
to
028824d
Compare
ced9fd7
to
93ad42d
Compare
66bf4fc
to
2bc2141
Compare
8a13250
to
b71478d
Compare
d22dfce
to
08b55d3
Compare
Changelog since
|
08b55d3
to
e33abae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
examples/src/main/java/com/mapbox/navigation/examples/core/AdasisActivity.kt
Outdated
Show resolved
Hide resolved
examples/src/main/java/com/mapbox/navigation/examples/core/AdasisActivity.kt
Show resolved
Hide resolved
* @param callback Message callback | ||
*/ | ||
@ExperimentalPreviewMapboxNavigationAPI | ||
fun setAdasisMessageCallback(adasisConfig: AdasisConfig, callback: AdasisV2MessageCallback) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a one-shot callback, right? If that's true, we call things like this observer, not callback.
Also maybe to register/unregister, as for all other observers we have. We also allow to unregister one specific observer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, we definately shold rename it to observer.
Also maybe to register/unregister, as for all other observers we have. We also allow to unregister one specific observer.
We can't do as we did with other observers because NN requires AdasisConfig
and if we registered an observer with config1
we won't be able to register new observer with config2
.
@0xDAD can we do anything with this? Can you think of use cases where customers might need to set multiple observers with different configs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not support multiple callbacks for adasis (and probably don't have to). ADASIS generation is enabled once user sets callback with corresponding config. What if end up with methods like startAdasisEhp(config, callback)
and stopAdasisEhp()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already provide something like that. Ok, let's leave it as is for now.
libnavigation-core/src/main/java/com/mapbox/navigation/core/adasis/AdasValueOnEdge.kt
Outdated
Show resolved
Hide resolved
*/ | ||
@ExperimentalPreviewMapboxNavigationAPI | ||
class AdasisConfigProfileLongTypeOptions private constructor( | ||
val lat: Boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, would anyone actually need latitude without longitude or vice versa?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are different profiles so from adasis configuration perspective they are separate options
* @param messageBuffer Message buffer in format specified via [AdasisDataSendingConfig.messageBinaryFormat] | ||
* @param context Additional context with metadata related to the current messages package | ||
*/ | ||
fun onMessage(messageBuffer: List<Byte>, context: AdasisMessageContext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expose any deserialization tools?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. If we're speaking about flat buffer format, deserialisation happens with 3rd party SDK and we probably shouldn't include it to our SDK. We agreed with NN that we can share deserialisation tools with customers who need it.
In case of need, we'll be able to add this functionality in future without breaking changes.
} | ||
|
||
private fun Lane.toValue(): Value { | ||
val str = "{\"currentLaneIndex\": $currentLaneIndex, \"laneCount\": $laneCount}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this expected that lane value is a json? Do we have some agreement on this? Value can also contain map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point, thanks for noticing!
@0xDAD could you please take a look? Any guarantee that this format won't be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently we expect json like that
{"currentLaneIndex": 1, "laneCount": 3}
docs say:
Lane sensor:
- value is an object with the following fields:
{"currentLaneIndex": 1, "laneCount": 3}
- For right-hand traffic 1 is the very right lane and so on
- currentLaneIndex == 0 means that lane information is not available
It may change or it may become deprecated. Depends on how NIO uses it. I'd leave that in the form of arbitrary json object just to make it as flexible as possible but it's up to you do decide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@0xDAD On platform side we accept object, not json and we convert object to json when ween to pass it from platform to C++ side.
There might be issues if you change expected json format and don't notify us, so this solution doesn't look like reliable. Makes sense to consider changing it on NN side.
Segment, Stub, AdasisConfigDataSending types
36fd4a4
to
61120d0
Compare
61120d0
to
36966f2
Compare
Description