fix(radio): Mix delay not working.

[philmoz]

Fixes #4361

The changes in PR 4256 to fix issue 4254 did not quite work as expected and mix delays stopped working.

Changes:

• Reworked the logic so delays work as they did prior to 4256.
• Renamed variables and structs for readability
• Disabled invalid test 'DelayOnSwitch2' (SWSRC_ON is not a valid switch value for a mix line).

Also tested that the issue raised in 4254 is still fixed.

[philmoz]

Needs to be applied to 2.9.x as well as 2.10.

[ParkerEde]

I've tested the PR. It's okay.

[claudevandaele]

Will it be added in the next release ?

[pfeerick]

mix delays stopped working.

In what context? Was this in conjunction with a "switch" condition? As an "ordinary" delay works fine.

[philmoz]

mix delays stopped working.

In what context? Was this in conjunction with a "switch" condition? As an "ordinary" delay works fine.

Yes - delays do not work when the mix line has a switch.

As far as I can see all delays now work with this PR (other than the disabled test). I don't know why this test worked previously.
Ok - delays with a switch set work in this PR; but delays without a switch now don't work. The previous PR was the other way around. Damn mixer code is really hard to follow :(

[pfeerick]

Ouch 🌵 . Yeah, if the source isn't a switch, looks like it's not delayed now. Since a switch within the context of a mixer line only enables that line when true. It does delay the initial response of the mix line when switched on though, and freezes the last value for the delay time when switched off.

I suspect when that's resolved, the test will probably pass again! 🤪

[philmoz]

I took a closer look at the 2.8 behaviour and it appears that the issue reported in #4254 was actually the intended behaviour and should not have been fixed.

The up/down delays are only applied to one of the possible state changes for a mixer line:

• If the mix line is conditional on FM then the delay is applied when changing FM activates or deactivates the line
• If it is not conditional on FM; but has a switch set, then the delay is applied to the change of switch state.
• If neither of the above holds then the delay is applied to the change of source value (stick, etc)

Delays are only applied to one of these scenarios, with FM change having highest priority, then switch state and finally source value.

The code is not set up to apply the delay to more than one state change and it would probably be extremely difficult to do so (as witnessed by the attempts so far).

I am going to revert the logic back to what it was before #4254 was "fixed".

[raphaelcoeffic]

I took a closer look at the 2.8 behaviour and it appears that the issue reported in #4254 was actually the intended behaviour and should not have been fixed.

Which means we need a much better documentation on that feature, which much better explains the implemented behaviour. The tremendous efforts put into this single feature to reverse engineer the intent is quite a problem, and prevents any further refactoring, given that we will want to preserve the agreed feature set for now.

[raphaelcoeffic]

@philmoz thx for the precise description. I must admit however that the way the feature is described, I don't understand much of its usefulness. I would probably prefer a more generalised behaviour, where basically the delay is applied to the result of that mix line (whatever caused the change). I believe this would be much better understandable.

[philmoz]

I would probably prefer a more generalised behaviour, where basically the delay is applied to the result of that mix line (whatever caused the change).

I believe this would not work for the case raised in #4361. If you look at what that user has set for CH4 there are three mix lines, the 2nd and 3rd are REPLACE lines both activated by the same switch; but with different delays.
When the switch is activated the second line becomes active first, and sets the CH4 value for a short period, then the third line becomes active and changes CH4 again.

If the delay only works on value change then this breaks as line 3 is the only one activated on switch change - this is what happened with PR #4254

[claudevandaele]

The delays apply to activate a rescue function and create a moment that briefly make the swachplate of a helicopter level (Normal mode_pitch "0°") and add pitch shortly afterwards with a curve. Heli will thus first go into safe (level) on activation to take off shortly afterwards as long as switch is held.

[philmoz]

Where are we with this? Needs to be applied to 2.9 as well as 2.10.

[pfeerick]

Should finally be able to look at this properly in the next few days.

[pfeerick]

Ok, so reading through #4254 and #4361 again, I agree we need to roll back the changes in changes in delay logic introduced in #4256, effectively making 2.9.3 behave the same as 2.9.1 and earlier.

But, during 2.11, we probably need to develop some proper specifications as to how/when we expect the delay to operate so that this part of the mixer code can either get overhauled properly or documented so there is no confusion in the future. i.e. I for one would have expected that the scenario outlined in #4254 should have worked (i.e. that just because a mix line doesn't belong to a FM - especially an inactive/unused FM), that the delay setting would be honoured). #4361 is more complicated, but I also would have expected (as an end user) for that to work.

[pfeerick]

I've prepared the 2.9 cherry-pick, and if no issues arise from this in the next few days, I'll push that to the 2.9 branch.

[claudevandaele]

Thanks for fixing the issue in 2.9.3 version !