Handle failures on port out commands #79
Conversation
Port out commands can fail (e.x obstruction of the movement), and it will get the while executon stuck. The diff handles failures and reports errors to the caller while at it, its useful to know when the command fails.
|
any objections? |
Codecov Report
@@ Coverage Diff @@
## master #79 +/- ##
==========================================
- Coverage 75.76% 75.20% -0.56%
==========================================
Files 18 20 +2
Lines 2080 2267 +187
==========================================
+ Hits 1576 1705 +129
- Misses 504 562 +58
Continue to review full report at Codecov.
|
| @@ -704,7 +704,7 @@ def bytes(self): | |||
|
|
|||
| def is_reply(self, msg): | |||
| return isinstance(msg, MsgPortOutputFeedback) and msg.port == self.port \ | |||
| and (msg.is_completed() or self.is_buffered) | |||
There was a problem hiding this comment.
I would like to understand how self.is_buffered is not needed anymore. It were used for buffered commands that were just put into buffer, waiting the execution.
There was a problem hiding this comment.
I thought it is dead code for now, where is it being set? It is always false for now, all I could find is a note "support buffering" in another place. Did I miss anything?
There was a problem hiding this comment.
It is not dead code. If peripheral is set to use buffered commands, this flag has to be respected. You are free to change the buffering flag anytime on peripheral, since it is a part of LEGO protocol.
There was a problem hiding this comment.
- Today none of them do?
- Before enabling them to do so, a proper SDK side state machine has to be implemented (the one that keeps track of the 2 pending commands inside the controller, rejects or buffers the rest on the SDK side (4.2 in the protocol). If I was implementing an asynchronous method that is what I would do, probably through in another command buffer on the SDK side as well, a callback function, to notify about execution of the specific command. But it sounded like you idea is to keep it simple(synchronous)? So what is the value of these buffered commands?
- (btw somewhat related to the sync/simple model). While initially experimenting with the lib I had a problem that took me while to figure out. I did a simple test "detect a press on a blue button, twist the motor as a result", It works once, then it gets into a deadlock, took me a while to figure out. Basically it seems users can't call the SDK from an SDK callback?
There was a problem hiding this comment.
- Probably no. But I want to keep the intent to support it. For example, you know it's there, you know it lacks good implementation. This is a positive thing IMO, somebody might contribute the implementation.
- I'm not "all or nothing" guy. I wish I had time to make proper implementation of buffered commands. Indeed, I would keep it as simple as possible, leaving complex state handling to the user of those buffered modes. Complicated and asynchronous is out of scope for this lib, unless we find simple wrapping.
- I am sure there can be bugs leading to deadlocks. It is hard to catch those, especially when multiple BLE backends have to be supported, each with own specifics of processing. In general, calling library functions from callbacks are fine, unless it logically leads to deadlock.
There was a problem hiding this comment.
I agree with keeping it simple and being against all or nothing, your library is awesome in that sense. It provides a very useful and simplified sync API. The fact that Lego protocol allows keeping the next bullet in the chamber, that's cool, but why? Anyway, I can bring back the "or self.is_buffered", but I think it will just create confusion for now, the contract of the SDK right now is "motor commands are blocking", they do not return until they either succeed or fail. With this flag, it will return immediately for some, and that return will sometimes be triggered by the engine starting that command, sometimes by finishing a previous command!? So my approach to "all or nothing" is introduce the functionality incrementally, but keep the API consistent and keep behavior well defined at every phase. Anyhow, here is a the latest demo: https://www.youtube.com/watch?v=myctUKspBio I will cleanup and put out the latest version of the code, hopefully tonight
There was a problem hiding this comment.
The video is amazing! I'm gonna show it to my daughter!
Port out commands can fail (e.x obstruction of the movement), and it
will get the while executon stuck. The diff handles failures and
reports errors to the caller while at it, its useful to know when
the command fails.