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

ADC Interrupt and DMA support #226

Merged
merged 15 commits into from
Jun 21, 2021
Merged

Conversation

datdenkikniet
Copy link
Contributor

@datdenkikniet datdenkikniet commented May 19, 2021

This PR adds support for sequences, interrupts and DMA requests to ADC1 by implementing from_adc for the dma::Transfer struct.

There is currently a problem with the L3 (also: L) bits in the SRQ1 register having the incorrect and inconsistent names across the parts. For now, I've feature gated this issue away. This problem is fixed in the latest source of the STM32L4 crate, but the fix has not been released yet.

@datdenkikniet datdenkikniet force-pushed the adc_dma branch 5 times, most recently from 20f5a18 to 575067b Compare May 19, 2021 11:47
@korken89
Copy link
Collaborator

Thanks for the PR!

Could you add an example to the the repo for easier testing?

@akloboucnik
Copy link
Contributor

akloboucnik commented Jun 1, 2021

Could I have a question about usage of the new Transfer API for the ADC? If I understand correctly it takes ownership of the ADC peripheral and I'm unable to ever get it back - so if I for example want to reconfigure the ADC to different parameters, or get another sample afaik I'm unable to because it's always owned by the one transfer struct created with from_adc function.

I've noticed that for example in the F4 HAL this is solved by the start function accepting a closure - https://github.com/stm32-rs/stm32f4xx-hal/blob/master/src/dma/mod.rs#L1236 and example of usage here: https://github.com/stm32-rs/stm32f4xx-hal/blob/master/examples/adc_dma_rtic.rs#L83.

Maybe this is beyond a scope of this PR and I'm fine with that, I'm just asking about the usage and - I'm also willing to help if wanted, needed.

@datdenkikniet datdenkikniet force-pushed the adc_dma branch 3 times, most recently from 575067b to d37e02f Compare June 1, 2021 16:07
@datdenkikniet
Copy link
Contributor Author

datdenkikniet commented Jun 1, 2021

Not quite: you can get the peripheral back through the wait method of the DMA channel (it's a little hidden, but actually implemented for all transfers for a specific channel). The Transfer I added builds on top of the already-existing Transfer stuff, it's just a little difficult to find everything in the (relative) jungle of macros.

I'm not sure if having a closure makes a lot of sense in this scenario, especially considering the fact that the DMA configuration depends on the ADC's configuration, so making changes to that would require changes to the DMA configuration. Perhaps there is something I'm missing there, though.

For further DMA discussions I'd recommend you check out #230, since we start using embedded-dma there, and the idea is that that will be merged before this PR.

@datdenkikniet datdenkikniet force-pushed the adc_dma branch 2 times, most recently from 14bccfd to 02af053 Compare June 1, 2021 17:50
@datdenkikniet
Copy link
Contributor Author

Have added a usage example now :)

@akloboucnik
Copy link
Contributor

akloboucnik commented Jun 3, 2021

Not quite: you can get the peripheral back through the wait method of the DMA channel (it's a little hidden, but actually implemented for all transfers for a specific channel.

Ok, this makes sense, I was able to use it in my code now - the usage example helped a lot, thank you!

@korken89
Copy link
Collaborator

Hi the new DMA interface is merged.
Could you have a look at updating this PR please?

@datdenkikniet
Copy link
Contributor Author

datdenkikniet commented Jun 21, 2021

I have updated this PR to be able to use the new DMA API now.

I seem to have lost the usage example in the process, so I'll have to find/re-add that before this can be merged.

Found it!

I've also disabled circular DMA requests for now, as I'm unsure how to properly implement it. I assume it'll require a CircBuffer.

@datdenkikniet
Copy link
Contributor Author

This PR is now ready for review.

@datdenkikniet datdenkikniet force-pushed the adc_dma branch 2 times, most recently from fcbf6d7 to 324886d Compare June 21, 2021 11:40
Copy link
Collaborator

@korken89 korken89 left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@korken89
Copy link
Collaborator

Any objections to merging @MathiasKoch ?

@MathiasKoch MathiasKoch merged commit 9cba0fb into stm32-rs:master Jun 21, 2021
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.

None yet

4 participants