- Notifications
You must be signed in to change notification settings - Fork1k
Initialize each ADC only once in Analogread#2133
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fpistm commentedSep 19, 2023
Hi@Candas1 |
Candas1 commentedSep 19, 2023
Bonjour Frederic, Thanks for considering this. Thanks in advance. |
fpistm commentedSep 19, 2023
It was used but as you change the code it do not match anymore the format. We provide a script to apply change. in CI/astyle and the rules are defined in the astylerc file:https://github.com/stm32duino/Arduino_Core_STM32/tree/main/CI/astyle |
Candas1 commentedSep 19, 2023
But it's even reporting code that I haven't changed. |
Candas1 commentedSep 19, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I got the style fixed locally by astyle. Do you trust this tool or it requires testing again ? |
fpistm commentedSep 19, 2023
You can force push. |
fpistm commentedSep 19, 2023
This is normal as you changed code block, so some indent are required 😉 |
Candas1 commentedSep 19, 2023
No worries, I pushed the fixes. |
fpistm commentedSep 19, 2023
@Candas1 |
Candas1 commentedSep 19, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
fpistm commentedSep 19, 2023
Thanks. I've made a test with a Nucleo-L476RG with your PR and it breaks the analogRead. |
Candas1 commentedSep 19, 2023
Any details what is happening ? |
fpistm commentedSep 19, 2023
I guess the ADC channel conf moved not help and probably more. |
Candas1 commentedSep 19, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Do you have a table of all models and the ADCs it has ? But do you think it's not worth trying ? |
fpistm commentedSep 19, 2023
List of ADC is not an issue,it is quite easy to do something generic. |
Candas1 commentedSep 19, 2023
Initializing the ADC for each adc sampling is also not the standard way to proceed to be honest. Maybe some families don't like changing the channel configuration while the ADC is on, that could be fixed as well. |
fpistm commentedSep 19, 2023
Well it was implemented like this to be arduino compatible as we made only one conversion.
Unfortunately, no, this would requires some investigations and I have no free bandwidth for this.
Probably if you read the comment it seems it depends of the ADC state. I guess using some LL would help on this but probably not simple and fast to do. |
Candas1 commentedSep 19, 2023
I have some B-G431B-ESC1 laying around, I can maybe try my implementation on those. Do you mind keeping this open and we decide later if it's too complicated ? |
CJT1111 commentedJan 2, 2024
Hello, can you upload your optimized code for a look? |
Candas1 commentedJan 2, 2024
Hi, I did an obvious mistake that I didn't upload yet in this PR. Now I got F3/F4/L4/G4/H5 nucleos I can test with. |
CJT1111 commentedJan 7, 2024
Thank you very much for your reply. I am looking forward to your sharing. Please call me when you share. |
Use LL functions insteadSigned-off-by: Candas1 <candascraig@hotmail.com>
modify for F1/F2/F4/F7/L1Signed-off-by: Candas1 <candascraig@hotmail.com>
Even after synching some changes were missing somehowSigned-off-by: Candas1 <candascraig@hotmail.com>
Candas1 commentedFeb 18, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Sorry for the late reply. The only doubt I have now is that it's not possible to change the resolution between reads if the adc is not reinitialized, need to check how to deal about it. |
HAL_ADC_Start is ok afterallSigned-off-by: Candas1 <candascraig@hotmail.com>
Signed-off-by: Candas1 <candascraig@hotmail.com>
Signed-off-by: Candas1 <candascraig@hotmail.com>
CJT1111 commentedFeb 20, 2024
That's all right, I'm looking forward to your effect. Finally, I have a suggestion: I have done a long experiment to improve the speed of ADC in Arduino development environment, for example DMA, but the Arduinol development environment can not call interrupt, I also consulted fpistm, he replied me that Arduino' official does not have DMA API, so he did not do it Method, so I hope the final optimized code can be a library like stevstrongl's [Arduino_.STM32] |
Candas1 commentedFeb 20, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The scope of this PR is only to optimize analogRead without impacting it's behavior and the projects using it. I have idea about an ADC library as part of SimpleFOC but it would require a much more complicated API, it's out of scope here. |
CJT1111 commentedFeb 21, 2024
Ok, thank you for your reply. |
Uh oh!
There was an error while loading.Please reload this page.
Summary
This PR improves the speed of analogread by initializing each ADC only once(mentioned inthis issue)
Currently when using analogread, the following steps are executed:
With this PR, if the ADC wasn't initialized yet by analogread, the following steps are executed:
stop adcdeinitialize adcIf the ADC was initialized by analogread, only the following steps are executed:
initialize adccalibrate adcstop adcdeinitialize adcI tested my changes on a STM32F103RCT6.
I ran analogread in a loop:
With this change, I was also able to run injected adc in parallel with analogread without interfering, the only prerequisite is that analogread should run first.
[EDIT] It was late so I forgot some details.
I tried reading the same pin from 3 different ADCs (PC2_ALT0, PC2_ALT1, PC2_ALT2) consecutively in a loop, it was working well, the init happened only once for each ADC. The same example wasn't working before my change, I was getting readings for one of the analogread calls only.
If you think users were using the previous behavior as a feature, I could also #ifdef the new behavior to make it optional.