Add vld support - Githubissues

mak-gitdev commented 2 years ago

VLD telegrams (RORG=0xD2) have variable payload between 1 and 14 bytes (EEP 3.1.1). Thus, depending on the ID of the telegram, the profile is not the same. Based on that ID EEP shortcut specified in the sensor configuration, current contribution retrieves that ID from MQTT message when sending and from radio packet when receiving. The ID is then passed to Python EnOcean for correct EEP handling.

duylong commented 2 years ago

Previously my STOP did not work on my D2-05-00 device despite sending the CMD to 2. Now it works with these modifications.

romor commented 2 years ago

Thanks a lot for your contribution!

I will be glad to merge it. However, could I ask you for some updates before that:

you reverted by change from baafb384, where I changed the default mqtt_keepalive timeout to 60 s. I guess this was not intended and by mistake? Could you please correct it again?
you completely removed the packet initialization with default data? Please remove it only for VLD packets or - even better - just remove it if default_data is not specified. Otherwise it is still needed.
- a breaking functional change is, to clear the data buffer after sending an MQTT message. Is this really required and why? I think this is somehow confusing, as the data does not get deleted if the packet is sent based on a reply message. And in this reply case, the data deletion must not be done as the user does not know when this happens.
in _send_packet() you seem to have a typo with double assignment of sender variable
I try to follow pylint rules as good as possible, to have a minimum of quality assurance done. To avoid the no-else-return and no-else-continue warnings, could you please remove the else branches after a return/continue statements in _on_mqtt_message() and _get_command_id().
- I also started to use autopep8. This is where the white space diffs come from, which makes your changes more difficult to review. You can leave your code as it is, but it will be changed again when I save the files the next time...

mak-gitdev commented 2 years ago

Hi @romor ,

Thanks for your comments and in general for this work. It is very useful. Please find below my answers to the points you identified:

you reverted by change from https://github.com/embyt/enocean-mqtt/commit/baafb384ae346d9a64dbf1c7d0a4a113f41c615a, where I changed the default mqtt_keepalive timeout to 60 s. I guess this was not intended and by mistake? Could you please correct it again?

Oops ! I really don't know how that happened. Sorry.

you completely removed the packet initialization with default data? Please remove it only for VLD packets or - even better - just remove it if default_data is not specified. Otherwise it is still needed.

Yep you're right. I forgot to handle that before issuing my pull request :( I made the changes to remove it unless default_data is specified.

in _send_packet() you seem to have a typo with double assignment of sender variable

Oops again ! :(

I try to follow pylint rules as good as possible, to have a minimum of quality assurance done. To avoid the no-else-return and no-else-continue warnings, could you please remove the else branches after a return/continue statements in _on_mqtt_message() and _get_command_id().

Yep. I made the changes.

I also started to use autopep8. This is where the white space diffs come from, which makes your changes more difficult to review. You can leave your code as it is, but it will be changed again when I save the files the next time...

Not a problem at all. It's just because I always configure my editor to remove white space.

a breaking functional change is, to clear the data buffer after sending an MQTT message. Is this really required and why? I think this is somehow confusing, as the data does not get deleted if the packet is sent based on a reply message. And in this reply case, the data deletion must not be done as the user does not know when this happens.

This point requires a little more discussion I think and in fact, I was waiting for your comment on this one :) First of all, I didn't really understand the use cases of _reply_packet(). That's why I didn't do anything there and waited for your comment. The "problem" is this. With VLD, telegrams fields can be different from one command ID to another. By not clearing the sensor data buffer from one send request to another, the content of the previous request is also used to create the current request radio packet. This will work as only the relevant fields are used to generate the radio packet but it will generate warnings in Python Enocean. As I wanted to remove those unwanted warnings, I thought clearing the data buffer might do the trick. Below is a debug log of such warnings (if needed, EEP is D2-01-0C):

2022-02-15 12:24:40,474 DEBUG: Received PUBLISH (d0, q0, r0, m0), 'test/req/CMD', ... (1 bytes) 2022-02-15 12:24:40,475 DEBUG: Got MQTT message: test/req/CMD 2022-02-15 12:24:40,477 DEBUG: test: CMD=8 2022-02-15 12:24:40,490 DEBUG: Received PUBLISH (d0, q0, r0, m0), 'test/req/PM', ... (1 bytes) 2022-02-15 12:24:40,491 DEBUG: Got MQTT message: test/req/PM 2022-02-15 12:24:40,492 DEBUG: test: PM=3 2022-02-15 12:24:40,497 DEBUG: Received PUBLISH (d0, q0, r0, m0), 'test/req/send', ... (0 bytes) 2022-02-15 12:24:40,498 DEBUG: Got MQTT message: test/req/send 2022-02-15 12:24:40,499 DEBUG: Trigger message to: test 2022-02-15 12:24:40,499 DEBUG: Retrieved command id from MQTT message: 0x8 2022-02-15 12:24:40,505 DEBUG: sensor data: {'CMD': 8, 'RM': 0, 'RE': 1, 'ep': 0, 'IO': 30, 'MD_LSB': 4, 'UN': 1, 'MD_MSB': 6, 'MAT': 60, 'MIT': 255, 'PM': 3} 2022-02-15 12:24:40,507 WARNING: Cannot find data description for shortcut RM 2022-02-15 12:24:40,508 WARNING: Cannot find data description for shortcut RE 2022-02-15 12:24:40,509 WARNING: Cannot find data description for shortcut ep 2022-02-15 12:24:40,510 WARNING: Cannot find data description for shortcut IO 2022-02-15 12:24:40,511 WARNING: Cannot find data description for shortcut MD_LSB 2022-02-15 12:24:40,512 WARNING: Cannot find data description for shortcut UN 2022-02-15 12:24:40,513 WARNING: Cannot find data description for shortcut MD_MSB 2022-02-15 12:24:40,514 WARNING: Cannot find data description for shortcut MAT 2022-02-15 12:24:40,514 WARNING: Cannot find data description for shortcut MIT

What do you think is the best way to remove these warnings ? At the moment, I commented the line which clears the buffer.

romor commented 2 years ago

Thanks for your updates! Perfect.

Concerning the warning on inexisting profile members: OK, I understand the motivation/problem now. Also for you to understand the "reply packets": Some Enocean modules, i.e. battery powered heating actuators (A5-20-01) demand a new set point from a master controller. They send a packet to the controller with such request and expects an response frame giving the new command (the heating set point).

I see the following possibilities to overcome the issue:

Don't care and ignore the warning message.
Avoid the warning by checking, whether the element is available. However, I think we should keep the warning if an inexisting data member is set because I guess the usual cause for this is a user error setting a wrong field name.
Always delete the data after a send, as you implemented it.
Introduce new clear command, similar to the send MQTT command, which clears the data on purpose.
Use the payload of the send command (which is currently unused) to indicate, whether it should delete the data.

The list is ranked in my personal rating, so I would prefer 5 down to 1. What is your impression on this? What would you suggest?

mak-gitdev commented 2 years ago

Okay, I may have got the use case for _reply_packet(). If I'm good, such devices need to have valid data available in /req each time they send their packet. If this is right, then I think the data buffer should not be deleted after each reply and we should go for your proposal number 5. For backward compatibility, empty payload could mean keep data buffer while any other would mean delete data buffer.

I was thinking about creating a second buffer dedicated to reply packets, we would call it the reply buffer, accessible using topic /reply, with data stored in sensor['reply'] instead of sensor['data'] and used by _reply_packet(), but it may be over complicated for now and would not allow backward compatibility.

romor commented 2 years ago

OK, let's go for proposal 5 then. We could set the payload to "delete" to trigger deletion or treat it as a boolean value with "1" being the request for deletion.

Do you like to implement this feature or shall I just merge the current status and extend it myself?

mak-gitdev commented 2 years ago

I will let you implement the feature :) Thanks a lot for your help !

romor commented 2 years ago

Done. Set the payload of the req/send packet to "clear" to empty the data buffer.

mak-gitdev commented 2 years ago

I just tested the "clear" feature and I ran into an issue. As MQTT payload is binary data, "clear" payload is encoded as python b'clear' which is also b'\x63\x6C\x65\x61\x72'. Thus any attempt to compare payload to "clear" will fail. Comparison must be like: if msg.payload.decode('UTF-8') == "clear": to convert payload from bytes to string

romor commented 2 years ago

Thanks. I fixed that.

embyt / enocean-mqtt

Add vld support #27