universam1 / iSpindel

electronic Hydrometer
http://www.ispindel.de
Other
827 stars 322 forks source link

Feature Request: Stable Data Check, short term sleep and reading delay #473

Closed quarky42 closed 3 years ago

quarky42 commented 3 years ago

I'm submitting a ...

Report

If there isn't already a check like this in place, I would suggest checking for a minimum level of stability in the measurement before reporting a data point. This logic would be unused in most cases, except if you just shook up or swirled up a batch you are fermenting.

Features

The process would look like this:

When the device wakes up and measures the tilt / specific gravity, it would then take 2 or 3 readings back to back over a short period of time. If the delta between these 3 readings is sufficiently small, everything would work like normal / as is currently.

If there is a delta between the back to back short-term readings that is sufficiently large, a flag would be set and a short term sleep would be created. Maybe someone just jostled the batch and the device woke up in the middle of it moving. Next time it wakes up, it would check for this flag and know that it has already missed one reading and act accordingly. (One idea is that if it delays one reading by a short period, then it doesn't delay the next reading and the user gets whatever noise in the data they are going to get.)

This feature would serve to smooth out the data if someone moved or jostled the wine or beer base by noticing the varying erroneous readings across a very short period of time. If it was a momentary disturbance, a 1-2 minute delay in the reporting of data wouldn't hurt anything and if it was still varying at that point, there must be something beyond the help of simple output smoothing like this.

A> there might already be something like this in the firmware, in which case this idea is moot. B> if there isn't something like this already, then maybe this can serve as the inspiration to create something even better than what I've written here. I only mean this as a means to explain how the output data could be smoothed if there was a period of time where the batch of liquid was being temporarily disturbed.

Thanks.

pppedrillo commented 3 years ago

Proposed approach can drain a battery very fast and generally will not work while active fermentation step when there is "storm in the glass" and ispindel disturbed by it.

quarky42 commented 3 years ago

I think you misunderstood the suggestion and/or I wasn't clear enough: I wasn't suggesting endless sleep delays just one. If it detects turbulent conditions it sleeps for a period of time that it could reasonably settle and does a measurement and report no matter what when it wakes back up.

I wasn't suggesting a low delta. Shaking or swirling a batch up will produce very high changes in tilt angle in a very short amount of time. What is the minimum time to detect this? Maybe less than a second. Maybe a fraction of a second. What is a good minimum tilt angle delta from one measurement to the next that would indicate shaking and not yeast activity?

A counter could be used to count the number of reading delays that have been triggered and set a limit to them. Likewise a counter could detect still conditions where the angle isn't changing wildly and then reset that counter.

Is reading the angle really that costly to the battery?

ErikdBr commented 3 years ago

I use brew-spy. Brew-spy is handling this already. It delays updating the graph to avoid to big fluctuations in SG caused by what you described or other causes. After shaking or swirling a batch(if you should do that at all) the wort will settle down very quickly.

quarky42 commented 3 years ago

I update every 10 minutes, but if have bad timing, I could end up shaking it just before an update.

thegreatgunbantoad commented 3 years ago

image

It already does a median average over 50 samples 2 ms seconds apart. That delay could easily be extended. Or you could do a variance calc on the data points and dump the data if the variance is a bit high.

However since BF will show you a smoothed graph if you want, there is not a great advantage to having this. Personally I'd rather record the bad datapoint as is.

Side note. Not a fan of this line: if (i >= MEDIANROUNDSMIN && isDS18B20ready()) relying on the C++ order of evaluation, I'd rather see appropriate brackets if ((i >= MEDIANROUNDSMIN) && isDS18B20ready()) but that's just my personal preference.

pppedrillo commented 3 years ago

@thegreatgunbantoad You can relax and be sure in operator order of evaluation. It is standard. https://docs.microsoft.com/en-us/cpp/c-language/precedence-and-order-of-evaluation?view=msvc-160 And it is C, not C++ btw

What I dont like here is checking a temp probe readiness before stop sampling the acceleration. Just why.

thegreatgunbantoad commented 3 years ago

@pppedrillo it's not the best coding practise to rely on the order of evaluation. The files are .cpp but the order likely comes from C anyway.

Good point on the temp sensor, not really sure why that function needs the temp sensor at all.

pppedrillo commented 3 years ago

@thegreatgunbantoad Why there in C would be this standardized order of evaluation if you couldn't rely on it? ;)

The files are .cpp

Sorry for confusion, I meant evaluation order was set up for C language. Ofc it was inherited by C++

thegreatgunbantoad commented 3 years ago

@pppedrillo its just bad practice in the same way as relying on BODMAS is. It's user hostile for newbies too.

It's scary the number of people that will get the answer to something like 2*3+9*4 wrong.

Of course the correct answer is the square root of pi raised to the power of Avogadro's number :)

thormj commented 3 years ago

Good point on the temp sensor, not really sure why that function needs the temp sensor at all.

Basically... if the temp sensor isn't ready (the DS1820 just dang slow), keep taking data and averaging it since you're going to be waiting for the temp sensor anyway. So... "until you get enough samples" and "until the temp sensor is ready to go," just keep averaging...

pppedrillo commented 3 years ago

Good point on the temp sensor, not really sure why that function needs the temp sensor at all.

Basically... if the temp sensor isn't ready (the DS1820 just dang slow), keep taking data and averaging it since you're going to be waiting for the temp sensor anyway. So... "until you get enough samples" and "until the temp sensor is ready to go," just keep averaging...

My pojnt was: Function name "getTilt" - should get the tilt. Not waiting for bingo results and not making a cup of coffee.

https://en.wikipedia.org/wiki/Single-responsibility_principle https://en.wikipedia.org/wiki/Separation_of_concerns

If someone need to wait for DS1820 - it can do it whenever it needs, for example - in getTemperature(). DS1820 readiness doens't matter for tilt sampling. And it might be so that it isn't necessary there to wait for ds1820 at all - eg it can gets ready any time after gyro sampling and before temp readings request. Thus check its readiness just before temp reading might be more efficient.

thormj commented 3 years ago

It's nice to do one thing at a time, but it tends to be slow and use battery life... So what it's doing now is

  1. "start up the DS1820" (slow)
  2. "start up the accelerometer"
  3. "take readings on the accelerometer until >29 samples have come by AND the DS1820 is ready
  4. Compute average of accelerometer [on my iSpindels, that's about 35 samples)
  5. Shut down accel
  6. Read DS1820
  7. Shut down DS1820

There's no point in going to step 6 before it's ready in the getTilt function; you'd just be waiting on it to become ready and miss samples 29-35. If you did 2-3-4-5-1-6, you're not parallelizing the hardware and you're keeping the (very hungry) ESP8266 awake for a lot longer than you need to (almost 70 "accel sample times" vs 35 right now).

It is kinda ugly philosophically, but it works a treat in practice (like cooking simmering marinara sauce while you're waiting for the chicken strips to be done; you may as well keep the marinara on simmer until the chicken is done); the "other, more manageable way" would be for the DS1820 to ring an interrupt when it was ready that would stop the TiltAveraging task... and that's why FreeRTOS and the like were created - to unspaghetti code by adding more "concepts" to organize it.... but that's its own pile of spaghetti (I use it in my BLE projects).

On the other hand... the MPU has a temp sensor. I'm going to see how that compares against the DS1820... On the third hand... I suppose you could pass in a "move to next step" function point and that would genericize it... we'd just be passing "isDS1820ready" into the new *ptrMoveOn() argument). Or I suppose you could rename the "getTilt" function "collectTiltDataUntilTempIsFinished."

On the 4th hand... that's why I duplicated the code a bit in my TiltCommands patch -- when I'm supposed to be sleeping, I don't really want 29 samples and temp data..... so take 2 and see if I should wake up (and set global variables to shuffle things behind the back, so not quite so clean either)....

universam1 commented 3 years ago

Simply put, forget the temperature information from the Accel it is ok for its internal DSP but totally unusable for our purposes.

pppedrillo commented 3 years ago

So what it's doing now is

1. "start up the DS1820"  (slow)

2. "start up the accelerometer"

3. "take readings on the accelerometer until  >29 samples have come by  AND the DS1820 is ready

4. Compute average of accelerometer [on my iSpindels, that's about 35 samples)

5. Shut down accel

6. Read DS1820

7. Shut down DS1820

If you did 2-3-4-5-1-6, you're not parallelizing the hardware and you're keeping the (very hungry) ESP8266 awake for a lot longer than you need to (almost 70 "accel sample times" vs 35 right now).

@thormj

function initPeripherals()

  1. "start up the DS1820" (slow)
  2. "start up the accelerometer"

    function getTilt()

  3. "take readings on the accelerometer until >29 samples have come
  4. Compute average of accelerometer

function getTemperature()

  1. wait until DS1820 is ready
  2. Read DS1820

function shutdownPeripherals()

  1. Shut down accel
  2. Shut down DS1820
thormj commented 3 years ago

Simply put, forget the temperature information from the Accel it is ok for its internal DSP but totally unusable for our purposes.

What's wrong with it (too noisy even with 29 samples or a lot of chip self-heating or ?)

thormj commented 3 years ago

Then there's a 7 AccelSample "busy wait" in step 5.... 7 "free" samples (plus or minus; I bet "29" was chosen to "be about the time it takes for the DS1820 to become ready") that aren't collected.

And for power saving, I'd put 7 before 5; no reason to keep it on while you're playing with the DS1820.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.