Some improvements to AirGradient-DIY.ino

[Cutyno]

• use preprocessor variable to reduce binary size
• add optional display
• reduce sensor polling due to global caching
• auto update values on display if forced update from webserver occured
• independend sensor update and display change intervall
• fix time problem between displayed values intervalls on display due to disabled sensors

[geerlingguy]

I like the overall idea behind this PR, but I don't have time right now to review it (just to set expectations for a timeline); would love to hear others' thoughts too.

[Cutyno]

I find this approach quite magnificent ;) It also workes fine as tested in the last two weeks

[Cutyno]

I have made some improvements in the code, but have come across a question:

What should we do with missing values or error messages from sensors?

• We send the previous successful value
• We send nothing and leave the sensor out of the message

I have roughly implemented the first suggestion, but I think the right way would be the second approach. Any thoughts?

[geerlingguy]

I kind of like both... though reporting the previous successful value can lead to people not knowing there's a problem.

[Cutyno]

I had the same problem as described in issue #24 and solved it by simply returning the last value if an error occurred during the transfer. There should be error handling, of course. My second suggestion has the drawback that it also shows the wrong value on the display or omits the sensor altogether because only the last value is stored.

[floyduk]

I have an issue with this and previous versions and the problem isn't in your code but your code treats it as an error.

My PM2.5 sensor often reads 0, which in the code is used as an error condition. But what if the proper PM2.5 reading is actually 0? My air particulates count does seem very low most of the time - it hovers between 0 and 2 a lot of the time unless I cook something. I do see it go up when I've been cooking so I'm confident that the sensor is working.

It's actually the AirGradient.cpp code that uses 0 as an error value:

int AirGradient::getPM2_Raw(){
  int pm02;
  DATA data;
  requestRead();
  if (readUntil(data)) {
    pm02 = data.PM_AE_UG_2_5;
    return pm02;
  } else {
    return 0;
  }
}

But this code doesn't differentiate between a real 0 value and an error.

The result, for me, is that for large chunks of the day my PM2.5 sensors are appearing broken.

[Cutyno]

Hello @floyduk, yeah thats a serious concern for me too. But to think about it. Does the particular sensor really works if it displays 0? This is a flaw in the implementation of the AirGradient which I want to improve for quite some time. They treat an error of the sensor with the value 0, which makes sense, because the sensor isn't counting any particles. Like @geerlingguy mentions the error of the sensor should be obvious to the user. One way to tackle the problem would be to teach grafana or prometheus to handle missing values as 0. Another approach would be to modify the sketch and the lib beneath to your need. Would love to see a PR on this matter.

[floyduk]

I think there is a difference between a 0 and an error. If you look through the AirGradient library code you can see that it's running a loop that checks for a result and if it isn't able to get a reading within an allotted time then it returns a 0. If only they'd made that error return a -1 instead then we could distinguish an error from a 0 value.

I included the getPM2_Raw() code above. Here's the ReadUntil() code that it calls:

bool AirGradient::readUntil(DATA& data, uint16_t timeout)
{
  _data = &data;
  uint32_t start = millis();
  do
  {
    loop();
    if (_PMSstatus == STATUS_OK) break;
  } while (millis() - start < timeout);

  return _PMSstatus == STATUS_OK;
}

As you can see this returns a true/false indicating whether it was able to get a reading. I won't bother including the loop() code because it's long but you can read it all on their github here: https://github.com/airgradienthq/arduino/blob/master/AirGradient.cpp

[Cutyno]

Don't get me wrong. I'm totally on the same page with you. But AirGradient already made this Design choice and I can't do anything about it. It's always a pain in the a** if you want to change existing and used interfaces. On top of that they apperently don't accept PR anymore. This whole library has many problems which is the reason I wan't to move away from this project compleatly. If you want to make a PR on top of my PR, feel free do do so. I'm happy and willing to review it and merge it if possible.

[floyduk]

Yeah that's fair. If I knew how to actually use the AirGradient code to compile it and then use it in the sketch then I might just do that but sadly I don't know how. I appreciate you doing as much as you have with the project. I think, given that AirGradient's library has this bug, I would modify your code simply to stop treating a 0 result as an error. In fact that's what I've done in my copy of it that is running on my air quality monitors.

[stale[bot]]

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

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

[stale[bot]]

This pull request is no longer marked for closure.

[Cutyno]

I was just looking for the option to disable display, I don't thin it's required with prometheus in place

You could just unplug the display. It would have the same effect. My approach uses the uC a little bit more efficient and exclude unused code.

[vveliev]

Yeah I like it!

I think this is more common approach in embedded electronics

[stale[bot]]

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

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

[Cutyno]

I guess since there is little to no movement, that there is no interesst in merging this request at all?

[stale[bot]]

This issue is no longer marked for closure.