Skip to content

Add ability to enable/disable the extended kalman filter (EKF)#2600

Merged
kmadsen merged 1 commit into
masterfrom
km-add-ability-to-toggle-ekf
Mar 17, 2020
Merged

Add ability to enable/disable the extended kalman filter (EKF)#2600
kmadsen merged 1 commit into
masterfrom
km-add-ability-to-toggle-ekf

Conversation

@kmadsen

@kmadsen kmadsen commented Mar 17, 2020

Copy link
Copy Markdown
Contributor

Description

Add ability to toggle the extended kalman filter in navigation native.

  • I have added any issue links
  • I have added all related labels (bug, feature, new API(s), SEMVER, etc.)
  • I have added the appropriate milestone and project boards

Goal

We want to be able to able to toggle the extended kalman filter path.

Implementation

Mapbox location native has a simple boolean check to see if the configuration supports EKF.

This will make it so we can enable/disable EKF at any point in the app lifecycle.

Testing

Please describe the manual tests that you ran to verify your changes

  • I have tested locally (including SNAPSHOT upstream dependencies if needed) through testapp/demo app and run all activities to avoid regressions
  • I have tested via a test drive, or a simulation/mock location app
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have updated the CHANGELOG including this PR

cc: @Guardiola31337 @mskurydin @LukasPaczos

@codecov-io

codecov-io commented Mar 17, 2020

Copy link
Copy Markdown

Codecov Report

Merging #2600 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #2600   +/-   ##
=========================================
  Coverage     29.04%   29.04%           
  Complexity     1063     1063           
=========================================
  Files           281      281           
  Lines         10831    10831           
  Branches        878      878           
=========================================
  Hits           3146     3146           
  Misses         7342     7342           
  Partials        343      343

@kmadsen kmadsen changed the title Add ability to toggle ekf Add ability to enable/disable the extended kalman filter (EKF) Mar 17, 2020

@LukasPaczos LukasPaczos left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Going forward we should come up with a strategy for mutable options. Should everything exposed in NavigationOptions be mutable? We could possibly be able to reset all available there options and pass them to interested modules with a setter.

@kmadsen
kmadsen force-pushed the km-add-ability-to-toggle-ekf branch from 19145e3 to eb23748 Compare March 17, 2020 16:15
@kmadsen
kmadsen force-pushed the km-add-ability-to-toggle-ekf branch from eb23748 to dbe8ea0 Compare March 17, 2020 16:42
@kmadsen
kmadsen merged commit 4ceecea into master Mar 17, 2020
@kmadsen
kmadsen deleted the km-add-ability-to-toggle-ekf branch March 17, 2020 17:19
@Guardiola31337

Copy link
Copy Markdown
Contributor

Going forward we should come up with a strategy for mutable options. Should everything exposed in NavigationOptions be mutable? We could possibly be able to reset all available there options and pass them to interested modules with a setter.

Capturing from @SiarheiFedartsou

As I can see from code we can change it any time(even between location updates, but I wouldn't recommend to do it)

It may work for this specific option but not for all of them. This question was brought in the past and we decided to only set it at construction time and maintain it during MapboxNavigation lifetime 👀 how it's implemented in the legacy code

private Navigator configureNavigator() {
Navigator navigator = new Navigator();
NavigatorConfig navigatorConfig = navigator.getConfig();
navigatorConfig.setOffRouteThreshold(options.getOffRouteThreshold());
navigatorConfig.setOffRouteThresholdWhenNearIntersection(options.getOffRouteThresholdWhenNearIntersection());
navigatorConfig.setIntersectionRadiusForOffRouteDetection(options.getIntersectionRadiusForOffRouteDetection());
navigator.setConfig(navigatorConfig);
return navigator;
}

We should work with @mapbox/navnative and see what options we may want to expose and if they can be updated on the fly.

On a different topic, I'm not a big fan of this solution as MapboxNavigation will keep growing and growing as we expose new options. I believe we should come up with a different approach that don't clutter the entry point of the SDK.

@SiarheiFedartsou

Copy link
Copy Markdown
Contributor

I think the best solution is to think about setConfig as if it is a part of constructor. I believe at some point we will move it to constructor as well as configureRouter.

@LukasPaczos

Copy link
Copy Markdown

I think the best solution is to think about setConfig as if it is a part of constructor. I believe at some point we will move it to constructor as well as configureRouter.

This would mean that the implementation from this PR is already semi-deprecated and will require breaking changes in the near future to remove the introduced here mutability.

In this case, I think we should refactor this option and pass it only in the NavigationOption when the MapboxNavigation is first created.

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.

5 participants