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

feat: added anemometer(wind speed) sensor #611

Merged
merged 14 commits into from
Feb 23, 2024

Conversation

QB4-dev
Copy link
Contributor

@QB4-dev QB4-dev commented Feb 17, 2024

This is driver for wind sensors like this one:
https://www.amazon.com/TOPINCN-Speeds-Anemometer-Environment-Anemometers/dp/B083JFLNR7

Tested on ESP8266

@UncleRus
Copy link
Owner

In general, I don’t really like this component that works on counting GPIO interrupts. This approach is still quite appropriate for the ESP8266, but the ESP32 has a hardware pulse counter.

If the component supported PCNT for esp32 and used GPIO interrupts as a fallback for esp8266, this would be a different matter.

In its current form, this component will not be included in the library, sorry.

@UncleRus UncleRus marked this pull request as draft February 18, 2024 09:06
@QB4-dev
Copy link
Contributor Author

QB4-dev commented Feb 18, 2024

In general, I don’t really like this component that works on counting GPIO interrupts. This approach is still quite appropriate for the ESP8266, but the ESP32 has a hardware pulse counter.

If the component supported PCNT for esp32 and used GPIO interrupts as a fallback for esp8266, this would be a different matter.

In its current form, this component will not be included in the library, sorry.

You are absolutely right. When I will have access to ESP32, I'll add ESP32 hardware pulse counter. Please leave this PR as is until I add ESP32 part

@QB4-dev
Copy link
Contributor Author

QB4-dev commented Feb 18, 2024

I have modified this driver according to your suggestions but not going very well...

There are two significant problems:

  1. ESP32C3 has no hardware PCNT so I cannot use PCNT, but it is still ESP32 and HELPER_TARGET_IS_ESP32 is not enough to setup interrupt based PPS counter. I disabled ESP32C3 support for now.

Maybe a macro like this will be good enough?

#define ESP_PCNT_SUPPORTED HELPER_TARGET_IS_ESP32 && !defined(CONFIG_IDF_TARGET_ESP32C3)
  1. ESP-IDF version < 5.0 uses old PCNT driver so it may be easier to fallback to ESP8266 style approach than writing new code with outdated driver

@QB4-dev
Copy link
Contributor Author

QB4-dev commented Feb 18, 2024

I decided to add described above changes.

Now driver is using PCNT on all ESP32 chips except ESP32C3 and on ESP-IDF v5.0.0 and above, and interrupts on ESP8266 and with older ESP-IDF

@UncleRus UncleRus marked this pull request as ready for review February 19, 2024 03:25
@UncleRus
Copy link
Owner

That's a horse of another color!

But please run clang-format on your code.
Then add a documentation page for your component to /docs/source/groups. Look at existing documentation pages to understand how to do this.
Don't forget to add a link to your documentation page in the table of contents (/docs/source/index.rst).

@QB4-dev
Copy link
Contributor Author

QB4-dev commented Feb 19, 2024

Ok. I am gonna add documentation and of course run clang-format on my source files(sorry about that).
It may be little late but how about to rename this driver to 'impulse-sensor'?
I have written it for anemometr, but it may be useful for variety of sensor with impulse output. For example fluid flow meters, shaft rotation sensors etc.

@UncleRus
Copy link
Owner

No problem, you can rename it

@QB4-dev
Copy link
Contributor Author

QB4-dev commented Feb 20, 2024

OK. Ready to review

@UncleRus
Copy link
Owner

Thank you!

@UncleRus UncleRus merged commit 853f704 into UncleRus:master Feb 23, 2024
40 checks passed
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