Takeoff command should check for NAN/Inf

[hamishwillee]

Vehicle.commands.takeoff could be called with a NAN or infinite value. We should probably check for those as either might result in a fly-away vehicle.

    def takeoff(self, alt=None):
        if alt is not None:
            altitude = float(alt)
            self.__module.master.mav.command_long_send(self.__module.target_system,
                                                    self.__module.target_component,
                                                    mavutil.mavlink.MAV_CMD_NAV_TAKEOFF,
                                                    0, 0, 0, 0, 0, 0, 0,
                                                    altitude)

[djnugent]

How does pymavlink handle NaN or infinity?

[hamishwillee]

No idea! Apparently ArduPilot doesn't check this either (according to @billbonney). My take on this is that it would be good API design to raise an exception for invalid input if NaN or Inf appears here.

[djnugent]

veh.commands.takeoff(float("nan")) is denied by arducopter veh.commands.takeoff(float("inf")) crashed arducopter or SITL!!!!

[billbonney]

I think in any case where you are converting a variable using float, you need to check for NaN or Inf. best of all would be check the value for altititude is within a set range in a global prefence, where alt isn't above the legal limit like 120m for example.

[hamishwillee]

@billbonney Not sure about the last point - legal limits change, and so might a reasonable alt for a different vehicle (e.g. ArduRocket or a balloon:-).

[billbonney]

@hamishwillee Agreed limits change, but having a default to stop flyaway from a programming mistake is a good idea I think. it's a global setting that can be changed, but its there so when you do a calculation that is way off, your drone doesn't disapear is a Good Thing. Maybe even having it set to lower value would make sense. even at 30m it seems miles away ;)

It would just require a function that verifies any altitude setting to be within your global max for alt.

[djnugent]

Cant we just read the altitude from the fence altitude parameter and have that be the limit? And if it does not exist, then limit it to 400 ft.

[hamishwillee]

I think we're all agreed that we should check for invalid input indicative of programmatic error (NaN or Inf). Re fly-away I think a check of some kind makes sense, but I'd argue that it should be in ArduPilot rather than DroneKit.

And if it does not exist, then limit it to 400 ft.

IMO no - this should always be on some parameter rather than hard coded. What that parameter is would be decided by ArduPilot team, if they agree with this idea.

[mrpollo]

I think we need to implement some validation throughout the api, since ardupilot is not enforcing this we should check against this type of problems and should be considered bugs and also reported upstream when needed.

@hamishwillee can you please report upstream?

@djnugent can you send a PR to enforce a float argument else raise a ValueError exception.

@billbonney we should definitely do something to enforce this type of restrictions, I think the right way to do it is to keep ourselves vendor agnostic, if a vendor wants to ship a drone that uses dronekit (eg: 3DR Solo) he should be able to easily enforce the type of checks that are true to his market, a standardized way to do this should be discussed by the tech committee of DK and implemented. Thanks for this suggestion. :+1:

[djnugent]

@mrpollo will do

[hamishwillee]

@mrpollo reported issue upstream https://github.com/diydrones/ardupilot/issues/2515

I am not so sure that we should enforce this type of restriction in DK. Mostly because I'm not sure how it should work "properly" with things like the geofence altitude. Personally I'd start with whoever owns the MAVLink protocol, and enforce whatever that group decides is the right behaviour.

[billbonney]

@hamishwillee I think it's a good idea for a 'arduino like' easy to use API to add some checks and balances for when a novice does something wrong. If it's not for novices, I wouldn't worry about it.

You can't really add it to MAVLink as that's a protocol, and not really target at a 'easy to SDK API' as is dronekit

[hamishwillee]

@billbonney Thanks for explanation. @mrpollo I guess action is for you to take this to the tech committee and then close this?

[hamishwillee]

@mrpollo @tcr3dr I am closing this as the checks on NaN and Inf are now present.

Ramon, Can you create some method in place to track we should definitely do something to enforce this type of restrictions, I think the right way to do it is to keep ourselves vendor agnostic, if a vendor wants to ship a drone that uses dronekit (eg: 3DR Solo) he should be able to easily enforce the type of checks that are true to his market, a standardized way to do this should be discussed by the tech committee of DK and implemented. (another issue or whatever)