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

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

Closed
francoisgeorgy opened this issue Feb 11, 2023 · 4 comments · Fixed by #232
Closed

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

francoisgeorgy opened this issue Feb 11, 2023 · 4 comments · Fixed by #232
Labels
firmware Software related issue

Comments

@francoisgeorgy
Copy link

francoisgeorgy commented Feb 11, 2023

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?

@francoisgeorgy francoisgeorgy added the firmware Software related issue label Feb 11, 2023
@redoxcode
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

redoxcode commented Feb 28, 2023

created the PR #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
Copy link
Collaborator

mjaskula commented Mar 2, 2023

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

@awonak awonak linked a pull request Mar 19, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
firmware Software related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants