Add ADC usage (for `stm32f303` devices) #47

strom-und-spiele · 2019-12-29T19:56:49Z

Close #20

Sh3Rm4n

These requests are mostly styling requests. I'll hope this is ok :)

I did not try to run it on hardware yet or looked the logic side of this PR.

But thanks for taking action and providing a ADC implementation. Looks good so
far at first glance. 👍

Cargo.toml

examples/adc.rs

src/adc.rs

Sh3Rm4n · 2020-01-05T13:20:08Z

Sadly not much of the ADC registers are documented in stm32-rs. Maybe we could submit a PR, which improves the situation. This would allow using variants, which provide nice type safety.

src/adc.rs

strom-und-spiele · 2020-01-06T14:44:34Z

Thanks for the review I'll try to patch it the next days.

strom-und-spiele · 2020-01-10T14:52:25Z

@Sh3Rm4n wrote:

Sadly not much of the ADC registers are documented in stm32-rs. Maybe we could submit a PR, which improves the situation. This would allow using variants, which provide nice type safety.

I was under the impression that the variant usage here is a type safe way to discriminate between say the stm32f303xB/C/D/E and the stm32f303x6/8 variants.
Or are you suggesting to add the discriminating definitions also to the stm32-rs package? (for as far as I understand it, the different handling of the variants is still needed here.)

Sh3Rm4n · 2020-01-10T16:35:13Z

In this case, stm32-rs does not make a distinction between stm32f303xB/C/D/E and the stm32f303x6/8, as it's code is generated from an svd file for the stm32f303 family, IIRC.

I meant, that I wished more ADC peripherals / registers are documented, so we could use the "variant" methods for the ADC registers. This makes the code more clean and makes the intention clearer, when manipulating the register values. And it provides more type safety.

The ADC registers are mostly documented, as we can see here.

~~But it's not worth the effort to use the variants, when there are not documented for the other stm32f3xx devices, yet.~~

... I justed noticed, that every ADC register is already documented? I don't remeber if this was already the case, when I last looked. Then this should allow us to use variant for all stm32f3xx controllers.

So there is no need to make an PR at stm32-rs, as this is already done.

I'm pretty busy right now, however I can have a look at this PR sometime this
weekend. :)

src/adc.rs

src/lib.rs

Sh3Rm4n · 2020-03-07T23:06:59Z

With the release of stm32f3 - v0.10.0, we could now take advantage of the type systems, as ADC is now fully documented! :) chkmode for example

src/adc.rs

Sh3Rm4n · 2020-03-08T00:38:59Z

Updated PR comment to link to the issue directly :)

Sh3Rm4n · 2020-03-12T22:33:46Z

Reminder (for later doesn't have to be implemented in this PR)

The current implementation does not consider using the otther ADC prescaler: (the one below)

This is a bit more complicated to implement. On the stm32f303b/c it does use PLLCLK, which does seem more common. On the other hand the stm32f303d/e does use the HSI directly.

Hoowever, inspecting this more carefully, this seems like a drawing error, as the Clock Mux on the stm32f303d/e has an HSI input, where nothing is connected to.

strom-und-spiele · 2020-03-13T11:08:59Z

The current implementation does not consider using the other ADC prescaler[…] This is a bit more complicated to implement.

Yes, I skipped this deliberately as I came to the same conclusion as you did ;)

Sh3Rm4n

Could you remove 36e02c6 and 7c7f8b4. I assume these are from #38, so i like to keep them there :)

strom-und-spiele · 2020-03-16T13:53:46Z

Could you remove 36e02c6 and 7c7f8b4. I assume these are from #38, so i like to keep them there :)

I have no clue how to do this without doing nasty stuff.
As those changes are already merged into master is it ok to just ignore this fauxpas?

Sh3Rm4n · 2020-03-16T14:47:35Z

