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

SRV_Channel: add k_soft_armed_out. #14705

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

phamelin
Copy link
Contributor

For multicopters, the servo output will simply monitors the armed state.

@Hwurzburg
Copy link
Collaborator

as far as I can tell this servo function ,despite its comment, does nothing in plane also...if we are adding it to multicopters, perhaps we should make it functional in Plane also at the same time...

@khancyr
Copy link
Contributor

khancyr commented Jun 29, 2020

I haven't check if it is possible yet, but I may be better to have this in AP_vehicle instead. That way we code it once for all vehicles types !

@IamPete1
Copy link
Member

Plane has AP_ICEngine, I would be temped to include that in copter rather than the other way round (I have no idea how involved that would be)

@peterbarker
Copy link
Contributor

Plane has AP_ICEngine, I would be temped to include that in copter rather than the other way round (I have no idea how involved that would be)

There is already a port of ICEngine to Copter; CanberraUAV used it during the OBC in 2016 on a Heli.

@peterbarker
Copy link
Contributor

Armed state is not the same as engine outputs enabled.

The ability to turn your motors (be dangerous) is not the same as your armed state.

What do you actually want to come out on that pin? The armed state, or "I'm dangerous, run away!"?

@phamelin
Copy link
Contributor Author

@peterbarker What I really meant is the armed state. I reused the EngineRunEnable parameter value because it was very close to what I needed for copters. I could had a new value (e.g. k_armed_out) if it's better.

Where should it implemented to monitor the armed state globally for all platforms? It could be implemented in AP_Arming, but it looks like the armed state for copter is managed in the motors library:

    // these functions should not be used by Copter which holds the armed state in the motors library
    Required arming_required();
    virtual bool arm(AP_Arming::Method method, bool do_arming_checks=true);
    virtual bool disarm(AP_Arming::Method method);
    bool is_armed();

@phamelin
Copy link
Contributor Author

@peterbarker What I really meant is the armed state. I reused the EngineRunEnable parameter value because it was very close to what I needed for copters. I could had a new value (e.g. k_armed_out) if it's better.

Where should it implemented to monitor the armed state globally for all platforms? It could be implemented in AP_Arming, but it looks like the armed state for copter is managed in the motors library:

    // these functions should not be used by Copter which holds the armed state in the motors library
    Required arming_required();
    virtual bool arm(AP_Arming::Method method, bool do_arming_checks=true);
    virtual bool disarm(AP_Arming::Method method);
    bool is_armed();

Replying to myself... After looking further into the code it looks like AP_Arming is subclassed for Copters but still use the AP_Arming::arm method, contrary to what the comment seems to indicate.. So implementing it in AP_Arming should do it. Am I wrong?

@IamPete1
Copy link
Member

That sounds like a good approach to me, do you actually want a PWM? We could also do a logical using relay. I wonder if this is for some sore of parachute system? Maybe we could augment AP_Parachute.

I have played with a Castle Creations ESC that you could setup a separate interlock PWM for, in that case we just did a direct RC pass-through.

@phamelin
Copy link
Contributor Author

That sounds like a good approach to me, do you actually want a PWM? We could also do a logical using relay. I wonder if this is for some sore of parachute system? Maybe we could augment AP_Parachute.

I have played with a Castle Creations ESC that you could setup a separate interlock PWM for, in that case we just did a direct RC pass-through.

I would prefer a logical output but I can deal with a PWM output. I am using a drone to deploy a tool once landed and we have to make sure that the tool is retracted before takeoff. I have a separate board to control the tool mechanism which takes that signal as an input.

@IamPete1
Copy link
Member

As this is not absolutely safety critical feature, this is a perfect use for scripting, there bindings are already there for checking arming and setting relay state. It would only be a dozen line of lua.

My vote would be to ditch the separate board doing the tool mechanism and do that in lua too ;)

@phamelin
Copy link
Contributor Author

@IamPete1 Since the desired feature is 'almost' already there and might be useful for many user, I would still vote for an additional servo function. I would also love to use the existing Pixhawk instead of a separate control board However, the control board does many other things and the Pixhawk wouldn't appropriate for this in terms of CPU usage and available I/O.

@IamPete1
Copy link
Member

I did the lua code, https://gist.github.com/IamPete1/e0fcb7ce3814b6da067bc2018e9472f7

It does both servo and relay output.

You might be suprised by what you can do in scripting, CPU usage is a none issue on a H7 board. IO is lacking but it is not a insurmountable issue.

@peterbarker
Copy link
Contributor

peterbarker commented Jul 1, 2020 via email

@Hwurzburg
Copy link
Collaborator

is it a good idea to have a switch with different behavior between vehicles? The current switch was supposed to be an ICE kill switch for planes and helis....turns out its only for helis, right?, but even then , having it as an arm state in Copter is different than what it is in Heli....this seems strange to me

@phamelin phamelin marked this pull request as draft July 2, 2020 19:43
@phamelin phamelin force-pushed the engine_run_enable_for_copter branch from 801b95f to 202fcd3 Compare July 6, 2020 16:16
@phamelin
Copy link
Contributor Author

