Skip to content

Conversation

@maor1993
Copy link
Contributor

@maor1993 maor1993 commented Aug 25, 2025

A lot of the code is based / copied from ringbuffered_v2.rs

I did make several changes I hope are accepted

1. V3 adc dma is not seperated to its own file to lower the amount of code duplication
2. ADC dma will not halt/kill DMA on overrun, allowing the user the freedom to handle the error as they wish
3. User has access to disable_adc , which allows to pause ADC and DMA

tested on stm32l412kb6u and stm32l432kc

@maor1993 maor1993 marked this pull request as draft August 25, 2025 02:07
@maor1993 maor1993 marked this pull request as ready for review August 27, 2025 09:22
@maor1993
Copy link
Contributor Author

@Dirbaio Okay to merge?

@maor1993
Copy link
Contributor Author

hey @lulf , any reason not to merge this?

Copy link

@jduranf jduranf left a comment

Choose a reason for hiding this comment

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

Hi @maor1993,

To add compatibility in a STM32G0, some minor changes should be done. I tested those modifications in a STM32G071CBUx, and seems to work fine.

@maor1993
Copy link
Contributor Author

maor1993 commented Oct 9, 2025

@jduranf can you merge?

@jduranf
Copy link

jduranf commented Oct 10, 2025

@jduranf can you merge?

Sorry, no. I'm like you, just wanting this PR to be merged

@maor1993
Copy link
Contributor Author

@Dirbaio , this PR is open for over a month, passes all checks and required by other users, is there a reason not merge this?

#[cfg(not(any(adc_g0, adc_u0)))]
T::regs().cfgr().modify(|reg| {
reg.set_discen(false);
reg.set_cont(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Using continuous and circular mode means you sample as fast as possible for the ADC. This means that the DMA buffer fills up really quickly and care must be taken when setting up the task that reads from the buffer.

As an alternative approach I think you should consider using triggered measurements to control the rate at which new samples are taken. I have started working on this for the g4 series #4786 but it would be good to develop similar APIs for all versions in the long run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, getting a fast as possible buffer was the point.
this is intended for audio applications where you want a constant stream of samples

in my case I needed to sample 4 analog inputs as fast as possible and only pass to a different task samples that pass some threshold.

as such, by setting the sequence to the highest sample time I was able to get super fast samples without needing to retrigger the ADC, essentially acting as a clock source for the audio pipeline

while the device I used, stm32l412kb does not support changing the clock to the ADC, other devices do.

alternatively you can also drive the conversion starts from a timer, as such this will ensure the ADC is always ready to provide samples and the relevant task will be awaken every time the DMA buffer is half full.

@xoviat
Copy link
Contributor

xoviat commented Nov 1, 2025

I've not been around for a while, but barring objections from @lulf and @Dirbaio I'm going to try to get these ADC pull requests merged. For me, ideally I would like to see a consistent interface across all versions. So let's see what we have here:

This version of adc ringbuffer:

  • pub stop_continuous_sampling
  • pub disable_adc
  • pub teardown_adc
  • pub async read
  • pub blocking_read
  • pub adc::into_ring_buffered
  • pub new
  • start_continuous_sampling

Existing version of adc ringbuffer:

  • pub set_sample_sequence
  • pub start
  • pub teardown_adc
  • pub blocking_read
  • pub async read
  • pub adc::into_ring_buffered
  • is_on
  • stop_adc
  • start_adc
  • set_channel_sample_time
  • set_channels_sample_time
  • stop
  • setup_adc

Just from a glance, I prefer what you've done, allowing the sequence to be passed to the constructor, rather than set as a method. To be honest, the existing ringbuffer seems like an attempt to work around the under-developed v2 mod.

The largest objection that I have is the public new constructor for the ringbuffer. There should be one way to obtain a ringbuffer, into_ring_buffer. The other objections are not really your fault, because they are a product of the existing ringbuffer_v2.

If you remove the pub new and one one objects, then in a few days I will merge this. I will then need to work to merge the ringbuffered.rs files, mostly by working on the existing v2.

@maor1993
Copy link
Contributor Author

maor1993 commented Nov 1, 2025

I've not been around for a while, but barring objections from @lulf and @Dirbaio I'm going to try to get these ADC pull requests merged. For me, ideally I would like to see a consistent interface across all versions. So let's see what we have here:

This version of adc ringbuffer:

* pub stop_continuous_sampling

* pub disable_adc

* pub teardown_adc

* pub async read

* pub blocking_read

* pub adc::into_ring_buffered

* pub new

* start_continuous_sampling

Existing version of adc ringbuffer:

* pub set_sample_sequence

* pub start

* pub teardown_adc

* pub blocking_read

* pub async read

* pub adc::into_ring_buffered

* is_on

* stop_adc

* start_adc

* set_channel_sample_time

* set_channels_sample_time

* stop

* setup_adc

Just from a glance, I prefer what you've done, allowing the sequence to be passed to the constructor, rather than set as a method. To be honest, the existing ringbuffer seems like an attempt to work around the under-developed v2 mod.

The largest objection that I have is the public new constructor for the ringbuffer. There should be one way to obtain a ringbuffer, into_ring_buffer. The other objections are not really your fault, because they are a product of the existing ringbuffer_v2.

If you remove the pub new and one one objects, then in a few days I will merge this. I will then need to work to merge the ringbuffered.rs files, mostly by working on the existing v2.

Thanks for the effort,

will pub(crate) new suffice? I'm uncertain how to implement adc::into_ring_buffer otherwise

@xoviat
Copy link
Contributor

xoviat commented Nov 2, 2025

will pub(crate) new suffice? I'm uncertain how to implement adc::into_ring_buffer otherwise

yes, that's fine.

@maor1993
Copy link
Contributor Author

maor1993 commented Nov 2, 2025

will pub(crate) new suffice? I'm uncertain how to implement adc::into_ring_buffer otherwise

yes, that's fine.

👍🏾 done

@xoviat xoviat added this pull request to the merge queue Nov 3, 2025
Merged via the queue into embassy-rs:main with commit 7e917fd Nov 3, 2025
8 checks passed
@lulf
Copy link
Member

lulf commented Nov 3, 2025

@xoviat Thanks for reviewing and merging, feel free to pick up the stm32 PRs that have been around for a while, I don't have much bandwidth or technical insight into stm32 to follow up on the more complex ones.

@xoviat
Copy link
Contributor

xoviat commented Nov 3, 2025

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants