Skip to content

Handle failures on port out commands #79

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.

Sign up for GitHub

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

Open
finik wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Handle failures on port out commands #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion pylgbst/messages.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Copy Markdown
Owner

@undera undera Sep 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@finik finik Sep 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Owner

@undera undera Sep 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@finik finik Sep 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Today none of them do?
  2. 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?
  3. (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?

Copy link
Copy Markdown
Owner

@undera undera Sep 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.
  2. 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.
  3. 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.

Copy link
Copy Markdown
Contributor Author

@finik finik Sep 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Owner

@undera undera Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The video is amazing! I'm gonna show it to my daughter!

and not (msg.is_in_progress() or msg.is_busy())


class MsgPortOutputFeedback(UpstreamMsg):
Expand Down Expand Up @@ -736,6 +736,9 @@ def is_discarded(self):
def is_idle(self):
return self.status & 0b1000

def is_busy(self):
return self.status & 0b10000


UPSTREAM_MSGS = (
MsgHubProperties, MsgHubAction, MsgHubAlert, MsgHubAttachedIO, MsgGenericError,
Expand Down
37 changes: 27 additions & 10 deletions pylgbst/peripherals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ def set_port_mode(self, mode, send_updates=None, update_delta=None):
def _send_output(self, msg):
assert isinstance(msg, MsgPortOutput)
msg.is_buffered = self.is_buffered # TODO: support buffering
self.hub.send(msg)
return self.hub.send(msg)

def get_sensor_data(self, mode):
self.set_port_mode(mode)
Expand Down Expand Up @@ -275,12 +275,15 @@ def _write_direct_mode(self, subcmd, params):
msg = MsgPortOutput(self.port, MsgPortOutput.WRITE_DIRECT_MODE_DATA, params)
self._send_output(msg)

def _is_successful(self, msg):
return not msg.is_discarded()

def _send_cmd(self, subcmd, params):
if self.virtual_ports:
subcmd += 1 # de-facto rule

msg = MsgPortOutput(self.port, subcmd, params)
self._send_output(msg)
return self._send_output(msg)

def start_power(self, power_primary=1.0, power_secondary=None):
"""
Expand All @@ -299,10 +302,12 @@ def start_power(self, power_primary=1.0, power_secondary=None):
if self.virtual_ports:
params += pack("<b", self._speed_abs(power_secondary))

self._send_cmd(cmd, params)
resp = self._send_cmd(cmd, params)

return self._is_successful(resp)

def stop(self):
self.timed(0)
return self.timed(0)

def set_acc_profile(self, seconds, profile_no=0x00):
"""
Expand All @@ -312,7 +317,9 @@ def set_acc_profile(self, seconds, profile_no=0x00):
params += pack("<H", int(seconds * 1000))
params += pack("<B", profile_no)

self._send_cmd(self.SUBCMD_SET_ACC_TIME, params)
resp = self._send_cmd(self.SUBCMD_SET_ACC_TIME, params)

return self._is_successful(resp)

def set_dec_profile(self, seconds, profile_no=0x00):
"""
Expand All @@ -322,7 +329,9 @@ def set_dec_profile(self, seconds, profile_no=0x00):
params += pack("<H", int(seconds * 1000))
params += pack("<B", profile_no)

self._send_cmd(self.SUBCMD_SET_DEC_TIME, params)
resp = self._send_cmd(self.SUBCMD_SET_DEC_TIME, params)

return self._is_successful(resp)

def start_speed(self, speed_primary=1.0, speed_secondary=None, max_power=1.0, use_profile=0b11):
"""
Expand All @@ -339,7 +348,9 @@ def start_speed(self, speed_primary=1.0, speed_secondary=None, max_power=1.0, us
params += pack("<B", int(100 * max_power))
params += pack("<B", use_profile)

self._send_cmd(self.SUBCMD_START_SPEED, params)
resp = self._send_cmd(self.SUBCMD_START_SPEED, params)

return self._is_successful(resp)

def timed(self, seconds, speed_primary=1.0, speed_secondary=None, max_power=1.0, end_state=END_STATE_BRAKE,
use_profile=0b11):
Expand All @@ -359,7 +370,9 @@ def timed(self, seconds, speed_primary=1.0, speed_secondary=None, max_power=1.0,
params += pack("<B", end_state)
params += pack("<B", use_profile)

self._send_cmd(self.SUBCMD_START_SPEED_FOR_TIME, params)
resp = self._send_cmd(self.SUBCMD_START_SPEED_FOR_TIME, params)

return self._is_successful(resp)


class EncodedMotor(Motor):
Expand Down Expand Up @@ -400,7 +413,9 @@ def angled(self, degrees, speed_primary=1.0, speed_secondary=None, max_power=1.0
params += pack("<B", end_state)
params += pack("<B", use_profile)

self._send_cmd(self.SUBCMD_START_SPEED_FOR_DEGREES, params)
resp = self._send_cmd(self.SUBCMD_START_SPEED_FOR_DEGREES, params)

return self._is_successful(resp)

def goto_position(self, degrees_primary, degrees_secondary=None, speed=1.0, max_power=1.0,
end_state=Motor.END_STATE_BRAKE, use_profile=0b11):
Expand All @@ -421,7 +436,9 @@ def goto_position(self, degrees_primary, degrees_secondary=None, speed=1.0, max_
params += pack("<B", end_state)
params += pack("<B", use_profile)

self._send_cmd(self.SUBCMD_GOTO_ABSOLUTE_POSITION, params)
resp = self._send_cmd(self.SUBCMD_GOTO_ABSOLUTE_POSITION, params)

return self._is_successful(resp)

def _decode_port_data(self, msg):
data = msg.payload
Expand Down