Scale manual control setpoint up/down stick -1 to 1

[MaEtUgR]

Describe problem solved by this pull request Fixes #9331 and works towards a solution for part of https://github.com/PX4/Firmware/issues/15874

Describe your solution Rescale manual_control_setpoint.z which represents the throttle stick value from a range of [0,1] to [-1,1].

Describe possible alternatives I think for nicer usage in all the different modules it would be helpful to extend the Sticks class to provide the common scaling and gestures (is stick below minimum) from one single place.

Test data / coverage Not yet tested.

Additional context This is also useful to be able to convert between RC modes (https://www.rc-airplane-world.com/rc-transmitter-modes.html) on the fly without having to recalibrate everything.

[MaEtUgR]

Reminder for myself to rebase on https://github.com/Auterion/PX4_firmware_private/pull/661 like discussed. EDIT: ✔️

[MaEtUgR]

@dagar

• I rebased this pr on https://github.com/PX4/PX4-Autopilot/pull/16270 like I wrote here: https://github.com/PX4/PX4-Autopilot/pull/16270#issuecomment-743057594
• I followed the flight review comments in https://github.com/PX4/PX4-Autopilot/pull/15949#discussion_r504416339 but both prs need flight review adaptation
• I brought back QGC not complying with https://mavlink.io/en/messages/common.html#MANUAL_CONTROL compatibility because it's for another pr to sync this change see https://github.com/PX4/PX4-Autopilot/pull/15949#discussion_r540927308

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

[tstastny]

I think for nicer usage in all the different modules it would be helpful to extend the Sticks class to provide the common scaling and gestures (is stick below minimum) from one single place.

I agree with you on this point. Seems a bit dangerous (error prone) that we are re-interpreting these manual controls everywhere they are used.

[MaEtUgR]

I rebased this pr on main and after investigating carefully I suggest using roll, pitch, yaw, and throttle for the naming of the axes because:

• It used to be that originally
  • in PX4: https://github.com/PX4/PX4-Autopilot/pull/922/files#diff-46577665f843b3c1f8f67ac8a331ccb48274ba41fc7c5b1ece223fc7ffe21424L68-L71
  • in MAVLink: https://github.com/mavlink/mavlink/pull/64/files#diff-9ef151b30f09d6d4f130434d941db479c3411ec15eca23580c5587b2c3b59ecdL1354-L1357
• Scanning through other drone-related projects and APIs the only convention I found is that one.
• I was looking for that when I joined PX4 and thought I'm the only one.
• @bkueng chose to use this as the most clear way to assign RC sticks to direct output passthrough https://github.com/PX4/PX4-Autopilot/blob/55563eba49f87624d7b73356ea3d421fee7ec081/src/lib/mixer_module/output_functions.yaml#L24-L27
• @junwoo091400 when he started immediately asked where these axes are and then added helpers for his use: https://github.com/PX4/PX4-Autopilot/blob/55563eba49f87624d7b73356ea3d421fee7ec081/src/modules/flight_mode_manager/tasks/Utility/Sticks.hpp#L66-L69
• Half of the usage of the stick assign to variables like roll, pitch, yaw, thrust
• Some usage has comments to remind the reader which one is which: https://github.com/PX4/PX4-Autopilot/blob/55563eba49f87624d7b73356ea3d421fee7ec081/src/lib/mixer_module/functions.cpp#L112-L115 https://github.com/PX4/PX4-Autopilot/blob/55563eba49f87624d7b73356ea3d421fee7ec081/src/modules/flight_mode_manager/tasks/Utility/Sticks.cpp#L54-L58
• RC mapping parameters are called this way https://github.com/PX4/PX4-Autopilot/blob/55563eba49f87624d7b73356ea3d421fee7ec081/src/modules/rc_update/params.c#L1168 https://github.com/PX4/PX4-Autopilot/blob/55563eba49f87624d7b73356ea3d421fee7ec081/src/modules/rc_update/params.c#L1200 https://github.com/PX4/PX4-Autopilot/blob/55563eba49f87624d7b73356ea3d421fee7ec081/src/modules/rc_update/params.c#L1299 https://github.com/PX4/PX4-Autopilot/blob/55563eba49f87624d7b73356ea3d421fee7ec081/src/modules/rc_update/params.c#L1267
• When watching Youtube videos on how to operate vehicles using a remote the sticks are refered as roll, pitch, yaw, throttle stick
• When asking multiple people about how to refer to the axes the fastest and most consistent answer was roll, pitch, yaw, throttle

The only two disadvantages I see are:

• For the pitch stick to be consistent with positive values are stick up right it maps to a negative pitch euler angle which is documented and in my eyes acceptable but possibly the main discussion point. I'd suggest to not switch the direction because:
  • Up right stick being positive makes mapping a lot easier
  • Switching throttle and pitch e.g. for RC mode 1 would then also involve switching sign and is more error-prone
  • Staying consistent with the existing direction (also in MAVLink)
• Stick input usages that completely differ from the conventional way of input might get shuffled when remapping the assignments e.g. for RC Mode 1,2,3,4 which was already the case before and I don't see an easy fix without undue complexity.

For the revamping of the Stick class to adapt and be reusable everywhere I suggest to do a follow-up pull request since this one is about the uORB interface.

[junwoo091400]

Was puzzled at first why Pitch could be a problem, but positive pitch command will result in pitch angle going negative direction (pitching down), but else than that I think this is a more intuitive / useful change for the future :+1:

[dagar]

Let's discuss this one on the dev call (or on a separate call) before finalizing.

In a lot of common cases I agree simply going to roll/pitch/yaw is clearer, but it may complicate other usage like rovers or underwater vehicles.

I think ultimately it might depend on if we want to try and keep manual_control mapping completely vehicle agnostic or not.

[junwoo091400]

FYI, iNAV also suffered from this 😊

https://github.com/iNavFlight/inav/pull/2171

[MaEtUgR]

I rebased. Not many conflicts. Some deprecated stick input usages fell away 👍

[MaEtUgR]

Style fixed and ModalAI target specific turtle mode adapted (https://github.com/PX4/PX4-Autopilot/blame/5edbc2f80a0b24082711e0d5095cebd3e3860253/src/drivers/actuators/modalai_esc/modalai_esc.cpp#L895).

[MaEtUgR]

I did hopefully a final rebase now and am planning to merge. So if you have objections please speak up.

[junwoo091400]

Leaving some of the discussions open, so that anyone can come back and revisit them (as they were not the scope of the PR)!

Now, time to change the definition of MANUAL_CONTROL message :wink: https://mavlink.io/en/messages/common.html#MANUAL_CONTROL

[MaEtUgR]

Thanks for your review @junwoo091400 sadly while doing further testing I found what I feared: QGC calibrates differently based on the RC mapping and throttle is in the calibration parameters trimmed differently such that PX4 as a result will map throttle to [0,1] so I'll have to work around that using a calibration translation. Why was it designed so complicated 🤯

https://github.com/mavlink/qgroundcontrol/commit/0577af2e944a0f53919aeb1367d580f744004b2c

[junwoo091400]

QGC calibrates differently based on the RC mapping and throttle is in the calibration parameters trimmed differently such that PX4 as a result will map throttle to [0,1]

Yes, it's coming from this commit: https://github.com/mavlink/qgroundcontrol/commit/0577af2e944a0f53919aeb1367d580f744004b2c

The throttle function of the RC gets it's trim value set to Minimum value directly (as a hot-fix commit from 7 years ago :laughing:), so QGC versions up till now (until we create a PR, if we want to fix this) was limiting the middle setting (trim) to always be the middle.

[junwoo091400]

Thanks for your review @junwoo091400 sadly while doing further testing I found what I feared: QGC calibrates differently based on the RC mapping and throttle is in the calibration parameters trimmed differently such that PX4 as a result will map throttle to [0,1] so I'll have to work around that using a calibration translation. Why was it designed so complicated 🤯

mavlink/qgroundcontrol@0577af2

As a first / sane step, I created a PR to revert this commit: https://github.com/mavlink/qgroundcontrol/pull/10497

[junwoo091400]

Why was it designed so complicated 🤯

Apparently because of 'spring-loaded' Transmitter 😆. @LorenzMeier any backstory on your commit?

[junwoo091400]

But actually @MaEtUgR, the QGC is fetching these values from the RC_CHANNELS message (https://mavlink.io/en/messages/common.html#RC_CHANNELS), and this doesn't necessarily relate to MANUAL_CONTROL mavlink message definition.

Meaning, the QGC PR: https://github.com/mavlink/qgroundcontrol/pull/10497 doesn't depend on changing the definition of MANUAL_CONTROL (which I thought was the case).

Then it seems like it's not such a big issue to tackle (QGC part can be solved by the PR above) 🤔

[junwoo091400]

MANUAL CONTROL message renaming PR is created regardless: https://github.com/mavlink/mavlink/pull/1922

[MaEtUgR]

@junwoo091400 Thanks a lot for your review, additional thoughts and suggestions. Your feedback is highly appreciated just slightly on the verbose side 😇

QGC is fetching these values from the RC_CHANNELS message doesn't depend on changing the definition of MANUAL_CONTROL

I know. The big problem is solving backward compatibility on all sides e.g. QGC, MAVLink to an ambiguous definition that was misused in various out-of-range or other "fits the case" solutions. I explicitly wanted this pr to only solve the PX4 autopilot side and care about MAVLink and QGC in the next step.

[MaEtUgR]

I had to rebase on https://github.com/PX4/PX4-Autopilot/pull/20647 and https://github.com/PX4/PX4-Autopilot/pull/20537 nothing complicated.

After the feedback from yesterday's dev call I found a good solution for the backward compatibility: I do the same as before but not adjusting the throttle trim parameter. That solves the problem of having an adapted parameter when downgrading but also makes sure that it all works like expected with the default calibration everyone gets through QGC. So the door is open for a new calibration and phasing out the old one.

I tested this again on hardware just to be sure. Also note that even though FMUv5x overflows this pr saves ~230bytes but main is overflowing more than that.

In my eyes this is now good to go 🎉

[junwoo091400]

I found a good solution for the backward compatibility: I do the same as before but not adjusting the throttle trim parameter. That solves the problem of having an adapted parameter when downgrading but also makes sure that it all works like expected with the default calibration everyone gets through QGC.

Ah that's such a good idea! (Took me 10 seconds to understand though haha) Changing the run-time applied trim value, and not modifying the parameter value itself is an elegant solution 👍

I guess we should get the QGC PR in quickly! If you have a QGC dev environment, could you test on a real vehicle with real RC tx/rx whether the QGC PR: https://github.com/mavlink/qgroundcontrol/pull/10497 works as expected (Set Trim to mid-value, not min)?

[MaEtUgR]

@junwoo091400 Thanks for your review!

I guess we should get the QGC PR in quickly!

I think we shouldn't over-hurry but rather avoid mistakes that could lead to user problems.

If you have a QGC dev environment

I currently don't sorry. But as I wrote in https://github.com/mavlink/qgroundcontrol/pull/10497#issuecomment-1322296361 we need to be careful. I'd guess with that pr the trim value doesn't necessarily get set to the middle but to where the throttle stick is at the beginning of the calibration which would be not deterministic and often at the bottom. Also think about the case where someone uses the latest QGC including that change but PX4 1.13 or main from yesterday. It would result in a configuration making half of the stick not work anymore for no reason. Let's go through it cleanly on the QGC side.

[junwoo091400]

Yayyyy thanks for this @MaEtUgR !!

[DronecodeBot]

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/driving-backwards-with-a-boat-rover-in-manual-mode/25714/6