Create setter functions rather than setting attributes directly

[hamishwillee]

This adds setter functions for mode, armed, home_location, airspeed and groundspeed attributes and adds documentation to mark the old attributes as deprecated.

The reason for this change is that the attribute setters have unexpected behaviour (for users) - specifically, setting the value of these attributes does not change the value immediately and may or may not work.

By adding appropriately named setter functions we make it clear that the messages may or may not be received and we've added waiting versions that will make best effort to verify the success of the message.

Overall these methods are safer and easier than the attributes, and reduce the amount of code that developers need to write.

The attribute setters will be removed in a future version.

[hamishwillee]

@peterbarker This is the first draft of the API and example code changes required for the attribute setters functions. I still have to update more examples, add test code, and update the docs. Before I do so I want to confirm the new methods are OK. Please review, comment, but don't merge.

[tcr3dr]

To clarify, no methods or backcompat features should be removed until a theoretical 3.0, in keeping with semver. It makes a good guarantee for end users they can use any 2.x without code breaking.

I strongly recommend we drop the try_. I expect set_home_location to throw an exception if it fails, with or without a try_ prefix; this is my experience with DKPY2 and Python in general, also a documentation issue more than a keyword issue. And in doing so, we avoid our example code looking extremely verbose. I apologize for how tempting this is to bikeshed though!

[hamishwillee]

@tcr3dr Yes, we don't plan on removing any time soon, but certainly updating would be a major version change.
W.r.t. function naming we actually can't raise exceptions in non-waiting mode. Even in waiting mode we can only raise an exception in some cases. So there is no real contract to raise an exception on fail. The use of try_ in the name was supposed to indicate simply that these can fail.

That said, I'm not sure that it really conveys what we wanted. Originally I had send_set_mode_message() . My preference is just to follow your suggestion and dump the try_. @peterbarker last chance for rebuttal.

[peterbarker]

I stand by my original assessment. Unless set_home_location blocks, it can't throw an exception on failure. send_set_mode_message (which I think I had previously suggested) pre-supposes that you are setting modes by sending messages, something which can be hidden from the user.

I guess another option is to make "set_home_location" non-void. It returns true if your mode is already what you're trying to set it to. This is going to break down in the cases where you can't tell what the current value actually is....

[hamishwillee]

@peterbarker Sure we can't enforce exceptions on fail and we don't even try to in the non waiting mode. But try_set_mode is clearly just as ambiguous a naming approach because @tcr3dr thought that it was about exceptions. I just think its a little ugly - and if you're always going to have an ambiguous situation, then best to just document things as well as possible.

[hamishwillee]

@mrpollo Any thoughts on API naming here - Peter and I are in deadlock over the prefix "try_"

[mrpollo]

Why do we think a string is a valid option here?

[hamishwillee]

Why not? For a user a mode really is just a name.

[mrpollo]

I agree that we might need to further clarify the interfaces, the try_ conventions are good but I would also like to keep the set_ interfaces, pseudo-code:

def try_set_mode(mode):
  # ... retry logic
  return set_mode(mode)

Ideally we should aim to be more "event" based, while this API is going to clear up lots of confusion when talking back to the autopilot and the expectancy of an immediate result, I'm worried we are also painting a picture of a more linear based programming pattern where we don't need callbacks.

I say we go ahead with this change, but we sail carefully when creating more example apps and show the real power of our event based patterns.

Great work guys.

[hamishwillee]

@mrpollo . I take your point. How about we split into two functions - syncrhonous and asyncrhonous. and use those in the name? That works for me, because an asyncronous API is not guarantee to complete.

So your code would be:

def sync_set_mode(mode):
  # ... retry logic
  return async_set_mode(mode)

That is a bit more complicated that the "wait" API, but a lot simpler for users and for testing - and also removes the concern that "just one paradigm here" is overly pushed.

[mrpollo]

i really like this idea

[mrpollo]

@hamishwillee big delay here, I don't want to use much of your time, but this is a great changeset we should merge, I would say we opt to async as default which means we don't prepend async_ and only prepend sync_ where we need to, everything else looks good

[hamishwillee]

@mrpollo Happy to update as you have suggested, but I can't get this back in sync in its current state with the master branch. Cursed git keeps pushing me in circles after rebase. So if you can rebase, I can do the work to fix as discussed?

[hamishwillee]

@mrpollo Belay that. I did a crappy "normal merge". No idea why rebase was being difficult but back to normal now.

[hamishwillee]

@mrpollo I've modified the mode setters in the way I believe you wanted. Before I go and roll out to other cases can you confirm that it is what you wanted?

Basically there are now two setters:

• set_mode() replaces the attribute setter. This just sends the mode change message. This can fail in the same way as the current "attribute setter".
• I would still be very happy to call this async_set_mode(). While this explains the behaviour in the docs it still feels that we need something to say "this could fail".
• I think this does fix one of Peter's concerns with the attribute setter that with this method we we don't imply necessarily that the value will change immediately (as we did when setting the attribute). Not sure
• sync_set_mode() - the setter that has retries and timeouts.

@peterbarker will not be completely happy but hopefully happy enough. What do you both think?

[peterbarker]

On Wed, 13 Apr 2016, Hamish Willee wrote:

• set_mode() replaces the attribute setter. This just sends the mode
  change message. This can fail in the same way as the current "attribute
  setter".
• I would still be very happy to call this async_set_mode(). While
  this explains the behaviour in the docs it still feels that we need
  something to say "this could fail".

:-) "try_set_mode". Wait, where's my ship?

 +  I think this does fix one of Peter's concerns with the attribute
    setter that with this method we we don't imply necessarily that the
    value will change immediately (as we did when setting the
    attribute). Not sure

Yes, absolutely. If I set a variable I expect it to be set, damnit! :-)

• sync_set_mode() - the setter that has retries and timeouts.

@peterbarker will not be completely happy but hopefully happy enough. What
do you both think?

+1

[hamishwillee]

@mrpollo @peterbarker OK, I've updated the methods as discussed, and also the documentation and some tests (I don't plan on adding extra tests for the sync_set cases because its hard to test all execution paths - so perhaps a little extra inspection there).

Please review. If you're OK with this then feel free to merge.

[hamishwillee]

@mrpollo @peterbarker Any chance to review and merge? I'm not keen to re-merge these changes yet again.

[hamishwillee]

@peterbarker @mrpollo Can we get these reviewed. VERY OLD CHANGES now, but the original justifications still exist.

[hamishwillee]

@peterbarker @mrpollo Can we get these reviewed. VERY OLD CHANGES now, but the original justifications still exist.