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

Improve configuration for MSP GNSS #4344

Merged
merged 7 commits into from
Feb 16, 2025
Merged

Conversation

haslinghuis
Copy link
Member

@haslinghuis haslinghuis commented Feb 14, 2025

image

@haslinghuis haslinghuis added this to the 11.0 milestone Feb 14, 2025
@haslinghuis haslinghuis self-assigned this Feb 14, 2025
Copy link

netlify bot commented Feb 14, 2025

Deploy Preview for origin-betaflight-app ready!

Name Link
🔨 Latest commit 128131d
🔍 Latest deploy log https://app.netlify.com/sites/origin-betaflight-app/deploys/67afbd7df9558d000863df9c
😎 Deploy Preview https://deploy-preview-4344.dev.app.betaflight.com
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -12,7 +12,7 @@ export function updateTabList(features) {
isExpertModeEnabled && FC.CONFIG?.buildOptions?.includes("USE_SERVOS"),
);

$("#tabs ul.mode-connected li.tab_gps").toggle(features.isEnabled("GPS"));
$("#tabs ul.mode-connected li.tab_gps").toggle(isExpertModeEnabled && FC.CONFIG?.buildOptions?.includes("USE_GPS"));
Copy link
Member

@nerdCopter nerdCopter Feb 14, 2025

Choose a reason for hiding this comment

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

GPS is not showing (Configuration-Tab) on virtualFC in this PR. It does show in master.

Copy link
Member

@nerdCopter nerdCopter Feb 14, 2025

Choose a reason for hiding this comment

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

is this indeed for the configuration tab toggle? if so, would rather not require expert-mode -- will confuse pilots.

Copy link
Member Author

Choose a reason for hiding this comment

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

  • Does show here (the first time):
    image
  • Now switching expert mode on off on it removes even more tabs.

Copy link
Member

Choose a reason for hiding this comment

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

this:
image

Copy link
Member

Choose a reason for hiding this comment

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

i dont know why but ad449a6d is still as above sreenshot.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did you mean to remove DISPLAY ?

Copy link
Member

@nerdCopter nerdCopter Feb 14, 2025

Choose a reason for hiding this comment

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

no, i mean GPS does not exist here. GPS hidden.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes correct - the GPS feature has been moved to GPS tab now to be able to configure MSP GPS without having to activate GPS sensor in ports tab (stills needs to enable MSP on the UART used off course).

Copy link
Member

Choose a reason for hiding this comment

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

oh, okay. i did not follow that idea. interesting.
What if it remains in both locations, since "features" were always in configuration tab?
This should be discussed by all devs, i feel.

Copy link
Member Author

Choose a reason for hiding this comment

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

Already did the same for receiver, motor and pid tuning tab

@nerdCopter

This comment was marked as outdated.

@haslinghuis
Copy link
Member Author

haslinghuis commented Feb 14, 2025

@nerdCopter no idea why the tooltip does not work - so added it manual

Copy link
Member

@nerdCopter nerdCopter left a comment

Choose a reason for hiding this comment

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

  • i'm good with moving it [GPS-toggle], if everyone else approves.

@ctzsnooze
Copy link
Member

Thanks for these improvements!

I don't have an MSP GPS to test, but seems to have no problems with my setup with a serial GPS.

One small thing... if there is no serial port assigned to GPS, the 'enable GPS' switch at the top is greyed out. That's great! The tooltip then says, "Configure Port scenario first", and that makes sense as well. However, I am able to move the switch to 'on', even though there is no serial port available. If I save that change, and come back to the GPS tab, it remains off.

I don't know if it's feasible prevent the switch from moving into the 'on' position when there is no GPS Serial or MSP port available. That would be a tiny bit nicer. But I wouldn't worry about it too much, the user will eventually check the tooltip.

One other thing about the tooltip: even if the port is correctly configured for GPS in the ports tab, the tooltip says the same "Configure Port scenario first" message. Perhaps it should say "Enable/Disable GPS data. If GPS cannot be enabled, check that a Port is assigned to GPS in the Ports Tab" or something like that? Not sure how best to do this. Already this is a big improvement.

Copy link

@haslinghuis haslinghuis merged commit 57bc463 into betaflight:master Feb 16, 2025
9 checks passed
@haslinghuis haslinghuis deleted the msp-gps branch February 16, 2025 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Enabling GPS with MSP protocol is confusing
4 participants