Make all attributes read-only and have explicit setter methods

[hamishwillee]

Most attributes are read-only but we have a number of writable attributes: Vehicle.mode, Vehicle.armed, Vehicle.airspeed and Vehicle.groundspeed, Vehicle.home_location.

There is nothing wrong with writable attributes "in general" but in this use case they aren't great because they imply attributes can be written synchronously and immediately ... and in our application they can't. This leads to "hey what" moments, means that we need to write somewhat complicated code to check the success of a write, and makes it easy to miss these checks.

The proposal is to make all the attributes read-only and move to explicit setters. The setters would be asynchronous, but provide wait_ready to allow synchronous use, timeout and callback for handling a fail case (with an exception being raised if there is no callback defined):

send_message_set_armed(wait_ready=False, timeout=10, retries=1, callback=None)

Our current code should look like something this semi-pseudo code (it actually doesn't, and many of our examples will fall into an infinite loop if arming fails):

    vehicle.armed = True    

    # Confirm vehicle armed before attempting to take off
   arming_count=0
    while not vehicle.armed:      
        print " Waiting for arming..."
        vehicle.armed = True
        arming_count+=1
        if arming_count==20:
            raise Exception("arming failed")
        time.sleep(1)

Changing to the new API would be the same if used asynchronously, but could saftely used synchronously as shown below:

    vehicle.send_message_set_armed(wait_ready=True)  

Thoughts?

[peterbarker]

If set_armed corresponds to is emitting a single "change mode" message, and that message gets lost, that last example is an infinite loop. This is all through our example code.

It could be fixed by dropping vehicle.set_armed() into the loop - but its an easy pit to fall into.

[peterbarker]

I would recommend having methods always named by what they do. "set_armed" does not set the vehicle to armed. If it sends a message to the vehicle indicating it should arm - call it send_message_set_armed(). "try_set_armed()" would be even better as it removes the details as to how the arming is achieved.

[hamishwillee]

True. I've reworded this to capture your points. I'd probably change to send_arm_command(). I wouldn't use "try" because we always send the message, it just doesn't always arrive :-)

[hamishwillee]

This will be addressed by #566