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

Add scale to ground speed for >= API 1.45 #466

Merged
merged 1 commit into from
Jan 24, 2023

Conversation

haslinghuis
Copy link
Member

Fixes: #465

@haslinghuis haslinghuis added this to the 1.7 milestone Jan 14, 2023
@haslinghuis haslinghuis self-assigned this Jan 14, 2023
@klutvott123
Copy link
Member

Did this change? Or was it always like that?

Comment on lines 20 to 24
if apiVersion >= 1.45 then
fields[#fields + 1] = { t = "Ground Speed", x = x, y = inc.y(lineSpacing), sp = x + sp, min = 30, max = 3000, vals = { 7, 8 } }
else
fields[#fields + 1] = { t = "Ground Speed", x = x, y = inc.y(lineSpacing), sp = x + sp, min = 0, max = 3000, vals = { 7, 8 }, scale = 100 }
end
Copy link
Member

Choose a reason for hiding this comment

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

Did the min limit change? Hasn't it always been 0.3m/s? Are you sure you're scaling it for the right api version here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Min has changed. See below.

Copy link
Member

Choose a reason for hiding this comment

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

It's still 0.3m/s in the configurator, but sure, we can do 0. Just need to decide if it's supposed to be in m/s for api version >= 1.45 or below

Copy link
Member

Choose a reason for hiding this comment

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

@haslinghuis This has to be changed around. This adds the scaling for apiVersion < 1.45

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed :)

@haslinghuis
Copy link
Member Author

haslinghuis commented Jan 14, 2023

Yes scale seem to be wrong before (in any case looking at https://github.com/betaflight/betaflight/pull/11834/files
Seems to be changed in 1.45 (at least the scale).

😕 Might need to update the label:

{ "RETURN SPEED CM/S", OME_UINT16 | REBOOT_REQUIRED, NULL, &(OSD_UINT16_t){ &gpsRescueConfig_rescueGroundspeed, 0, 3000, 1 } },

@klutvott123
Copy link
Member

Yes scale seem to be wrong before (in any case looking at https://github.com/betaflight/betaflight/pull/11834/files Seems to be changed in 1.45 (at least the scale).

😕 Might need to update the label:

{ "RETURN SPEED CM/S", OME_UINT16 | REBOOT_REQUIRED, NULL, &(OSD_UINT16_t){ &gpsRescueConfig_rescueGroundspeed, 0, 3000, 1 } },

I think the label should be consistent with the configurator label, and the number itself should be shown as in the configurator.

This is the kind of bugs we get when labels aren't used. "needs coordination with betaflight configurator" lets us look at the milestone and pick up all the changes that have been made to user changeable parameters. Sure it might be that the person implementing the changes in firmware are doing the configurator PR as well, but still labels should be used.

@haslinghuis
Copy link
Member Author

Configurator UI seems not have been updated ground speed. Last time it was updated: betaflight/betaflight-configurator#1946

@haslinghuis
Copy link
Member Author

@ctzsnooze - can you help us out here?

@klutvott123 klutvott123 merged commit ff0763c into betaflight:master Jan 24, 2023
@haslinghuis haslinghuis deleted the change-ground-speed branch January 24, 2023 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Speed values in GPS tab
2 participants