Skip to content

ADC Multi-sampling #9315

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

Closed
wants to merge 31 commits into from
Closed

ADC Multi-sampling #9315

wants to merge 31 commits into from

Conversation

bill88t
Copy link

@bill88t bill88t commented Jun 6, 2024

This PR introduces a way to read multiple ADC samples in one go and additionally moves ADC object initiation and calibration outside of get_value for Espressif. This vastly improves ADC performance on Espressif.

A new AnalogIn property has been added, sample_size which can be both read and written to setting the number of samples taken on every .value read.
Alongside the property, there is also the samples keyword that can be used to set sample_size during the AnalogIn object construction.

The default sample size is set to 2 on all ports. As it was, only espressif did take 2 samples, with all the rest of the ports taking one.

Task checklist:

  • Implement on espressif.
  • Test on espressif.
  • Implement on raspberrypi.
  • Test on raspberrypi.
  • Implement on atmel-samd.
  • Test on atmel-samd.
  • Implement on cxd56.
  • Test on cxd56.
  • Implement on mimxrt10xx.
  • Test on mimxrt10xx.
  • Implement on nordic.
  • Test on nordic.
  • Implement on silabs.
  • Test on silabs.
  • Implement on stm.
  • Test on stm.

As a note, I cannot test stm, silabs, mimxrt10xx, cxd56.
Those will have to be tested by someone else.

@bill88t bill88t changed the title [DO NOT MERGE, EXPERIMENTAL] ADC sample keyword and persistent pin init. [DO NOT MERGE, EXPERIMENTAL] ADC samples keyword and persistent pin init. Jun 6, 2024
@dhalbert
Copy link
Collaborator

dhalbert commented Jun 6, 2024

If you reload before .deinitializing an analogio object it cannot be remade until reset.

We can fix this by by making the AnalogIn object have a finalizer. It's on the list to do that anyway.

@bill88t
Copy link
Author

bill88t commented Jun 8, 2024

Is there any desire to see this completed?

It makes sampling a lot faster.
It doesn't break the existing api.

@dhalbert
Copy link
Collaborator

dhalbert commented Jun 8, 2024

I might have done this for #8690. If you want to complete it to check AnalogIn off in that issue, that would be great. Look for how finalizers are done in, say, #9165. Note that finalizer is spelled finaliser in function names due to the British/Australian spelling difference. The basic thing to do is to say it has a finalizer when you construct it, and to make __del__() call deinit(). You also need to remove any global init/reset.

@bill88t
Copy link
Author

bill88t commented Jun 9, 2024

Implemented finalisers. Now I just need to implement the sampling logic on all ports.

@bill88t bill88t changed the title [DO NOT MERGE, EXPERIMENTAL] ADC samples keyword and persistent pin init. ADC samples keyword, only initialize once on espressif, add finalisers to AnalogIn objects Jun 9, 2024
@bill88t
Copy link
Author

bill88t commented Jun 9, 2024

Updated the title an opening comment.

Also, @dhalbert turns out it's still possible to trigger a watchdog safemode with just CONFIG_FREERTOS_UNICORE.
You just have to be unlucky enough to .value during a dhcp leash renewal.
Well, with this PR this also goes away, since the object gets initialized only once.
EDIT: I was wrong, microcontroller.cpu.temperature can watchdog safemode.

Do you maybe want me to also include CONFIG_FREERTOS_UNICORE in this PR too?
Or are you going to PR it seperately?

@dhalbert
Copy link
Collaborator

dhalbert commented Jun 9, 2024

EDIT: I was wrong, microcontroller.cpu.temperature can watchdog safemode.

That probably uses a an ADC.

Do you maybe want me to also include CONFIG_FREERTOS_UNICORE in this PR too?
Or are you going to PR it seperately?

No, I am not prepared to turn off the second core yet. I would rather debug the underlying problem.

@bill88t
Copy link
Author

bill88t commented Jun 10, 2024

Note, the actions on 39d26d6 should not have succeeded.
Viewing them from:
https://github.com/bill88t/circuitpython/actions/runs/9453514997
they appear successful.

It didn't build any boards.

@bill88t
Copy link
Author

bill88t commented Jun 12, 2024

While I'm at it, should I maybe add a property for configuring sample size after the object has been created?

As is, the sample size can only be set during object creation like a so:
adc = analogio.AnalogIn(board.ADC_PIN, samples=5000)

I kinda think like adding a adc.samples property would be helpful both for visibility and ease of use.

@bill88t bill88t marked this pull request as ready for review June 14, 2024 12:09
@bill88t
Copy link
Author

bill88t commented Jun 14, 2024

This PR is pretty much ready, it just need testing.
Please refer to the opening comment.

I will test on nordic and atmel-samd when I have some free time.
EDIT: nordic tested.

@bill88t bill88t changed the title ADC samples keyword, only initialize once on espressif, add finalisers to AnalogIn objects ADC Multi-sampling Jun 14, 2024
@bill88t bill88t marked this pull request as draft June 20, 2024 18:29
@bill88t bill88t marked this pull request as ready for review June 20, 2024 18:37
@bill88t
Copy link
Author

bill88t commented Aug 8, 2024

Forgot this existed, tested on samd51. Now I have tested every port I have.

There is nothing else to be done from my side.

@bill88t
Copy link
Author

bill88t commented Aug 9, 2024

Fixed merge conflicts, please validate against RP2350 too.

adc.sample_size = 65535
adc.value

@bill88t
Copy link
Author

bill88t commented Aug 12, 2024

I cannot get that one samd board to fit. I don't know what to do from here.

@tannewt tannewt requested a review from dhalbert August 12, 2024 17:03
@bill88t
Copy link
Author

bill88t commented Aug 13, 2024

The only thing I see that could possibly be disabled is CIRCUITPY_PS2IO..

@tannewt
Copy link
Member

tannewt commented Aug 13, 2024

Honestly, I'm not sure I want to add sample number configuration as an API. I'd be ok if the ESP did multiple samples internally but adding an API impacts all ports (as you see) and all ADC drivers. It is backwards compatible but any code that starts using it won't work elsewhere that hasn't added it.

Multiple samples can be taken from Python too if you really want to too.

@bill88t
Copy link
Author

bill88t commented Aug 13, 2024

Sure, it's new api, but something that arguably should have been there from the start.
If you need precision you kinda have to take multiple samples.
Taking them from python is fine, but python is notoriously slow on for loops.
This is something not so complicated, that passes on the burden to the underlying core.

but any code that starts using it won't work elsewhere that hasn't added it.

Yea, but how often is an application developed and it's expected for it to work on earlier version of CircuitPython?
Besides, it could trivially be tested with a hasattr, if being version agnostic is a must.

impacts all ports (as you see)

This is why I followed standard procedure and created a task list.
While I do not have every single port, I did test everything I had.
On those I did not have, I was extra cautious with the code as to do it with the least number of changes, to avoid unforeseen issues, skipping optimizations.

@tannewt
Copy link
Member

tannewt commented Aug 13, 2024

Sure, it's new api, but something that arguably should have been there from the start.
If you need precision you kinda have to take multiple samples.

analogbufio can be used to take multiple samples at once.

Taking them from python is fine, but python is notoriously slow on for loops.
This is something not so complicated, that passes on the burden to the underlying core.

It is simple but likely to not be used. (We've gone 7 years without it.) Furthermore, it has the code size issue on SAMD. Newer APIs like analogbufio don't have this problem because they aren't on the small SAMD21s.

Yea, but how often is an application developed and it's expected for it to work on earlier version of CircuitPython?

I'm more concerned about other places that implement this API like drivers and Blinka.

Besides, it could trivially be tested with a hasattr, if being version agnostic is a must.

hasattr can't check for a kwarg.

I'd prefer not to change the AnalogIn API at all. Instead, I'd be ok adding internal multisampling for ESP.

@bill88t
Copy link
Author

bill88t commented Aug 13, 2024

hasattr can't check for a kwarg.

It's an arg and an attribute, it can be set during creation or after.
adc.sample_size = 65535
So the check would be: hasattr(adc, "sample_size").

In either case, i'll close the PR. It could probably be optimized to fit, but if analogbufio can do it then it's redundant.

@bill88t bill88t closed this Aug 13, 2024
@bill88t bill88t deleted the adc_paranoia branch August 13, 2024 18:38
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