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

Standardize arm_and_takeoff #209

Open
tcr3dr opened this issue Jul 16, 2015 · 4 comments
Open

Standardize arm_and_takeoff #209

tcr3dr opened this issue Jul 16, 2015 · 4 comments
Milestone

Comments

@tcr3dr
Copy link
Contributor

tcr3dr commented Jul 16, 2015

From @hamishwillee:

@mrpollo I have tried to standardise on this arm_and_takeoff in all my examples - previously there was an ad-hoc mess of different pre-arm checks (including disabling the checks altogether).

In 99% of cases I think this sort of approach is what people will want to do, so it makes sense to make it easily accessible in a shared libary - part of Vehicle.commands?

We would need to make the system robust so that it would timeout if arming failed, and cope with the case of being called if the vehicle is already airborne (and what to do if not at the correct height). Note also that this method is (deliberately) synchronous, which means that you can't send other commands to the vehicle (e.g. set ROI) while this is executing.

An example of this code: https://github.com/diydrones/dronekit-python/blob/9130f1021b81e7be88f5d548c0ddcfcbf4231e10/examples/guided_set_speed_yaw/guided_set_speed_yaw.py#L16

@tcr3dr
Copy link
Contributor Author

tcr3dr commented Jul 16, 2015

Before making an all-encompassing function do accomplish this task, let's analyze its components and some hypothetical subcommands:

await_initialization()

"""
Arm vehicle and fly to aTargetAltitude.
"""
print "Basic pre-arm checks"
# Don't let the user try to fly while autopilot is booting
if vehicle.mode.name == "INITIALISING":
    print "Waiting for vehicle to initialise"
    time.sleep(1)

await_gps()

while vehicle.gps_0.fix_type < 2:
    print "Waiting for GPS...:", vehicle.gps_0.fix_type
    time.sleep(1)

arm()

print "Arming motors"
# Copter should arm in GUIDED mode
vehicle.mode    = VehicleMode("GUIDED")
vehicle.armed   = True
vehicle.flush()

while not vehicle.armed and not api.exit:
    print " Waiting for arming..."
    time.sleep(1)

takeoff(altitude)
await_altitude(altitude, variance=0.05)

print "Taking off!"
vehicle.commands.takeoff(aTargetAltitude) # Take off to target altitude
vehicle.flush()

# Wait until the vehicle reaches a safe height before processing the goto (otherwise the command 
#  after Vehicle.commands.takeoff will execute immediately).
while not api.exit:
    print " Altitude: ", vehicle.location.alt
    if vehicle.location.alt>=aTargetAltitude*0.95: #Just below target, in case of undershoot.
        print "Reached target altitude"
        break;
    time.sleep(1)

How do we feel about this level of granularity? I feel we should start by refactoring out small components, then arrive at either a consistent set of commands or one larger arm_and_takeoff command afterward.

@hamishwillee
Copy link
Contributor

@tcr3dr The split looks logical. At the end it makes sense to me that there just be one big fat function.

My concerns with the method as it is (for your consideration):

  • Reporting of what is holding up initialisation. I quite like checking against mode initialising but perhaps we can get more granularity on this. Certainly I'd consider reporting boot and calibrating states when MPVehicle: add system_status property #156 is put in.
  • How to handle error cases - e.g. it just doesn't arm. Perhaps the mode wasn't able to change.
  • This method is synchronous. That is useful because you can call it and know that the next movement command won't be executed until you reach the right height. However you might reasonably want to set the ROI or perform some other action that it should be possible to in parallel. With this method you can't until you get to the end of takeoff. You can't do it before either, because (I believe) the first time you change the type of movement command you're using this counts as a "mode" change to the autopilot - so the yaw or ROI or whatever you set gets cleared. This is probably something we just need to live with and document
  • It would be best if the script need not assume that the vehicle is landed when the method is called. At the moment I don't think this is an issue - all the arming and mode checks will fall through and I think the autopilot will just ignore the takeoff command if you're already in the air.
  • Should we add some observer mechanism (generic) for notification of command completion (other than just the fact that it is synchronous)?
  • This is a very different API than all the others - e.g. for a start it is synchronous. Perhaps this implies that it should live in a separate "helper" library. Problem then is we have to start thinking about passing in vehicle objects etc.

@tcr3dr tcr3dr modified the milestones: v1.5.0, v1.4.0 Jul 21, 2015
@atomictom
Copy link

Just a thought: why not have the methods be asynchronous but return something like a Future which can either be ignored or waited on.

First you would get a future like so:

takeoff_future = vehicle.takeoff(100)

Then later on, if you want to wait until the previous command succeeds (or fails):

takeoff_future.wait()
if takeoff_future:
    print "Drone has reached target altitude"
else:
    print "Takeoff failed with error {}".format(takeoff_future.error)

Here I decided to have the truthiness of the future be indicative of an error, but you could use a property like takeoff_future.success.

Since you're not always waiting for a value, but sometimes for a condition to be met, you could set a callback to check for success:

takeoff_future.callback = lambda: vehicle.location.alt >= desired_altitude * .95

Though, callbacks can be a bit unsightly in Python, so another idea is to have each function take in a future as an optional (keyword) parameter so that you can create a future specific to the function and fill in properties prior to using it.

future = Future(takeoff) # pass in the function you want a future for and it will preload some defaults
future.variance = .05 # this field is unique to a takeoff future

vehicle.takeoff(100, future=future)
future.wait()
if not future:
    print future.error

@hamishwillee
Copy link
Contributor

@atomictom I like the idea - very C#. We need an approach that can be used broadly across the whole API to handle asynchronous behaviour in a common way. My preference is something like this.

@tcr3dr tcr3dr modified the milestones: Minor, v1.5.0 Aug 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants