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

Adding AHT21 Temperature/Humidity Sensor #236

Merged
merged 9 commits into from
Nov 12, 2024
Merged

Conversation

thussaiththelaw
Copy link
Contributor

@brentru brentru requested a review from tyeth November 12, 2024 18:24
@brentru
Copy link
Member

brentru commented Nov 12, 2024

@tyeth Want to take a look at this user-contributed PR?

@tyeth tyeth self-assigned this Nov 12, 2024
Copy link
Contributor

@tyeth tyeth left a comment

Choose a reason for hiding this comment

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

Hey @thussaiththelaw 👋 thanks for contributing this new component!

I think the folder name just needs lowercasing, you can probably use git mv AHT21 aht21 from inside the i2c folder (+commit/push), and with that change then the Github Actions checks should pass.

We also prefer to ideally include a documentation link and product purchase link (but amazon links change too frequently / no international shipping), so if you could add the documentationURL property pointing to http://www.aosong.com/en/products-60.html and if you know of any other product links then let us know, the rest looks great!

To match this component definition, there will need to be a corresponding driver specified in the WipperSnapper firmware.
I'm not 100%, but I believe the sensor should work with the Adafruit AHTX0 library which we use for the AHT20. If you want to go and test that first then you can try adding the sensor to a wippersnapper device as an AHT20 instead of AHT21, or try running an Arduino example from the Adafruit AHTx0 repository:
https://github.com/adafruit/Adafruit_AHTX0/tree/master/examples

If that works then there just needs to be a minor change to the wippersnapper firmware to add the aht21 as another alias similarly to the aht20, see these lines where 4 device names are aliased to the same driver (add a fifth by duplicating L220 and updating the name):
https://github.com/adafruit/Adafruit_Wippersnapper_Arduino/blob/main/src/components/i2c/WipperSnapper_I2C.cpp#L219-L222
If not then we may need to add a new driver specifically for the AHT21.

Let us know how you get on!

Need to fix name of folder. Trying to delete then add it back.
Fixed folder naming issues.
I added website links to view the documentation, as well as to purchase a board with the sensor on it.
@tyeth
Copy link
Contributor

tyeth commented Nov 12, 2024

@thussaiththelaw, nice one updating the folder name and documentation url, it looks like the image needs to be 400x300 or 4:3 ratio (ideally in a relatively tight crop).

Best to remove the product purchase url as we're only really after proper retailers (and we were adding an exception for each domain). If you do wish to add the "productURL" field too, then use the same as the docs url.

I saw the firmware PR come in, that should get you UF2 files and binary copies of the app/firmware. It looks like the aht20 and aht21 share the same i2c address so you can probably test your AHT21 sensors functionality now as-is by choosing to add AHT20 component to your online wippersnapper device using the new component button.

Removed purchaseURL (not a proper retailer) and fixed picture size to 4x3
@thussaiththelaw
Copy link
Contributor Author

@tyeth followed your instructions. Hopefully this is it. Thanks for your clear directions and well written replies!

Copy link
Contributor

@tyeth tyeth left a comment

Choose a reason for hiding this comment

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

Sorry I should have also said to update the Vendor to ASAIR, as they make the chip and we're no longer linking to a vendor specific product link.
Then I'll merge this.

At that point it becomes live on io.adafruit.com, but the component will have an in-development banner. We eventually add a field called published and set that to true (it defaults false) once a new version of WipperSnapper is released.

@tyeth
Copy link
Contributor

tyeth commented Nov 12, 2024

@thussaiththelaw I actually preferred the other image, with no pins, but change the background size to match 400x300? Doesn't matter too much if you can't pull it off

@tyeth tyeth merged commit ec51a43 into adafruit:main Nov 12, 2024
13 checks passed
@tyeth
Copy link
Contributor

tyeth commented Nov 12, 2024

Should be live, thanks for the extra changes 👍

As said, I'll publish it to a non dev component once we do a firmware release. That shouldn't affect your usage of it (grab your firmware from the Actions page or your Checks section on the pull request in firmware repo, see summary page at bottom for build artifacts)

@tyeth
Copy link
Contributor

tyeth commented Nov 12, 2024

I'd forgotten than contributor PRs need the import run by a maintainer before it goes live on the server. Done now

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