added dac

[David-OConnor]

Basic features only. Could get more complicated with triggers, multi-channel features etc, but for now, would prefer to keep it simple and general, to avoid becoming too provincial.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eldruin (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[eldruin]

Thank you for this proposal!
As a reminder, implementations proving this are necessary before merge. See here.

[David-OConnor]

Implementation

[eldruin]

Sorry I forgot to mention the try_ prefix.
An additional concern is whether set_voltage can be implemented/makes sense for every DAC.

[mendelt]

@eldruin Thanks for pointing me in this direction.
This looks very similar to what I proposed here #240 I created one example implementation for the Microchip mcp4725 I2C DAC that I could easily adapt to this DAC trait if you need help with example implementations.

I was working on an example implementation for a multichannel DAC but got stuck on how to implement a .split method that would return one DAC instance for each channel. But that mostly has to do with my limited knowledge of no-std programminging in Rust. I could try to continue that too.

There are a couple of things I'm not sure about. The set_voltage is not something all DACs can implement. Many DACs use an external reference to scale their output so the scaling factor for value to voltage is not known beforehand. And I'm not sure its that useful, in most applications the DAC output will be scaled to some other value using a voltage divider or opamp right after the input anyway so the user of this trait will have to add her own scaling anyway and I'm not sure if using floats for this is always a good idea.

The set_enabled and disabled methods could be useful. But most DACs I've seen have more extensive power-down modes. And as this trait would represent a single channel for a multi channel DAC. How would this work if the DAC does not allow enabling/disabling single channels? My choice would be to keep this to the DAC driver specific implementation and only have a try_set_output.

Also I would make the Value a generic parameter of the trait. I think that would be more consistent with how its done in similar embedded hal traits like here https://github.com/rust-embedded/embedded-hal/blob/master/src/blocking/serial.rs for example.

[ryankurte]

hmm, it's probably worth considering in the context of the pwm and adc traits to try and be consistent with associated types / channels / constants etc. also i also wonder if there is a big difference between a single channel DAC and a multi-channel DAC with, one channel?

[eldruin]

@mendelt Thank you for your input!
I think a generic type is preferable to an associated type if there will be several implementations for the different types, like SingleChannelDac<u8>, SingleChannelDac<u16>. I am not sure if that would be necessary for DACs or rather each device as a fixed number of bits.
@ryankurte Agreed on comparing with Adc and specially Pwm/PwmPin traits, which look quite similar.

[mendelt]

also i also wonder if there is a big difference between a single channel DAC and a multi-channel DAC with, one channel?

In my mind this trait could also be used for multi channel dacs without change. The dac driver could implement a .split() method that returns a tuple of SingleChannelDac instances, one for each channel. I was working on something like that as an example for my implementation of this. But got stuck on how to implement sharing the i2c between the channels and not enough time for figuring this out.
https://github.com/mendelt/ad5668/tree/dac-hal
When I have time I can try to rebase on this PR and see if I can get this working.

[mendelt]

I think a generic type is preferable to an associated type if there will be several implementations for the different types, like SingleChannelDac<u8>, SingleChannelDac<u16>. I am not sure if that would be necessary for DACs or rather each device as a fixed number of bits.

Ah, that makes sense, and I can't imagine a situation where you'd want to use the same dac with multiple word lenghts.

[eldruin]

@mendelt sharing can be implemented with a RefCell. You can have a look here and here for an example with GPIO.

[mendelt]

@mendelt sharing can be implemented with a RefCell.

Thank you! I was looking at using a RefCell too but was unsure if that would be the right solution. I think that means you cannot move dac-channels to an interrupt handler for example right? Or should that not be something a driver is concerned with?
But maybe I should ask this question somewhere else. I'm afraid I'm hijacking this thread.

[Rahix]

@mendelt: Maybe a proxy for DAC's in shared-bus could help here?

[eldruin]

I think that means you cannot move dac-channels to an interrupt handler for example right?

I am not sure how that would fit. I would need to play around with it.

[mendelt]

Implemented this for a multichannel DAC (8 channel AD5668) with a split method based on a RefCell like @eldruin suggested.
For now this is based on my version of the DAC trait because the set_voltage, enable and disable do not make much sense for this. But other than that it should be easy to move to this DAC trait;

https://github.com/mendelt/ad5668/tree/dac-hal
The error handling here does need some cleanup and I still need to test this on real hardware.

[David-OConnor]

Maybe we remove the enable part. On the two DACs I have experience with it's applicable (STM32's onboard DAC, and MCP4921), but if it's not universal, then probably not appropriate.

What I'm also struggling with is arguments passing to it; eg &mut rcc.apb1, which is a peripheral clock; this may be not feasible, or it may require more trait work than I know how to do.

[mendelt]

What I'm also struggling with is arguments passing to it; eg &mut rcc.apb1, which is a peripheral clock; this may be not feasible, or it may require more trait work than I know how to do.

Maybe I'm missing what you mean here. But isn't that part of the setup for a specific DAC? Dependencies like clocks or pins or i2c/spi busses etc can be passed to a specific DAC implementation in a constructor method specific to that DAC? There is no need to make that part of the DAC trait.

[David-OConnor]

I guess there's not much that can be extracted into a generic trait

[David-OConnor]

I switched the f303 impl to using infallible traits built into the struct, and added a set_value only method to a trait there. I'm suspicious we're not going to be able to make much more than that generic.

stm32-rs/stm32f3xx-hal#133

pub trait SingleChannelDac {
    /// Error type returned by DAC methods
    type Error;
    type Word;

    /// Output a constant signal, given a bit word.
    fn try_set_value(&mut self, value: Self::Word) -> Result<(), Self::Error>;
}

[mendelt]

I switched the f303 impl to using infallible traits built into the struct, and added a set_value only method to a trait there. I'm suspicious we're not going to be able to make much more than that generic.

Yeah, but I don't think that's a bad thing. It is a simple abstraction but useful.
Let me know when you have this committed, I can add examples on how to use this trait in a single channel i2c DAC and a multi-channel spi one.
Not sure what else needs to be done to get this merged?

[David-OConnor]

Done

[mendelt]

Looks like only half of the move of dac.rs came through. The file is gone from src but its not in blocking yet.

[mendelt]

Works!
Got the 8 channel ad5668 driver compiling: https://github.com/mendelt/ad5668/tree/dac-hal
And the simpler 1 channel mcp4725: https://github.com/mendelt/mcp4725/tree/dac-hal

I'll see if I can test this with real devices later this week. But there are no code changes so I'm pretty confident that works. And I don think thats really the point of these example implementations anyway.

[David-OConnor]

Nice! I updated the stm32f3 impl above to reflect this new API too. Ie moved the other stuff into infallible, non-EH methods.

[eldruin]

Could this be made to support multiple channels as well similarly to adc::OneShot?
I think pretty much a copy of it changing the method to try_set_value would work.
For the record, the adc is a better example than pwm for which outstanding issues exist.

[mendelt]

Could this be made to support multiple channels as well similarly to adc::OneShot?
I think pretty much a copy of it changing the method to try_set_value would work.

Not sure if I understand this correctly. The SingleChannelDac is already pretty much a copy of the adc::OneShot right? But with a configurable word length and in my opinion a better trait name. Not sure whats one shot about the OneShot
This trait works pretty well for my 8-channel example so far.

The only thing from the adc that could be added is the Channel trait for runtime identification of channel numbers. Tbh. I have a hard time to see any real use-cases for that. I don't know of any dacs with variable channel assignment. You initialize a dac, split into channels and from there it should be clear which channel is which. But if anyone really uses that then maybe the adc trait should be generalized? There is nothing specifically adc about it.

[eldruin]

I was talking about making a Dac trait with multi-channel support (instead of splitting into single-channel ones).
We could have a single-channel DAC trait and a multi-channel one like Pwm vs. PwmPin:

pub trait Channel<DAC> {
    type ID: Copy;
    fn channel(&self) -> Self::ID;
}

pub trait Dac<DAC, Word, CHAN: Channel<ADC>> {
    type Error;
    fn try_set_value(&mut self, value: Word, channel: &mut CHAN) -> Result<(), Self::Error>;
}

pub trait SingleChannelDac {
    type Error;
    type Word;
    fn try_set_value(&mut self, value: Self::Word) -> Result<(), Self::Error>;
}

I am still thinking whether the Word is better as an associated type or a type parameter.

[mendelt]

Ah sorry, got it.
Maybe we could try to move forward with the single channel dac over here and create another pr for the multi channel version then? I think that was @David-OConnor s original intent anyway.

You actually convinced me that an associated type would be a better fit here (and maybe in most places) I looked around and I don't see that many drivers, even for peripheral types that do support multiple word-lengths, implement it. I think most serial::Read and serial::Write implementations just use u8 for example. For DACs it makes even less sense.

[eldruin]

What do you think @David-OConnor?

[David-OConnor]

I haven't used a multi-channel DAC before, so can't say what makes the most sense.

[mendelt]

I like the extra flexibility, but I have some trouble imagining a use for this. There might be dacs that have a reduced precision low-power mode? Or support low precision fast operation? But I haven't encountered them.
On the other hand I don't think this impedes the default case where a dac will only use a single word-size in any way. So I maybe we should just go for it.

@eldruin just out of curiosity, for the multichannel example. What is the use of the Channel trait? Cant you just use a type parameter or associated type to determine how to select channels? I think the primary use case is to allow for the use of tuples to select channels in banks right?

[eldruin]

@mendelt yes, I do not have examples of selectable precision DACs right now but that would not be very far fetched, I would say. You could also have signed/unsigned implementations, if that fits your DAC specially well.

About the Channel trait, you can have a look at the Adc PR.

[eldruin]

Ok, this is almost good to go.
Just two missing things:

• Add an entry about this trait to the changelog
• Add this trait to the prelude

[eldruin]

Sorry I forgot about the squashing. Could you squash all these commits into one?
It would be rather confusing to look at all 12 commits about this trait in the history.
There is also a conflict in the changelog.

[eldruin]

Ok, almost there!

• There is still a conflict in the Changelog. Please rebase this branch to the master branch and resolve it.
• Also, could you name the commit something like "Added single-channel DAC trait"?

Thank you!

[David-OConnor]

Done

[David-OConnor]

Done

[eldruin]

Thank you!
I suggested a last fix to the documentation. Could you accept it and squash everything into one commit again?

[eldruin]

Hmm, there is still a conflict in the changelog file.

[David-OConnor]

Do you know how to fix it? IIRC I tried both ways - when it had the try_set_state method for OutputPin using an input PinState value. line, GH flagged an error.

What should the Added section of the changelog look like?

[eldruin]

Ok that worked. Thanks!
As proof for drivers we have AD5668 and MCP4725. @mendelt did you find time to test the drivers on real devices?
As proof for MCUs there is the stm32f3xx-hal#133 PR. What is the status there? I think this would be the last requirement.

[David-OConnor]

Oh, great! I must have made some mistake before.

[David-OConnor]

Thoughts?

[eldruin]

Is there an MCU HAL implementation of this trait somewhere? The PR I linked above was closed.

[burrbull]

"try_"

[David-OConnor]

I would wrap the set_value() method in this STM32 HAL module, once this PR is merged and released. Until released, I don't think implementing it makes sense.

I would not use it on any real projects in this state though, without enable(), disable() methods.

[David-OConnor]

After a discussion on Matrix and further consideration, I don't think DAC is appropriate for EH.