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

Initialize each ADC only once in Analogread #2133

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

Candas1
Copy link

@Candas1 Candas1 commented Sep 18, 2023

Summary
This PR improves the speed of analogread by initializing each ADC only once(mentioned in this issue)

Currently when using analogread, the following steps are executed:

  • initialize adc
  • calibrate adc
  • configure channel
  • start adc
  • poll for conversion
  • stop adc
  • deinitialize adc

With this PR, if the ADC wasn't initialized yet by analogread, the following steps are executed:

  • initialize adc
  • calibrate adc
  • configure channel
  • start adc
  • poll for conversion
  • stop adc
  • deinitialize adc

If the ADC was initialized by analogread, only the following steps are executed:

  • initialize adc
  • calibrate adc
  • configure channel
  • start adc
  • poll for conversion
  • stop adc
  • deinitialize adc

I tested my changes on a STM32F103RCT6.
I ran analogread in a loop:

  • before the change, I was getting a frequency of up to 11khz
  • after the change, I was getting a frequency of up to 25khz

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.

@Candas1 Candas1 changed the title Initialize each ADC only one in Analogread Initialize each ADC only once in Analogread Sep 18, 2023
@fpistm fpistm added the enhancement New feature or request label Sep 19, 2023
@fpistm
Copy link
Member

fpistm commented Sep 19, 2023

Hi @Candas1
Thanks for this PR.
I will review it, IIWR, for H7 ADC is specific as some of them could be directly connected to the pin (dual pad)

@Candas1
Copy link
Author

Candas1 commented Sep 19, 2023

Bonjour Frederic,

Thanks for considering this.
I already see Astyle failed, I think this wasn't used when analog.cpp was first committed.
Can I run Astyle on vscode and just let it prettify the code as you expect it ? are you using a particular set of rules ?

Thanks in advance.

@fpistm
Copy link
Member

fpistm commented Sep 19, 2023

I already see Astyle failed, I think this wasn't used when analog.cpp was first committed.

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

Candas1 commented Sep 19, 2023

But it's even reporting code that I haven't changed.
Thank you, just found out the astylerc. No worries I will check that.

@Candas1
Copy link
Author

Candas1 commented Sep 19, 2023

I got the style fixed locally by astyle. Do you trust this tool or it requires testing again ?
I can push the new changes but I see one check is still running.

@fpistm
Copy link
Member

fpistm commented Sep 19, 2023

I got the style fixed locally by astyle. Do you trust this tool or it requires testing again ? I can push the new changes but I see one check is still running.

You can force push.

@fpistm
Copy link
Member

fpistm commented Sep 19, 2023

But it's even reporting code that I haven't changed.

This is normal as you changed code block, so some indent are required 😉

@Candas1
Copy link
Author

Candas1 commented Sep 19, 2023

No worries, I pushed the fixes.
I will use astyle from now on.

@fpistm
Copy link
Member

fpistm commented Sep 19, 2023

@Candas1
please, could you share the sketch you used to measure ADC performance?

@Candas1
Copy link
Author

Candas1 commented Sep 19, 2023

@fpistm I created this simplified example for you, I hope it's ok
I measured the performance with an oscilloscope

@fpistm
Copy link
Member

fpistm commented Sep 19, 2023

Thanks.

I've made a test with a Nucleo-L476RG with your PR and it breaks the analogRead.

@Candas1
Copy link
Author

Candas1 commented Sep 19, 2023

Thanks.

I've made a test with a Nucleo-L476RG with your PR and it breaks the analogRead.

Any details what is happening ?

@fpistm
Copy link
Member

fpistm commented Sep 19, 2023

Any details what is happening ?

I guess the ADC channel conf moved not help and probably more.
I've suspected this PR would not be so simple as we have to support 20 series and several ADC IP version. STM32F1 is probably the most easiest to manage.
Moreover, looking at your code, the ADC index seems not correct for me as you assumes they are contiguous. Anyway, IIWR on some series you can have ADC1 and ADC3 without ADC2.

@Candas1
Copy link
Author

Candas1 commented Sep 19, 2023

Moreover, looking at your code, the ADC index seems not correct for me as you assumes they are contiguous. Anyway, IIWR on some series you can have ADC1 and ADC3 without ADC2.

Do you have a table of all models and the ADCs it has ?
That seemed obvious to me. But knowing this, I can propose a fix, that's not a problem.

But do you think it's not worth trying ?

@fpistm
Copy link
Member

fpistm commented Sep 19, 2023

List of ADC is not an issue,it is quite easy to do something generic.
Main issue is it does not work by simply skipping the init.

@Candas1
Copy link
Author

Candas1 commented Sep 19, 2023

Initializing the ADC for each adc sampling is also not the standard way to proceed to be honest.
But if you have some hints what goes wrong I can have a look.

Maybe some families don't like changing the channel configuration while the ADC is on, that could be fixed as well.

@fpistm
Copy link
Member

fpistm commented Sep 19, 2023

Initializing the ADC for each adc sampling is also not the standard way to proceed to be honest.

Well it was implemented like this to be arduino compatible as we made only one conversion.

But if you have some hints what goes wrong I can have a look.

Unfortunately, no, this would requires some investigations and I have no free bandwidth for this.

Maybe some families don't like changing the channel configuration while the ADC is on, that could be fixed as well.

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

Candas1 commented Sep 19, 2023

I have some B-G431B-ESC1 laying around, I can maybe try my implementation on those.
But if I cannot reproduce it I cannot fix it.

Do you mind keeping this open and we decide later if it's too complicated ?
If some people are interested they can also try on their hardware, I saw other issues that are connected.

@fpistm fpistm marked this pull request as draft December 18, 2023 15:06
@CJT1111
Copy link

CJT1111 commented Jan 2, 2024

Hello, can you upload your optimized code for a look?

@Candas1
Copy link
Author

Candas1 commented Jan 2, 2024

Hi,

I did an obvious mistake that I didn't upload yet in this PR.
Still it wasn't working well on Families other than F1.
F1 seems less picky as Guillaume said.

Now I got F3/F4/L4/G4/H5 nucleos I can test with.
I need to get back at it and try the low level functions.
I will share it.

@CJT1111
Copy link

CJT1111 commented Jan 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 instead

Signed-off-by: Candas1 <candascraig@hotmail.com>
modify for F1/F2/F4/F7/L1

Signed-off-by: Candas1 <candascraig@hotmail.com>
Even after synching some changes were missing somehow

Signed-off-by: Candas1 <candascraig@hotmail.com>
@Candas1
Copy link
Author

Candas1 commented Feb 18, 2024

Thank you very much for your reply. I am looking forward to your sharing. Please call me when you share.

Sorry for the late reply.
I updated the code, it's simpler now but it might be slower than expected.
I tested it on G4, need to do more tests.

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 afterall

Signed-off-by: Candas1 <candascraig@hotmail.com>
Signed-off-by: Candas1 <candascraig@hotmail.com>
Signed-off-by: Candas1 <candascraig@hotmail.com>
@CJT1111
Copy link

CJT1111 commented Feb 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]
(https:/github.com/rogerclarkmelbourne/Arduino_.STM32), he has an extra myADC library inside
(https:/github.com/rogerclarkmelbourne/Arduino._STM32/tree/master/STM32F1/libraries/STM32ADc), if we can do that, it's really a good move.

@Candas1
Copy link
Author

Candas1 commented Feb 20, 2024

so I hope the final optimized code can be a library like stevstrongl's [Arduino_.STM32]

The scope of this PR is only to optimize analogRead without impacting it's behavior and the projects using it.
I also wanted analogread to not impact other use case (injected adc).
So far I achieved it by skipping the Mspinit/deinit, but I need to test much mure. My changes are visibles in this PR.
I shared some additional tips here.

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

CJT1111 commented Feb 21, 2024

Ok, thank you for your reply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

3 participants