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

possibility to create htu with existing i2c object #1

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

Conversation

kevinkk525
Copy link

@kevinkk525 kevinkk525 commented Oct 30, 2017

Adds the possibility to create a htu object with an existing i2c object, so you can create it before and use it afterwards without knowing which devices created the i2c object.
I think this should be possible.
Many libraries just expect an i2c object, so now it is up to the user.

Also the temperature and humidity are functions now. Makes more sense to me especially as I was converting the library to be asynchronous (separate file).

What do you think?
Thanks for your work,

htu21d.py Outdated
Args:
write_address (int): address to write to
:return:
"""
import time
Copy link
Owner

Choose a reason for hiding this comment

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

an import does not belong here.

Copy link
Author

Choose a reason for hiding this comment

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

you are totally right.

htu21d.py Outdated
self.i2c.start()
self.i2c.writeto_mem(int(self.ADDRESS), int(write_address), '')
self.i2c.stop()
data = bytearray(3)
time.sleep_ms(50)
Copy link
Owner

Choose a reason for hiding this comment

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

im not really sure if this is needed. i did not need it at all. Will see if readfrom_into is a blocking request.

Copy link
Author

Choose a reason for hiding this comment

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

according to the datasheet (https://cdn-shop.adafruit.com/datasheets/1899_HTU21D.pdf) you should wait at least 50ms before reading the result. All arduino libraries do the same.
Can't remember if I added this because I had problems, but I probably did.

htu21d_async.py Outdated
from machine import I2C, Pin
import uasyncio as asyncio

class HTU21D(object):
Copy link
Owner

Choose a reason for hiding this comment

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

why do you copy the complete class instead of inherit it?

Copy link
Author

Choose a reason for hiding this comment

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

I tested adding async functions to the original library first but my tests showed me that I could save RAM by copying the class and removing the sync functions, so I went for that way.

htu21d_async.py Outdated

async def humidity_async(self):
raw = await self._issue_measurement_async(self.ISSUE_HU_ADDRESS)
if raw is None or raw==0:
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 also do a if not raw or is there any specific reason to do not?

Copy link
Owner

Choose a reason for hiding this comment

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

and why is 0 a bad value? 0 is perfectly fine and would give a relative humidity value of -6% which is the minimum of the htu.

Copy link
Author

Choose a reason for hiding this comment

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

0 theoretically is fine but sometimes I got the raw value 0 which was obviously an error so I tried to add the 0 to the errors.

htu21d_async.py Outdated


async def temperature_async(self):
raw= await self._issue_measurement_async(self.ISSUE_TEMP_ADDRESS)
Copy link
Owner

Choose a reason for hiding this comment

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

missing space before =

htu21d_async.py Outdated

def __init__(self, scl=None, sda=None, i2c=None):
"""Initiate the HUT21D
Args:
Copy link
Owner

Choose a reason for hiding this comment

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

missing new line

htu21d_async.py Outdated
async def _issue_measurement_async(self,write_address):
while self.running:
await asyncio.sleep_ms(160)
self.running=True
Copy link
Owner

Choose a reason for hiding this comment

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

missing spaces around assignment

htu21d_async.py Outdated
self.i2c.stop()
data=bytearray(3)
except:
print("Error in HTU instruction")
Copy link
Owner

Choose a reason for hiding this comment

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

print statements aren't good ideas for error reporting.
I would like to see an exception raised if something happens there instead of a print
and silently ignoring the error and just returning none.
I also would like to see a more explicit exception catch instead of a "catch" all.

Copy link
Author

Choose a reason for hiding this comment

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

You are right, the print was a leftover from debugging, errors in these parts mostly indicate that the sensor is not connected (or not working correctly). Have to check the exact error to catch it. Did not pay attention to that as I only wanted to add the running = False which is basically a (cheap) lock preventing multiple readings at the same time, as this is possible with async. Or should that lock be removed and one should just handle this in code using the sensor?

htu21d_async.py Outdated
data=bytearray(3)
except:
print("Error in HTU instruction")
self.running=False
Copy link
Owner

Choose a reason for hiding this comment

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

missing spaces around assignment

htu21d_async.py Outdated
self.i2c.readfrom_into(self.ADDRESS, data)
if not self._crc_check(data):
raise ValueError()
raw=(data[0]<<8)+data[1]
Copy link
Owner

Choose a reason for hiding this comment

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

missing spaces around assignment.

@julianhille
Copy link
Owner

Btw. im sorry that i forgot to thank you for the very first PR ;P

@kevinkk525
Copy link
Author

kevinkk525 commented Nov 19, 2017

Was using your library in my esp8266 smart home system and modified it for asyncio and wanted to share it back to you. Was my first pull request, so thanks for your detailed answers and especially for the library!

Please check the outdated ones too, github got some of those wrong.. they are still in the code, i just added the requested lines and spaces around assignments.

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