-
Notifications
You must be signed in to change notification settings - Fork 17.3k
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
Simplify Autorotation Mode Switching and Consolidate Autorotation State in RSC #27786
base: master
Are you sure you want to change the base?
Simplify Autorotation Mode Switching and Consolidate Autorotation State in RSC #27786
Conversation
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.
just a quick look.
2f74720
to
fd536e5
Compare
4201b9a
to
395f524
Compare
Added to DevCallEU for a quick look over/general feedback. Not looking to get this merged today. |
cf7e8c5
to
dda9046
Compare
dd680e5
to
3cb4d51
Compare
6e22ddc
to
e5a8278
Compare
I am pretty happy with this PR now. It has been tested IRL now. For complete transparency, this code was actually tested with the code in #28209 on top. I do like this change to de-couple the flight mode from the interlock. It is far less daunting to test the flight mode for the first time without removing the power with the interlock. This makes for a much nicer user setup and test work flow. The key bit that we need to ensure is working well, is the autorotation state inside the RSC as that code will be available to users to download on 4.6-dev once this is merged. This plot illustrates the RSC autorotation state transitioning from "active" to "bailing out" to "stopped" During test the vehicle bailed out from the glide smoothly. The setup that I tested in real life is using a governor in the ESC, and the autorotation signal window was used to invoke a rapid spool up. I have tested this code with Heli's head speed governor, a lot in Real Flight. It does work, however, I will say that Heli's governor does not play that nice with autorotations. However, this is an issue with way that the governor is currently written. The over-speed/under-speed faults are significantly more likely to be tripped when recovering from an autorotation and the governor just gives up at that point. This is an existing issue, that is not a problem with this code being added. I am simply raising this for awareness now, and I will address the issue with the governor in a future PR. Also for complete transparency, since testing this IRL I have done some restructuring to move the H_RSC_AROT params to the RSC_Autorotation object. I did not change the logic of the mode, and have re-tested in the sim. It was just something I noticed in testing, that we should have the RSC_AROT params hidden if not enabled. Param conversions have been added for moving the parameters. This has also been tested. |
@MattKear I agree with the premise of this PR. It seems a little odd having to switch over to autorotate mode and then disable interlock to initiate the autorotation. I have tested this in Real Flight sim. I plan to look at the follow on PR this week. |
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.
@MattKear I looked through this a little more thoroughly. It is a pretty extensive change, for sure. A little surprised to see the new RSC_autorotation library but it looks like it simplified a bunch in RSC and the motors library.
Did you check the cooldown feature to ensure it still works as designed. I know there is a test for the turbine start but I'm not sure if there is one for the cooldown timer.
I am happy with this and approved but want to ensure the cooldown timer still works.
Thanks for taking a look at this. Yeah, the rework was for a few reasons:
|
No, I have not tested the turbine cool down functionality. What should I look for to see that it still works? |
@MattKear checking it is pretty easy. It is used for ICE or turbine engines. You will need to set a non zero H_RSC_IDLE. It will raise that by 50% for the cooldown time specified in H_RSC_CLDWN in seconds after motor interlock is disabled (RSC State is Idle). You should also verify that it doesn’t use the cooldown timer feature if the aircraft is in autorotation. |
@bnsgeyer ok, thanks Bill. I will test and report back. |
|
||
// check if we need to use autorotation idle throttle | ||
if (autorotation.get_idle_throttle(_idle_throttle)) { | ||
// if we are here then we are autorotating |
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.
@MattKear this guarantees the aircraft is in autorotation even if the throttle idle value is zero. I guess if you’re in our autorotation, it returns a true value for this call?
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.
If you are in the autorotation mode the function autorotation.get_idle_throttle() populates the _idle_throttle variable with the value set by H_RSC_AROT_IDLE and returns true. This is then assigned to the control output and returns early. if we are not in the autorotation then _idle_throttle is not modified and the autorotation.get_idle_throttle() returns false giving access to the subsequent cases.
In the case of both turbine and piston, they would have to set H_RSC_AROT_IDLE to match H_RSC_IDLE to maintain the engine at idle during the autorotation.
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.
@MattKear I fixed the cooldown feature in this PR. it required that I change the functionality of get_idle_throttle(). One because when AROT_IDLE was zero then it would use the cooldown idle. and two, for some reason it would always use the cooldown throttle even when I used the autorotation state. Look at what I did and let me know what you think.
@Ferruccio1984 checkout and test the changes. I think that works as it was meant to work.
@@ -147,6 +148,10 @@ const AP_Param::GroupInfo AP_MotorsHeli::var_info[] = { | |||
// @User: Standard | |||
AP_GROUPINFO("COL_LAND_MIN", 32, AP_MotorsHeli, _collective_land_min_deg, AP_MOTORS_HELI_COLLECTIVE_LAND_MIN), | |||
|
|||
// @Group: RSC_AROT_ | |||
// @Path: ../AP_Motors/AP_MotorsHeli_RSC.cpp |
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.
// @Path: ../AP_Motors/AP_MotorsHeli_RSC.cpp | |
// @Path: ../AC_Autorotation/RSC_Autorotation.cpp |
This should fix the test that isn't passing CI
This PR does the following:
Reasons for this:
This PR is WIP. I still have the following to address: