Added DAC with basic features #133

David-OConnor · 2020-09-01T03:11:54Z

Includes builtin DAC trait that would ideally be in embedded-hal. Missing noise and tri wave features (commented-out and partially implemented) and triggers, but I think it's worth merging even without.

The 12-bit left mode doesn't work; probably needs bit arith on the value word integer.

edit: Added triggers and (probably broken) tri wave and noise.

David-OConnor · 2020-09-03T00:39:20Z

Added a modified version of #93, with most f the review requests added.

That module is badly in need of being added. I understand it's not perfect, but it's good enough. I got rid of LSI/HSE support as requested, to avoid exposing the extra RCC regs, per the discussion.

teskje · 2020-09-03T08:43:56Z

Could you please split the DAC and RTC implementations into two self-contained PRs? This will make it easier to keep track of these two features and increase the chances that somebody finds time to review one or both of them.

David-OConnor · 2020-09-03T17:00:51Z

The other PR showed no progress, and I accidentally committed the rtc file and modified rcc file since I use it in my projects. I would be thrilled if that were merged.

I added most of your review changes to that PR, but don't know how to justify the unsafe blocks. Would you be willing to merge that PR if it were modified to reflect what I have in this one? How would you like to proceed?

teskje · 2020-09-03T18:56:45Z

I would suggest you open a new PR that duplicates #93 with the RTC-related changes from here. Make sure all CI checks pass, then I'll review it and if it looks good, we can merge and close #93.

And from this PR here, just remove all the RTC-related changes (by rewriting the history and force-pushing) so it contains only the DAC feature. And then the same procedure: Make sure the CI tests pass, the maintainers will review, and if everything looks good we'll merge it.

Hope that clears it up.

David-OConnor · 2020-09-03T23:26:38Z

Done. RTC pr

David-OConnor · 2020-09-06T22:59:07Z

Any idea why it's failing CI? ##[error]Unable to create clippy annotations! Reason: HttpError: Resource not accessible by integration.

teskje · 2020-09-07T06:10:47Z

That error is related to the Github Action we are using (see https://github.com/actions-rs/clippy-check#limitations) and is not the reason clippy fails, so you can ignore it.

The clippy error is:

error: unused import: `panic_semihosting`
 --> examples/dac.rs:4:5
  |
4 | use panic_semihosting;
  |     ^^^^^^^^^^^^^^^^^
  |
  = note: `-D unused-imports` implied by `-D warnings`


error: this import is redundant

Replace that line with:

use panic_semihosting as _;

David-OConnor · 2020-09-11T23:16:54Z

Can anyone figure out why this line doesn't work, but its equivalent for right-hand work? It's probably bit arithmetic I'm missing.

DacBits::TwelveL => self.regs.dhr12l1.modify(|_, w| unsafe { w.bits(val) }),

David-OConnor · 2020-09-19T15:29:09Z

Moved away from trait-based approach, since I don't think it's general enough to make it into EH.

examples/dac.rs

src/dac.rs

eldruin · 2020-10-12T07:05:20Z

@ra-kete What is your intention/thoughts regarding this PR? We are thinking about merging the SingleChannelDac trait into embedded-hal. See: rust-embedded/embedded-hal#247

teskje · 2020-10-12T09:27:10Z

@eldruin I would like to merge the PR as soon as the remaining open review comments have been addressed.

eldruin · 2020-10-12T10:35:48Z

@ra-kete Sounds good, thanks. I will hold the merge in embedded-hal then.

David-OConnor · 2020-10-12T12:52:46Z

Ready for merge. This represents an improvement to the crate by adding missing functionality, with a simple API, and no breaking changes.

eldruin

The one TODO about correctness is still there.
I would also recommend squashing the changes before merging.

src/dac.rs

teskje · 2020-10-13T10:29:54Z

@eldruin No worries, I will squash anyway when I merge.

@David-OConnor Please don't just mark review comments as "resolved" without at least commenting why you think they can be ignored.

David-OConnor · 2020-10-13T14:37:50Z

I did a squash, and now the Files changed tab is reflecting changes from other PRs.

I marked the comments resolved when I asked you questions and you didn't respond to them.

teskje · 2020-10-13T15:14:24Z

I did a squash, and now the Files changed tab is reflecting changes from other PRs.

A squash is simply the act of collecting multiple commits into a single one, it shouldn't pull in other changes. If you can't make it work, please just reset back to the state before. I'll do the squash when merging.

I marked the comments resolved when I asked you questions and you didn't respond to them.

Okay, there seems to be some communication issue here. I am not aware of any questions you asked me. Where did you ask me? I normally would expect such questions as comments under my review comments.

David-OConnor · 2020-10-14T04:44:37Z

Thanks - squash fixed! For the questions, they're in the review comments.

David-OConnor · 2020-10-14T23:56:46Z

I recommend merging, then fine-tuning in a separate PR.

teskje · 2020-10-15T05:32:39Z

I think I already made clear my view that code we merge should be of high quality and production-ready. This crate has enough code that needs refactoring already, no reason to willingly add more.

I don't see why we can't just fix the few known issues in this PR? None of them look very difficult to me. If you don't feel up to it, let me know. I can fix them myself in this PR.

David-OConnor · 2020-10-15T13:45:26Z

Please fix them yourself.

teskje · 2020-10-18T11:39:33Z

I've updated the PR to the best of my knowledge. @strom-und-spiele, @David-OConnor, would be good if you could look at it again to ensure I didn't mess up any of the finer DAC details.

strom-und-spiele · 2020-10-18T14:00:28Z

LGTM.
Making Commits based on topics made it easy to understand what you did each step.
Selecting channels based on the Pin pulls down complexity and allows for ergonomic handling.
On Monday I'll set an oscilloscope up in my office to check triangle and LSFR generation. If I can make it work with the given code in a reasonable amount of time I'd vote for adding it to the example and keeping it.

Apart form that: thanks @David-OConnor and @ra-kete for you work.

David-OConnor · 2020-10-18T14:07:37Z

Looks good!

teskje · 2020-10-18T14:27:22Z

@strom-und-spiele Okay, sounds good! I'm all for keeping this functionality if we can verify it. I'll hold off merging until you have the results then.

Sh3Rm4n

I think this is missing a free() method to give back ownership of the Pin, see C-FREE

To build, the examples need a specific board. Right now the examples are only implemented for the STM32F3DISCOVERY. This board is equipped with a stm32f303xc, which is now the required feature.

strom-und-spiele · 2020-10-19T13:02:12Z

Updated the required features for all examples that were still referencing stm32f303 as this is conflicting with setting up the pins correctly.

Playtime for today is over. Couldn't make the current example work. This is the error I was stuck with last:

$ cargo run --example dac --features stm32f303xc
    Finished dev [unoptimized + debuginfo] target(s) in 0.11s
     Running `arm-none-eabi-gdb -q -x openocd.gdb target/thumbv7em-none-eabihf/debug/examples/dac`
Reading symbols from target/thumbv7em-none-eabihf/debug/examples/dac...
0x080065de in ?? ()
openocd.gdb:10: Error in sourced command file:
Dwarf Error: Cannot find DIE at 0x940 referenced from DIE at 0x8e5 [in module /home/bastian/tmp/stm32_dac/stm32f3xx-hal/target/thumbv7em-none-eabihf/debug/examples/dac]

teskje · 2020-10-19T13:19:57Z

I think this is missing a free() method to give back ownership of the Pin, see C-FREE

Good catch, I will add that. I think for this I'll have to make the Dac abstraction generic over the output pin.

@strom-und-spiele Looks like you're building in debug mode. Have you tried --release?

strom-und-spiele · 2020-10-19T14:48:39Z

--release made the example build with an output of ~1.3 V. This might be b.c. the multimeter on my desk is not that precise, the input is too low or the reference voltage on the board is not 3V but lower.

I'll be back in the office on Wednesday and will try to find out what the reason for the under voltage is.

$ cargo size --example dac --features stm32f303xc
    Finished dev [unoptimized + debuginfo] 
   text	   data	    bss	    dec	    hex	filename
  21080	      0	      4	  21084	   525c	dac
$ cargo size --example dac --features stm32f303xc --release
    Finished release [optimized + debuginfo]
   text	   data	    bss	    dec	    hex	filename
   4712	      0	      4	   4716	   126c	dac

according to the datasheet, a nnnxC device should have 256 KBytes of flash. So what is it, I'm missing here? ^^

strom-und-spiele · 2020-10-21T16:03:03Z

So I hooked it up to the osci and I get the following with a freshly out-of-the-box STM32F3DISCOVERY and v_ref =3.3 (as in the example code)

set_value   - measured Volts
128         - 1.49
255         - 2.915
256         - 0 (do we want this to happen?)

set_voltage - measured volts
1.0         - 0.9
1.5         - 1.3
3.0         - 2.7
3.3         - 2.915
5.0         - 1.5 (do we want this to happen?)

changed v_ref to 2.915 and got:

set_voltage - measured volts
1.0         - 1.0
1.5         - 1.5
2.5         - 2.5
5.0         - 2.1 (do we want this to happen?)

Need to rush home now but will try the dropped patterns tomorrow.

src/dac.rs

Sh3Rm4n · 2020-10-21T20:11:04Z

src/dac.rs

+    /// Set the DAC voltage. `v` is in Volts.
+    pub fn set_voltage(&mut self, volts: f32) {
+        let val = match self.bits {
+            DacBits::EightR => ((volts / self.vref) * 255.) as u32,


Maybe there is a better, robust and explicit version (e.g. u32::try_from(...)).
I'm not sure it is needed, though.

Sh3Rm4n · 2020-10-21T20:53:16Z

So I hooked it up to the osci and I get the following with a freshly out-of-the-box STM32F3DISCOVERY and v_ref =3.3 (as in the example code)
set_value   - measured Volts
128         - 1.49
255         - 2.915
256         - 0 (do we want this to happen?)

Seems like an u8 (256) bit overflow?

strom-und-spiele · 2020-10-21T23:47:28Z

So I hooked it up to the osci and I get the following with a freshly out-of-the-box STM32F3DISCOVERY and v_ref =3.3 (as in the example code)
set_value   - measured Volts
128         - 1.49
255         - 2.915
256         - 0 (do we want this to happen?)
Seems like an u8 (256) bit overflow?

sorry, I was in a rush and just c&p'ed the same phrase over and over. This is definitely an overflow issue. the implied question was whether the expected behavior is for the value to overflow.
Say the ref voltage is 2V. If I set the output voltage to 3.3V the actual output voltage will be 1.3V. This might be what I wanted but it might as well be totally wrong. IMHO the best reaction would be to abort during compile time and force the user to apply a "mod 2" operation explicitly in the calling code.

Sh3Rm4n · 2020-10-22T07:05:44Z

IMHO the best reaction would be to abort during compile time and force the user to apply a "mod 2" operation explicitly in the calling code.

As set_voltage() and set_value() can not fail they should not fail. Tricks like these should be included in the infallible version.

If this overflow error is not preventable, e.g. value > v_ref, then set_value() and set_voltage() should fail / return a Result.

strom-und-spiele · 2020-10-22T16:50:46Z

Couldntt make the wave generation work on short notice. I advise merging without them.

However I noticed that there was no way to actually trigger the software trigger, so I added it.

Sh3Rm4n · 2020-10-23T06:46:03Z

src/dac.rs

+    }
+
+    // Select and activate a trigger. See f303 Reference manual, section 16.5.4.
+    pub fn set_trigger(&mut self, trigger: Trigger) {


I wonder if we need a "get_set_trigger" function to check which trigger is set?
Not really needed, more like a nice to have.

Unless we have a convincing use-case for it now, I would leave it out for simplicity. We can always add it later if it's actually needed.

src/dac.rs

teskje · 2020-10-25T13:21:57Z

.cargo/config

@@ -1,5 +1,5 @@
 [target.thumbv7em-none-eabihf]
-runner = "arm-none-eabi-gdb"
+runner = "arm-none-eabi-gdb -q -x openocd.gdb"


@strom-und-spiele Do you think it is a good idea to add a reference to a file that is not part of this repo?
IMO we probably shouldn't even specify GDB as a runner, as there are other options. But not sure what the intentions were when this was added.

this was not intended.

teskje · 2020-10-25T13:23:25Z

examples/dac.rs

+    let dac_pin = gpioa.pa4.into_analog(&mut gpioa.moder, &mut gpioa.pupdr);
+
+    // Set up the DAC
+    let reference_voltage = 2.915;


@strom-und-spiele Where is that voltage value coming from?

Ah, I see you got that from experimentation. Is this a value we can expect to be the same on each user of an F3DISCOVERY? If not, I think we should document how a user can find there own correct value. There must be some way to measure/infer the VREF for a given board, no?

I wasn't so sure either and checked the schematics of the stm32f3discovery again.

VREF is a stabilized version VDD.
VDD is directly connected via Jumper 3 to 3V
3V is powered by the 5V supply and adjusted by a BAT60JFILM which has a voltage drop that depends on the power being used, so it's roughly 3V. Less if your chip is running something more demanding.

I suggest

replacing the 2.9… by a solid 3

add to the docs the info that the ref value should be known by the schematics and can be roughly determined by setting set_value to 255 and measuring the output

add a hint to the docs in the example

teskje · 2020-10-25T14:03:09Z

I implemented/responded to all remaining review comments, except for the issue of handling invalid voltages/values.

There is not much we can do at compile time. We could limit setting an 8-bit value to a u8, but for setting 12-bit values Rust doesn't provide us a fitting data type. IMO it's better to handle both cases uniformly be returning a Result. Same is true for setting a voltage, I don't see what we could do at compile time to ensure no invalid value can be set.

So I propose making both set_value and set_voltage fallible. This would also work well with the SingleChannelDac trait, which already assumes that conversions can fail.

strom-und-spiele · 2020-10-25T23:52:51Z

There is not much we can do at compile time. We could limit setting an 8-bit value to a u8, but for setting 12-bit values Rust doesn't provide us a fitting data type. IMO it's better to handle both cases uniformly be returning a Result. Same is true for setting a voltage, I don't see what we could do at compile time to ensure no invalid value can be set.

While nothing stops us from using a twelve bit type I agree that uniformity is nicer here.

David-OConnor mentioned this pull request Sep 2, 2020

added dac rust-embedded/embedded-hal#247

Closed

David-OConnor mentioned this pull request Sep 15, 2020

Support for DACs #51

Closed

teskje requested changes Sep 25, 2020

View reviewed changes

teskje requested changes Sep 27, 2020

View reviewed changes

src/dac.rs Outdated Show resolved Hide resolved

src/dac.rs Outdated Show resolved Hide resolved

src/dac.rs Outdated Show resolved Hide resolved

src/dac.rs Outdated Show resolved Hide resolved

src/dac.rs Outdated Show resolved Hide resolved

src/dac.rs Outdated Show resolved Hide resolved

eldruin reviewed Oct 13, 2020

View reviewed changes

teskje requested changes Oct 13, 2020

View reviewed changes

src/dac.rs Outdated Show resolved Hide resolved

src/dac.rs Outdated Show resolved Hide resolved

added DAC functionality

92067dc

Merge remote-tracking branch 'upstream/master' into dac

81d8255

removed debug

0a2d3d3

David-OConnor requested a review from strom-und-spiele October 15, 2020 13:51

teskje approved these changes Oct 18, 2020

View reviewed changes

teskje added 2 commits October 18, 2020 16:20

Merge branch 'master' into dac

174a94b

dac: Add changelog entry

676a621

Sh3Rm4n requested changes Oct 18, 2020

View reviewed changes

examples: Set specific chip.

b8e6f56

To build, the examples need a specific board. Right now the examples are only implemented for the STM32F3DISCOVERY. This board is equipped with a stm32f303xc, which is now the required feature.

Sh3Rm4n requested changes Oct 21, 2020

View reviewed changes

dac: Add a way to trigger the software trigger

1d6943a

Sh3Rm4n reviewed Oct 23, 2020

View reviewed changes

teskje reviewed Oct 25, 2020

View reviewed changes

teskje added 3 commits October 25, 2020 14:27

Merge branch 'master' into dac

2eb0773

dac: Minor improvements to doc comments

85e8c5d

dac: Provide a free destructor method

fec90c2

David-OConnor and others added 2 commits October 31, 2020 19:47

Merge branch 'master' into dac

5b7b3d4

Merge branch 'master' into dac

5da9e77

David-OConnor closed this Nov 4, 2020

Added DAC with basic features #133

Added DAC with basic features #133

Conversation

David-OConnor commented Sep 1, 2020 • edited Loading

David-OConnor commented Sep 3, 2020 • edited Loading

teskje commented Sep 3, 2020

David-OConnor commented Sep 3, 2020

teskje commented Sep 3, 2020

David-OConnor commented Sep 3, 2020

David-OConnor commented Sep 6, 2020

teskje commented Sep 7, 2020

David-OConnor commented Sep 11, 2020

David-OConnor commented Sep 19, 2020

eldruin commented Oct 12, 2020

teskje commented Oct 12, 2020

eldruin commented Oct 12, 2020 • edited Loading

David-OConnor commented Oct 12, 2020 • edited Loading

eldruin left a comment

Choose a reason for hiding this comment

teskje commented Oct 13, 2020

David-OConnor commented Oct 13, 2020 • edited Loading

teskje commented Oct 13, 2020 • edited Loading

David-OConnor commented Oct 14, 2020

David-OConnor commented Oct 14, 2020 • edited Loading

teskje commented Oct 15, 2020

David-OConnor commented Oct 15, 2020 • edited Loading

teskje commented Oct 18, 2020

strom-und-spiele commented Oct 18, 2020

David-OConnor commented Oct 18, 2020

teskje commented Oct 18, 2020

Sh3Rm4n left a comment

Choose a reason for hiding this comment

strom-und-spiele commented Oct 19, 2020

teskje commented Oct 19, 2020

strom-und-spiele commented Oct 19, 2020 • edited Loading

strom-und-spiele commented Oct 21, 2020

Choose a reason for hiding this comment

Sh3Rm4n commented Oct 21, 2020

strom-und-spiele commented Oct 21, 2020

Sh3Rm4n commented Oct 22, 2020 • edited Loading

strom-und-spiele commented Oct 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teskje commented Oct 25, 2020

strom-und-spiele commented Oct 25, 2020 • edited Loading

David-OConnor commented Sep 1, 2020 •

edited

Loading

David-OConnor commented Sep 3, 2020 •

edited

Loading

eldruin commented Oct 12, 2020 •

edited

Loading

David-OConnor commented Oct 12, 2020 •

edited

Loading

David-OConnor commented Oct 13, 2020 •

edited

Loading

teskje commented Oct 13, 2020 •

edited

Loading

David-OConnor commented Oct 14, 2020 •

edited

Loading

David-OConnor commented Oct 15, 2020 •

edited

Loading

strom-und-spiele commented Oct 19, 2020 •

edited

Loading

Sh3Rm4n commented Oct 22, 2020 •

edited

Loading

strom-und-spiele commented Oct 25, 2020 •

edited

Loading