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

Copter/Rover: add ability to disarm rudder arming #9390

Merged
merged 11 commits into from Sep 11, 2018

Conversation

rmackay9
Copy link
Contributor

This PR allows arming and/or disarming via the sticks to be disabled. This has been requested by both by the TradHeli maintainers (@ChristopherOlson, @bnsgeyer) and also by @IamPete1 who is working on Rover's sailboat support.

In particular this PR makes the following changes:

  • moves the ARMING_RUDDER parameter from Plane up to the AP_Arming class so that it can be used by Copter and Rover.
  • Parameter conversion for Plane so that the old ARMING_RUDDER parameter value is copied to the new location. @tridge I'm surprised the way I've done this works so I would like to check with you.
  • some minor formatting fixes

This PR does not affect Sub or Tracker.

This has been tested on Plane, Rover and Copter on a Pixhawk1 flight controller. For the test I set the ARMING_RUDDER parameter to 0, 1 and 2 and tested that I was able to arming from the sticks (or not) as defined by the parameter description.

// @Description: Allow arm/disarm by rudder input. When enabled arming can be done with right rudder, disarming with left rudder. Rudder arming only works in manual throttle modes with throttle at zero +- deadzone (RCx_DZ)
// @Values: 0:Disabled,1:ArmingOnly,2:ArmOrDisarm
// @User: Advanced
AP_GROUPINFO_FLAGS_FRAME("RUDDER", 6, AP_Arming, _rudder_arming, ARMING_RUDDER_DEFAULT, AP_PARAM_NO_SHIFT, AP_PARAM_FRAME_PLANE |
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure AP_PARAM_NO_SHIFT here is wrong, from the comments and a vague recollection it's only used for index 0 in the table, which this is not.

@@ -4,20 +4,6 @@
#include "AP_Arming.h"
#include "Plane.h"

const AP_Param::GroupInfo AP_Arming_Plane::var_info[] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't tested, but I wouldn't be surprised if this broke all of the plane ARMING parameters that people already had, since this significantly alters the table structure. The minimum change is to retain the table and only remove the rudder sub entry (and leave a comment indicating the entry can't be used).

ARMING_RUDDER_ARMONLY = 1,
ARMING_RUDDER_ARMDISARM = 2
};
ArmingRudder rudder_arming() const { return (ArmingRudder)_rudder_arming.get(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe call the function get_rudder_arming_type(void)? The function name of rudder_arming() isn't very clear from an outside caller what is necessarily happening.

@rmackay9
Copy link
Contributor Author

rmackay9 commented Sep 11, 2018

@WickedShell, thanks very much for the review, really great. I've made the changes you've suggested, rebased on master and forced pushed to this PR's branch.

In particular you were right that the Plane's arming parameters were being corrupted. I've retested again by setting all of Plane's ARMING_xxxx parameters to unique values using master, then loaded this change and confirmed that the values have remained as they were.

@rmackay9
Copy link
Contributor Author

.. added a comment that index 3 should not be used in Plane's AP_Arming var_table as discussed with @WickedShell. Also rebased on master and force-pushed. I'll merge once Travis is happy. txs!

@tridge tridge merged commit 0665ac5 into ArduPilot:master Sep 11, 2018
@rmackay9 rmackay9 deleted the rudder-arming branch September 11, 2018 11:31
@bnsgeyer
Copy link
Contributor

@rmackay9 will this make it into Copter-3.6?

@rmackay9
Copy link
Contributor Author

rmackay9 commented Sep 12, 2018

@bnsgeyer, I hadn't decided actually.. we have at least one more release candidate.. any new additions risk delaying the release of course.

@MidwestAire
Copy link
Member

@rmackay9 I pre-tested this by backporting to 3.6, and squashed all the commits into one. Built it, flew it and tested it and it works as it should - I could not get it to do anything in Copter that would be unexpected behavior.

https://github.com/ChristopherOlson/ardupilot/commit/0a80ce2b16be36bc18d62fbcfca533ce64cc1a5e

@rmackay9
Copy link
Contributor Author

@ChristopherOlson, great. I'll include it in Copter-3.6.0-rc10 which should go out within the next few days.

@rmackay9 rmackay9 moved this from PRs to 3.6.0-rc10 in Copter 3.6 Backports Sep 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Copter 3.6 Backports
  
3.6.0-rc10
Development

Successfully merging this pull request may close these issues.

None yet

5 participants