AP_RCProtocol: Reject invalid rc input values from transmitter

[muramura]

I noticed during PX4-Autopilot development that transmitters can output invalid RC values.
PX4 filters them in the INPUT_RC driver, but ArduPilot did not.
After seeing similar reports in the ArduPilot Japan community, I added a fix in IOMCU to reject invalid RC inputs and prevent unexpected behavior.

I received a flight log from a user and performed an analysis.
Through the log analysis, I discovered RC input values that were outside the calibrated MIN–MAX range.

CH5 : 961
CH9: 2085

[peterbarker]

@rmackay9 was looking at adding sanity checks on a per-channel basis, rather than done as a batch as you have in here. This will let us handle the case where RC overrides are only making some of the channels valid.

[peterbarker]

We can't change these defaults.

[muramura]

I can’t determine whether the MIN and MAX values come from calibration.
Therefore, I manually set the maximum and minimum values.
Is there a flag to check whether calibration has been performed?

Is it reasonable to assume that if the MIN and MAX values are set to 1100 and 1900, it means that calibration has not been performed?-

I want to prevent incorrect commands as soon as possible by rejecting invalid input values.
For issues that may occur unpredictably, I would like to handle them within ArduPilot.

[peterbarker]

Is it reasonable to assume that if the MIN and MAX values are set to 1100 and 1900, it means that calibration has not been performed?

No, the user may have adjusted their transmitter to suit the ranges.

[muramura]

@peterbarker san
Since there are no transmitters with MIN and MAX values of 800 and 2200, I think it’s reasonable to use these values as a check.

In the pre-arm check, if the values are 800 or 2200, it can notify that the radio calibration has not been performed.
I want to use these values as the default to indicate an uncalibrated state.

[peterbarker]

No, we can't change these. The defaults may have been allowed for by the user on the transmitter.

By changing the value of these defaults you will wildly change the way the stick values from many users will be interpreted by ArduPilot!

[Hwurzburg]

this is very bad for throttle PWM failsafe!....will prevent that from occurring...you must be able to reduce throttle PWM below the min param (which sets low throttle) from TX! unless I misunderstand some nuance

EDIT:review comment dismissed

[muramura]

@Hwurzburg san
If the throttle PWM value is lower than the MIN value, it is treated as a valid input.
This condition has been properly taken into account.

        if (i == thr_idx && v < ch->get_radio_min()) {
            continue;
        }

not applicable

[muramura]

@Hwurzburg san
I’m saying that if a transmitter sends invalid values, then that transmitter should not be used — but that’s not the right approach.
If the transmitter sends invalid values, ArduPilot should filter them instead.
Transmitters send channel values cyclically at 100 Hz, 70 Hz, or similar rates.

Please explain the disadvantages of excluding those values.

ArduPilot detects a radio failure if it cannot receive valid data continuously for 500 milliseconds.
If the receiver or transmitter sends invalid values for 500 milliseconds, it will be recognized as a radio failure.
During this period, ArduPilot continues to use the previous values for up to 500 milliseconds, which is quite a long time.

Since SBUS transmits offset values for all channels, I believe it’s acceptable to exclude the entire frame if even a single invalid value is detected.

[muramura]

I want to resolve this issue as soon as possible.
I have already experienced this phenomenon with a transmitter from a certain manufacturer.
When a flight mode is assigned to a switch, the flight mode suddenly changes.
Since ArduPilot does not verify the switch state against the current flight mode, it does not return to the original mode.

As an emergency measure, we have assigned a channel for forced disarm.
If an invalid value triggers a forced disarm, the aircraft will crash.

We want to reduce the risk of treating invalid values as valid ones.

[rmackay9]

Hi @muramura,

Thanks very much for the PR. I'm sure we can get this or an equivalent fix into 4.7.

BTW it's unfortunate that SBUS doesn't include a CRC byte. It's somewhat amazing that such a widely used and trusted protocol wouldn't have such an easy way to allow the receiver to reject errors.

[muramura]

@rmackay9 san
SBUS uses only simple error detection with EVEN parity.
If the transmitter sends an incorrect PWM OFFSET value in the SBUS frame, it will not result in a CRC error.

[peterbarker]

Bizarre as this might sound... but no, we can't reject transmitter values because they are outside the calibrated range!

We run the risk of breaking a whole bunch of people's setups in dangerous ways if we do.

The problem is that currently we clamp these out-of-bound values to be within-range. That means someone can currently have a mis-match between their radio and the flight controller's boundaries. At the moment that's fine - they will just end up with a dead-zone where they will be commanding full stick input to the autopilot when they're only at 90% of the travel of the stick (some people may deliberately do that!) We can't just start rejecting these out-of-band values - the value we use in ArduPilot should continue to be the clamped value, not the last valid input we received.

Further, these RC input values can come from places-other-than-RC-receivers. RC overrides, manual control and script input spring to mind.

.... are we sure that this just isn't a receiver which has been incorrectly set up? This looks like it might be dropping to bind-time values, and the throttle value hasn't been set appropriately in those bind-time values.

[peterbarker]

No, we can't change these. The defaults may have been allowed for by the user on the transmitter.

By changing the value of these defaults you will wildly change the way the stick values from many users will be interpreted by ArduPilot!

[peterbarker]

This is a layering violation - AP_RCProtocol shouldn't know about RC_Channel.

Why did you attempt this here rather than in RC_Channel?