search

modm-io / modm

modm: a C++23 library generator for AVR and ARM Cortex-M devices
https://modm.io
Mozilla Public License 2.0
720 stars 128 forks source link

[stm32] Extend G0 adc to support adc dma #1136

Closed victorandrehc closed 2 months ago

victorandrehc commented 5 months ago
  1. Adds IRQ and DMA capabilities for the G0 ADC implementation.
  2. Adds Vbat, Vref and temperature channel bits on the ADC.
  3. Adds an example to the G0 adc with dma usage. The example is similar to the one present on stm32f401re.
chris-durand commented 4 months ago

gently pokes @chris-durand

Totally forgot about this one … Will look at it tomorrow.

chris-durand commented 4 months ago

@victorandrehc I've pushed some changes to modify the driver in a way that it also supports DMA and conversion sequences on STM32F0. Could you check if the change would be acceptable for your use case?

The changed API additionally supports another sequence mode where the channels to be converted are specified as a bit mask. That's the only method available on F0 or if you need channels 16-18 to be part of the sequence or the length is bigger than 8.

The enableScanMode() and disableScanMode() methods are not required anymore and channels are configured using the following three functions:

single channel mode:

bool setChannel(Channel channel);

in-order sequence conversion mode with CHSELRMOD:

bool setChannels(std::span<const Channel> channels);

sequence conversion mode specified as mask:

bool setChannels(ChannelMask_t channels);
victorandrehc commented 4 months ago

@chris-durand I am going to review this later this week (probably on Wednesday)

victorandrehc commented 4 months ago

Hi @chris-durand after some careful analysis we think your suggestion suit us fine.

victorandrehc commented 3 months ago

Hi @chris-durand, is there anything i can do to help you merging this PR?

chris-durand commented 3 months ago

Hi @victorandrehc,

thanks for the reminder and sorry for the long wait. I did some more minor clean-up and consider this ready to merge in the current state. My original intention was to add and test an additional F0 DMA example but I was busier than expected and didn't find the time. We can merge this without it.

One last thing I felt I had to change was the naming of the enableDmaRequests() / disableDmaRequests() methods. The name suggests that disableDmaRequests() would completely disable the generation of DMA requests, but it actually doesn't. It configures whether requests would still be generated after DMA transfer completion for DMA circular mode. As a reader of application code who isn't familiar with the driver implementation it would be hard to understand that from the function names.

To clean this up I'd suggest replacing enableDmaRequests(), disableDmaRequests(), enableDmaMode(), disableDmaMode() with a single setDmaMode(mode) function that takes a DmaMode enum parameter with the values Disabled, OneShot and Circular like in the F3/G4/H7 driver. That also prevents erroneously setting enableDmaRequests() without enabling DMA. I've pushed that change as an additional commit. Would you be fine with it? If yes, we can go ahead and merge.

I'm kind of confident I didn't break anything but if a short hardware test is easy for you I'd appreciate it.