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

Add DPT232 #5

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Add DPT232 #5

wants to merge 9 commits into from

Conversation

greiginsydney
Copy link

Hi Michael,

This PR adds encoding and decoding of DPT 232.

I've never written unit tests before, but I think what I've done is valid:

pi@captureKNX-Tijl:~/tests/knxdclient $ python3 -m unittest
..Send request
Send request
.Send request
Send request
..Error while waiting for next packet from KNXd: TimeoutError(). Exiting from receive loop.
........................
----------------------------------------------------------------------
Ran 29 tests in 2.922s

OK
pi@captureKNX-Tijl:~/tests/knxdclient $

Let me know if anything's not quite right and I'll amend as required.

If you're OK with it I'll add more DPTs as users of my captureKNX bring them to my attention??

Thanks.

- Greig.

Copy link
Owner

@mhthies mhthies left a comment

Choose a reason for hiding this comment

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

Hi Greig,

thanks for your contribution! Of course, I'm happy to add more KNX DPTs! Until now I've been lazy and only added the few DPTs needed for the KNX devices at my parents' home.

However, please take a look at my comments about the Python types returned/accepted by the decode/encode functions. I'd like to keep them as consistent and semantically fitting as possible.

knxdclient/encoding.py Outdated Show resolved Hide resolved
knxdclient/encoding.py Outdated Show resolved Hide resolved
test/test_conversion.py Outdated Show resolved Hide resolved
self.assertEqual(f'{0x801B10:X}',
knxdclient.decode_value(bytes([0x80, 0x1B, 0x10]), knxdclient.KNXDPT.COLOUR_RGB))
self.assertEqual(f'{0x123456:X}',
knxdclient.decode_value(bytes([0x12, 0x34, 0x56]), knxdclient.KNXDPT.COLOUR_RGB))
Copy link
Owner

Choose a reason for hiding this comment

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

You could add a "roundtrip" test here that checks that decode_value(encode_value(some_value, ...), ...) == some_value.

This should also reveal the issue with the deviating datatypes, discussed in my other comments.

Copy link
Author

Choose a reason for hiding this comment

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

Ta. I shall tinker. (This first one might take us a little while, but it should speed any future ones. I appreciate the hand-holding).

@greiginsydney
Copy link
Author

Thanks Michael. I'll get onto these. (Sorry for the slow response - I didn't receive any notification from GitHub that you'd responded).

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.

2 participants