undera / pylgbst

Python library for LEGO® PoweredUp devices
MIT License
554 stars 121 forks source link

LEDRGB: Current color can't be obtained/notified from the hub #111

Closed ysard closed 2 years ago

ysard commented 2 years ago

Hi, I have an issue with this code (on Technic Hub):

The color code obtained through a callback or with explicit access is always '0'. The code used to change the color is ok (the correct color is displayed on the hub). It seems that the received message is always the following one: MsgPortValueSingle({'payload': b'00', 'port': 50})

What can go wrong with the lib ?

def callback(color):
      print("Color has changed: %s" % color)

conn = get_connection_auto(hub_mac="xx:xx:xx:xx:xx:xx"
myhub = MoveHub(conn)
sleep(1)
print("ATTACH callback")
myhub.led.subscribe(callback)
print("led color:", myhub.led.color)
print("led set to red:")
myhub.led.color = 9
print("led color:", myhub.led.color)
print("led set to green:")
myhub.led.color = 6
print("led color:", myhub.led.color)
input("pause")

Log on stdout:

ATTACH callback
DEBUG:hub:Send message: MsgPortInputFmtSetupSingle({'payload': b'32000100000001', 'needs_reply': True, 'port': 50, 'mode': 0, 'updates_enabled': True, 'update_delta': 1})
DEBUG:hub:Waiting for sync reply to MsgPortInputFmtSetupSingle({'payload': b'32000100000001', 'needs_reply': True, 'port': 50, 'mode': 0, 'updates_enabled': True, 'update_delta': 1})...
DEBUG:hub:Notification on 13: b'0a004732000100000001'
DEBUG:hub:Decoded message: MsgPortInputFmtSingle({'payload': b'', 'port': 50, 'mode': 0, 'upd_delta': 1, 'upd_enabled': 1})
DEBUG:hub:Found matching upstream msg: MsgPortInputFmtSingle({'payload': b'', 'port': 50, 'mode': 0, 'upd_delta': 1, 'upd_enabled': 1})
DEBUG:hub:Fetched sync reply: MsgPortInputFmtSingle({'payload': b'', 'port': 50, 'mode': 0, 'upd_delta': 1, 'upd_enabled': 1})
DEBUG:peripherals:Implied update is enabled=1
DEBUG:peripherals:Implied update delta=1
DEBUG:peripherals:Already in target mode, no need to switch
DEBUG:hub:Send message: MsgPortInfoRequest({'payload': b'', 'needs_reply': True, 'port': 50, 'info_type': 0})
DEBUG:hub:Waiting for sync reply to MsgPortInfoRequest({'payload': b'3200', 'needs_reply': True, 'port': 50, 'info_type': 0})...
DEBUG:hub:Notification on 13: b'0500453200'
DEBUG:hub:Decoded message: MsgPortValueSingle({'payload': b'00', 'port': 50})
DEBUG:hub:Found matching upstream msg: MsgPortValueSingle({'payload': b'00', 'port': 50})
DEBUG:hub:Handling msg with <bound method Hub._handle_sensor_data of <pylgbst.hub.MoveHub object at 0x7666cd10>>: MsgPortValueSingle({'payload': b'00', 'port': 50})
Color has changed: 0
DEBUG:hub:Fetched sync reply: MsgPortValueSingle({'payload': b'00', 'port': 50})
led color: (0,)
led set to red:
DEBUG:peripherals:Implied update is enabled=1
DEBUG:peripherals:Implied update delta=1
DEBUG:peripherals:Already in target mode, no need to switch
DEBUG:hub:Send message: MsgPortOutput({'payload': b'', 'needs_reply': False, 'port': 50, 'is_buffered': False, 'do_feedback': True, 'subcommand': 81, 'params': b'0009'})
DEBUG:hub:Waiting for sync reply to MsgPortOutput({'payload': b'3211510009', 'needs_reply': True, 'port': 50, 'is_buffered': False, 'do_feedback': True, 'subcommand': 81, 'params': b'0009'})...
DEBUG:hub:Notification on 13: b'050082320a'
DEBUG:hub:Decoded message: MsgPortOutputFeedback({'payload': b'', 'port': 50, 'status': 10})       
DEBUG:hub:Found matching upstream msg: MsgPortOutputFeedback({'payload': b'', 'port': 50, 'status': 10})                                                                                              
DEBUG:hub:Fetched sync reply: MsgPortOutputFeedback({'payload': b'', 'port': 50, 'status': 10})    
DEBUG:peripherals:Implied update is enabled=1                                                      
DEBUG:peripherals:Implied update delta=1
DEBUG:peripherals:Already in target mode, no need to switch
DEBUG:hub:Send message: MsgPortInfoRequest({'payload': b'', 'needs_reply': True, 'port': 50, 'info_type': 0})
DEBUG:hub:Waiting for sync reply to MsgPortInfoRequest({'payload': b'3200', 'needs_reply': True, 'port': 50, 'info_type': 0})...
DEBUG:hub:Notification on 13: b'0500453200'
DEBUG:hub:Decoded message: MsgPortValueSingle({'payload': b'00', 'port': 50})
DEBUG:hub:Found matching upstream msg: MsgPortValueSingle({'payload': b'00', 'port': 50})
DEBUG:hub:Handling msg with <bound method Hub._handle_sensor_data of <pylgbst.hub.MoveHub object at 0x7666cd10>>: MsgPortValueSingle({'payload': b'00', 'port': 50})
Color has changed: 0
DEBUG:hub:Fetched sync reply: MsgPortValueSingle({'payload': b'00', 'port': 50})
led color: (0,)
led set to green:
DEBUG:peripherals:Implied update is enabled=1
DEBUG:peripherals:Implied update delta=1
DEBUG:peripherals:Already in target mode, no need to switch
DEBUG:hub:Send message: MsgPortOutput({'payload': b'', 'needs_reply': False, 'port': 50, 'is_buffered': False, 'do_feedback': True, 'subcommand': 81, 'params': b'0006'})
DEBUG:hub:Waiting for sync reply to MsgPortOutput({'payload': b'3211510006', 'needs_reply': True, 'port': 50, 'is_buffered': False, 'do_feedback': True, 'subcommand': 81, 'params': b'0006'})...
DEBUG:hub:Notification on 13: b'050082320a'
DEBUG:hub:Decoded message: MsgPortOutputFeedback({'payload': b'', 'port': 50, 'status': 10})
DEBUG:hub:Found matching upstream msg: MsgPortOutputFeedback({'payload': b'', 'port': 50, 'status': 10})
DEBUG:hub:Fetched sync reply: MsgPortOutputFeedback({'payload': b'', 'port': 50, 'status': 10})
DEBUG:peripherals:Implied update is enabled=1
DEBUG:peripherals:Implied update delta=1
DEBUG:peripherals:Already in target mode, no need to switch
DEBUG:hub:Send message: MsgPortInfoRequest({'payload': b'', 'needs_reply': True, 'port': 50, 'info_type': 0})
DEBUG:hub:Waiting for sync reply to MsgPortInfoRequest({'payload': b'3200', 'needs_reply': True, 'port': 50, 'info_type': 0})...
DEBUG:hub:Notification on 13: b'0500453200'
DEBUG:hub:Decoded message: MsgPortValueSingle({'payload': b'00', 'port': 50})
DEBUG:hub:Found matching upstream msg: MsgPortValueSingle({'payload': b'00', 'port': 50})
DEBUG:hub:Handling msg with <bound method Hub._handle_sensor_data of <pylgbst.hub.MoveHub object at 0x7666cd10>>: MsgPortValueSingle({'payload': b'00', 'port': 50})
Color has changed: 0
DEBUG:hub:Fetched sync reply: MsgPortValueSingle({'payload': b'00', 'port': 50})
led color: (0,)
pause
undera commented 2 years ago

The original LED peripheral code did not contain any functions for subscribing to get sensor data. After your change that introduced "sensor properties", you have added one. It seems that LED should not have that property, as it does not support getting the LED state...

If I am wrong, please point me to the place, where old implementation were accessing that information.

ysard commented 2 years ago

Properties are just bindings of pre-existing functions; the old code is here:

https://github.com/undera/pylgbst/blob/6fc3bf4e04367a803b8666f181a9c79c86e386ea/pylgbst/peripherals.py#L235-L239

I may be misinterpreting the API but to me it means that this data can be obtained from the hub via get_sensor_data(). Subscribing to this color data is showed on the old doc : LED

Example:

print("ATTACH callback")
def callback(*args):
    print("Color has changed:", args)
myhub.led.subscribe(callback)

print("led set to red")
myhub.led.set_color(9)  
print("led set to green")
myhub.led.set_color(6)  

print("current color - mode 0", myhub.led.get_sensor_data(0))
print("current color - mode 1", myhub.led.get_sensor_data(1))

Result:

ATTACH callback
led set to red
led set to green
current color - mode 0 (0,)
Color has changed: (0,)
current color - mode 1 (0, 0, 0)
Color has changed: (0, 0, 0)

Indeed, I can see that it's the explicit access via get_sensor_data() that triggers the callback and that the callback showed in the doc is never called otherwise.

If these functions (get_sensor_data() and _decode_port_data()) shouldn't be used in this class, maybe they should be overwritten to raise something like "NotImplementedError". And the doc should be updated to remove any callback reference. All this to avoid misuse.

I can propose a PR to fix all of this, according to your choice.

Some review of other projects:

Note: => I'm more likely to believe the unofficial doc than the partial and out of date Lego one.

undera commented 2 years ago

Thanks for the thorough investigation.

I'm starting to remember that in my early experiments I saw that LED peripheral does send some data back, thus I have included the code to decode the messages. It is highly likely that HW sends those messages, but it's a sort of "incomplete" functionality on HW side.

When I've done "capabilities" scan on Hub's peripherals, some undocumented sensor modes come out. I tried to include them all into the library, even if it's a not documented (by LEGO) functionality.

I guess we can just put a note in documentation, saying that sensor data getting from LED does not produce meaningful results.

ysard commented 2 years ago

Thanks for the feedback, you're totally right; similarly, the headlight device brightness recovery shouldn't work either as it's not documented but it does... The PR should fix that;

I was mistaken because the description of the 2 devices is similar: => no input_modes, 1, some output_modes for both

LEDRGB:

modes: {'mode_count': 2, 'input_modes': [], 'output_modes': [{'Mode': 0, 'Name': 'COL O', 'Raw range': [0.0, 10.0], 'Percent range': [0.0, 100.0], 'SI value range': [0.0, 10.0], 'Units': '', 'Mapping': {'input': [], 'output': ['Discrete [0, 1, 2, 3]', 'Supports Functional Mapping 2.0+']}, 'Value encoding': {'datasets': 1, 'type': '8 bit', 'total_figures': 1, 'decimals': 0}}, {'Mode': 1, 'Name': 'RGB O', 'Raw range': [0.0, 255.0], 'Percent range': [0.0, 100.0], 'SI value range': [0.0, 255.0], 'Units': '', 'Mapping': {'input': [], 'output': ['Absolute [min..max]']}, 'Value encoding': {'datasets': 3, 'type': '8 bit', 'total_figures': 3, 'decimals': 0}}], 'capabilities': {'logically_combinable': False, 'synchronizable': False, 'can_output': True, 'can_input': False}

Headlight:

modes: {'mode_count': 1, 'input_modes': [], 'output_modes': [{'Mode': 0, 'Name': 'LPF2-LIGHT', 'Raw range': [-10.0, 10.0], 'Percent range': [-100.0, 100.0], 'SI value range': [-10.0, 10.0], 'Units': '', 'Mapping': {'input': [], 'output': ['Relative [-1..1]']}, 'Value encoding': {'datasets': 1, 'type': '8 bit', 'total_figures': 1, 'decimals': 0}}], 'capabilities': {'logically_combinable': False, 'synchronizable': False, 'can_output': True, 'can_input': False},