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

RC channel handling with MAVLink overrides #14390

Merged
merged 2 commits into from
Jun 22, 2020

Conversation

WickedShell
Copy link
Contributor

This is a cleanup with the handling of MAVLink overrides that can cause garbage data to be consumed.

There is a particular vehicle setup to make it happen:

  • Do not use a RC receiver at all
  • Send any form of MAVLink RC input that doesn't provide a value for all the channels (a prime way to do this is MANUAL_CONTROL)

The situation that arises is as follows:

  1. The RC control_in and radio_in are both at 0 from the initial reset
  2. The GCS sends a MANUAL_CONTROL message. This message specified say the roll and throttle channels, and left the others as do not use. This causes us to flag that we have new overrides.
  3. We call read_input which sees that we have new overrides so it polls all the channels to update themselves
  4. The channels poll themselves, the ones with an override value happily just use that and update themselves, however for a channel that didn't have a value, such as say pitch what happens is they call the hal method to read the value for that channel, which has no way to flag it didn't have anything real. The effect is garbage data gets trickled up, and used for the channel.

This patch works around this by assuming we can't trust the HAL to manage this case for us. It just wasn't quickly obvious to me how this was going wrong when I tried to investigate ChibiOS's implementation. SITL also went wrong, and I didn't see a good way to solve this without a deep dive into HAL's that I didn't have a way to test.

This still needs more bench testing before coming in, I've only managed to SITL test the described case above so far, I just wanted this presented for earlier review, and guidance.

@DonLakeFlyer
Copy link
Contributor

Should MANUAL_CONTROL be disregarded if there is also RC input?

@DonLakeFlyer
Copy link
Contributor

The problem comes about with QGC using MANUAL_CONTROL to support joysticks, both real and virtual tablet ui thumb sticks. If the vehicle suddenly responds to MANUAL_CONTROL messages when it wasn't before then all sort of craziness can happen as shown in the link QGC issue. Even with the QGC virtual joystick bug fixed, the joystick positions could still be in a bad place like full throttle down. So suddenly the vehicle could fall out of the sky if it switches to MANUAL_CONTROL unexpectedly. The user is in control of the joystick/virtual stick so if they are not paying attention to the positions of those badness occurs.

@WickedShell
Copy link
Contributor Author

@DonLakeFlyer I'm not sure I'm following. All this patch is doing is correcting a bug where if you sent MANUAL_CONTROL for a selected set of channels, but not all channels we would copy in garbage RC data for the other channels. If you hadn't ever connected a receiver this could give you weird inputs by accident. MANUAL_CONTROL will still supersede the normal RC stuff the way it did before.

@DonLakeFlyer
Copy link
Contributor

The question relates to how ArduPilot reacts to getting both rc input and MANUAL_CONTROL input at the same time. Is that allowed?

@DonLakeFlyer
Copy link
Contributor

If you look at the linked QGC issue there is a report of using an RC to takeoff and then suddenly MANUAL_CONTROL took over and all hell broke loose.

@WickedShell
Copy link
Contributor Author

Ah, we always prioritize the MAVLink messages, unless you've set a parameter to ignore them (you can also set a parameter to ignore the physical RC receiver if you want). (And maybe there is a RC switch option to turn them off, I can't recall on that point)

@rrr6399
Copy link
Contributor

rrr6399 commented May 27, 2020

What if the default values for non-overridden channels with no RC receiver was trim rather than 0?

@WickedShell
Copy link
Contributor Author

@rrr6399 I'd mostly agree with you, except for running into two problems surprisingly quickly:

  1. People who haven't yet calibrated their inputs/need to adjust something where trim is totally off
  2. We have this ongoing issue about what channels get trim set or not (IE sometimes it matters for throttle, sometimes not depending on vehicle/options, then for stuff like switches it's meaningless).

The point of not copying in here is we don't update anything, which has the critical point that control_in which is the notionally mapped value used for stuff like roll/pitch/yaw/throttle is always 0 which should be always safe/the same as trim. It doesn't help with the switch case of course.

@DonLakeFlyer
Copy link
Contributor

we always prioritize the MAVLink messages

But what does that mean in reality? In this users case it seems like the RC was in control and then after some amount of time MANUAL_CONTROL suddenly took over. The MANUAL_CONTROL messages should have been being sent from the start. But the RC still worked and then it flipped over to joystick.

@WickedShell
Copy link
Contributor Author

But what does that mean in reality?

It's really simple, if you sent a MAVLink message that contains an override, and you haven't disabled the override we use that value for the channel until RC_OVERRIDE_TIME has elapsed (3 seconds in the linked issue). Then we will take the next RC input frame that is decoded validly and use that as the new signal for the channel.

The linked issue doesn't present a tlog which makes this impossible to tell, but it looks like they suddenly started sending the message part way through the flight which is why it went wrong. We really need to see the actual tlog here, not a youtube playback.

@DonLakeFlyer
Copy link
Contributor

It's really simple, if you sent a MAVLink message that contains an override, and you haven't disabled the override we use that value for the channel until RC_OVERRIDE_TIME has elapsed (3 seconds in the linked issue). Then we will take the next RC input frame that is decoded validly and use that as the new signal for the channel.

Hmm, that all sounds pretty reasonable. Maybe they turned on virtual joystick while the vehicle was flying. Need to ask.

@@ -412,6 +415,7 @@ class RC_Channels {

uint32_t last_update_ms;
bool has_new_overrides;
bool _has_had_rc_receiver; // true if we have had a direct detach RC reciever, does not include overrides
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bool _has_had_rc_receiver; // true if we have had a direct detach RC reciever, does not include overrides
bool _has_had_rc_receiver; // true if we have had a directly-attached RC receiver (does not include overrides)

@tridge
Copy link
Contributor

tridge commented Jun 16, 2020

needs rebase

@tridge tridge merged commit e4d0484 into ArduPilot:master Jun 22, 2020
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.

None yet

6 participants