phamelin commented Jul 6, 2020

I updated the pull request according to the comments. However, there's one thing I'm not fully satisfied about is how to initialize the default output value for the new servo function k_soft_armed_out. For now, the default value is assigned in SRV_Channel::aux_servo_function_setup(void) but I think it may not be the proper place. Any idea?

@phamelin phamelin marked this pull request as ready for review July 6, 2020 19:30
@rmackay9
Copy link
Contributor

rmackay9 commented Jul 6, 2020

@phamelin, how about relying on the lua scripting method?

@phamelin
Copy link
Contributor Author

phamelin commented Jul 7, 2020

@rmackay9 The system targeted by this functionality should be manufactured in several units and delivered to commercial customers (read: non-developers). In addition, this is used for a security function in the system. With this in mind, I am reluctant to use a Lua script for this purpose. We really want the system configuration to be as simple as possible, that is to say 1) download the official ArduCopter firmware and 2) download the manufacturer parameters. Also, the fact that the script is on the SD card adds an additional point of failure, e.g. a SD card replacement may cause the script not to be ran and a failure of the system.

@Hwurzburg
Copy link
Collaborator

if they would have to download manufacturer's parameters anyway, wouldn't it be better to provide them one customized firmware download from your site with the apj containing the parameters also?....that way its assured to be exactly what you expect them to use and you can manage such that future firmware releases, that MAY not have your verified and tested behaviours, are guarded against....I believe that is what some custom systems manufacturers do...

@phamelin
Copy link
Contributor Author

phamelin commented Jul 7, 2020

@Hwurzburg I didn't know that we can package custom parameters in the apj file. Thanks for the info! Just putting it there for future reference: https://ardupilot.org/dev/docs/apjtools-intro.html . We will probably end up using a custom firmware, but it would still be nice to have this functionality built in, to minimize patch maintenance.

@IamPete1
Copy link
Member

IamPete1 commented Jul 7, 2020

@phamelin you can also package lua scripts into apj files using the same tool, then it is no longer reliant on the SD card. I'm not sure if this is documented anywhere however.

@Hwurzburg
Copy link
Collaborator

@IamPete1 a couple of short bullets for the procedure and I will flesh out and add to the wiki in the dev section

@phamelin phamelin changed the title AP_Motors: implement EngineRunEnable servo output function for multicopters. SRV_Channel: add k_soft_armed_out. Jul 7, 2020
@peterbarker
Copy link
Contributor

@phamelin did you want to pursue this PR?

@phamelin
Copy link
Contributor Author

phamelin commented May 5, 2021

@peterbarker I would like to, but from what I understand, I am instead being suggested to use a script for this. Personally, I prefer not to use a script for this and instead use the SRV_Channel as proposed in this patch. If you're still willing to merge this patch I can rebase it.

@rmackay9
Copy link
Contributor

rmackay9 commented May 6, 2021

We could discuss it on a dev call but in general this is a request to use a PWM output as a way to pass a single piece of state to an external system. armed/disarmed is certainly an important bit of state but is it important enough? AP vehicles have lots of other state as well that could arguably also be passed out using PWM but we couldn't possibly support them all.

I wonder if this requirement comes from a desire to add support for multiple autopilots.. if "yes" then it might be better to investigate the full solution instead of just this one request.

@phamelin
Copy link
Contributor Author

phamelin commented May 6, 2021

@rmackay9 I perfectly understand your point about the other states that could be available and it might not be a good idea to make them available on RC outputs. Indeed, for most states the MAVLink communication should be used. However, I think the armed state is important enough to make it in the RC outputs. In our case, it triggers a safety input on a third-party board which control an actuator, which isn't compatible with MAVLink. The position of the actuator is controlled with another RC output taken directly from the RC receiver. In other words, it ensures that the actuator moves to its safe position before taking off. Taking off with the actuator in an unsafe position would lead to a crash.

@magicrub
Copy link
Contributor

magicrub commented May 9, 2021

I'm ok with a hard coded change like this (aka not in scripting) as long as it's genetic for all vehicles.

Copy link
Contributor

@tridge tridge left a comment

Choose a reason for hiding this comment

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

Also, the fact that the script is on the SD card adds an additional point of failure, e.g. a SD card replacement may cause the script not to be ran and a failure of the system.

lua scripts can be built into ROMFS, so they are embedded in flash, just like C++ code

libraries/SRV_Channel/SRV_Channel_aux.cpp Show resolved Hide resolved
@tridge
Copy link
Contributor

tridge commented May 10, 2021

i see a few problems with this:

  • I suspect most companions would want a GPIO, not a PWM
  • soft armed doesn't take account of safety state
  • lua scripts can be embedded in ROMFS, so can be safe and a lot more flexible

@phamelin phamelin force-pushed the engine_run_enable_for_copter branch from 202fcd3 to 91c78c7 Compare May 13, 2021 18:26
@phamelin
Copy link
Contributor Author

@tridge I was suggested by @peterbarker to use the hal.util->get_soft_armed() at first. As I understand, obtaining the "armed" state is not trivial. Do you have any suggestion to replace the soft armed?

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.

9 participants