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

Serial read_line fixes #56

Merged
merged 2 commits into from Jan 20, 2021
Merged

Serial read_line fixes #56

merged 2 commits into from Jan 20, 2021

Conversation

ajschmidt8
Copy link
Collaborator

@ajschmidt8 ajschmidt8 commented Dec 22, 2020

This PR updates the read_line method (and an associated function, filter_ad2prot_byte) in the SerialDevice class.

In serial_device.py, self._buffer += buf[0] was changed to self._buffer += buf so that the byte objects can be correctly concatenated (buf[0] returns an int, so it was trying to incorrectly concatenate a byte object with an int prior to this change).

In util.py, filter_ad2prot_byte was updated to correctly return a byte object (as indicated in filter_ad2prot_byte's docstring) instead of an int/str and also to compare against the ASCII values for \n and \r instead of the string representations.

These changes were tested locally and seemed to resolve the issues described in #40 and #47.

@ajschmidt8
Copy link
Collaborator Author

ajschmidt8 commented Dec 22, 2020

I believe the remaining unit test failures existed before my code changes.

@ajschmidt8
Copy link
Collaborator Author

@f34rdotcom, is there anything I can do to help this PR get merged? I was hoping you could cut a release once this is merged because there are some issues with the Home Assistant integration that this PR as well as #52 and #53 would help fix.

@ajschmidt8
Copy link
Collaborator Author

@f34rdotcom, any movement on this? I've heard from a lot of Home Assistant users who are affected by the problem that this PR aims to resolve. I'm happy to continue helping here but I need some direction from you on what's next.

@endlesscoil
Copy link
Collaborator

endlesscoil commented Jan 20, 2021

Fix looks good. I agree with you on the tests.. looks to be issues with the tests themselves.

@f34rdotcom Merged into dev, but I'll leave the merge from dev -> master and the pypi push up to you.

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.

None yet

2 participants