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 improvements for dma #396

Merged
merged 1 commit into from
Jan 10, 2022
Merged

adc improvements for dma #396

merged 1 commit into from
Jan 10, 2022

Conversation

gernoteger
Copy link
Contributor

This PR should fix adc with dma issues as mentioned in #282: "DMA Transfer consumes peripheral".

Changes:

  • DMA Transfer exposes a reference to the peripheral, currently only for PeripheralToMemory
  • the ADC provides a closure similar to sample_to_millivolts
  • a demonstration of its use in the adc_dma_rtic example.

As of now this PR is not complete; I kindly ask to review the concept before I complete the implementation, documentation and squashing.

@burrbull
Copy link
Member

It is interesting way.
Is there sense to convert to u16 implicitly if you already have u32?

Squash, please.

@burrbull
Copy link
Member

cc @gernoteger
Rebase and squash

@gernoteger
Copy link
Contributor Author

It is interesting way. Is there sense to convert to u16 implicitly if you already have u32?
The u16 was also used original sample_to_millivolts, I just didn't want to change maths in this PR. Thinking of it, this conversion (also in the original) may lead to a loss of resolution: when using 12 bit ADCs, we get 4096 possible steps from the ADC, while a 3.3V VRef will, only produce 3300 Steps.

@burrbull
Copy link
Member

burrbull commented Jan 2, 2022

You've broken git

@gernoteger
Copy link
Contributor Author

Sorry. I'm confused; should I quit working on this PR, and continue with #406 instead?

@burrbull
Copy link
Member

burrbull commented Jan 3, 2022

This PR is preferable but it should be 1 - 2 commits rebased on master branch with no Merge.

@gernoteger
Copy link
Contributor Author

gernoteger commented Jan 4, 2022

I fixed the git issue. Still, I'd like to tackle the issue with the lost resolution by converting samples to milliseconds. Probably this con go to a different PR, what do you think?
The Changelog still needs some fixing

src/dma/mod.rs Outdated
@@ -1059,6 +1059,11 @@ where
}
}

/// Access the owned peripheral for reading
pub fn peripheral(&self) -> &PERIPHERAL {
Copy link
Member

Choose a reason for hiding this comment

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

DMA code for this HAL is not mine, so I need to clarify is such access safe?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@gernoteger I still can't merge this as this method gives unsafe access any DMA payload.
Possible solution:
b275233

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for doing all the work. I incorporated your suggestion nearly 1:1, only fixed a small typo in a comment

@burrbull
Copy link
Member

burrbull commented Jan 4, 2022

I fixed the git issue. Still, I'd like to tackle the issue with the lost resolution by converting samples to milliseconds. Probably this con go to a different PR, what do you think?

I'm afraid I don't understand what lost resolution you mean. milliseconds? time?

@gernoteger
Copy link
Contributor Author

gernoteger commented Jan 4, 2022 via email

@gernoteger gernoteger force-pushed the adc23-dma branch 2 times, most recently from 3268f3c to cc67977 Compare January 7, 2022 20:02
@gernoteger
Copy link
Contributor Author

I documented the resolution issues mentioned earlier in #412. I think we should handle this in a different PR.
From my point of view this PR is feature complete and can be merged.

providing access to calibrated voltage conversion to approved wrappers
@burrbull
Copy link
Member

bors r+

@bors bors bot merged commit 59aa101 into stm32-rs:master Jan 10, 2022
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.

2 participants