geerlingguy / airgradient-prometheus

AirGradient Prometheus exporter.
MIT License
176 stars 59 forks source link

Complete rework of the Sketch #23

Closed Belphemur closed 2 years ago

Belphemur commented 2 years ago

Hey @geerlingguy,

Firstly, let me thank you for introducing me to the world of Arduino, I always wanted a microcontroller project and AIrGradient and your video was the push I needed. Especially when you paired the sensor with a full ansible setup for prometheus and grafana \o/ Just awesome.

I've dived into the code, and realized ... I hate arduino IDE, so I've taken the time to rewrite the sketch using VS Code with PlatformIO plugin. Such a breeze to use with proper library management, a one file to setup the board that I'm using. It made me like C++ again.

So here we go, here is my fork: https://github.com/Belphemur/AirGradient/

Changes

  1. I've broken the code into multiple files and classes
    1. Metrics::Gatherer takes care of pulling the metrics from the sensor at different interval and save them in memory.
    2. Prometheus::Server takes care of providing the HTTP server and rely on the Gatherer to get the metrics.
  2. After reading the forum of AirGradient, I changed the way the board interact with the PMS5003 sensor to increase its lifespan (source: forum)
    1. Basically, I wake up the sensor every 120 sec for 30 secs. Do the reading, and ask it to go back to sleep. The forum post says that this could increase the lifespan from 3 years to 18 years 👍
  3. All the different configuration got moved into a Configuration folder to be easily modified.
  4. I've added a couple of mitigation against getting wrong data
    1. When getting -1 or 65k for CO2, I retry the reading with a 10ms delay. Works 99% of the time.
    2. Only accept 0 as PM2 when the previous reading was under 10. To avoid the weird 0 spike in the graph. (Usually, it mean there was a a problem getting the value from the sensor).

I let your decide if you'd like me to make a PR for it.

geerlingguy commented 2 years ago

I like the basic idea, though I don't think I want to do a full rewrite for this library again (I just accepted one a few weeks back, and I may be switching to ESPHome at some point, too...).

However, it sounds like a few of the changes (like the extra bit for -1 CO2 values and potentially making the PMS5003 sensor last a bit longer) might be decent enough to break out into their own issues (whether you want to submit PRs for them or not, I know it's a bit of a burden to just extract particular bits out into separate PRs).

Belphemur commented 2 years ago

I understand, it's also a complete change of dev environment.

I can easily port the CO2 reading fix, however for the PMS5003, I brought another library, so that would require updating the guide on how to setup the sketch. At that point, I'd really suggest going away from the sketch and move to PlatformIO instead.

In ESPHome, the PMS5003 currently doesn't have the wakup/sleep ability implemented. The underlying code managing the PMS5003 would need to be updated to be able to send the wakup/sleep commands.

I imagine the same is true for the Sensair S8 to get accurate CO2 readings and reject the -1/65k.

Edit: Last thing that I changed, I read the CO2, Temp and Humidity every 5s instead of on-demand.

Thelvaen commented 2 years ago

I was considering rewriting the sketch as well, with the following in mind :

With what you're saying about the PM5003, i might need to use a 3rd ticker to get the PMS5003 extended life.

Also I was considering displaying all values at once on the screen, because, with the BME280, CO² & PM sensor, you have 5 values to be displayed, that means if you want to see the value that was just displayed, you might have to wait 20 seconds to get the value you want while it cycle through the sensors ...

Belphemur commented 2 years ago

@Thelvaen All that is implemented in my fork.

I use arduino-timer lib to take care of all the timing logic.

Thelvaen commented 2 years ago

@Belphemur yeah i saw that, I'd still rather use the ESP Ticker built-in features that are based on the hardware timer/interrupt mechanism and do not need an additional library.

// Declaring Tickers component
Ticker SensorTicker;
Ticker ScreenTicker;

And in setup()

  SensorTicker.attach_ms(MEASURE_FREQ, FetchSensors);
  delay(1500);
  ScreenTicker.attach_ms(SCREEN_FREQ, UpdateScreen);

Also, I made a thing to use the (once again) built-in EEPROM class to implement a basic REST API to define the temperature offset see #19 :)

Belphemur commented 2 years ago

@Thelvaen Ho, I didn't know about the ESP built-in Ticker. I'll definitely switch the code to use that instead of the timer library.

I saw your work on #19, I wonder thought if using the JSON lib isn't overkill for setting a simple temperature offset. Wouldn't it be better to just be a text/plain with a parsed string to int ?

Thelvaen commented 2 years ago

@Belphemur yeah definitely, that why my BME280 is still WIP, I wanted to write a parser for that offset to send plain text rather than to had an heavy JSON library, it's just that I wanted the thing to work :)

Thelvaen commented 2 years ago

@Belphemur I believe that what we have in mind (at least for some parts) is not entirely compatible with the vision @geerlingguy have for his project (and I have no problem with him having a different vision & objectives), so I believe we might want to continue either on your fork or on mine (I am in favor of using Platform.IO as well as for a restructuring of the project - having sensor related stuff in separate files from web server stuff, or from screen stuff).

stale[bot] commented 2 years ago

This issue has been marked 'stale' due to lack of recent activity. If there is no further activity, the issue will be closed in another 30 days. Thank you for your contribution!

Please read this blog post to see the reasons why I mark issues as stale.

stale[bot] commented 2 years ago

This issue has been closed due to inactivity. If you feel this is in error, please reopen the issue or file a new issue with the relevant details.