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

MC acro: add separate params to configure yaw expo #9358

Merged
merged 2 commits into from
Apr 26, 2018
Merged

Conversation

bkueng
Copy link
Member

@bkueng bkueng commented Apr 23, 2018

So that it can be customized better according to personal preferences.

The limitation of 16 chars did not allow to use a better param name.

@hamishwillee This probably requires updates to the user-guide.

This matches the maximum rates for the attitude controller.
So that it can be customized better according to personal preferences.

The limitation of 16 chars did not allow to use a better param name.
@bkueng bkueng requested a review from MaEtUgR April 23, 2018 12:49
@dagar
Copy link
Member

dagar commented Apr 23, 2018

I realize why this currently makes sense, but could we briefly consider if things like manual input expo should be kept generic?

In the past we've discussed getting the manual handling completely out of the attitude and rate controllers for all vehicle types and moved into a new "stick mapper" module. At that point we could cleanly keep all of this input configuration generic with new features applying to all vehicle types.

I only mention this now to avoid possible parameter churn, unnecessary platform bloat (MC_ACRO_EXPO_Y/FW_ACRO_EXPO_Y/GND_ACRO_EXPO_Y), or fragmentation (feature gaps or nearly identical features implemented differently per vehicle now requiring different documentation).

* @decimal 2
* @group Multicopter Attitude Control
*/
PARAM_DEFINE_FLOAT(MC_ACRO_SUPEXPOY, 0.7f);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect this to be MC_ACRO_SUPEXPO_Y for consistency and readability

Copy link
Member Author

Choose a reason for hiding this comment

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

As I wrote:

The limitation of 16 chars did not allow to use a better param name.

So yes your suggestion makes more sense, but exceeds the maximum length unfortunately.

@@ -395,8 +394,20 @@ PARAM_DEFINE_FLOAT(MC_ACRO_Y_MAX, 540.0f);
PARAM_DEFINE_FLOAT(MC_ACRO_EXPO, 0.69f);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename MC_ACRO_EXPO_RP for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

Renaming existing params is a pain, and I try to avoid it if at all possible. We can do it within the scope of a larger refactoring.

Copy link
Member

Choose a reason for hiding this comment

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

I think it should not be that big of a deal in this case. You can change the name if you want.

@@ -409,6 +420,20 @@ PARAM_DEFINE_FLOAT(MC_ACRO_EXPO, 0.69f);
*/
PARAM_DEFINE_FLOAT(MC_ACRO_SUPEXPO, 0.7f);
Copy link
Contributor

Choose a reason for hiding this comment

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

MC_ACRO_SUPEXPO_RP ?

@hamishwillee
Copy link
Contributor

@bkueng Assuming this goes ahead, the only place superexpo is mentioned is in the Acro mode.

As I understand it we now have three params that would need to be added.

  • MC_ACRO_EXPO (Roll, pitch) and MC_ACRO_EXPO_Y (Yaw). These define whether the sticks inputs are mapped linearly or as a cubic function.
  • MC_ACRO_SUPEXPOY and MC_ACRO_SUPEXPOY - IFF a cubic function, affects the curve to define how flat the response is - ie a larger value creates an effective "deadzone" of response in the centre of the joysticks.

Is that correct?

@bkueng
Copy link
Member Author

bkueng commented Apr 24, 2018

@hamishwillee

MC_ACRO_SUPEXPOY and MC_ACRO_SUPEXPOY - IFF a cubic function, affects the curve to define how flat the response is - ie a larger value creates an effective "deadzone" of response in the centre of the joysticks.

I think it's easiest if you play with different param values and see how the curve changes: https://www.desmos.com/calculator/yty5kgurmc. g is the superexpo and f is the expo param, r is the maximum rate. Superexpo can be chosen entirely independent from expo.

@dagar

I only mention this now to avoid possible parameter churn, unnecessary platform bloat (MC_ACRO_EXPO_Y/FW_ACRO_EXPO_Y/GND_ACRO_EXPO_Y), or fragmentation (feature gaps or nearly identical features implemented differently per vehicle now requiring different documentation).

I agree, we should not have one expo param per type. But what you definitely want is different settings for different flight modes. I suggest the following steps moving forward:

  • Merge this as is if everyone agrees
  • when introducing expo to fixed-wing acro (which is planned), choose names that are vehicle-unspecific
  • do the refactoring of MC acro params to use the generic ones
  • do the internal refactoring & stick mapper and more renaming if needed.

@hamishwillee
Copy link
Contributor

MC_ACRO_SUPEXPOY and MC_ACRO_SUPEXPOY - IFF a cubic function, affects the curve to define how flat the response is - ie a larger value creates an effective "deadzone" of response in the centre of the joysticks.

I think it's easiest if you play with different param values and see how the curve changes: https://www.desmos.com/calculator/yty5kgurmc. g is the superexpo and f is the expo param, r is the maximum rate. Superexpo can be chosen entirely independent from expo.

That helps A LOT thanks.

While strictly accurate, it is hard to understand the relationships from the descriptions in the parameters. I'm not even sure that someone would necessarily understand that these parameters help provide the mapping between stick position and setpoint.

While we could try an explain all this in the parameters, I suggest we update the Acro mode docs to explain this and use your link to demonstrate it. We might then link to the new guide from the parameter documentation?

MC_ACRO_EXPO(FLOAT) 
- Acro Expo factor applied to input of pitch and roll axes (see also [Link to Acro mode input scaling])
- Comment: 0 Purely linear input curve 1 Purely cubic input curve

@MaEtUgR
Copy link
Member

MaEtUgR commented Apr 26, 2018

  • Merge this as is if everyone agrees
  • when introducing expo to fixed-wing acro (which is planned), choose names that are vehicle-unspecific
  • do the refactoring of MC acro params to use the generic ones
  • do the internal refactoring & stick mapper and more renaming if needed.

Exactly what I wanted to suggest.

@hamishwillee See #8036 for the original implementation pr.

ie a larger value creates an effective "deadzone" of response in the centre of the joysticks.

Not exactly, the goal is to have a high turn rate with maximum stick input but to have a zone of lower sensitivity for small corrections close to the stick center. A deadzone is not lower sensitivity, it's a zone where there's zero action to the stick to compensate for hardware tolerances and calibration. According to my opinion you don't want to have a deadzone in acro mode because results in a gap for your anyways necessary contiunous control input. A deadzone is desirable e.g. in position hold such that when you let go of the stick it does not drift at all because of your stick inaccuracy.

Copy link
Member

@MaEtUgR MaEtUgR left a comment

Choose a reason for hiding this comment

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

I thought about it when implementing the rc curves but assumed I'm only increasing parameter number without someone actually using a different yaw value. And it's easy to add like you did right now. lgtm

@@ -347,7 +347,7 @@ PARAM_DEFINE_FLOAT(MC_YAWRAUTO_MAX, 45.0f);
*
* @unit deg/s
* @min 0.0
* @max 1000.0
* @max 1800.0
Copy link
Member

@MaEtUgR MaEtUgR Apr 26, 2018

Choose a reason for hiding this comment

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

Did you do experiements about the overshoot not accidentally breaching the 2000deg/s gyro limit barrier? I don't have experience with >1000 deg/s rates.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, not that high, but I did go over 1000. Would be interesting to know indeed.
Looks like others are actually going to the 2000 limit: betaflight/betaflight-configurator#997

@bkueng
Copy link
Member Author

bkueng commented Apr 26, 2018

@hamishwillee I agree, it's very hard to explain it with a textual description only. We can add the link to the param documentation, but I'm not sure if it helps, because I don't think QGC will show it as clickable link.
The fancy solution would be that QGC shows the curve on a stick mapping setup page.

@bkueng bkueng merged commit f2cee99 into master Apr 26, 2018
@bkueng bkueng deleted the acro_yaw_expo branch April 26, 2018 12:17
@hamishwillee
Copy link
Contributor

hamishwillee commented Apr 27, 2018

@bkueng @MaEtUgR That is very helpful thank you.

  1. I have created a stick mapping section in Acro mode docs for old/current implementation - that can be extended when this is accepted:

  2. Suggest we consider an update to the parameter descriptions to something like below. They aren't perfect but they do make it more clear that this is Acro mode, that expo stands for exponential, that it is for tuning the input curve, and that the supexpo (I just see it isn't "super" but "sup" grrr) is related to the expo.

  • MC_ACRO_EXPO - Acro mode "exponential" factor for tuning input curve shape (applied to input of all axis: roll, pitch, yaw)
  • MC_ACRO_SUPEXPO - Acro mode "superexpo" factor for refining input curve shape tuned using MC_ACRO_EXPO (applied to input of all axes: roll, pitch, yaw).

@MaEtUgR
Copy link
Member

MaEtUgR commented May 2, 2018

@hamishwillee That's very nice documentation, if desired I could provide a calculator to get the maximal angular rate, expo and superexpo parameters from the wide spread not independent parametrized rates configuration of betaflight and all the related https://oscarliang.com/rc-roll-pitch-yaw-rate-cleanflight/. Such that users who have some experience with this configuration can convert their favorite settings... Probably this would even make sense to be embedded in the UI.

@hamishwillee
Copy link
Contributor

@MaEtUgR Well, it is certainly better than what was there before (nothing) :-). I like betafight's explanation too, though I have tried to be more terse and let the diagrams do the talking. Do you think it is worth/OK to provide that link as "further information"?

I don't understand exactly what you are proposing above (ie I don't know what "wide spread" means in " provide a calculator to get the maximal angular rate, expo and superexpo parameters from the wide spread"

Are you suggesting you can create a tool to get a user's settings in CleanFlight and convert that into our parameters?

If not, can you explain what you are proposing, who would use it, and where would it live? Sorry if I am being dumb.

@MaEtUgR
Copy link
Member

MaEtUgR commented Jun 4, 2018

@hamishwillee Sry for reaction time. Nothing to worry about your explanation, I'm not suggesting to change it.

What I was offering is a small conversion tool to use on the website that converts the parameters most race firmwares which indirectly fork from cleanflight, betaflight and all these to ours. They use RC Rate, RC Expo, Super Rate which all depend on each other and are therefore hard to tune. I could write up a small script for the calculations back and forth.

EDIT: I created an issue for that: #9591

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants