search

svalouch / python-rctclient

Python client for RCTs Serial Communication Protocol
https://rctclient.readthedocs.io
GNU General Public License v3.0
46 stars 27 forks source link

Fixed wrong order of address/id in frame decode #11

Closed schlotzz closed 3 years ago

schlotzz commented 3 years ago

The frame order has been defined by RCT as followed: start | command | length | address (if plant modificator bit set) | id | data | crc

The encoding method uses the right order of fields (address before id), but the decoding method swapped the order.

svalouch commented 3 years ago

Hi, thanks for catching that. Unfortunately, by the time I saw your PR I had already rewritten the frame parser, so I incorporated it directly. I have to take your word for the correctness, however, as I lack a setup to test plant communication and the vendor has not agreed to send me documentation. Please let me know if the current implementation is correct now.

So, since the code in this project was based on OpenWB's implementation originally, their code contains their same problem. It would be awesome if you could send the patch to them, too https://github.com/snaptec/openWB/blob/master/modules/bezug_rct/rct.py#L246-L251

svalouch commented 3 years ago

Actually, I'd like to add plant communication to the (admittedly pretty small) test-suite. Do you happen to have a packet dump (tcpdump or wireshark, i.e. .pcap or .pcapng) of plant communication that you could send me, the more the better (via email preferrably, as packet dumps could include device names, serial numbers and network settings)?

philoxio commented 3 years ago

Maybe this'll help?

svalouch @.***> schrieb am Fr., 21. Mai 2021, 19:36:

Hi, thanks for catching that. Unfortunately, by the time I saw your PR I had already rewritten the frame parser, so I incorporated it directly. I have to take your word for the correctness, however, as I lack a setup to test plant communication and the vendor has not agreed to send me documentation. Please let me know if the current implementation is correct now.

So, since the code in this project was based on OpenWB's implementation originally, their code contains their same problem. It would be awesome if you could send the patch to them, too

https://github.com/snaptec/openWB/blob/master/modules/bezug_rct/rct.py#L246-L251

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/svalouch/python-rctclient/pull/11#issuecomment-846125257, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMAWCJUGOKSYNPCDXKBUTLTTO2KYTANCNFSM44354XWA .

schlotzz commented 3 years ago

Actually, I'd like to add plant communication to the (admittedly pretty small) test-suite. Do you happen to have a packet dump (tcpdump or wireshark, i.e. .pcap or .pcapng) of plant communication that you could send me, the more the better (via email preferrably, as packet dumps could include device names, serial numbers and network settings)?

It's a pity, but currently I haven't got any further information or packet dumps about plant communication. :-(

Did @philoxio send you a dump?

schlotzz commented 3 years ago

Hi, thanks for catching that. Unfortunately, by the time I saw your PR I had already rewritten the frame parser, so I incorporated it directly. I have to take your word for the correctness, however, as I lack a setup to test plant communication and the vendor has not agreed to send me documentation. Please let me know if the current implementation is correct now.

You're welcome. I noticed the wrong order while implementing the RCT protocol in PHP for my own home automation software based on your work and the official RCT documentation. RCT asked me to treat the documentation confidential, therefore I'm currently not allowed to share it with you.

So, since the code in this project was based on OpenWB's implementation originally, their code contains their same problem. It would be awesome if you could send the patch to them, too https://github.com/snaptec/openWB/blob/master/modules/bezug_rct/rct.py#L246-L251

Thanks for pointing that out. I already left a pull request there...

svalouch commented 3 years ago

It's a pity, but currently I haven't got any further information or packet dumps about plant communication. :-(

Okay, no worries :-)

Did @philoxio send you a dump?

Looks like mails sent to GitHub are stripped of attachments, so I don't know. Actually, I made a poor choice of words above: I do have a copy of the protocol documentation, just not officially from RCT.

Thanks for pointing that out. I already left a pull request there...

Thank you!

Alright, thank you very much for checking the code and good luck with your implementation :-) I'm gonna close the PR now as the change has already been applied in a different way.