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

Removed duplicate legacy gyro_lpf validation from msp.c #5247

Merged
merged 1 commit into from Feb 21, 2018

Conversation

etracer65
Copy link
Member

Fixes #5039

Additional validation in interface/msp.c was resetting the gyro and pid denoms incorrectly under the assumption that any non-zero gyro_lpf value means the gyro is in 1KHz refresh rate. The correct validation is already been performed by fc/config.c so the extra legacy validtions are not needed.

Additional validation in msp.c was resetting the gyro and pid denoms incorrectly under the assumption that any non-zero gyro_lpf value means the gyro is in 1KHz refresh rate.  The correct validation is already been performed by fc/config.c so the extra legacy validtions are not needed.
@etracer65
Copy link
Member Author

etracer65 commented Feb 20, 2018

@mikeller - May want to cherry-pick this into the 3.2 maintenance branch if there any further releases planned.

Copy link
Collaborator

@fujin fujin left a comment

Choose a reason for hiding this comment

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

good spotting, simple fix!

@mikeller mikeller added this to the Betaflight v3.3 milestone Feb 20, 2018
@mikeller
Copy link
Member

@etracer65: No more 3.2 releases are planned. But thanks for noting it.

@mikeller mikeller merged commit f75c079 into betaflight:master Feb 21, 2018
@etracer65 etracer65 deleted the fix_gyro_lpf_reset_gyro_rate branch February 21, 2018 15:08
@ctzsnooze
Copy link
Member

Good fix.

But @etracer65 identified and I agree that 'experimental' won't work on MPU 6000.

And I agree with @etracer65 that when in 32k mode it should be possible to select 3k and 8k filter options, and that the terminology 'set gyro_lpf = off' is confusing when actually it results in 'set gyro_lpf = 256'.

Since, as @etracer65 noted, hardware filter options of 5 and 10 would never be used, maybe the choices list should be 8k, 3k, 250, 176, 92, 41, 20.

This would work for any gyro; if the gyro doesn't support >250 it gets reset to 250 after the save when 8k or 3k is input. Or the user cannot see 8k or 3k if not available. Either is OK by me.

Should this suggestion be in a separate thread, or leave it here?

@mikeller
Copy link
Member

@ctzsnooze: Probably start a new issue for the discussion, or continue on #5039. This pull request is just an architectural fix, and the discussion will probably not be found when it's tracked on here.

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.

None yet

4 participants