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

Move GPS configuration to GPS tab #3326

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

haslinghuis
Copy link
Member

No description provided.

@haslinghuis haslinghuis added this to the 10.10.0 milestone Feb 7, 2023
@haslinghuis haslinghuis self-assigned this Feb 7, 2023
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@blckmn
Copy link
Member

blckmn commented Feb 7, 2023

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> FAIL
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> PASS
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> FAIL
  • assigned to an approver -> PASS
  • approver count at least three -> FAIL

@McGiverGim
Copy link
Member

Thanks! This is a good move. As always, it will be great some text and some screen capture in the PR ;)

@haslinghuis
Copy link
Member Author

image

image

@github-actions

This comment has been minimized.

Copy link
Member

@McGiverGim McGiverGim left a 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.

@haslinghuis
Copy link
Member Author

haslinghuis commented Feb 7, 2023

@McGiverGim

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.

@KarateBrot
Copy link
Member

KarateBrot commented Feb 8, 2023

I think it would be best to put the GPS configuration first. Example layout:

| GPS Configuration   | GPS                  |
----------------------------------------------
| GPS Signal Strength | Current GPS Location |

@haslinghuis
Copy link
Member Author

image

@McGiverGim
Copy link
Member

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 you are right. I think remember some PR to add the feature for NMEA but maybe I'm wrong.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@KarateBrot
Copy link
Member

I think a grey divider line would be nice here (like in the original configuration window in the "Configuration" tab)

grafik

@haslinghuis
Copy link
Member Author

image

@github-actions

This comment has been minimized.

@nerdCopter
Copy link
Member

nerdCopter commented Feb 8, 2023

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?

@github-actions

This comment has been minimized.

@McGiverGim
Copy link
Member

McGiverGim commented Feb 8, 2023

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?

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".

@haslinghuis
Copy link
Member Author

haslinghuis commented Feb 8, 2023

The GPS feature is driven by the ports tab sensor input field.
Disabling the feature on the GPS tab leaves the port setting in tact and removes the GPS tab.
To re-enable GPS we have to save and reboot on the Port tab.

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 ???

@McGiverGim
Copy link
Member

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.

@haslinghuis
Copy link
Member Author

haslinghuis commented Feb 8, 2023

Moving the feature switch from GPS to configuration tab seems to be most consistent.

For future work thinking about replacing features with port functions but also like to do something with the build config.

grafik

grafik

@github-actions

This comment has been minimized.

@sonarcloud
Copy link

sonarcloud bot commented Feb 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

Do you want to test this code? Here you have an automated build:
Betaflight-Configurator-Android
Betaflight-Configurator-Linux
Betaflight-Configurator-macOS
Betaflight-Configurator-Windows
WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!

@haslinghuis haslinghuis merged commit 309bd43 into betaflight:master Feb 8, 2023
@haslinghuis haslinghuis deleted the gps-commands branch February 8, 2023 22:24
@KarateBrot
Copy link
Member

@haslinghuis
Moving the feature switch from GPS to configuration tab seems to be most consistent.

That makes a lot of sense because GPS is a feature in the BF firmware. It fits right in. 👌

grafik

@KarateBrot
Copy link
Member

@haslinghuis I think there is a bug. My GPS tab won't show up even if GPS is activated:

grafik

@haslinghuis
Copy link
Member Author

@KarateBrot have you enabled GPS on ports tab?

@haslinghuis
Copy link
Member Author

image

@KarateBrot
Copy link
Member

KarateBrot commented Feb 8, 2023

Yes I have "GPS" enabled in the ports tab. I don't have a GPS receiver connected but it should still show me the GPS tab, no?

Edit: Also tested this on another FC with GPS connected but same behavior:

grafik

grafik

@haslinghuis
Copy link
Member Author

haslinghuis commented Feb 8, 2023

Windows, Linux, OSX ???

Have tested these scenario's with and without GPS on Linux. Cannot reproduce on Linux ?
Using a fresh clone for testing on master

@KarateBrot
Copy link
Member

Oh, sure, sorry. It's Windows...

@haslinghuis
Copy link
Member Author

K will check there too, but it's has a much slower processor :)

@haslinghuis
Copy link
Member Author

haslinghuis commented Feb 8, 2023

Working on Windows too (without GPS attached).

Did you build from github? If so might need to clean cache, dist, debug and node_modules.
Did you flash with GPS build option enabled?

@HThuren
Copy link
Member

HThuren commented Feb 8, 2023

Maybe more alignment
image

@haslinghuis
Copy link
Member Author

@HThuren - check NMEA and the box resizes as less options are available.

@KarateBrot
Copy link
Member

@haslinghuis
Working on Windows too (without GPS attached).

Did you build from github? If so might need to clean cache, dist, debug and node_modules.

I cleaned all these and built again using

yarn install
yarn start

but it still doesn't show up in the tabs. Weird...

Did you flash with GPS build option enabled?

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. 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: COMPLETED
Development

Successfully merging this pull request may close these issues.

6 participants