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

FlightTaskOrbit: add parameter for orbit #12751

Closed
wants to merge 2 commits into from

Conversation

Stifael
Copy link
Contributor

@Stifael Stifael commented Aug 20, 2019

This PR adds FlightTaskOrbit parameters for:

  • max centripetal acceleration
  • max radius
  • min radius
  • max velocity

* @decimal 1
* @group FlightTaskOrbit
*/
PARAM_DEFINE_FLOAT(FLT_ORB_A_MAX, 2.0f);
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we use MPC_ACC_HOR(_MAX)? here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two issues using the same parameter

  1. FLT_ORB_A_MAX is the centripetal acceleration and MPC_ACC_HOR(_MAX) is more meant for linear acceleration (which basically defines the velocity setpoint ramp)
  2. they have different default values. MPC_ACC_HOR_MAX is set to 5, and up to now the centripetal acceleration used in orbit was set to 2.

Although I also support in having fewer parameters, at the same time I see a problem with having one parameter for several different modes. For instance, MPC_ACC_HOR is now used for FlightTaskAutoLine, FlightTaskAutoLineSmoothVel, FlightTaskManualPosition and FlightTaskManualPositionSmoothVel. Since this parameter is still a tuning parameter defining the setpoint ramp, it might very well be the case that different flight-modes require a different value of this parameter.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a difference between "centripetal acceleration" and "linear acceleration", it's the same thing. Also, since that value is used to saturate the speed and radius, using the MPC_ACC_HOR (used in all auto modes, default value of 3m/s2) seems safe.

I only see the point if for some reason you want to have less acceleration during orbit than during normal flight, but why would you like that?

Copy link
Contributor Author

@Stifael Stifael Aug 21, 2019

Choose a reason for hiding this comment

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

I don't see a difference between "centripetal acceleration" and "linear acceleration", it's the same thing

not really. linear acceleration MPC_ACC_HOR (at least that's was the case when I added it) was added to control the velocity-ramp along the line during Auto-mode. There is no ramp-limit perpendicular to the line. For Manual Position-control, it is different, but then again I consider it as a tuning parameter to achieve a certain user-feel.

"centripetal acceleration", on the other hand, is more about the change in velocity-vector.

From a testing perspective, all I know is that we currently use MPC_ACC_HOR = 6 and the orbit centripetal-acceleration was set to 2. If we combine both into one parameter, then this requires additional testing.

Copy link
Member

@MaEtUgR MaEtUgR Aug 31, 2019

Choose a reason for hiding this comment

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

I don't see a difference between "centripetal acceleration" and "linear acceleration",

It's physically the same but in reality there's a big difference just because of the timespan the drone usually applies them. In an Orbit if you pull the radius smaller you will hit the acceleration limit faster than most people might expect and if the radius is the only thing you change you will stay accelerating with this maximum value 100% of the orbit time. 2m/s is my experimentally found value that also makes sense to fly with a bigger drone. A racer could easily just burn its battery with "double the thrust" but with a bigger drone you might want quick brakes from high speeds that have short acceleration periods but not extended periods of time spent with high thrust values.

It's similar to a "cruise acceleration" like the cruise speed if that makes sense...

The flood of parameters is the reason why I didn't add them yet but since the feature is used more and more it should be configurable.

@hamishwillee hamishwillee added the Documentation 📑 Anything improving the documentation of the code / ecosystem label Aug 20, 2019
@hamishwillee
Copy link
Contributor

hamishwillee commented Aug 20, 2019

This has a docs impact, specifically http://docs.px4.io/master/en/flight_modes/orbit.html#parameterslimits will need to be updated.

I should remember to make updates, but can one of you agree to ping me when this goes in? (or make the changes in docs yourselves)?

@MaEtUgR MaEtUgR force-pushed the px4/feature/increased-orbit-radius-develop branch from 2c16593 to 72d7d31 Compare August 31, 2019 11:50
@MaEtUgR
Copy link
Member

MaEtUgR commented Aug 31, 2019

I rebased on master without conflicts and pushed a commit on top to fix my comments. Feel free to correct as you wish. Be aware that there's a conflicting bigger pr here: #12664 Since that change is already in production and well tested I would like to get it in before this one just to safe time.

@MaEtUgR MaEtUgR force-pushed the px4/feature/increased-orbit-radius-develop branch 2 times, most recently from cf0a731 to 1133ef5 Compare August 31, 2019 12:45
MaEtUgR
MaEtUgR previously approved these changes Aug 31, 2019
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 rebased after #12664 and SITL tested.
@Stifael please tell me if I misinterpreted or broke something with the corrections.

@MaEtUgR MaEtUgR force-pushed the px4/feature/increased-orbit-radius-develop branch from 1133ef5 to 2166b2f Compare October 13, 2019 14:44
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.

Rebased again after #13058
@Stifael If there are no objections I'll merge this soon.

PARAM_DEFINE_FLOAT(ORBIT_ACC_MAX, 2.0f);

/**
* Minimum radius of orbit
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, are these to prevent someone shrinking a radius too low? Why does it have a maximum value of 10m?

Copy link
Member

@MaEtUgR MaEtUgR Oct 17, 2019

Choose a reason for hiding this comment

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

Yes, if the radius is <1m the vehicle doesn't do anything useful anymore and it can be confusing. I don't know why the maximum is 10m. Probably the minimum radius should not be higher than the default radius of the circle QGC initially puts on the map for you to drag.

@hamishwillee
Copy link
Contributor

Will this be in PX4 v1.10?

Docs to update are here: http://docs.px4.io/master/en/flight_modes/orbit.html

@stale
Copy link

stale bot commented Jan 15, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Jan 15, 2020
@LorenzMeier
Copy link
Member

Closing this as stale.

@LorenzMeier LorenzMeier deleted the px4/feature/increased-orbit-radius-develop branch January 17, 2021 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation 📑 Anything improving the documentation of the code / ecosystem Multirotor 🛸 stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants