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

ADC tests #6

Open
palmerr23 opened this Issue May 11, 2017 · 14 comments

Comments

Projects
None yet
3 participants
@palmerr23

palmerr23 commented May 11, 2017

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

@danieleff

This comment has been minimized.

Show comment
Hide comment
@danieleff

danieleff May 11, 2017

Owner

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.

Owner

danieleff commented May 11, 2017

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.

@danieleff

This comment has been minimized.

Show comment
Hide comment
@danieleff

danieleff May 11, 2017

Owner

I added stm32XXxx_hal_adc_ex.c to the core. Can you try again?

Owner

danieleff commented May 11, 2017

I added stm32XXxx_hal_adc_ex.c to the core. Can you try again?

@danieleff

This comment has been minimized.

Show comment
Hide comment
@danieleff

danieleff May 12, 2017

Owner

Actually I just put in all HAL source files. Compile time is slightly longer, but oh well.

Owner

danieleff commented May 12, 2017

Actually I just put in all HAL source files. Compile time is slightly longer, but oh well.

@palmerr23

This comment has been minimized.

Show comment
Hide comment
@palmerr23

palmerr23 May 12, 2017

I'll give it a go!

palmerr23 commented May 12, 2017

I'll give it a go!

@palmerr23

This comment has been minimized.

Show comment
Hide comment
@palmerr23

palmerr23 May 12, 2017

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

palmerr23 commented May 12, 2017

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

@danieleff

This comment has been minimized.

Show comment
Hide comment
@danieleff

danieleff May 12, 2017

Owner

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.

Owner

danieleff commented May 12, 2017

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.

@palmerr23

This comment has been minimized.

Show comment
Hide comment
@palmerr23

palmerr23 May 13, 2017

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

palmerr23 commented May 13, 2017

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

@fpistm

This comment has been minimized.

Show comment
Hide comment
@fpistm

fpistm May 13, 2017

We planned to do an extension for ADC. This will need some discussion.

fpistm commented May 13, 2017

We planned to do an extension for ADC. This will need some discussion.

@palmerr23

This comment has been minimized.

Show comment
Hide comment
@palmerr23

palmerr23 May 14, 2017

Excellent.

Happy to contribute to the discussion and the code.

Richard

palmerr23 commented May 14, 2017

Excellent.

Happy to contribute to the discussion and the code.

Richard

@palmerr23

This comment has been minimized.

Show comment
Hide comment
@palmerr23

palmerr23 May 15, 2017

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

palmerr23 commented May 15, 2017

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

@danieleff

This comment has been minimized.

Show comment
Hide comment
@danieleff

danieleff May 15, 2017

Owner

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?

Owner

danieleff commented May 15, 2017

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?

@palmerr23

This comment has been minimized.

Show comment
Hide comment
@palmerr23

palmerr23 May 16, 2017

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

palmerr23 commented May 16, 2017

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

@fpistm

This comment has been minimized.

Show comment
Hide comment
@fpistm

fpistm May 16, 2017

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.

fpistm commented May 16, 2017

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.

@palmerr23

This comment has been minimized.

Show comment
Hide comment
@palmerr23

palmerr23 May 17, 2017

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.

palmerr23 commented May 17, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment