Allen-Synthesis / EuroPi

EuroPi: A reprogrammable Eurorack module based on the Raspberry Pi Pico
Creative Commons Zero v1.0 Universal
431 stars 84 forks source link

[Bug Report] AnalogInput calibration improvement when using only two points #221

Closed francoisgeorgy closed 1 year ago

francoisgeorgy commented 1 year ago

Hi,

I think there's a little bug in the way the voltage is computed when the calibration has been done with only two point (0 and 10).

In AnalogueInput.read_voltage() the code :

cv = 10 * (reading / INPUT_CALIBRATION_VALUES[-1])

should take INPUT_CALIBRATION_VALUES[0] into account and becomes :

cv = 10 * ((reading - - INPUT_CALIBRATION_VALUES[0]) / (INPUT_CALIBRATION_VALUES[-1] - INPUT_CALIBRATION_VALUES[0]))

This is especially important when reading low voltage. If, for example, we have :

INPUT_CALIBRATION_VALUES = [384, 44634]

The result, with reading = 384 is

# current code: 
cv = 10 * (384 / 44634) = 0.08603306896088184

# new code:     
cv = 10 * ((384 - 384) / (44634 - 384)) = 0.0

And with reading = 44634 :

# current code: 
cv = 10 * (44634 / 44634) = 10.0

# new code:     
cv = 10 * ((44634 - 384) / (44634 - 384)) = 10.0

We can see that the 0V computation (with reading=384) is now correct and that the max voltage is not affected.

A similar issue is present in AnalogueInput.percent() :

current code :

max_value = max(reading, INPUT_CALIBRATION_VALUES[-1])

proposed correction :

max_value = max(reading, (INPUT_CALIBRATION_VALUES[-1] - INPUT_CALIBRATION_VALUES[0]))

What do you think? Am I correct?

redoxcode commented 1 year ago

I think you're right! Good catch! I never looked at the actual implementation of the calibration before. I made a fork that should fix the issue in all relevant places: https://github.com/redoxcode/EuroPi/blob/calibration-fix/software/firmware/europi.py Maybe you can double check these changes for me before I open a PR?

(some tests still fail, but it's only one contrib script (tests/contrib/test_europi_turing_machine.py) so I guess it's an issue of this test and not the code changes)

francoisgeorgy commented 1 year ago

Your fix looks good to me. Thank you very much for your time. I could have opened a PR.

I don't have the time to run the tests and test with my EuroPi today but can do it in a few days if you want. But, your code is fine and I don't see any problem.

redoxcode commented 1 year ago

created the PR https://github.com/Allen-Synthesis/EuroPi/pull/232 Even if I haven't calibrated my modules yet, I am very interested in improving the Ain and Output accuracy, since it opens up more use cases for the europi. Edit: I have to work out why that test fails...

mjaskula commented 1 year ago

I took a quick look at the test failures and didn't see anything obvious. I'll look again after some other priorities.