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

Plane: maximum altitude for FBWB and cruise mode #17019

Closed
Supermagnum opened this issue Mar 26, 2021 · 18 comments · Fixed by #19732
Closed

Plane: maximum altitude for FBWB and cruise mode #17019

Supermagnum opened this issue Mar 26, 2021 · 18 comments · Fixed by #19732

Comments

@Supermagnum
Copy link

Supermagnum commented Mar 26, 2021

Feature request

Maximum altitude in centimeters or meters that FBWB, and CRUISE modes will allow. If you attempt to ascend above this altitude then the plane will level off. A value of zero means no limit.

Platform
[ ] All
[ ] AntennaTracker
[ ] Copter
[X] Plane
[ ] Rover
[ ] Submarine

Useful where the law sets limitations on how high above ground it is permitted to fly.
Perhaps also useable for FBWA mode as it can be helpful for those who are learning to fly .

@IamPete1
Copy link
Member

We have already, ALT_HOLD_FBWCM for a min height. We do also currently support a max alt geofence. It does not stop you braking the limit but will take over and fly you home.

@rmackay9 rmackay9 changed the title Maximum altitude for FBWB and cruise mode Plane: maximum altitude for FBWB and cruise mode Mar 26, 2021
@rmackay9 rmackay9 removed the Copter label Mar 26, 2021
@rmackay9
Copy link
Contributor

Copter will already stop at the top of the altitude fence so removing the copter part of this request

@mmk0102
Copy link
Contributor

mmk0102 commented Mar 28, 2021

Can I take this task?
I am a newcomer. It is enough to make it work:

  • add method Plane::check_fbwb_maximun_altitude();
  • add parametr ALT_HOLD_FBWCM_MAX ?

@IamPete1
Copy link
Member

@mmk0102 that would be great, I would be tempted to rename the existing check_fbwb_minimum_altitude to check_fbwb_altitude and do them both in the one function. I the new param might as well be in meters, were not so constrained on param space as we were when the min param was added. Jump onto the AP discord if you need any pointers https://discord.gg/hTvezxYQ

@mmk0102
Copy link
Contributor

mmk0102 commented Mar 28, 2021

Well, ALT_HOLD_FBWCM (min alt) will be in cm, ALT_HOLD_FBWCM_MAX (max alt) will be in meters, all right?
And check_fbwb_minimum_altitude() move to general check_fbwb_altitude().

@mmk0102
Copy link
Contributor

mmk0102 commented Mar 28, 2021

And section Fly-by-wire control is full
// 120: Fly-by-wire control
k_param_airspeed_min = 120,
...
k_param_use_reverse_thrust = 129,
I can use this (from Sensor parameters section):
k_param_imu = 130, // unused
k_param_altitude_mix, // deprecated
or should I add it in the end ?

@IamPete1
Copy link
Member

Yeah, you should add the new param at the end of the list. Feel free to open a draft PR of what you have, it will be easier to review changes that way.

@mmk0102
Copy link
Contributor

mmk0102 commented Mar 31, 2021

Did it as draft PR.

@mmk0102
Copy link
Contributor

mmk0102 commented Apr 9, 2021

Well, can we merge it? Or something still wrong?

@IamPete1
Copy link
Member

IamPete1 commented Apr 9, 2021

@mmk0102 Your PR is still draft, you can convert it to a full PR once its ready

@shiv-tyagi
Copy link
Contributor

@mmk0102 Are you still working on this issue?

@Supermagnum
Copy link
Author

Any news here? Is this resolved?

@IamPete1
Copy link
Member

IamPete1 commented Jan 5, 2022

@Supermagnum No update, PRs welcome.

@Supermagnum
Copy link
Author

@Supermagnum No update, PRs welcome.

I hope someone resolves this soon.

@khanasif786
Copy link
Contributor

@IamPete1 does the last changes done by @mmk0102 are Right. I am currently testing those.

@khanasif786
Copy link
Contributor

I am a newcomer. While @mmk0102 has done most of the work on these. All credit goes to him. There was only 1 mistake, that instead of using 100 he used 1000 to change meter to cms. I have tested it in FBWB mode, it's working. @IamPete1 Sir, Please suggest changes if any.

@Supermagnum
Copy link
Author

Any progress on this? It looks like it's ready for merge.

@khanasif786
Copy link
Contributor

@Supermagnum, Sorry for the late reply. It is in progress currently i was busy somewhere.. We are doing this using fence max and min altitude instead of adding a new parameter for fBWB limit.

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

Successfully merging a pull request may close this issue.

7 participants