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

ESP32: Add AM2320 support #3202

Closed

Conversation

thornley-david
Copy link

@thornley-david thornley-david commented Jul 8, 2020

  • This PR is for the dev-esp32 branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/*.

Ported almost raw from ESP8266 implementation (https://github.com/nodemcu/nodemcu-firmware/blob/release/app/modules/am2320.c).

Ported raw from 8266 implementation, still using busy-wait for read function
fix whitespace
@thornley-david thornley-david changed the title Added AM2320 support ESP32: Add AM2320 support Jul 8, 2020
@marcelstoer
Copy link
Member

I'd like to land #3135 from @nwf first and then for you to rebase.

@thornley-david
Copy link
Author

thornley-david commented Jul 9, 2020 via email

@nwf
Copy link
Member

nwf commented Jul 9, 2020

Giving the C a quick once-over, there doesn't seem to be any reason that this should be a C module (or ESP32 specific) and not just some Lua code: the sensor communicates over I2C, there appear to be no timing requirements, and all math is on small integers. Maybe see #3197 or the similar Lua modules (e.g., bh1750 or ds2321 or mcp23008)?

ETA: if that's true, then there's no reason to stay blocked behind #3135.

@thornley-david
Copy link
Author

thornley-david commented Jul 10, 2020

@nwf, good point. I was just trying to reuse/migrate what was already there, however if there is a better way I am happy to rewrite it.

There are a couple of microsecond timing delays in the read negotiation, but they are likely not strict. To be honest, I am not aware if these can be transitioned to lua efficiently, and I was also not sure if the methodology I used was even correct when migrating (os_delay_us -> ets_delay_us).

I'll take a look at the sample modules in detail and disband the PR if I manage to rewrite it as a lua module.

@marcelstoer
Copy link
Member

I'll take a look at the sample modules in detail and disband the PR if I manage to rewrite it as a lua module.

Have you come to a conclusion yet?

@@ -54,6 +54,12 @@ config LUA_MODULE_ADC
Includes the adc module. This module provides access to the
adc1 hardware.

config LUA_MODULE_AM2320
Copy link
Member

Choose a reason for hiding this comment

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

In case you rebase this will have to become NODEMCU_CMODULE_AM2320.

@stale
Copy link

stale bot commented Aug 28, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 28, 2021
@stale stale bot closed this Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants