Skip to content

Conversation

@jneem
Copy link
Contributor

@jneem jneem commented Oct 1, 2023

I don't understand why the adc reading provides values in some undefined units instead of in mV directly. At first I thought it was for extra precision (because ref_mv / 4096 is less than 1, and so you lose some of the lower bits when converting to mV), but in fact the calibration code is getting the reading in mV first and then multiplying by 4096 / ref_mv so the precision was never there and it all just seems like wasted work.

I've only done it for esp32c3 so far, but if it sounds like a reasonable change then I'll do it for the others.

Thank you!

Thank you for your contribution.
Please make sure that your submission includes the following:

Must

  • The code compiles without errors or warnings.
  • All examples work.
  • cargo fmt was run.
  • Your changes were added to the CHANGELOG.md in the proper section.
  • You updated existing examples or added examples (if applicable).
  • Added examples are checked in CI

Nice to have

  • You add a description of your work to this PR.
  • You added proper docs for your newly added features and code.

@jessebraham
Copy link
Member

Sorry for the delay in getting to this. Unless @bjoernQ or @MabezDev have some reason not to then I think these changes are fine to make, yeah. Not sure why it was implemented like this in the first place, I didn't write this driver.

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 12, 2023

Somehow missed this one. Seems to make sense for calibrated reads

Might be worth to look into esp-idf again

@jneem
Copy link
Contributor Author

jneem commented Oct 21, 2023

Ok, I had a look in idf and I'm pretty sure this matches with what they're doing there. Specifically, for linear calibration I'm looking at components/esp_adc/adc_cali_line_fitting.c the main (only?) conversion function there is cali_raw_to_voltage, which does the linear calibration and returns the result as a voltage.

On a related note, I think our previous code was wrong for the curve calibration, because in components/esp_adc/adc_cali_curve_fitting.c they work with the linearly-calibrated voltages. Our previous code worked with scaled (by 4096 / ref_mv) values and tried to compensate by also scaling the polynomial coefficients. But the coefficient scaling was wrong: the nth coefficient should have been scaled by (4096 / ref_mv)^n in order to properly scale the polynomial.

@jneem jneem marked this pull request as ready for review October 21, 2023 19:56
Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@jessebraham jessebraham merged commit ff80b69 into esp-rs:main Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants