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.

Sign up for GitHub

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

Jump to bottom

Introduce new ADC API for DMA-based high sampling rate #231

Closed
wants to merge 3 commits into from
Closed

Introduce new ADC API for DMA-based high sampling rate #231

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 20, 2018 •
edited by ghost
Loading

Continuation of the PR #175
I had to open a new PR because @damienWi6labs is no longer on this project.

I followed @fpistm comments (see #175).
DMA layer is now fully independent of the peripheral.
DMA has been tested only with ADC peripheral.

Wiki page about this method available here.
Example provided here.

damien16130 and others added 3 commits March 20, 2018 16:35
@damien16130
Add DMA abstraction layer
f263691 
This layer is fully independant from the peripheral.
Tested only with ADC peripheral.

Signed-off-by: dhl <[email protected]>
Add ADC conversion using DMA to increase sampling rate
933395d 
Signed-off-by: fpr <[email protected]>
Add analog read DMA method to the analog Arduino API
9244cac 
Signed-off-by: fpr <[email protected]>
@ghost ghost added enhancement New feature or request waiting feedback Further information is required labels Mar 20, 2018
@ghost ghost requested review from LMESTM, fpistm and VVESTM March 20, 2018 15:45
This was referenced Mar 20, 2018
Closed
Closed
@fpistm fpistm added the on going Currently work on this label Mar 21, 2018
@fpistm
Copy link
Member

fpistm commented Mar 21, 2018

Hi @fprwi6labs,
thanks for this PR. One question: have you take in account my remarks on wiring_analog files on #175?

@ghost
Copy link
Author

ghost commented Mar 21, 2018

Hi @fpistm
This PR is fully based on your remarks. Did I miss something for wiring_analog files?

@fpistm
Copy link
Member

fpistm commented Mar 21, 2018

Currently not it seems ok. Thanks

@fpistm fpistm assigned fpistm and ghost and unassigned fpistm and ghost Mar 23, 2018
@fpistm fpistm added this to the 1.2.1 milestone Mar 23, 2018
@fpistm fpistm removed waiting feedback Further information is required on going Currently work on this labels Apr 25, 2018
@fpistm fpistm removed this from the 1.2.1/1.3.0 milestone May 28, 2018
@ropa35
Copy link

ropa35 commented Jul 21, 2018

How to use?

@fpistm
Copy link
Member

fpistm commented Jul 23, 2018

@ropa35,
this is a PR, if you want to use it, you will have to merge it on the core source, best way is to use git clone of the core.

tve
tve reviewed Aug 6, 2018
View reviewed changes
cores/arduino/stm32/analog.c
if (HAL_ADC_DeInit(AdcHandle) != HAL_OK)
{
return 0;
}
Copy link

Choose a reason for hiding this comment

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

I find it pretty bothersome that for each call to adc_read_value_dma the ADC gets deinitialized, then initialized, finally sampled, and then deinitialized again. Yes, this is amortized over lData samples, but still... How lkong does all this de-init/init/de-init take? Why not create an API that lets the user init things, and then call a very lightweight function to collect a batch of samples?

@tve
Copy link

tve commented Aug 6, 2018

IMHO this suffers from the same problems that the current analogRead implementation suffers, which is that the API only provides a single call which has to init everything from scratch, collect the samples, and then deinit everything.

(What benefit does using the HAL give over using LL?)

@fpistm
Copy link
Member

fpistm commented Aug 7, 2018

Yes you're right that's why this PR is not merged.
HAL is higher level API than LL. HAL function do several check and operations per function while in LL you have to code all by yourself (mainly direct access to register).
The main constraint to HAL is the code size.
Legacy, LL was not at the right level. Now they are fully deployed and that's why I've planned to move/use LL when I get time.

@tve
Copy link

tve commented Aug 7, 2018

I started an STM32ADC library here: https://github.com/tve/goobies/blob/master/libraries/STM32ADC/src/STM32ADC.h
It is adapted from the STM32ADC library in RogerClark's core.
I would appreciate if you could give it a look and let me know if you think this is more along the lines of what you are thinking. I'm hoping the implementation can be "just a bunch of LL_ calls".
I tried to define it such that simple usage requires very few calls. I don't have experience with DMA & ADC so those functions are pretty straight from RogerClark's core and I'll have to see when I get there...
My main constraint is that I pretty much only have STM32L0 boards to test with. I think I have an F401 nucleo somewhere I can perhaps dig out...

@fpistm
Copy link
Member

fpistm commented Aug 7, 2018

I will have a look soon. Thanks. Do not hesitate to provide a dedicated PR if you want.

@tve
Copy link

tve commented Aug 8, 2018

I can't do a PR until I have something that shows sign of life :-). I posted the link mainly to get feedback on the interface before I start coding too much and get attached to an implementation...

ghent360
ghent360 reviewed Jan 29, 2019
View reviewed changes
cores/arduino/stm32/analog.h
@@ -56,6 +79,8 @@ void dac_stop(PinName pin);
uint16_t adc_read_value(PinName pin);
void pwm_start(PinName pin, uint32_t clock_freq, uint32_t period, uint32_t value, uint8_t do_init);
void pwm_stop(PinName pin);
bool adc_read_value_dma(PinName pin, uint32_t *pData, uint32_t lData,
void (*callback)(void *user_data), void *functionParameter);
Copy link
Contributor

Choose a reason for hiding this comment

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

you can probably rename the last argument to user_data safely.
Can you allso add comment what happens if there is an error: does the CB get called? Do we need on_error callback?

cores/arduino/stm32/dma.c
{
void *DMAx_Instance = NULL;

if((DMAx == NC) || (channelx == NC)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion add: || DMAx < 1 || DMAx > 2 || channelx < 0 || channelx > 7

cores/arduino/stm32/dma.c
DMAHandle[DMAx][channel].Init.Direction = DMA_PERIPH_TO_MEMORY;
DMAHandle[DMAx][channel].Init.PeriphInc = DMA_PINC_DISABLE;
DMAHandle[DMAx][channel].Init.MemInc = DMA_MINC_ENABLE;
DMAHandle[DMAx][channel].Init.PeriphDataAlignment = DMA_PDATAALIGN_WORD;//DMA_PDATAALIGN_HALFWORD;
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't ADC halfword?

@fpistm fpistm added invalid This doesn't seem right abandoned No more work on this labels Mar 27, 2019
@fpistm
Copy link
Member

fpistm commented Mar 27, 2019

Close this one as it will never merged as it is.
This should be covered by #5.

@fpistm fpistm closed this Mar 27, 2019
@fpistm fpistm mentioned this pull request Jun 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LMESTM LMESTM Awaiting requested review from LMESTM

@fpistm fpistm Awaiting requested review from fpistm

@VVESTM VVESTM Awaiting requested review from VVESTM

Assignees

@fpistm fpistm

Labels
abandoned No more work on this enhancement New feature or request invalid This doesn't seem right
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fpistm @ropa35 @tve @ghent360 @damien16130