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

Issue 1936 - Trim audio at max/min trim (Alternate) #1955

Merged
merged 1 commit into from
Jul 14, 2022
Merged

Issue 1936 - Trim audio at max/min trim (Alternate) #1955

merged 1 commit into from
Jul 14, 2022

Conversation

robertjmcintyre
Copy link
Contributor

@robertjmcintyre robertjmcintyre commented May 12, 2022

After all the back and forth in the other PR for this issue, I decided to do a more complete fix rather than a "minimal change" fix. Either would be fine, I believe, but I think this one is better. So, I'll create it as an actual PR vs. a draft and abandon the other if this one is accepted.

One concern I had was the scenario where you've got extended trims enabled, and you're normal trim values, and then you disable extended trims. What would the experience be, and would it wait for you to make a trim adjustment before updating the trim value to respect the new (lower) limit? Answer: no, it doesn't. As soon as you disable Extended Trims it respects the Normal Trims limits and adjusts if necessary, in the simulator. In the radio itself though it doesn't, until you move the out of bound trim once, and then it will correct to limit.

@pfeerick pfeerick linked an issue May 12, 2022 that may be closed by this pull request
1 task
@raphaelcoeffic
Copy link
Member

@pfeerick have you had time to look at this?

@pfeerick pfeerick added this to the 2.8 milestone Jun 27, 2022
@pfeerick pfeerick added the enhancement ✨ New feature or request label Jun 27, 2022
Copy link
Member

@pfeerick pfeerick left a comment

Choose a reason for hiding this comment

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

Finally, yes ;) On TX16S, this works great - tells you it's at max trim when at end, and tells you each time you press it again if that is still the case, rather than the same pitched beep.

This fixes what could be perceived as a bug... whereby if you change from extended trim, to 'normal' trim range, and the trim was in the extended range, it would wind back until it's in range (but not let you go back out of range). Now, it jumps back to the maximum for the trim on the first click. IMO this really needs to be caught when the option is changed, and be the focus of another PR. i.e. when you untick "extended trim" and you are in the extended range, a caution should come up, only allowing you to reset the trim or cancel, forcing you to move it back first if it needs to be done gradually.

@Eldenroot
Copy link
Contributor

@RJMcInty - please check this issue with trims as well :) thank you! #2050

@robertjmcintyre
Copy link
Contributor Author

... IMO this really needs to be caught when the option is changed, and be the focus of another PR. i.e. when you untick "extended trim" and you are in the extended range, a caution should come up, only allowing you to reset the trim or cancel, forcing you to move it back first if it needs to be done gradually.

I agree with you and spent a bit of time looking for the code executed when this option is changed but couldn't find it. I'll open another issue for that.

@pfeerick pfeerick merged commit 7cff5ef into EdgeTX:main Jul 14, 2022
@robertjmcintyre robertjmcintyre deleted the rjmcinty/issue-1936_alternate branch July 15, 2022 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Max/min "trim reached" message not persistent
4 participants