Readd output value limit

[khancyr]

Hi, after #5519 , it seems that the limitation on output have disappeared leading in erroneous pwm output like in #5817
This PR thus relimit pwm calculated form angles, and limit all pwm between servo_min and servo_max

[WickedShell]

@khancyr This needs to be parameter or API controlled, deepstall for example uses an explicit PWM that is outside the min/max of the SRV channel.

[khancyr]

@WickedShell hummm ok! I though there was a reason behind this change... it is kind of problematic I think, it is safer to limit the ouput by default ( for simple people like me) and add a way to bypass it on need rather than the opposite.

[OXINARF]

@khancyr This needs to be parameter or API controlled, deepstall for example uses an explicit PWM that is outside the min/max of the SRV channel.

@WickedShell How does that work? I mean, in my perspective min/max should be configured to the limit values the system should use. If those values aren't to be respected what are they used for?

[magicrub]

A deep stall is usually triggered by giving an extreme pitch command from the tail that moves it almost perpendicular which quickly drags the aircraft well below stall speed. You never want to do that during normal flight when really all you want to do is pitch up hard. A single spike of full pitch will end your flight.

Sounds like what we need is an exception to give a boost of a certain pwm us or percent given certain conditions, such as during a land and the land type is deep stall... But I can't think of a good way to do that with params.

[EShamaev]

I think that PWM min/max should never be output over the set limits. The limits are usually set at hardware max throws of servos and going beyond that will simply damage the plane.
If there are limits that should be active on normal conditions (which are less tham PWM min/max) - they must be set as one more set of options.

[tridge]

I agree with @WickedShell that we need a parameter. It should default to limiting to the specified range, but allow the user to ask for an "overdrive" for things like manual deepstall

[rmackay9]

Or maybe the code itself could have functions that limit or do not limit the output? I think most parts of the code (Sprayer, etc) never want the outputs to be beyond the limits. If there are a few cases that we know of that we do want to allow going beyond the limits, those could use a special function perhaps.

[EShamaev]

I think that there should be 1 (one) interface for accessing servos and ESC's. We already have 2 libraries that do that: SRV and Motors. It would be bad to have 3rd, 4th and so on places where there will be direct access. It breaks the structure and greatly complicates the support and adding new features.

[lucasdemarchi]

@EShamaev agreed. Maybe we could continue respecting the limits and if and only if/when doing deep stall, we reconfigure the library to allow temporarily going beyond that value.

[khancyr]

I try to look briefly at deepstall code, and I don't find extrem value. Here is what I understant :
On plane, servo for fin (?), tail ? (sorry I lack of knowledge on these words...) normaly go from -45° to 45 ° right ? In scaled value its -0.5 to 0.5 .
For deepstall (and surely other place), the command need to be -90° to 90° aka -1 to 1 in scaled value.
So in fact, value in deepstall can break the servo limit set in parameters. But should it goes under 1000 or above 2000 ?

So what about changing the signature of void SRV_Channel::calc_pwm(int16_t output_scaled) to something like void SRV_Channel::calc_pwm(int16_t output_scaled, bool enforce_limits=false) and make the limitation inside this function. So we keep the limit by default as it was before the RC slipt and add a simple way to bypass the limitation

[EShamaev]

What I am trying to say is that there are mechanical limits. No matter what happens, if you go beyond those - plane will be either damaged or servo get stuck or arm gets broken away.
If the deepstall logic that disrespects limits gets to master - for sure I will always be turning this function off for my planes as the consequences will be fatal.

[khancyr]

@EShamaev Agree ! But I think we can't force the extrem limit to always be 1000 - 2000 because off real PWM support ( don't remember exactly how it works here...). So limiting to servo out min/max to what is in parameters should be safe enough in most case (unless a user enter wrong value ...)

Looking in deepstall code https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_Landing/AP_Landing_Deepstall.cpp#L303 , I don't think they try to outpass 1000 - 2000 or even the servo limit

[WickedShell]

Alright so some summaries on the deepstall part for those following along.

The specific scenario that is being covered with deepstall is that the normal flight envelope (with really agressive control) uses +/-10 degrees of servo movement, however to enter a stable deepstall I need ~35 degrees or more of elevator to stall the aircraft. The surface is massively effective however, and normal flight can't operate within that evelope. At the moment deepstall is managed by providing a target PWM to lock the elevator to which will then be held during the stall https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_Landing/AP_Landing_Deepstall.cpp#L290 So the core of the problem is I have a specific flight stage here that needs very different PWM values then any other flight stage of the aircraft and we don't have a logical split for them.

Tridge and I have discussed implementing this as an overdrive percentage rather then a PWM value, but the initial implementation I pushed for sticking with a PWM as we knew that worked safely, and wanted to get it merged in.

The easiest workaround is probably to have a set_output_pwm_unconstrained() call that allows the caller to bypass any range clamping on the surface and get the output PWM they requested (and then deepstall could call this instead).

The other scenario where I absolutely need surfaces to not be constrained to the servo limits is when the overrides are active, as again I can manually deepstall (or manipulate flight surfaces in other ways) well outside the envelope the autopilot is allowed. The good news is unless you modify the PX4IO layer this behavior is already correct and present. If I ever have to fly a board that doesn't have a PX4IO (or equivelent) I will probably introduce a PR that has an optional flag to indicate that manual should be unconstrained as well.

@EShamaev I'm very familiar with all the problems about servos out of range, this is actually something I've suffered from after the entire servos/rc split, as trimming out an aircraft is now much much more complicated if you ever transfer the pixhawk or attempt to recal servos at the moment. At least I haven't seen any good way to set new limits easily without using say the overrides, or a script to copy from the RC cal to servo cal as a one off... (This is a seperate topic however)

[EShamaev]

