Issue 1936: Trim audio at max/min trim

[robertjmcintyre]

Fixes #1936

Summary of changes:

• Change the condition under which the Min/Max audio track is played so that subsequent trim movement after reaching endpoint plays the min/max audio track (rather than a normal beep)
• Improve code readability by inverting beepTrim meaning, making it consistent with the name

The only outcome of this change is that at max trim, the Min/Max is played rather than a beep. The trim values still work exactly the same, etc.

Tested in simulator and on x10s. Regarding concerns about having the track played multiple times, the killEvents(event) prevents the "hold the trim and have it play repeatedly". If you time button presses right repeatedly, you can get it to play constantly, but you're doing that to yourself, vs. the radio just sitting there with audio buffered.

Note: I debated changing the use of beepTrim (which I did) vs. changing the name to something like "supressBeepTrim". Since the normal scenario would be to play the beep, it made sense to keep the name and usage "positive" vs. "negative" (in the active high/active low sense).

I have another question/comment on code near this change and will annotate it in the file itself.

[robertjmcintyre]

There's a section of the code that I don't get:

edgetx/radio/src/opentx.cpp

Lines 1037 to 1043 in 9070a55

	// Clip at hard limits
	if (after < TRIM_EXTENDED_MIN) {
	after = TRIM_EXTENDED_MIN;
	}
	else if (after > TRIM_EXTENDED_MAX) {
	after = TRIM_EXTENDED_MAX;
	}

Because of the block just before it, and the setting of tMax and tMin above that, I don't think that it will ever be executed. I tried to come up with some scenario where it was OK for it to run past the non-extended trim limit and need that section to keep you from going past the extended trim limit, but I couldn't.

Am I missing a scenario?

[gagarinlg]

I think you are right, maybe some thought it would be better to make absolutely sure, that the limits are applied, regardless of the code above.
Try to bale the lines and maybe the commit comment is helpful.

[robertjmcintyre]

I did put some TRACE statements in those blocks and exercised the code as much as I know how, and they were never hit. Good idea with the commit history, of course! Through the last 6+ years that code has been moved around a bit and the notion of the tMax and tMin was introduced, and I think that's when that block became obsolete.

[pfeerick]

I probably still wouldn't remove it until 100% certain it can never go over, as it is the final catch all...

I've only skimmed it, but doesn't the code primarly only deal with the change of the value... so if it was already out of range (perhaps it's possible via lua, corrupt value, or something else?) it is the only thing clipping/ensuring the value is withing range?

[robertjmcintyre]

This bit:

edgetx/radio/src/opentx.cpp

Lines 1031 to 1035 in 9070a55

	// Allow only move into the right direction if over the limits
	if ((before < after && after > tMax) || // increasing over tMax
	(before > after && after < tMin)) { // decreasing under tMin
	after = before;
	}

that's just before the code in question seems to be handling making sure that the trim value stays within range.

Though I feel that check may be overly complicated, with the before < after && > tMax. It almost seems like the second check ("clip at hard limits") but with tMin and tMax is cleaner/easier to understand. And more concrete: if it's over tMax, set it to tMax, not to what it was before the increment. The only thing though is the trimInc: If it's > 1, then with the way it is now you could theoretically stop some number < trimInc away from full trim, and not be able to actually reach full trim. Which, in and of itself seems like a bug (albeit not a big one).

If it's alright, I'll open a "Dead code (?)" issue against that, and it can be investigated further. Not sure how you like to manage your project.

[gagarinlg]

This bit:

edgetx/radio/src/opentx.cpp

Lines 1031 to 1035 in 9070a55

	// Allow only move into the right direction if over the limits
	if ((before < after && after > tMax) || // increasing over tMax
	(before > after && after < tMin)) { // decreasing under tMin
	after = before;
	}

that's just before the code in question seems to be handling making sure that the trim value stays within range.
Though I feel that check may be overly complicated, with the before < after && > tMax. It almost seems like the second check ("clip at hard limits") but with tMin and tMax is cleaner/easier to understand. And more concrete: if it's over tMax, set it to tMax, not to what it was before the increment. The only thing though is the trimInc: If it's > 1, then with the way it is now you could theoretically stop some number < trimInc away from full trim, and not be able to actually reach full trim. Which, in and of itself seems like a bug (albeit not a big one).

If it's alright, I'll open a "Dead code (?)" issue against that, and it can be investigated further. Not sure how you like to manage your project.

The code is a bug in itself.
When you switch from extended to normal trims, the trims will not be clipped to the normal range until you move them there.

[raphaelcoeffic]

@RJMcInty should we close this in favour of #1955 ?

[robertjmcintyre]

@RJMcInty should we close this in favour of #1955 ?

I believe so, yes.