-
Notifications
You must be signed in to change notification settings - Fork 684
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
Chat: reliability enhancements #1289
Chat: reliability enhancements #1289
Conversation
4ca1aef
to
8365e5b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really only fussed by that sleeping.
@@ -26,6 +28,11 @@ def __init__(self, mpstate): | |||
# keep reference to mpstate | |||
self.mpstate = mpstate | |||
|
|||
# a dictionary of command_ack mavcmds we are waiting for | |||
# key is the mavcmd, value is -1 or the MAV_RESULT (e.g. 0 to 9) | |||
# we assume we will never be waiting for two of the same mavcmds at the same time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't dis-ambiguate the results anyway! I've pushed for the addition of an "opaque ID" supplied in the COMMAND_INT
message and returned in a COMMAND_ACK
to fix that problem, but didn't ever get traction on it...
You can ensure you never send a second command of a specific type while one is outstanding based on your dictionary, 'though. You would then need a timeout as the ACK may get lost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok thanks. The way the chat module works at the moment, the commands are only sent one-by-one and after sending it immediately waits for the ACK so there's actually no way for it to be waiting for two command_acks. It could still pickup the wrong ACK though if that ACK is in response to a similar command from some other sender... and your proposed change would help with that so I'm all for it.
BTW I could have simplified the dictionary down to just a single name-value pair but in python a dictionary is so easy to use I went with it.
8365e5b
to
316be97
Compare
I think I've addressed PeterB's requests.
Merged, thanks! |
Thanks!! |
This improves the reliability in a few ways:
I could squash down the assistant instructions to a single commit if this is preferred.
I've tested this and it certain improves reliability. Below is a typical test where the user asked it to takeoff before the arming checks cleared and it still managed to do it.