AP_Arming/AP_Scripting: allow scripts for pre-arm checks

[rmackay9]

This PR makes two somewhat unrelated changes:

• Resolves an issue with the onboard LOG message's "ArmChecks" field not displaying all the bits for the enabled checks. No big deal of course but just so we know, it was this PR that introduced the issue: Implement support for the RunCam protocol #12810
• Add the ability for Scripts to stop the vehicle from arming

Below is a screen shot from a bench test where the example script was run on a CubeBlack. This example script simply disallows arming the vehicle within 60 seconds of boot

Some known issues and thoughts on this change:

• In a follow-up PR I plan to extend support for companion computers to prevent arming using the MAV_CMD_ARM_AUTHORIZATION_REQUEST command
• Only 3 external "authorisers" are supported. This low number is pretty arbitrary and could be increased easily with only minimal impact on RAM usage. Each additional authoriser consumes about 42 bytes of RAM most of which is for the string to hold failure messages.
• We could modify the logic so that if there are any failed calls to get_external_auth_id() we disallow arming. This would protect against cases where there are more than 3 scripts attempting to restrict arming
• There is a slight loophole in that we only protect against arming after the script has made a call to get_external_auth_id(). Theoretically it might be possible for a vehicle to become armed before the script has made the call to get_external_auth_id() although I think in practice this is not possible.

[rmackay9]

By the way, here is a screen shot of the ARM message before and after the fix. For this test I set LOG_DISARM = 1 and ARMING_CHECK = 65536 (for camera). We can see in the "before" that the ArmChecks field appears as "0" while in "after" it correctly shows "65536"

[rmackay9]

I've found and fixed a couple of issues that I had missed. I also added another example script but perhaps we don't need two example scripts and can drop one of them.

[WickedShell]

Outside of the inline comments below, I'd really like to request a name for something other then "external" for this. External tends to mean outside the flight controller/vehicle and calling anything that scripting sets up seems out of place. Because I'm poking at the name I guess I have to offer an alternative, for which I submit "auxiliary" as these are really extra arming checks coming from anywhere other then the core flight code. (I'm open to other names, I just don't like external).

[WickedShell]

I'd still really like to change this, if we use a Bitmask we can support far more checks, and if we only store the most recent failure from any source then we can only allocate a single buffer, which is far more flexible, and eats less RAM.

[rmackay9]

I don't think using a bitmask is worth the effort or additional complexity it would add. The structure is only 7 bytes per authoriser so there's very limited room for shrinking the basic structure.
I'll switch it to a single failure message buffer but we should note that this comes at the cost of not being able to display all arming errors at once from independent scripts or companion computers. I think it's very possible we could get an enhancement request to show all the failures at once

[WickedShell]

6 bytes total would support 32 arming checks with my proposal though... Using the proposed structure here would be 224 bytes to support that many checks. 3 checks is just not enough for arming checks as far as I can tell, I'd easily like to have more then that many for scripts.

[rmackay9]

No, I think if we switch it to a single failure message and make the AuxAuthStates class based on a uint8_t it will consume just 32 bytes for 32 authorisers..

[rmackay9]

While modifying this to use a single fail_msg buffer, one issue is this situation:

1. authoriser 1 fails, set failure msg to "fail 1"
2. authoriser 2 fails, sets failure msg to "fail 2"
3. authoriser 2 passes

In this situation, authoriser 1 has failed but we've lost it's failure message. We need to tell the user that we've failed arming.. but can no longer say why

[rmackay9]

I think I've addressed all of @WickedShell's comments now (thanks for the review!) and I think it's in better shape now. Any further comments are very welcome!

I think I've addressed all of WickedShell's requests so dismissing the review, thanks again for taking the time to look at this PR.

I think I've addressed the majority of the request so dismissing this review, thanks!

[rmackay9]

I've rebased on master and re-tested and it seems to be working still. I'll merge once it passes travis.

Thanks again for the review

[WickedShell]

@rmackay9 If you just merge a PR after making changes that defeats all the checks the review process is supposed to be bringing you. The point of the previous review is there were things which has to be improved. I'm happy you addressed them, but after making changes a review should still be done again. Just instant merging after you've pushed up changes to address a review defeats the quality of the review process.

Particularly since you didn't understand one of the review comments, and just merged over it anyways, without waiting for any discussion on how to properly address it. This is extremely frustrating as someone doing reviews if we can't achieve a consensus while it's in the PR stage then why bother presenting reviews and try to improve the quality of new features.

[rmackay9]

@WickedShell, Yes, I understand your point of view but please also understand mine:

• The PR has been open for review for two weeks and I've quickly addressed all issues raised
• I've been very careful about testing it thoroughly and providing proof of the testing
• It's already been approved by one of the other senior devs

I very much appreciate the review you provided and it did catch at least one real issue but my feeling is that we were beyond catching any real bugs and were at the point of differences in style.

As a side note, I think scripting is a really important feature and we should try to speed up those merges that are simply adding bindings which should be pretty safe.