-
Notifications
You must be signed in to change notification settings - Fork 130
Ignore parameter values out of range #197
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
Conversation
src/Main/utils.cpp
Outdated
} | ||
|
||
int utils::correctPMS(int value) { | ||
if (value < 10) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not allow values below 10? My sensors regularly go below that. Maybe limit it to 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean I don't have to. Even that issue says 0 should be the lowest value for PM data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things:
Only negative PM values (below 0) need to be ignored. 0..10 are valid readings and should be allowed.
Also, if a value is outside the range, the value should be completely ignored.
So for example Temperature -100 should NOT return -40 but be ignored. This would apply to all checks.
For this way we don't need to correct value in range, just need to verify it's valid or not before use(show on display or sync to cloud). The correction method will be remove and replace check valid valid method. bool utils::validTemperature(float value) {
if (value >= VALID_TEMPERATURE_MIN && value <= VALID_TEMPERATURE_MAX) {
return true;
}
return false;
} @airgradienthq How do you think? |
@pnt325 yes I think that's how it should work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I was asked to review this, but I don't know internals. So from a basic look at it, it looks good to me. 😛
Correct sensor value has out of range #190