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.

Sign up for GitHub

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

Jump to bottom

Plane: Add VTOL descent rate, convert existing rate to climb rate only #16927

Merged
merged 1 commit into from
Mar 23, 2021
Merged

Plane: Add VTOL descent rate, convert existing rate to climb rate only #16927

merged 1 commit into from
Mar 23, 2021

Conversation

Hwurzburg
Copy link
Collaborator

Co-authored-by: Reko Meriö K9260@student.jamk.fi
designate existing pilot climb rate for ascent only
add pilot descent rate max param

IamPete1
IamPete1 requested changes Mar 16, 2021
View reviewed changes
ArduPlane/quadplane.cpp Outdated Show resolved Hide resolved
ArduPlane/quadplane.cpp
@@ -39,14 +39,24 @@ const AP_Param::GroupInfo QuadPlane::var_info[] = {
AP_SUBGROUPPTR(pos_control, "P", 17, QuadPlane, AC_PosControl),

// @Param: VELZ_MAX
Copy link
Member

Choose a reason for hiding this comment

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

It is safe to just change it to VELZ_MAX_UP, not sure if its better to or not tho

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wanted to allow people who have changed the value from defualt to still use it when copying params back into a new firmware load... or a new board....thought changing the param name would prevent that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

discuss at devcall

Copy link
Contributor

Choose a reason for hiding this comment

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

If one is down, the other should be up, otherwise it's just confusing, IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

could parameter conversion somehow be used to convert Q_VELZ_MAX, someone tries to set that from an old param file into UP and DN of that value, allowing old param files to work with the new code that changes VELX_MAX to MX_UP?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope. We don't have any parameter change hooks to do something like that. If we did we'd already have done a lot more sanity checking on some params when people try to set them.

@Hwurzburg Hwurzburg force-pushed the qpvelz branch 2 times, most recently from 3b51226 to 97f8ba0 Compare March 17, 2021 13:29
@Hwurzburg
Copy link
Collaborator Author

climbrate

IamPete1
IamPete1 reviewed Mar 17, 2021
View reviewed changes
ArduPlane/qautotune.cpp Outdated Show resolved Hide resolved
@Hwurzburg Hwurzburg force-pushed the qpvelz branch 2 times, most recently from 84c1569 to 28a5393 Compare March 17, 2021 22:08
IamPete1
IamPete1 reviewed Mar 17, 2021
View reviewed changes
ArduPlane/quadplane.cpp Outdated Show resolved Hide resolved
@peterbarker
Copy link
Contributor

Would be nice to see an autotest for this.

@robustini
Copy link
Contributor

robustini commented Mar 19, 2021 •
edited
Loading

This is absolutely useful, after all it has already been present in the Copter for years.
It is correct that the two speeds are separated, since during the ascent we can exaggerate, but not in the descent.
For example a non-vectored tailsitter can go up to 5 meters per second, but in descent it is better not to exceed 1/1.5 m/s.

@Hwurzburg Hwurzburg force-pushed the qpvelz branch 3 times, most recently from 4a450ba to e73d835 Compare March 21, 2021 13:02
peterbarker
peterbarker reviewed Mar 21, 2021
View reviewed changes
Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

Looks fine to me - but the name question needs to be settled.

ArduPlane/quadplane.cpp Outdated Show resolved Hide resolved
ArduPlane/quadplane.cpp Outdated Show resolved Hide resolved
ArduPlane/quadplane.h Outdated Show resolved Hide resolved
ArduPlane/quadplane.cpp Outdated Show resolved Hide resolved
@Hwurzburg Hwurzburg force-pushed the qpvelz branch 4 times, most recently from 82ba4ae to 22b8502 Compare March 22, 2021 03:27
@Hwurzburg
Plane: Add VTOL descent rate, convert existing rate to climb rate only
fc85e93 
Co-authored-by: Reko Merio K9260@student.jamk.fi
Co-authored-by: Peter Barker <pb-gh@barker.dropbear.id.au>
@rmackay9
Copy link
Contributor

I think to be consistent with Copter it would be good to call the parameters PILOT_SPEED_UP and PILOT_SPEED_DN

@Hwurzburg Hwurzburg added the WikiNeeded needs wiki update label Mar 23, 2021
@peterbarker peterbarker merged commit 8aabf7c into ArduPilot:master Mar 23, 2021
@peterbarker
Copy link
Contributor

Merged, thanks @Hwurzburg

@Hwurzburg Hwurzburg deleted the qpvelz branch March 27, 2021 12:55
@Hwurzburg Hwurzburg removed the WikiNeeded needs wiki update label Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WickedShell WickedShell WickedShell left review comments

@peterbarker peterbarker peterbarker left review comments

@IamPete1 IamPete1 IamPete1 requested changes

@tridge tridge tridge approved these changes

Assignees
No one assigned
Labels
Plane
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Hwurzburg @peterbarker @robustini @rmackay9 @WickedShell @tridge @IamPete1 @CraigElder