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

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

Merged
merged 2 commits into from
May 19, 2024

Conversation

victorandrehc
Copy link
Contributor

  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.

@victorandrehc victorandrehc force-pushed the feature/extend_g0_adc branch 2 times, most recently from 4d0f6cc to a217cc4 Compare February 26, 2024 19:47
@victorandrehc victorandrehc changed the title Feature/extend g0 adc [stm32] Extend G0 adc to support adc dma Feb 26, 2024
@salkinium salkinium added this to the 2024q1 milestone Mar 1, 2024
@victorandrehc victorandrehc force-pushed the feature/extend_g0_adc branch 2 times, most recently from b5423b8 to 219b8d6 Compare March 8, 2024 09:35
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Yes from my side, but I'm not the expert here. *gently pokes @chris-durand*

@chris-durand
Copy link
Member

gently pokes @chris-durand

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

examples/nucleo_g070rb/adc_dma/adc_dma.hpp Outdated Show resolved Hide resolved
examples/nucleo_g070rb/adc_dma/main.cpp Outdated Show resolved Hide resolved
src/modm/platform/adc/stm32f0/adc_impl.hpp.in Outdated Show resolved Hide resolved
@chris-durand
Copy link
Member

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

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

@victorandrehc
Copy link
Contributor Author

victorandrehc commented Mar 22, 2024

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

@rleh rleh modified the milestones: 2024q1, 2024q2 Apr 1, 2024
@victorandrehc
Copy link
Contributor Author

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

@chris-durand
Copy link
Member

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.

Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

I'll merge it this weekend.

wald-zat and others added 2 commits May 19, 2024 13:20
Co-authored-by: Christopher Durand <christopher.durand@rwth-aachen.de>
Co-authored-by: Christopher Durand <christopher.durand@rwth-aachen.de>
@salkinium salkinium added the ci:hal Triggers the exhaustive HAL compile CI jobs label May 19, 2024
@salkinium salkinium merged commit 901a3dc into modm-io:develop May 19, 2024
38 of 39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
advanced 🤯 ci:hal Triggers the exhaustive HAL compile CI jobs feature 🚧
Development

Successfully merging this pull request may close these issues.

5 participants