fw_pos_control_l1: apply pitch setpoint offsets centrally

[dagar]

From #11785.

Negative pitch setpoint - If you have a negative pitch setpoint to use during level flight, you are essentially wanting the plane to nose down (or up) to not change altitude while in the air. The issue is that during takeoff if you look at pitch setpoint you will see that it is actually the specified pitch minus the setpoint (we have takeoff pitch set to 12º but are getting 7º of pitch up on takeoff because we have a negative set point of 5 degrees (FW_PSP_OFF). What we are seeing is a real “airframe” pitch of 7 degrees instead of 12 (because of the offset and it thinks it is 12º) and the elevator up is sort of giving 12º up but not really since when level on the ground the elevator is technically giving 5º pitch down.
Quick solution is probably to simply let runway takeoff pitch setpoint (RWTO_PSP) able to be a positive number. It is currently not allowed to be.

The pitch and pitch limits are shifted by the pitch setpoint offset before sent into TECS. This was being done inconsistently only for position and loiter setpoint types.

Related #7098

[dagar]

@ryanjAA

[dagar]

Since this PR will require testing I'm going to pull #11373 in as well.

[LorenzMeier]

This change breaks for users who were relying on the takeoff pitch field in QGC to set up their mission. That it is now being ignored comes unexpected. This would need to be coordinated with a QGC release. I would instead let the parameter override the mission parameter if it is non-zero.

[dagar]

This change breaks for users who were relying on the takeoff pitch field in QGC to set up their mission. That it is now being ignored comes unexpected. This would need to be coordinated with a QGC release. I would instead let the parameter override the mission parameter if it is non-zero.

If ignoring the existing mavlink parameter is unacceptable then we'd need more than a coordinated release to prevent potential confusion. How about we reject the mission upload if the field isn't 0 (or NAN) with a warning?

[LorenzMeier]

That’s fine with me.

[LorenzMeier]

@dagar SITL build failure

[dagar]

Needs to be limited to FW only (and has a typo).

[ERROR] [1555181955.941107127, 30.000000000]: FCU: Takeoff rejected, remove deprecated minium pitch
[ERROR] [1555181955.994412932, 30.056000000]: FCU: IGN MISSION_ITEM: Busy
[ERROR] [1555181956.939554436, 31.004000000]: WP: upload failed: param1 has an invalid value

[ryanjAA]

To understand the change in basic terms. If we have the following assumptions:

Pitch offset of negative 11 degrees (fw_psp_off)
Runway way takeoff pitch min set via parameter of 10 degrees positive (RWTO_MAX_PITCH)

Qgc takeoff pitch is set to 12 degrees.

Then expected pitch should be 21 degrees from the autopilots perspective (+11 for compensating from the negative psp offset and +10 for takeoff). and the qgc takeoff pitch is ignored. I’m assuming minimum pitch takeoff is is ignored although that doesn’t really matter since it would likely be so much higher. What about the max pitch? Is that going to need to be increased?

Am I understanding this correctly?

[LorenzMeier]

@ryanjAA Yes, although you would not be told by QGC that you need to set takeoff pitch to 0 and adjust it via the new param.

[ryanjAA]

Ok thanks, so it only will work if the takeoff pitch is 0 (in qgc mission planning) when creating and uploading the mission (from @dagar note above). I assume the mission simply wont upload (from above). I'll give this a try in SITL.

[dagar]

Actually, this is an important point to discuss. In current master there's a rather confusing mix of some of these pitch angle parameters being applied relative to FW_PSP_OFF, and some treated as absolute.

In the fixedwing position controller we have the following user configurable pitch settings in degrees.

• FW_P_LIM_MIN
• FW_P_LIM_MAX
• FW_TKO_PITCH_MIN
• FW_LND_FL_PMIN
• FW_LND_FL_PMAX
• FW_MAN_P_MAX

Plus

• FW_PSP_OFF

I would propose that all of these should be interpreted absolute, and not relative to FW_PSP_OFF. The FW_PSP_OFF parameter is then simply the expected pitch trim at cruise.

@ryanjAA @Antiheavy your input here would be greatly appreciated.

[Antiheavy]

I would propose that all of these should be interpreted absolute, and not relative to FW_PSP_OFF. The FW_PSP_OFF parameter is then simply the expected pitch trim at cruise.

This is basically what the parameter description says, so I guess that makes sense. https://github.com/PX4/Firmware/blob/20f870137b66c61df0ba0dcd6a96a8deab01f92e/src/modules/fw_att_control/fw_att_control_params.c#L433-L446

Can someone describe how FW_PSP_OFF is actually used in the controller?

[Antiheavy]

A related question is: does FW_PSP_OFF add on top of the absolute limits, such as "FW_P_LIM_MAX"? I think the answer is it should be allowed to add to the absolute limits, since it is an offset.

The use case I image is you have a fixed wing design where you'd like the lifting surfaces pitch datum to be different than the avionics system pitch datum.

[dagar]

A related question is: does FW_PSP_OFF add on top of the absolute limits, such as "FW_P_LIM_MAX"? I think the answer is it should be allowed to add to the absolute limits, since it is an offset.

Those limits are absolute. That's the change that created the different interpretations. #7098

At this point I think I could go either way as long as it's consistent and documented.

[Antiheavy]

@LorenzMeier @dagar this would be a good candidate for 1.10 if we can bring it over the fence?
Our main point of interest here is the new parameter FW_TKO_PITCH_MIN with which we've been flying for months now with great effect (our version is based on an old dead PR #11373).

FYI @Kjkinney

[CarlOlsson]

The comment above will need to be updated

[mrpollo]

This came up during the dev-call October 9th, 2019.

@dagar can you please rebase? looks like we need some test flight

cc @Antiheavy

[dagar]

@RomanBapst @sfuhrer perhaps we could talk this one through briefly on the next dev call? It shouldn't technically be that difficult, I just want to make sure we're all on the same page.

[RomanBapst]

@dagar @sfuhrer Ok, let's quickly discuss this so that we are on the same page and bring it in. I had a quick look and it makes sense to me. The way I see things:

All pitch limits should be absolute, when you tune TECS you are reading the absolute pitch angle, so it makes sense to have it this way.

[RomanBapst]

@dagar Do you have anything here that is not right? It all makes sense to me, I strongly agree to specify pitch limits globally always and also respect them always regardless of any offsets.
I guess this then just needs a rebase?

[dagar]

@dagar Do you have anything here that is not right? It all makes sense to me, I strongly agree to specify pitch limits globally always and also respect them always regardless of any offsets.
I guess this then just needs a rebase?

Yes, let's get this over the line.

[sfuhrer]

Closing in favor of #16792