-
-
Notifications
You must be signed in to change notification settings - Fork 936
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
Move GPS configuration to GPS tab #3326
Conversation
This comment has been minimized.
This comment has been minimized.
64f93c2
to
49fe259
Compare
This comment has been minimized.
This comment has been minimized.
AUTOMERGE: (FAIL)
|
Thanks! This is a good move. As always, it will be great some text and some screen capture in the PR ;) |
49fe259
to
9e9b7cc
Compare
This comment has been minimized.
This comment has been minimized.
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.
I don't know if the latest place in the page is the best for that, but it's ok to me.
Addition to the end seems to be the least intrusive for mobile users. I might be wrong but after looking at io/gps.c in firmware autobaud and autoconfig are only applicable for UBLOX protocol. Should we hide autobaud and autoconfig for NMEA. Not sure about MSP as this could use both protocols. |
I think it would be best to put the GPS configuration first. Example layout:
|
9e9b7cc
to
c5b8c77
Compare
I think you are right. I think remember some PR to add the feature for NMEA but maybe I'm wrong. |
This comment has been minimized.
This comment has been minimized.
3ab43ee
to
48e6abb
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bf0b7b0
to
e2bc810
Compare
Config-tab's GPS, OSD, LED_STRIP, Transponder not only enable the feature, but also tell the configurator to display the tab or not. With this change, it changes the paradigm. and the GPS tab will always be visible, even on non-GPS builds. Is this wanted? Would the other features follow the new pattern? |
This comment has been minimized.
This comment has been minimized.
Maybe we can do it when there is a GPS in the Ports tab selected? @haslinghuis will be difficult to do it this way for the tabs? Now that we enable the features depending if we have it selected at the ports tab. EDIT: Another option, less fancy, is to have the check to select the feature in the tab, but nothing more, occuping all the width. When enabled, then the rest of options "appear". |
The GPS feature is driven by the ports tab sensor input field. Only need to fix when the feature is disabled we don't return to the GPS tab while it's no longer in the menu. Now features are cloud build user options - this could be a start for removing the feature toggles ??? |
For this reason I recommended enable and disable features in the "firmware" part and not in the configurator part. If this was done in the firmware part, enable it when GPS selected in the ports tab, and removed when not, we would simply need to remove the toggle from the tab. Now is more difficult, because the user can let the things incoherent if uses CLI or preset to configure some port. Maybe we can try to remove the toggle from the GPS tab, hopping that any user that wants GPS configures it from the ports tab and not CLI or presets. But this will bit us later or soon. |
e2bc810
to
4ac9baa
Compare
This comment has been minimized.
This comment has been minimized.
4ac9baa
to
9cc4628
Compare
9cc4628
to
4f87c1e
Compare
Kudos, SonarCloud Quality Gate passed!
|
Do you want to test this code? Here you have an automated build: |
That makes a lot of sense because GPS is a feature in the BF firmware. It fits right in. 👌 |
@haslinghuis I think there is a bug. My GPS tab won't show up even if GPS is activated: |
@KarateBrot have you enabled GPS on ports tab? |
Windows, Linux, OSX ??? Have tested these scenario's with and without GPS on Linux. Cannot reproduce on Linux ? |
Oh, sure, sorry. It's Windows... |
K will check there too, but it's has a much slower processor :) |
Working on Windows too (without GPS attached). Did you build from github? If so might need to clean cache, dist, debug and node_modules. |
@HThuren - check |
I cleaned all these and built again using
but it still doesn't show up in the tabs. Weird...
Yes, I did. Otherwise the GPS switch would turn off automatically again and would not stay activated. Maybe this solves itself at some point. Don't worry about it. Otherwise I will probably hassle you about it sometime in the future. 😀 |
No description provided.