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

Allow ignoring RC input #9004

Merged
merged 5 commits into from
Jul 31, 2018
Merged

Conversation

WickedShell
Copy link
Contributor

This is a bit of a weird one, when using purely override sources I don't want to allow noise on an input pin to be decoded as a valid input (something I've seen before on long runs of a Pixhawk on the bench). This adds a bitmask that can flag either RC reciever or MAVLink override packets as invalid.

There was a smaller patch set then this, however it required running the check twice. This version collapses the RC_Channel::read() and RC_Channel::set_pwm() into a single helper function. This actually is better from an API/cleanliness perspective. It hides more of the internal state into RC_Channel. The only external caller of these functions was in SRV_Channels::copy_radio_in_out(), which is actually a bug as it was a NOP. (calling RC_Channel::read() simply returns a value, without updating any internal state)

@WickedShell
Copy link
Contributor Author

I tested this in SITL and with real hardware using Plane.

The copter changes on set_pwm() are very weird, and I'd like some careful review of them. As far as I can tell they are no-ops as it's always passed the current value for control_in which by definition is what it is already set to, but given the amount of code dedicated to it, I'd like someone to review that. (And given the current usage of the function it's name should probably be changed if removing the set's was correct)

@rmackay9
Copy link
Contributor

rmackay9 commented Jul 25, 2018

The last change is a big concern. I think there was a change at some point which as led to the throttle and other channels being passed through to the rest of the flight code when it shouldn't be. So I think we need to investigate if during an RC failsafe event (or partial event - i.e. missed frame or throttle pulled low) we see the flight mode code (in any mode!) react to the throttle going low.

@rmackay9
Copy link
Contributor

I take back what I said about the "Copter: Remove redundant throttle channel setting". It doesn't affect the current behaviour so I have no problem with it going in. We have an existing issue with the inputs being processed during a failsafe but I think we will resolve that by adding checks all over the place including in the flight mode code.

@tridge
Copy link
Contributor

tridge commented Jul 27, 2018

I'm not a fan of the parameter. I do think we need parameters to control RC input, but I'd prefer to see RC_PROTOCOL as an enum, defaulting to AUTO, plus a RC_OPTIONS bitmask for things like this.
I am looking fwd to removing hal.rcinput and instead use AP_RCProtocol, so I'd like to choose parameters now that would make sense with that change

@WickedShell WickedShell force-pushed the wickedshell/rc-ignore branch 3 times, most recently from 65c7956 to a34c175 Compare July 27, 2018 07:54
@WickedShell
Copy link
Contributor Author

WickedShell commented Jul 27, 2018

Reworked to RC_OPTIONS Rover CI was slowed down because it has a race condition problem otherwise (seen on other PR's as well as this one recently, I just had a lot of problems with it on this one which prompted the change).

@tridge
Copy link
Contributor

tridge commented Jul 30, 2018

can you make it a AP_Int32? otherwise we'll just run out of bits.

@WickedShell
Copy link
Contributor Author

@tridge done

@tridge tridge merged commit 90216f7 into ArduPilot:master Jul 31, 2018
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

3 participants