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

AP_MSP:add option for backward compatibility #26966

Closed
wants to merge 1 commit into from

Conversation

Hwurzburg
Copy link
Collaborator

@Hwurzburg Hwurzburg commented May 3, 2024

#26304 will break any existing configuration relying on raw rc over MSP that use the AP RC input reversing parameter currently....this is primarily VTX and OSD stick commands and the user would have gotten used to the reversed sick patterns

while it is possible to reconfigure by changing both the TX reverse (assuming it has that ability...many old simple TXs do not but we may elect to consider them obsolete now) and the AP RCx_ reverse param...it is still a breaking setup change for some.
Our wiki recommends both ways of adjusting RC reverse...in one section it say avoid the RC reverse param if possible using TX reverse, in another it says don't mess with the TX reverse use the RC reverse...this wiki contradiction will be changed to the former recommendation but the previous PR still breaks some existing setups...with this option we offer those cases an easy reversion which can be mentioned in the wiki in the multiple RC setup mentions.

I am asking @timtuxworth to test this PR both with and without the option bit set in one of his setups for both VTX and HDzero if possible

note:I welcome coding efficiency suggestions since I am not skilled in that for sure

@timtuxworth
Copy link
Contributor

Our wiki recommends both ways of adjusting RC reverse...in one section it say avoid the RC reverse param if possible using TX reverse, in another it says don't mess with the TX reverse use the RC reverse...this wiki contradiction will be changed to the former recommendation

I just want to say for the record I definitely don't agree with this with this. IMO the golden rule is "all settings on the AP if at all possible", so I think "don't mess with the TX reverse use the RC reverse" is correct.

The alternative means "all settings on the AP except for RC channels which should be done on the TX" - what could justify making this exception?

@timtuxworth
Copy link
Contributor

I'd suggest that rather than being "set the flag to get the OLD behavior", maybe it should be "set the flag to get the NEW behaviour" - so no impact on existing setups.

@timtuxworth
Copy link
Contributor

timtuxworth commented May 3, 2024

Some testing results using my HD-Zero goggles:
4.5.1 OFFICIAL - sticks work as expected in the googles.

  • whether or not I set RC_REVERSED the menu comes up.
  • if I reverse the channel on the TX the sticks need to be reversed to get the menu to come up

4.6.0 master - which has #26304

  • if I set RC_REVERSED on the AP then the sticks have to be reversed to get the menu to come up but
  • if I set RC_REVERSED + also "invert" the output on EdgeTX then it goes back to normal

This PR

@Hwurzburg
Copy link
Collaborator Author

yes dropped a conditional negative as I was debugging...fixed now so option is to provide OLD behavior....please retest...
we can debate what the default for the option should be later...

@MichelleRos
Copy link
Contributor

if I set RC_REVERSED + also "invert" the output on EdgeTX then it goes back to normal

This is the exact point and purpose of my previous PR.
After my PR, if the sticks are going into the control systems correctly, then they'll go into MSP correctly.
The only time they'll be reversed in MSP is if your stick inputs are going reversed into the control systems (like with your A-Tail plane we discussed in my original PR), in which case your vehicle is set up incorrectly and you should fix that rather than wanting an option added to enable the old (wrong) behaviour.

Thus, I really don't think we should be adding this option at all.

@timtuxworth
Copy link
Contributor

Whatever the history, we are where we are now. I'd still like to understand the philosophy for setting everything, from scaling, to expo, to servo direction, trims, everything relating to control configuration gets set on AP. Except one thing! RC reversal. Why that one exception?

@MichelleRos
Copy link
Contributor

Except one thing! RC reversal. Why that one exception?

Not just RC reversal, but RC scale too (I.e. max, min, trim). And the reason for that is because not everyone has an OpenTX transmitter. A lot of people have transmitters where they can't adjust these things, and it might be reversed relative to what ArduPilot expects (I used one like this for the first year or two's worth of Blimp development). Thus you'd need to set the _REVERSED parameters. Same effect as what'd happen if you reversed one or more channels in OpenTX: You'd have to also reverse it in ArduPilot to get it going into the controllers in the right direction.

Before my PR, people in this situation would just find that their eg HDZero stick commands are in the wrong direction and there's nothing they can do about it since changing _REVERSED has no effect on that and they can't change what their remote outputs. After my PR, people in this situation would find that as long as their vehicle is set up so the commands go into the controllers in the correct direction, then their HDZero stick commands would also work in the correct directions.

@timtuxworth
Copy link
Contributor

timtuxworth commented May 5, 2024

Except one thing! RC reversal. Why that one exception?

Not just RC reversal, but RC scale too (I.e. max, min, trim).

We now have scaled passthrough options for all RC channels, so scaling can (should) also be done on the AP.

@timtuxworth
Copy link
Contributor

timtuxworth commented May 5, 2024

Except one thing! RC reversal. Why that one exception?

And the reason for that is because not everyone has an OpenTX/EdgeTX transmitter. A lot of people have transmitters where they can't adjust these things, and it might be reversed relative to what ArduPilot expects.
Before my PR, people in this situation would just find that their eg HDZero stick commands are in the wrong direction and there's nothing they can do about it since changing _REVERSED has no effect on that and they can't change what their remote outputs. After my PR, people in this situation would find that as long as their vehicle is set up so the commands go into the controllers in the correct direction, then their HDZero stick commands would also work in the correct directions.

So if this is about "people in this situation" with specific/limited transmitters, is it not a good idea to make it optional/selectable as this (Henry's) PR does, so that it can be used by those people, but doesn't affect those who don't need it?

Copy link
Contributor

@MichelleRos MichelleRos left a comment

Choose a reason for hiding this comment

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

Since you requested my review, I thought I should probably put one in.
This PR uses an extra 128 bytes of flash, and while that's not much, I also don't see why this is necessary. The only "backward compatibility" this provides, AFAICT is compatibility with incorrectly setup aircraft. Much better for such users to fix their aircraft.

Thus, I am against this PR, unless someone has an example where this would be necessary in a correctly-setup aircraft.

@MichelleRos
Copy link
Contributor

So if this is about "people in this situation" with specific/limited transmitters, is it not a good idea to make it optional/selectable as this (Henry's) PR does, so that it can be used by those people, but doesn't affect those who don't need it?

My point was that people with limited transmitters need the fix in my PR to get correct stick commands. People who have OpenTX can work around it (by having specific reversals in OpenTX), but it's still more convenient to not have to do this.
Thus I don't see why you would want the less convenient behaviour back.

@tridge
Copy link
Contributor

tridge commented May 6, 2024

I'm not keen on this, I think users need to fix their broken setups

@Hwurzburg Hwurzburg closed this May 7, 2024
@Hwurzburg Hwurzburg deleted the msp_option branch May 7, 2024 00:01
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.

5 participants