ADC tests
palmerr23 opened this issue · 15 comments
Daniel,
I've been testing ADC functions.
1. Issue with _weak prototypes for interrupt functions in stm32f4xx_hal_adc.c
When I started working on ADC interrupts I got the following error:
...Arduino\hardware\STM32GENERIC\STM32\system/STM32F4/HAL_Src/stm32f4xx_hal_adc.c:958: undefined reference to `HAL_ADCEx_InjectedConvCpltCallback'
For some reason the _weak prototype for HAL_ADCEx_InjectedConvCpltCallback( ) at stm32f4xx_hal_adc_ex.c : 769 isn't providing coverage, when the function isn't needed.
Including a null routine in the user code fixes this issue (not ideal) - the additional null routine isn't required in Frederic's implementation.
2. ADC Extended functions missing
The extended ADC functions in stm32f4xx_hal_adc_ex.c are not available.
The solution is to add the second #include in the code below at stm32f4xx_hal_conf.h : 269
and properly define the callback function in local code.
#ifdef HAL_ADC_MODULE_ENABLED
#include "stm32f4xx_hal_adc.h"
#include "stm32f4xx_hal_adc_ex.h"
#endif /* HAL_ADC_MODULE_ENABLED */
I suspect the library spl.a might need recompilation as well - as including the stm32f4xx_hal_adc_ex.h file in user code, or in stm32f4xx_hal_conf.h, doesn't make the error go away.
Richard
This is because not every HAL is compiled in.
The extra stuff is in STM32/libraries/STM32_HAL/src/
If you copy stm32XXxx_hal_adc_ex.c to STM32/cores/arduino/stm32_HAL/ it will work. (Or include STM32_HAL.h)
The reason is compilation time. If compilation time was not a problem, I would just put everything in the core.
I will move the mostly used ones (adc_ex, sdram, dac, ), and put the others in menu item maybe... I don't know.
I added stm32XXxx_hal_adc_ex.c to the core. Can you try again?
Actually I just put in all HAL source files. Compile time is slightly longer, but oh well.
I'll give it a go!
Daniel,
yes, it works now.
I have working ADC HAL demos for:
-
Polling
-
Simple interrupt
-
DMA (Single Channel)
-
DMA (Dual Channel Interleaved) DMA Mode3 8 bit, and DMA Mode2 12 bit
I can see no reason why they shouldn't work for all F4s.
They are uploaded to my repo at: https://github.com/palmerr23/Black-F407VET6-STM32Generic
Is there somewhere a more higher level API for arduino for ADC (other than the basic analogRead() of course)? (teensy?)
The HAL examples are nice, and could be the basis of a more user-friendly API.
Daniel,
I agree that a tidy-up might be nice to make things more simple, and a ready reference for the options available for ADC_Config() . The readme.txt files for the examples are helpful, but quite wordy. I deleted a few, and have edited and reinstated them. There are a some good general tutorials already online, e.g. http://embedded-lab.com/blog/stm32-adc-2/ maybe a short how-to might be in order - I'll have a think about this.
Experience has shown that for the ADCs, once you get past the basic "read a value", the code always gets tweaked one way or another. Having an overly clever API gets in the way, particularly if the need is for fast response or high data rates (not using DMA). If anything, mixed HAL / LL usage is the direction things tend to go if there's a serious need for ADC.
In the Teensy space, Pedro Villanueva's ADC library is the benchmark, I believe. https://github.com/pedvide/ADC
He implements a set of per-parameter function calls, e.g. setResolution(), rather than filling in a full Init structure and then passing it to a master config routine (which seems to be the STM approach for both HAL and LL). Both are valid and equally useful.
When I developed a set of Teensy-based audio limiters with sub millisecond attack late last year, I used Pedro's config routines and then wrote the ADC interrupt handler from scratch.
@ollie and I have just been discussing high-level vs low-level library support in the Stm32duino forums http://stm32duino.com/viewtopic.php?f=39&t=1977&start=50
Our conclusion - keep it simple and STM standardised at the library level with a broad set of examples for folks to adapt.
Exceptions to this for ADCs might be measureVBAT() or measureChipTemp() which could be delivered through a standard API. Sadly, they are only useful in very specific circumstances. Any decent library written for battery operation will manage VBAT configuration directly. ChipTemp is not much use, except for hostile operational environments.
Can you think of any places where there would be substantial improvements via a higher level API?
Richard
We planned to do an extension for ADC. This will need some discussion.
Excellent.
Happy to contribute to the discussion and the code.
Richard
Daniel,
Some overnight thinking...
Except for multi-mode, a single ADCinit(ADCx, ADCpin, SampleTime, Resolution, ADCmode) could work. Mode would combine Single/Continuous, Interrupt/DMA/Polling option and single/multiple channels. These could be broken out into several variables, but that might allow some combinations that are illegal (or silly).
For multi modes, ADCx would be ignored and an optional SampleDelay parameter would be needed at the end. It could also be added to the ADCstart() function below.
If multiple Channels are to be converted, either a function with variable length arguments, or passing an array of pins could work, issued before ADCstart()
ADCsetChannels(ADCx, channelArray) - ADCinit(...ADCpin...) could be set to MULTICHANNEL == -1
External/timer triggers could be handled in a similar way. ADCtrigger(ADCx, TrigVal) could be issued before ADCstart.
ADCstart(ADCx, nSamples) and a local handler for Interrupts (DMA/ADC) would be needed for non-polling modes. The rest could be handled in the library.
Special functions could handle the uncommon options e.g. DataAlignLeft.
Does this approach appeal?
Richard
If you look around, almost every Arduino library is based on c++ class, as in ADC.init() ADC.whatever()...
As for the actual functions, names, functionality, I do not have experience with analog that much.
fpistm, do you have an overview of your API?
Daniel,
The Class option is excellent where there might be a need for multiple independent instances - e.g. displays, external hardware, particularly where they need to be logically isolated and the data structures instanced on the fly.
Pedro uses this approach with the Teensy library.
When you're dealing with an internal peripheral, particularly one where synchronisation between instances (multimode) is needed, there's not much benefit. As well, the ADC data structures are already instanced via the #includes for the ADCs - which is quite sensible as there are a small, fixed number of ADCs.
That's a personal view, and I'd be keen to hear Frederic's viewpoint - as there are multiple benefits in a common approach.
On the API front, another option (Pedro used this one), is to have a set of atomic functions to do various simple tasks, where there aren't complex interactions: setResolutionADC(16), setSampleTimeADC(56), etc
Then have a start() function for each operating mode e.g. startSynchronizedSingleRead()
This reduces the number of parameters in the specific mode calls, which is a good thing!
Richard
It's look a good approach. Currently, I have other stuff to study. I will ask to a colleague to bring his point of view.
Here's a set of atomic ADC config functions from http://embedded-lab.com/blog/stm32-adc-2/
ADC1_Enable();
clr_ADC1_settings();
set_ADC_mode(independent_mode);
set_ADC1_data_alignment(right_alignment);
set_ADC1_scan_conversion_mode(disable);
set_ADC1_continuous_conversion_mode(disable);
set_ADC1_sample_time(sample_time_71_5_cycles, 1);
set_ADC1_external_trigger_regular_conversion_edge(EXTI_11_trigger);
set_ADC1_regular_number_of_conversions(1);
set_ADC1_regular_sequence(1, 1);
set_ADC1_regular_end_of_conversion_interrupt(enable);
ADC1_calibrate();
start_ADC1();
I'm not sure this is really any better than setting the values in an init structure. The main issue is the combination of settings needed to get things going properly can be quite confusing.
Daniel,
yes, it works now.
I have working ADC HAL demos for:
- Polling
- Simple interrupt
- DMA (Single Channel)
- DMA (Dual Channel Interleaved) DMA Mode3 8 bit, and DMA Mode2 12 bit
I can see no reason why they shouldn't work for all F4s.
They are uploaded to my repo at: https://github.com/palmerr23/Black-F407VET6-STM32Generic
Very good work, I've made it compatible with stm32_dma Library inside the main package. Just update the stm32_dma Library to include configuration also for ADC1,2,3 (I uploaded a Pull Request). Then you can delete the 2 files from your examples:
- stm32f4xx_it.c
- stm32f4xx_it.h
Also these don't need any more:
#include "stm32f4xx_hal.h"
#include "stm32f4xx_hal_adc.h"
In: void HAL_ADC_MspInit(ADC_HandleTypeDef* hadc)
add/comment:
stm32DmaAcquire(&hdma_adc, ADC_DMA, hadc->Instance, true);
//hdma_adc.Instance = ADCx_DMA_STREAM;
//hdma_adc.Init.Channel = ADCx_DMA_CHANNEL;
...
...
//HAL_NVIC_SetPriority(ADCx_DMA_IRQn, 0, 0);
//HAL_NVIC_EnableIRQ(ADCx_DMA_IRQn);
And it will work. In this way you can manage multiple DMA channels with the stm32_dma Library.