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

Added takeoff/land with given average velocity commands to high level commander #624

Merged

Conversation

ntamas
Copy link
Contributor

@ntamas ntamas commented Sep 24, 2020

As discussed with @krichardsson via email.

This PR adds support for two additional commands in the high level commander: takeoff with given average velocity and landing with given average velocity. It also adds support for specifying altitudes relative to the current altitude in these commands. Zero and negative velocities instruct the Crazyflie to pick a "safe" takeoff / landing velocity instead, which is currently set to 0.5 m/sec.

I haven't had the chance to test this in our lab yet (only at home on my desk), hence the PR is still in draft status. I plan to do the lab tests tomorrow -- until then, any feedback is appreciated.

(For what it's worth, it would be nice to have a GO_TO_WITH_MAX_VELOCITY command as well in the high level commander, with separate limits for the XY plane and the Z axis. Having safe defaults for these would allow higher level interfaces to provide a "go to" command that takes the target coordinate only and figures out the rest).

@krichardsson
Copy link
Contributor

Thanks for the pull request!

It looks good to me :-)
Just a couple of comments:

  1. you could use a variable or define for the default 0.5 m/s, that would make it easier to modify if someone wants a different value.
  2. Even fancier would be to connect it to a parameter to make it possible to modify in runtime
  3. Nice touch with the relative heigh!

GO_TO_WITH_MAX_VELOCITY could be interesting, the question might be what max velocity is? I suppose it will depend on positioning system, environment, controller and so on. It should be possible to find a reasonable default value though and if it is configurable, it can be adjusted based on the system.

@ntamas
Copy link
Contributor Author

ntamas commented Sep 25, 2020

So we gave it a test today in the lab and it seemed to work.

Even fancier would be to connect it to a parameter to make it possible to modify in runtime

Sure, why not? Do you have any preference for the name of the parameter and the parameter group to put this in?

GO_TO_WITH_MAX_VELOCITY could be interesting, the question might be what max velocity is?

It might not have been completely obvious; the command would receive the maximum allowed velocity in the XY plane and in the Z direction as parameters, and use the smaller of the two to plan the trajectory (depending on how much you need to move horizontally and vertically). The idea would be that one could simply specify, say, 1.5 m/sec in XY and 0.5 m/sec in Z and let the controller figure out whether it needs to hit the limit vertically or horizontally.

@krichardsson
Copy link
Contributor

Sure, why not? Do you have any preference for the name of the parameter and the parameter group to put this in?

Maybe something like hlCommander.vtoff and hlCommander.vland?

It might not have been completely obvious; the command would receive the maximum allowed velocity in the XY plane and in the Z direction as parameters, and use the smaller of the two to plan the trajectory (depending on how much you need to move horizontally and vertically). The idea would be that one could simply specify, say, 1.5 m/sec in XY and 0.5 m/sec in Z and let the controller figure out whether it needs to hit the limit vertically or horizontally.

Ah! I get it. Could you please add a new pull request for this functionality?

@naiveHobo
Copy link

In addition to takeoff/land with average velocity, it would make sense to add a go to command with average velocity too right?

@ntamas
Copy link
Contributor Author

ntamas commented Sep 28, 2020

Yes, that was the subject of the last paragraph in my first comment. I'll try to address this in a separate PR, although this will require more extensive changes to planner.c if I am not mistaken.

@ntamas ntamas marked this pull request as ready for review September 29, 2020 19:13
@krichardsson krichardsson merged commit 0d09f70 into bitcraze:master Oct 1, 2020
@krichardsson
Copy link
Contributor

Merged. Thanks!

@knmcguire knmcguire added this to the next-release milestone Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants