Skip to content

Add support for setting unit of measure when updating Number items #27

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

Merged
merged 6 commits into from
Mar 6, 2022

Conversation

arroyoj
Copy link
Contributor

@arroyoj arroyoj commented Jan 25, 2022

@sim0nx, first of all, thank you for this great library for communicating with the OpenHAB REST API.

This PR adds support for updating OpenHAB Number items of QuantityType and setting the unit of measure. Currently, the library only supports retrieving the unit of measure. In my specific case, I was trying to update a Number:Temperature item that is stored in OpenHAB in Fahrenheit, using values from a sensor that only reports in Celsius. OpenHAB can handle the unit conversion as long as the string sent to the REST API includes the proper unit of measure, but NumberItem.update() could only be called with a float or int. This PR allows the NumberItem.update() and NumberItem.command() methods to also accept a string of the form "[numeric value] [unit of measure]" or a tuple of (float, str) to represent numbers with units.

While adding this feature, I also discovered that the OpenHAB REST API does not support the full latin-1 encoding but only accepts ascii or utf-8. If the degree symbol (°) is encoded as latin-1 (as is the default when a str is provided as data to the underlying requests library), my OpenHAB server (v3.1.0) returned a Bad Request error when updating the Number item. However, encoding as utf-8 was fine. A similar problem occurred if a degree symbol was set in a String item: the value was accepted but improperly decoded. Therefore, I switched the check for non-latin-1 characters to check for non-ascii characters. We could also consider always encoding the final string to utf-8 for simplicity.

Lastly, the unit of measure was also exposed in the Item.__str__() method.

Thanks for considering this PR.

@arroyoj
Copy link
Contributor Author

arroyoj commented Jan 28, 2022

While trying to fix the tests that were failing on Travis in the original PR, I discovered that NumberItem does not handle scientific notation. OpenHAB will return scientific notation for very small numbers, such as the "almost zero" value that occurred when getting OpenHAB to convert 32°F to 0°C (floating point math seemed to make it ~2e-32 instead of zero). In addition, while Python defaults to using lowercase e but will accept both uppercase E and lowercase e for scientific notation, OpenHAB will only accept uppercase E through the REST API. I updated the regular expression and REST string formatting to handle scientific notation and added a test for it.

The other failing Travis tests appear to be due to the asynchronous nature of OpenHAB. Updating an item state in OpenHAB will return success from the REST API before the item state has actually been updated. This made the update state --> assert new state paradigm fail in many of the tests run on Travis, even though they worked fine on my faster local OpenHAB instance. My workaround was to add a sleep delay after updating for the tests that were failing the most. The tests appeared to be running clean on Travis, but now it looks like a different test is showing the same issue in one of the Travis jobs. Please let me know if you would like me to try to add additional delays to fix the other tests, or just ignore them since they will likely pass the next time Travis is run.

@sim0nx
Copy link
Owner

sim0nx commented Feb 7, 2022

Hi @arroyoj ... thanks your for PR and explanations! Makes total sense and it's great to have those cases fixed, I myself haven't encountered yet :-)

Regarding the failing test, yes please add another sleep to make travis happy.
Would be nice to have travis just run through the tests ... else this will pop up again in the future I am afraid

@arroyoj
Copy link
Contributor Author

arroyoj commented Feb 9, 2022

@sim0nx, I added in an additional delay for the scientific notation test that failed. All the tests seem to be passing now.

@sim0nx
Copy link
Owner

sim0nx commented Mar 6, 2022

@sim0nx, I added in an additional delay for the scientific notation test that failed. All the tests seem to be passing now.

Sorry for the delay, missed this one

@sim0nx sim0nx merged commit 0a1c2c4 into sim0nx:master Mar 6, 2022
@arroyoj arroyoj deleted the update_with_uom branch March 8, 2022 00:23
@arroyoj
Copy link
Contributor Author

arroyoj commented Mar 8, 2022

@sim0nx, thanks for merging in the PR.

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