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

WIP: Warn about unusual sensor readings #78

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mmtj
Copy link

@mmtj mmtj commented Oct 31, 2019

Related issue: #77

This PR attempts to warn user about unusual sensors readings by log messages. In current state it is more like crude attempt and I am open to suggestions, how to make this PR better.

There certainly is room for improvements, just to mention few:

  • Is there a generic way to report such readings from any SensorDriver instead of impletement it in every driver read_temps()?
  • Is it better to use string/stringstream in section of code or predefined string using #define in header file?

@vmatare
Copy link
Owner

vmatare commented Oct 31, 2019

As in my comment on the issue: It won't work this way. Some implementation-related remarks presupposing you actually want to go through with the mentioned noise estimation:

  • The entire logic should go into the TemperatureState class.
  • You should be able to just << everything into the log(...) function.
  • Missing noise should be checked/reported in the run(...) function, but only after enough data have been gathered.

@vmatare vmatare added the enhancement Idea for an improvement or a new feature label Oct 31, 2019
@vmatare
Copy link
Owner

vmatare commented Oct 31, 2019

Giving it some more thought: If we want to warn/fail only if the noise is actually 0, the noise estimation can (and should) be rather cheap.
Note also that in DANGEROUS mode it should be logged as an error, but thinkfan should keep running. That's what the error(...) function is for.

@mmtj
Copy link
Author

mmtj commented Oct 31, 2019

Giving it some more thought: If we want to warn/fail only if the noise is actually 0, the noise estimation can (and should) be rather cheap.

Sound reasonable. But just to be on the same side, what exactly do you mean by noise here?

@vmatare
Copy link
Owner

vmatare commented Nov 1, 2019

what exactly do you mean by noise here?

Well, the noise in the temperature readings. If there is no noise in a sensor, we can safely assume that sensor isn't measuring anything.

That said, we don't need a "correct" noise measure. We could e.g. just sum up the temperature changes during the last N cycles. If that sum is 0 for a large enough N, we know something must be wrong.

@mmtj
Copy link
Author

mmtj commented Nov 4, 2019

Ok, that sound fairly simple and I'll try to implement it.

@@ -41,6 +41,7 @@
#include "config.h"
#include "message.h"

const int NOISE_MEASUREMENT = 10;

Choose a reason for hiding this comment

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

constexpr int NOISE_MEASURE_COUNT = 10;

@@ -299,6 +303,20 @@ void TemperatureState::add_temp(int t)
int diff = t - *temp_;
*temp_ = t;

if (diff == 0) {
if (*noise_counter < NOISE_MEASUREMENT) {

Choose a reason for hiding this comment

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

Could you explain why do we need the whole vector noise_counters when we only use the first element?
If I understand your logic, then you just need to keep one static count, increase that count every time diff==0 and compare that count with a fix threshold. In this case, you don't even need noise_counter and noise_counters as member variables.

@wonbinbk
Copy link

Oh just realize this is from 2019. Sorry for digging it up. Do we still need this feature though?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Idea for an improvement or a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants