Incorrect sampling in AnalogSensor.h

[JavierAder]

Device

Lolin

Version

1.18

Bug description

Hi. I suppose the idea of ​​the samples variable is to achieve some kind of average between several measurements as the final result.

espurna/code/espurna/sensors/AnalogSensor.h

Lines 122 to 138 in 0976bdb

	static unsigned int _rawRead(uint8_t pin, size_t samples, Delay delay) {
	// TODO: system_adc_read_fast()? current implementation is using system_adc_read()
	// (which is even more sampling on top of ours)
	unsigned int last { 0 };
	unsigned int result { 0 };
	for (size_t sample = 0; sample < samples; ++sample) {
	const auto value = ::analogRead(pin);
	result = result + value - last;
	last = value;
	if (sample > 0) {
	espurna::time::critical::delay(delay);
	yield();
	}
	}
	return result;
	}

But if I'm not mistaken, that code is simply returning the last value read from analogRead.

I think that a way to approximate the average (without fear of generating overflows) can be

            unsigned int last { 0 };
            unsigned int result { 0 };
            for (size_t sample = 0; sample < samples; ++sample) {
                const auto value = ::analogRead(pin);
		if (sample == 0)
		   last = value;
		 //cumulative average plus weighted difference  
                result = last + (value - last)/(sample+1);
                last = result;
                if (sample > 0) {
                    espurna::time::critical::delay(delay);
                    yield();
                }
            }

            return result;

Surely there are other ways to approximate the average, that's the one that occurred to me. I think it is correct, or at least it does not generate crazy results.

Steps to reproduce

No response

Build tools used

No response

Any relevant log output (when available)

No response

Decoded stack trace (when available)

No response

[mcspr]

Apparently, c22b369 changed a lot of neighbouring sensors.

Commit above does something different; instead of waiting for the sample(s) immediately, value is cached to satisfy the delayed reading timing and is sampled in tick() instead of pre() / value()
Only difference is how close in time the actual value is to the reading routine... but, I would guess it is already controlled by the reading time, or it could simply continue sampling and reset _sample num to zero w/ some delay in-between

[JavierAder]

Apparently, c22b369 changed a lot of neighbouring sensors.

Commit above does something different; instead of waiting for the sample(s) immediately, value is cached to satisfy the delayed reading timing and is sampled in tick() instead of pre() / value() Only difference is how close in time the actual value is to the reading routine... but, I would guess it is already controlled by the reading time, or it could simply continue sampling and reset _sample num to zero w/ some delay in-between

But the problem I see isn't about the reading timing, but rather the way it calculates the average; in previous version clearly does it well:
return sum / _samples;
c22b369#diff-bd3fd1ad9a14d529a9c282ebe43555d33e7f3464e844c8220e9fccb79c5740cbL110

[mcspr]

Commit above i.e. commit a1ffa9f, average is calculated as sum divided by sample total. Weights can be added, if necessary

I also meant to point out time spent in yield() compared to time it can spend in main task.

espurna::time::critical::delay(delay); // cpu spin, nothing happens
yield(); // task yield to scheduler, wait sdk, resume here

[JavierAder]

Commit above i.e. commit a1ffa9f, average is calculated as sum divided by sample total. Weights can be added, if necessary

I also meant to point out time spent in yield() compared to time it can spend in main task.

espurna::time::critical::delay(delay); // cpu spin, nothing happens
yield(); // task yield to scheduler, wait sdk, resume here

ah, sorry, I hadn't seen your last commit... You are reading the sensor in tick().
Does this method have timing restrictions?
I'm asking you about this because of the use I'm trying to make of analog multiplexers; Multiplexers require some delay between setting the address and reading it, and this delay has to occur every time analogRead() is called.

Btw, I built a version of my code with your modifications; I'll test it as soon as I can.

[mcspr]

Does this method have timing restrictions?

Sensor::tick() is called every Arduino loop(), so it can be treated similarly.
(no clever coroutine code, just two tasks. for now :)

The general idea was caching value for value(). Actually reading this cached value has to fit somewhere between sensor module readings which call pre() and value()