-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Conversation
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.
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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MC_ACRO_SUPEXPO_RP ?
@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.
Is that correct? |
I think it's easiest if you play with different param values and see how the curve changes: https://www.desmos.com/calculator/yty5kgurmc.
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:
|
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?
|
Exactly what I wanted to suggest. @hamishwillee See #8036 for the original implementation pr.
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. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@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. |
@bkueng @MaEtUgR That is very helpful thank you.
|
@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. |
@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. |
@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 |
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.