Have a look at git rebase. Pretty useful subcommand. Though the interesting part enfoldes, when you use `git rebase --interactive <commit hash you want to rebase to.

There you have the option to remove commits, change the message of the commits, reorder commits and more. Here is another official article about rewriting the git history.

Remember, you have to force push to the branch, when changing the commit history. I suggest using --force-with-lease as the safer alternative to --force

Hope this helps :)

As those changes are already merged into master is it ok to just ignore this fauxpas?

I assume that they would be ignored, but they still show up in the "files changed" part, so I'm not really sure about this.

strom-und-spiele · 2020-04-06T11:17:21Z

Huge thanks to @Sh3Rm4n for reviewing and teaching me about rebasing.

Sh3Rm4n

Looks good to me overall. Seems to finally come along.

Currently, I have no way to test it, so I trust you that it is working :)
Maybe i'' catch some time in the next days and try it out myself.

src/lib.rs

src/adc.rs

strom-und-spiele · 2020-04-07T16:09:35Z

@#47 (comment) Thanks for catching that typo (again) is fixed in bedfa0f
~~the code does not fit the issue though - did you mean something else?~~ /edit got it

Sh3Rm4n · 2020-04-12T11:01:42Z

Hey I tested your example yesterday and it worked for me on my f3 discovery board! :)

I did write down some notes for review but this is still in progress.

However two things that came to my mind:

Can you be more explicit, that the pa0 you are using in the example is the user button on the discovery board. It is pretty clever to use it, btw. That means that every one who owns a discovery board can test the example immediately.

And the user should now, that the ADC has a 12 bit resolution, so that the maximum expected value is 4095 :)

strom-und-spiele · 2020-04-12T16:25:27Z

Can you be more explicit, that the pa0 you are using in the example is the user button on the discovery board. It is pretty clever to use it, btw. That means that every one who owns a discovery board can test the example immediately.

And the user should now, that the ADC has a 12 bit resolution, so that the maximum expected value is 4095 :)

Thanks for the input - it made me think how much information to include in the output and in the comments. where to spread it and then I went down the rabbit hole of explaining how to properly convert adc values.
Not sure I like it that way but now we can talk about something…

teskje · 2020-07-17T14:02:14Z

@strom-und-spiele What's the status on this PR? I haven't look at the code again, but from the comments here it looks like it's almost ready to merge. Would be great if we could get this over the finish line, ADC support is something I'd personally like to see too.

strom-und-spiele · 2020-07-17T14:09:39Z

I've been using it productively as a base for my ADC needs ever since.
I'll set aside some time on the weekend to rebase it and we can merge it (for stm32f303 only btw)

teskje · 2020-07-17T15:13:04Z

Awesome! I'll give the code a quick review today then.

Co-Authored-By: Sh3Rm4n <f.vioel@gmail.com>

Skip rustfmt in the example when building the adc The reader should check each line to make sure they understand what they are passing.

i.e. - use context sensitive read/write functions - clear flag after usage

Clocksettings are checked during setup and as the setup can fail, Option<adc> is returned now.

Note: In fn calibrate, `while self.rb.cr.read().adcal().is_not_complete() {}` would be better than `while !self.rb.cr.read().adcal().is_complete() {}` However this seems to lack in our version of the PAC In fn disable, `self.rb.cr.modify(|_, w| w.aden().disable());` would be better than `self.rb.cr.modify(|_, w| w.aden().clear_bit());` However this seems to lack in our version of the PAC

src/adc.rs

teskje

Just a few small things now :)

src/adc.rs

This is the reset value of the ADC. Now when running the example it is possible to get values other than 0 and 4095 by pressing the user button shortly.

This has all been addressed.

Sh3Rm4n requested changes Jan 5, 2020

View reviewed changes

Sh3Rm4n reviewed Jan 5, 2020

View reviewed changes

src/adc.rs Outdated Show resolved Hide resolved

strom-und-spiele requested a review from Sh3Rm4n January 10, 2020 14:38

teskje reviewed Jan 18, 2020

View reviewed changes

src/adc.rs Outdated Show resolved Hide resolved

teskje reviewed Jan 18, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

Sh3Rm4n reviewed Mar 7, 2020

View reviewed changes

src/adc.rs Outdated Show resolved Hide resolved

strom-und-spiele mentioned this pull request Mar 13, 2020

Rework PLL calculation and utilize oscillator clock divison #67

Merged

Sh3Rm4n requested changes Mar 15, 2020

View reviewed changes

Sh3Rm4n mentioned this pull request Apr 1, 2020

Support DMA #66

Open

strom-und-spiele force-pushed the adc branch from c98797c to 3f53c2e Compare April 6, 2020 11:16

Sh3Rm4n previously requested changes Apr 6, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/adc.rs Outdated Show resolved Hide resolved

strom-und-spiele requested a review from Sh3Rm4n April 11, 2020 17:29

strom-und-spiele mentioned this pull request Apr 18, 2020

Support for ADCs #20

Closed

strom-und-spiele and others added 14 commits July 18, 2020 12:51

Apply consistent namings from code review

f1ee27c

Co-Authored-By: Sh3Rm4n <f.vioel@gmail.com>

Update consistency 2

e2d37c8

Implement Default as Trait.

3e29640

Update Example

5b8b96a

Skip rustfmt in the example when building the adc The reader should check each line to make sure they understand what they are passing.

removing deprecated unsafe blocks from adc

3e84d05

Tread EOS flag better

1988a0d

i.e. - use context sensitive read/write functions - clear flag after usage

Make use of CKMODE_A

204fa84

Allows CKMODE to be set when initalizing the adc

a09800b

Clocksettings are checked during setup and as the setup can fail, Option<adc> is returned now.

Update adc example with more context

86c049b

drop panic-halt dependency

c130f7a

replace stm32 by pac

54cf1ed

update example comments

d8a007f

fixup! update example comments

09af232

refactoring based on review

9ea3dcf

strom-und-spiele force-pushed the adc branch from 76280b6 to 9ea3dcf Compare July 18, 2020 11:10

strom-und-spiele added 7 commits July 18, 2020 14:47

update feature gates

1d812ae

remove u8 annotations

5e86d48

resorted pin => channel mapping

3258be6

panic on wrong settings instead of returning None obj

a9832b9

refactor CkMode and Align

776c5fd

-newlines and indents

0c472c7

teskje reviewed Jul 18, 2020

View reviewed changes

src/adc.rs Outdated Show resolved Hide resolved

teskje reviewed Jul 18, 2020

View reviewed changes

src/adc.rs Outdated Show resolved Hide resolved

src/adc.rs Outdated Show resolved Hide resolved

teskje requested changes Jul 18, 2020

View reviewed changes

src/adc.rs Outdated Show resolved Hide resolved

strom-und-spiele added 2 commits July 19, 2020 12:57

fix some errors from the review

ff7a430

Change default Sampletime to T_1

26610c6

This is the reset value of the ADC. Now when running the example it is possible to get values other than 0 and 4095 by pressing the user button shortly.

teskje approved these changes Jul 19, 2020

View reviewed changes

teskje merged commit 2ea16b4 into stm32-rs:master Jul 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ADC usage (for `stm32f303` devices) #47

Add ADC usage (for `stm32f303` devices) #47

strom-und-spiele commented Dec 29, 2019 •

edited by Sh3Rm4n

Loading

Sh3Rm4n left a comment

Sh3Rm4n commented Jan 5, 2020

strom-und-spiele commented Jan 6, 2020

strom-und-spiele commented Jan 10, 2020

Sh3Rm4n commented Jan 10, 2020

Sh3Rm4n commented Mar 7, 2020

Sh3Rm4n commented Mar 8, 2020 •

edited

Loading

Sh3Rm4n commented Mar 12, 2020

strom-und-spiele commented Mar 13, 2020

Sh3Rm4n left a comment

strom-und-spiele commented Mar 16, 2020

Sh3Rm4n commented Mar 16, 2020 •

edited

Loading

strom-und-spiele commented Apr 6, 2020

Sh3Rm4n left a comment

strom-und-spiele commented Apr 7, 2020 •

edited

Loading

Sh3Rm4n commented Apr 12, 2020

strom-und-spiele commented Apr 12, 2020 •

edited

Loading

teskje commented Jul 17, 2020

strom-und-spiele commented Jul 17, 2020

teskje commented Jul 17, 2020

teskje left a comment

Add ADC usage (for stm32f303 devices) #47

Add ADC usage (for stm32f303 devices) #47

Conversation

strom-und-spiele commented Dec 29, 2019 • edited by Sh3Rm4n Loading

Sh3Rm4n left a comment

Choose a reason for hiding this comment

Sh3Rm4n commented Jan 5, 2020

strom-und-spiele commented Jan 6, 2020

strom-und-spiele commented Jan 10, 2020

Sh3Rm4n commented Jan 10, 2020

Sh3Rm4n commented Mar 7, 2020

Sh3Rm4n commented Mar 8, 2020 • edited Loading

Sh3Rm4n commented Mar 12, 2020

strom-und-spiele commented Mar 13, 2020

Sh3Rm4n left a comment

Choose a reason for hiding this comment

strom-und-spiele commented Mar 16, 2020

Sh3Rm4n commented Mar 16, 2020 • edited Loading

strom-und-spiele commented Apr 6, 2020

Sh3Rm4n left a comment

Choose a reason for hiding this comment

strom-und-spiele commented Apr 7, 2020 • edited Loading

Sh3Rm4n commented Apr 12, 2020

strom-und-spiele commented Apr 12, 2020 • edited Loading

teskje commented Jul 17, 2020

strom-und-spiele commented Jul 17, 2020

teskje commented Jul 17, 2020

teskje left a comment

Choose a reason for hiding this comment

Add ADC usage (for `stm32f303` devices) #47

Add ADC usage (for `stm32f303` devices) #47

strom-und-spiele commented Dec 29, 2019 •

edited by Sh3Rm4n

Loading

Sh3Rm4n commented Mar 8, 2020 •

edited

Loading

Sh3Rm4n commented Mar 16, 2020 •

edited

Loading

strom-und-spiele commented Apr 7, 2020 •

edited

Loading

strom-und-spiele commented Apr 12, 2020 •

edited

Loading