Skip to content
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.

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

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

Closed
wants to merge 1 commit into from

Conversation

schlotzz
Copy link

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.

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
Copy link
Owner

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
Copy link
Owner

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
Copy link

philoxio commented May 22, 2021 via email

@schlotzz
Copy link
Author

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
Copy link
Author

schlotzz commented May 23, 2021

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
Copy link
Owner

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.

@svalouch svalouch closed this May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants