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

Adjust dmin limit logic #1382

Merged
merged 1 commit into from Apr 28, 2019

Conversation

Projects
None yet
3 participants
@etracer65
Copy link
Member

commented Apr 14, 2019

Previously dmin values would reset to 0 if the user decreased the D value below the current setting for dmin on a given axis. Now adjust the max for the dmin value to be D - 1 and set the dmin value to that. Prevents unexpected resets of dmin to 0 when adjusting the D gains.

Adjust dmin limit logic
Previously dmin values would reset to 0 if the user decreased the D value below the current setting for dmin on a given axis. Now adjust the max for the dmin value to be D - 1 and set the dmin value to that. Prevents unexpected resets of dmin to 0 when adjusting the D gains.
@McGiverGim

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

Have you tested what happens if a user changes in CLI the d_min value to be higher than D (to disable it)? I suspect this will enable it again with value D-1.

@etracer65

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

Looks like we need some logic in the config check in the firmware to reset d_min values to 0 if >= D gain I thought that was already there but it's not. So even before this PR the behavior was inconsistent. The Configurator would set d_min to 0 if it was >= D but the firmware wouldn't with CLI changes. So I don't know the best approach. I think it makes sense to add the 0 reset if d_min >= D in the CLI. But the Configurator behavior was unpleasant this way (why I made the PR) as the user could do a couple of clicks of D and not realize they turned off d_min. It's kind of a mess either way.

@McGiverGim

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

Yes, there are two parts:

  • In the Configurator don't let move over the D value. To disable only zero is valid. This is being done in other parts, for example the cutoff and the center of the notch filters, limiting the max property.
  • Reset to zero in the firmware if we want mimic the Configurator behavior.

@mikeller mikeller added the RN: Bugfix label Apr 15, 2019

@mikeller mikeller added this to the 10.6.0 milestone Apr 15, 2019

@etracer65

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

Added the "Don't Merge" label while we discuss the overall UX.

@McGiverGim Yes, I think I'll add the 0 reset in the firmware to make it consistent. Then I think this PR will be fine. The part that I wanted to address was if the user is decreasing D, when they reached the d_min value it would revert to 0. The same would happen if they adjusted the setting in the CLI but that's more explicit by the user as compared to clicking the down arrow a few times in the Configurator and not noticing the the other parameter changing. So no matter what I think there's going to be some inconsistency. Just trying to minimize the more likely cases the user might stumble across. Overall d_min is another case of a feature that has a dependency/dual-use on another parameter that could have been better designed.

@mikeller

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

@etracer65: Is this good to go now?

@etracer65

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2019

@mikeller The code works fine so from that perspective it's good to go. Still struggling with the overall UX caused by the poor decisions around the parameters (the "do this unless this is greater than that" silliness). It doesn't look like we have any better way to fix it so I guess this is at least better than before. This PR addresses the unexpected reset to 0 if the user is decreasing the D gain and doesn't realized they've lowered it to the same level as the d_min value.

@etracer65 etracer65 removed the Don't merge label Apr 28, 2019

@mikeller mikeller merged commit b493006 into betaflight:master Apr 28, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.