@WickedShell Actually I find it much easier to trim and setup now when you have separate ranges for RCIn and RCOut.

Anyway, I think this deepstall landing should be well documented for users as not every airframe is capable to do it regarding aerodynamics and also this out of band servo action.

[WickedShell]

@EShamaev side note on deepstall I've only flown that code with a rudder only aircraft, aileron aircraft are hooked up there in theory but I haven't flight tested that yet. I do have an aircraft that has ailerons that can do deepstalls but it is hasn't been tested at all yet.

The advantage of the overdrive percentage approach tridge mentioned is it would work for dual elevator aircraft, or possibly on vtails/elevons (although I think there may be some other problems on this type of airframe that I dont know the answerers to yet)

[EShamaev]

I reread all this thread from beginning and still do not understand one thing. Why do we need to override limits at all?

I propose following:

1. You setup PWM max/min limits to the settings that you airframe is maximum capable of without damage. For your plane it is +-45 degrees for example (it includes range for deepstall).
2. Then you setup PID loop so that in normal flight conditions you stay in say +-10 degree range or whatever range you need. So you will never get out of it (PID controller will not allow to do it with max integration limit and P gain) and it is well under the PWM min/max.
3. For deepstall you override PID controller and output your needed settings but it will also stay in the range of PWM min/max (set up to hardware limits).

This way we will keep safety limits feature of PWM min/max and all logic for deepstall should work as well.

[OXINARF]

I don't know much about planes so I can be missing things, but I share @EShamaev's view here. I look at this a bit like Copter's spin min, spin max, spin arm parameters. We have a PWM range for the ESCs but then we have parameters that limit that range further.
I'm not sure of the best way to have this in Plane, but it is very strange to me that there would be no way to set a hard limit.

[WickedShell]

@EShamaev That doesn't work. Either I give the plane the potential authority to blow through the mechanical limits on the one axis (including nonsensical PWM values), or I don't. For all the reasons outlined here that doesn't work.

So lets give some numbers so that this makes more sense;
1100 is max pitch down,
1300 is trim
1500 is max pitch up
2000 is deepstall

So for me to do what you are proposing I would need to set the min PWM value to 600 which is completely out of spec. That just doesn't apply sanely. I very genuinely need a range for normal flight and a range for the deepstall mode. For now, without complicating everyone else's life that just means a way to move outside the servo range. I just don't see any way for that to ever be sane with what you proposed with manipulating the PID ranges. And even within the PID range stuff there is a difference in tuning if I max out a control surface for a period of time (rarely happens now but can) vs allowing it to continue onwards.

[WickedShell]

I'd also like to point out that plane never had a hard limit before... Manual you got what you requested, and if you asked for a PWM out (the same call that deepstall uses) you got that PWM. If you call via the scaled functions you will be fine, this can only go wrong if you call set_output_pwm() which should rarely be used. As stated we can clamp that version if we provide an unclamped interface as well which nothing else really needs to use for now.

[EShamaev]

@WickedShell Then this "unclamped" interface should be a part of SRV_Channel interface. And not be direct call that goes by it.
This project (Aurdpilot) already has too many "hacks around" and IMHO we should not add more. It is already grew too big and complicated to hold in head all such cases when implementing new things and fighting bugs.
I would really like Tridge to comment on this issue as it is not only the limits of servos that are discussed but overal concept on wether we follow OOP model or continue plugging the holes with bypassing and mixing responsibilites of classes.

[tridge]

I'd like to have code like this:
SRV_Channels::set_output_unlimited_once(SRV_Channel::Aux_servo_function_t function);
it would set a flag (a bit in a 16 bit mask) on all matching output channels (where function matches). That flag would allow the next output to be unlimited. It would be auto-cleared by SRV_Channels::output_ch_all()
I think that will make for a reasonably safe interface, while meeting the needs of special cases like deepstall.

[khancyr]

@tridge @WickedShell Hello, does this solution suit you ?

[khancyr]

rebased and corrections from review
add SRV_Channel::set_output_unlimited_once() . @OXINARF It was that you were talking about ? But I didn't find a place to use it as the current usage or only on channel functions

[OXINARF]

Indentation wrong.

This doesn't work. Zero left-shifted whatever number of times is still zero 🙂 It needs to be limited_pwm_mask &= ~(1U << ch_num);

[khancyr]

ok done!

[OXINARF]

@khancyr Yes, that was I suggested. I think it is fine not having any user of that API now, it allows to uniformize the static methods and class methods.

By the way, this was talked in the dev call and @tridge asked for more time so he can check all usage of the unlimited method - it touches a lot of places in Plane.

[khancyr]

rebased and correct lastest error

[khancyr]

@tridge @OXINARF Could we merge the first commit that shouldn't disturbe plane but will solve the value over 2000 in Rover that broke master ?

[khancyr]

rebased, remove the first commit that was already merged with #6190

[khancyr]

rebased on master

[amilcarlucas]

Sorry Pierre, can you rebase ?

[khancyr]

rebase and fix conflict

[khancyr]

@tridge ping !

[khancyr]

rebased and remove rover change that were already merged

[tridge]

@khancyr what are the remaining use cases where this is needed?
We've been moving to use scaled outputs instead of direct PWM outputs. I believe this is now happening in rover too. With scaled outputs the PWM value is already limited.
With non-scaled outputs the caller has supplied a PWM directly, so it seems sensible to obey that provided PWM. Having to call set_output_unlimited_once() to make the API do what its supposed to do seems very strange to me.
Are there any remaining cases where this is needed?

[khancyr]

@tridge
I think you are right. I don't have other use case in mind. The only thing left in these patchs is to force the pwm range to 1000 - 2000 by default. But if we use scaled value it should be already constraint .
I read the correction again, and it can be close.