Allow re-using ADC readings instead of re-reading all the time

[chrisib]

Add the reuse_last_reading parameter to analogue reader classes to allow re-using the previous reading rather than always taking a new sample.

Adds a new experimental BufferedReader class that allows re-using the last ADC reading instead of always re-sampling every time .percent(), .choice(...), etc... gets called.

[mjaskula]

Repeating my comments from discord here:

Personally I don't see the need for the readers to cache a value, and would prefer that scripts that need it cache it themselves, backed up by the fact that only one script out of 30 has asked for it. That said, the fact that the daisy's API caches by default means that I'm probably wrong.

[required] I'd like to see how this is used by a script, and how this api change improves that script. That is, I'd like to document the motivating use case in PRs that change the firmware api. Could you update a script to use these features? (I believe you said it was Pam's that would benefit.)

[chrisib]

As I've described on Discord and in the MR summary, the principle benefit is that this would allow you to read e.g. the value of ain and re-use that value for multiple subsequent bits of processing without incurring the performance hit of re-sampling the ADC every time.

e.g. suppose ain is being read as a s&h input, with the raw voltage sent to cv1 and an attenuated copy sent to cv2 with some sort of a GUI being displayed to visually indicate the incoming voltage (e.g. a bar graph).

cv1.voltage(ain.read_voltage())
cv2.voltage(ain.read_voltage() * ain.percent())
oled.blit(ain.choice(voltage_icons))

In this example the adc is sampled 3 times in succession, once for each ain.*() function call, meaning (depending on the speed of the signal coming into ain the reading used for the output voltages and the reading used for choosing may be wildly different from each other (this would likely happen with noise being patched in, or a sufficiently fast LFO).

By allowing the re-use of the last-read sample you eliminate this desynchronization, and make the code faster by not making unnecessary ADC reads.

cv1.voltage(ain.read_voltage())
cv2.voltage(ain.read_voltate(reuse_last_reading=True) * ain.percent())
oled.blit(ain.choice(voltage_icons, reuse_last_reading=True))

Yes, there are ways this could be accomplished with a wrapper class, or by using local variables and additional functions. But if the argument being made is that the firmware should be user-friendly, I'd argue that both of those solutions are less user-friendly than simply exposing the ability to bypass the ADC reads when that behaviour is desirable.

EDIT
Also, looking forward to EuroPi-X, where there are more analogue inputs, we could have a situation like this:

cv1.voltage(ain1.read_voltage() * k1.percent())
cv2.voltage(ain1.read_voltage() * k2.percent())
cv3.voltage(ain2.read_voltage() * k1.percent())
cv4.voltage(ain2.read_voltage() * k2.percent())

where CV1-4 are attenuated S&H copies of both analogue inputs. Using the existing implementation, this 4 line bit of code would result in 8 ADC reads, instead of the 4 that are actually required to achieve the desired output.

Looking towards eventual multi-threading, being able to control which thread is actually accessing the ADC hardware may become more important as well. On one thread, having a block that calls .percent() on all the inputs (knobs & analogue-in jacks) to read the ADC values, while the other thread passively accesses the latest-read values will likely be the desired implementation, rather than having both threads trying to read from the ADC (possibly concurrently).

[mjaskula]

Yes, there are ways this could be accomplished with a wrapper class, or by using local variables and additional functions. But if the argument being made is that the firmware should be user-friendly, I'd argue that both of those solutions are less user-friendly than simply exposing the ability to bypass the ADC reads when that behaviour is desirable.

This is the part that is my main issue, I guess. Why is it the API's responsibility to cache this value? If the client wants to use the value for two different calculations why not use a local variable?

Also the examples you provide feel like straw-men arguments. I'm not sure you would really write this code. (Perhaps including the changes to Pam's would help me understand in this respect.) In any code where I saw a client making a non trivial API call, expecting the same value returned, I would ask them to cache it in a local variable. I wouldn't ask the API to change to accommodate the need to use the value on several lines of code.

For an example that uses a different API, consider this Pams code. In the method PamsOutput.update_menu_visibility() there is a non trivial call wave_shape = self.wave_shape.get_value() which reads the value from a Settings object. After this call, that value is used several times in the method. Would it be better for the Settings class to cache the value and provide a way to access it via something like self.wave_shape.get_value(reuse_last_reading=True)? I don't think so. I think that the existing api is perfectly reasonable as is, and expect that the caller creates a local variable in order to reuse the value, as you have done.

Again, the fact that the API of a 'similar' firmware (daisy) provides this functionality, makes me think that I'm probably wrong here, but I just don't understand the motivation.

[chrisib]

Given the feedback I've removed the changes to the core firmware and instead added a wrapper class that accomplishes the same thing, but without impacting anything else.

[awonak]

[optional] I think it'd be helpful to add an example of how to create an instance of a BufferedKnob in the docstring.

[chrisib]

Done.