Analog output for Arduino Uno

[jeroenvlek]

The Arduino supports PWM on a few ports, yet looking through ports.rs, the HAL only seems to allow digital outputs. Is there an example of doing analog writes? Does this require including a lower level implementation?

[Rahix]

Yeah, PWM support was not yet reimplemented after the big refactor last year :/ There are numerous bits and pieces of code in various issues which show how to set it up manually in the meantime: #230 #194 #232

Of course the plan would be to reimplement PWM support in the HAL, similar to how it existed in the old version:

• https://github.com/Rahix/avr-hal/blob/old/avr-hal-generic/src/pwm.rs
• https://github.com/Rahix/avr-hal/blob/old/chips/atmega328p-hal/src/pwm.rs

[jeroenvlek]

So roughly the old implementation needs to be moved over to the new avr-hal, divided over the HALs of the respective boards for the pins that support PWM, and then unified where possible into something like Pin.into_analog_output() in ports.rs?

[stappersg]

... divided over the HALs of the respective boards for the pins that support PWM, and then unified where possible into something like Pin.into_analog_output() in ports.rs?

Please, pretty please, name PWM output PWM output. (Stop using PWM output "analog" output.)

[jeroenvlek]

... divided over the HALs of the respective boards for the pins that support PWM, and then unified where possible into something like Pin.into_analog_output() in ports.rs?
Please, pretty please, name PWM output PWM output. (Stop using PWM output "analog" output.)

Sure, this was inspired by analogWrite() in the C api. Calling it Pin_into_pwm_output() sounds also good :)

[Rahix]

So roughly the old implementation needs to be moved over to the new avr-hal, divided over the HALs of the respective boards for the pins that support PWM, and then unified where possible into something like Pin.into_analog_output() in ports.rs?

I think the main roadblock here was coming up with a nice design that

1. caters to the "analog output" usecase of just wanting any old PWM waveform e.g. for controlling LED brightness.
2. but at the same time allows precise control of the waveform for users who needs things like exact frequency and dutycycle or even phase-correct PWM... it is especially tricky because multiple pins are controlled by the same timer and thus cannot be controlled separately...

But as that's going to be a bigger task, maybe we can start with a simple_pwm module that just re-adds the old functionality again?

Sure, this was inspired by analogWrite() in the C api. Calling it Pin_into_pwm_output() sounds also good :)

In the old code, it was into_pwm() so I would vote to got with that or your into_pwm_output() which is more consistent with the other into_*() methods...

[jeroenvlek]

Yes, that sounds good and I'm willing to take a shot at it. This will be the biggest thing I've done in Rust so far (admittedly not very big), so let me start with the easier task, then I can get acquainted with that part of the framework.

[jeroenvlek]

Small update: Due to time constraints I can only occasionally look at it. I'm starting to learn to switch between macro and non-macro code. The avr-hal-generic crate still had a pwm.rs module. I'm now trying to connect that to ports.rs (massive file, btw) where I've created a pin mode called PwmOutput. Still finding out how to connect the macro in pwm.rs to the ports.rs. My suspicion is to create a make_pwm in PinOps and connect it there. Then I will update the atmega-hal crate to match that new implementation and add an example for the Uno board to make sure it works.

I aim to end up with something like:

let pins = arduino_hal::pins!(dp);
let mut timer0 = Timer0Pwm::new(tc0, pwm::Prescaler::Prescale64);
let mut dir_pin = pins.d5.into_pwm_output(timer0);

(as opposed to pins.d5.into_output().into_pwm_output(&mut timer0))

If anyone is waiting for me, but thinks they can do it faster, please go for it :) However, my ambition still is to learn and finish this!

[Rahix]

The avr-hal-generic crate still had a pwm.rs module.

Oh, oops... Well, be careful with it, it is most likely very imcompatible with the new port.rs...

My suspicion is to create a make_pwm in PinOps and connect it there.

Hm, fundamentally this is configured in the timer instead of the GPIO port so this should rather go into the TimerOps or however we should call it. The only requirement for the pin is that it is configured as an output. From there, the timer can "override" the pin to put it into PWM mode.

(as opposed to pins.d5.into_output().into_pwm_output(&mut timer0))

Not sure I understand the intention here? If you move the timer into the pin, you cannot re-use it for other pins anymore which means you can only use a subset of all PWM pins at the same time...

[Dieschdel]

I faced the exact same problem, so I made myself a quick workaround using the suggestions stated before (currently only works for 8 bit timers, i.e. TC0 and TC2 and lacks some customizability).

I started by creating a PWM wrapper. This could theoretically house more functions related to the PWM module.

The decision to make Pwm generic could be discussed. On the one hand, it enables generic handling by future functions, on the other hand it increases complexity without significantly decreasing code size.

pub struct Pwm<TC> {
    pub inner: TC,
}

pub trait IntoPwm<CSX, TC> {
    fn into_pwm(self, prescale: CSX) -> Pwm<TC>;
}

impl IntoPwm<CS0_A, TC0> for TC0 {
    fn into_pwm(self, clock_select: CS0_A) -> Pwm<TC0> {
        self.tccr0a.write(|w| w.wgm0().pwm_phase().com0a().match_clear().com0b().match_clear());
        self.tccr0b.write(|w| w.cs0().bits(clock_select as u8));

        return Pwm::<TC0> { inner: self };
    }
}

// implementation for TC2 (different registers and types otherwise identical)...

Each (capable) pin then implements a trait, enabling it to (temporarily) access a passed PWM, and write to its OCR register.

Due to time constraints, I couldn't get around making these functions work in "safe" mode.
We cannot make pwm mutable, or use "safe" ways to modify the register, since pins 5 and 6 share the same pwm (which intern owns both ocr0a and ocr0b).

pub trait AnalogWrite<TC> {
    fn analog_write(&self, pwm: &Pwm<TC>, value: u8);
}

impl AnalogWrite<TC0> for Pin<Output, PD5> {
    fn analog_write(&self, pwm: &Pwm<TC0>, value: u8) {
        pwm.inner.ocr0b.write(|w| unsafe { w.bits(value) });
    }
}

impl AnalogWrite<TC0> for Pin<Output, PD6> {
    fn analog_write(&self, pwm: &Pwm<TC0>, value: u8) {
        pwm.inner.ocr0a.write(|w| unsafe { w.bits(value) });
    }
}

// implementation for Pins PD3 (pin3) and PB3 (pin11) using TC2...

The resulting workflow would look as follows. (Pretty close to what @jeroenvlek suggested).

let dp = Peripherals::take().unwrap();
let pins = pins!(dp);

let pwm = dp.TC0.into_pwm(CS0_A::PRESCALE_256);
let pin5 = pins.d5.into_output();
let pin6 = pins.d6.into_output();

pin5.analog_write(&pwm, 255);
pin6.analog_write(&pwm, 128);

Someone with more time and Rust+Arduino knowledge than me can probably figure out a way better solution to this problem.

Until then, I hope this helps at least someone :D

[jeroenvlek]

Oh, oops... Well, be careful with it, it is most likely very imcompatible with the new port.rs...

Ah, good to know. My assumption was that it was kept around as a starting point. I'll restart and see what needs to be kept there.

Hm, fundamentally this is configured in the timer instead of the GPIO port so this should rather go into the TimerOps or however we should call it. The only requirement for the pin is that it is configured as an output. From there, the timer can "override" the pin to put it into PWM mode.

Check. I was just following the old code and naively thought that the port should configure the timer. Looking at @SimonLeibfritz example it follows more a Scala-esque "pimp my library" pattern.

(as opposed to pins.d5.into_output().into_pwm_output(&mut timer0))

Not sure I understand the intention here? If you move the timer into the pin, you cannot re-use it for other pins anymore which means you can only use a subset of all PWM pins at the same time...

Simply meant as API ergonomics. The old code had the into_output().into_pwm_output() example. It would be more dev friendly to call into_pwm_output() directly on the pin.

[jeroenvlek]

I faced the exact same problem, so I made myself a quick workaround using the suggestions stated before (currently only works for 8 bit timers, i.e. TC0 and TC2 and lacks some customizability).

I started by creating a PWM wrapper. This could theoretically house more functions related to the PWM module.

The decision to make Pwm generic could be discussed. On the one hand, it enables generic handling by future functions, on the other hand it increases complexity without significantly decreasing code size.

pub struct Pwm<TC> {
    pub inner: TC,
}

pub trait IntoPwm<CSX, TC> {
    fn into_pwm(self, prescale: CSX) -> Pwm<TC>;
}

impl IntoPwm<CS0_A, TC0> for TC0 {
    fn into_pwm(self, clock_select: CS0_A) -> Pwm<TC0> {
        self.tccr0a.write(|w| w.wgm0().pwm_phase().com0a().match_clear().com0b().match_clear());
        self.tccr0b.write(|w| w.cs0().bits(clock_select as u8));

        return Pwm::<TC0> { inner: self };
    }
}

// implementation for TC2 (different registers and types otherwise identical)...

Each (capable) pin then implements a trait, enabling it to (temporarily) access a passed PWM, and write to its OCR register.

Due to time constraints, I couldn't get around making these functions work in "safe" mode. We cannot make pwm mutable, or use "safe" ways to modify the register, since pins 5 and 6 share the same pwm (which intern owns both ocr0a and ocr0b).

pub trait AnalogWrite<TC> {
    fn analog_write(&self, pwm: &Pwm<TC>, value: u8);
}

impl AnalogWrite<TC0> for Pin<Output, PD5> {
    fn analog_write(&self, pwm: &Pwm<TC0>, value: u8) {
        pwm.inner.ocr0b.write(|w| unsafe { w.bits(value) });
    }
}

impl AnalogWrite<TC0> for Pin<Output, PD6> {
    fn analog_write(&self, pwm: &Pwm<TC0>, value: u8) {
        pwm.inner.ocr0a.write(|w| unsafe { w.bits(value) });
    }
}

// implementation for Pins PD3 (pin3) and PB3 (pin11) using TC2...

The resulting workflow would look as follows. (Pretty close to what @jeroenvlek suggested).

let dp = Peripherals::take().unwrap();
let pins = pins!(dp);

let pwm = dp.TC0.into_pwm(CS0_A::PRESCALE_256);
let pin5 = pins.d5.into_output();
let pin6 = pins.d6.into_output();

pin5.analog_write(&pwm, 255);
pin6.analog_write(&pwm, 128);

Someone with more time and Rust+Arduino knowledge than me can probably figure out a way better solution to this problem.

Until then, I hope this helps at least someone :D

This is great. If we rename analog_write to into_pwm_output() we already have half the rent (paraphrasing a German proverb ;) ). Probably have to pour that into a macro and make sure that it only compiles for the correct pins.

My assumption about "roughly moving over the old implementation into the new one" was maybe a bit too optimistic :) I hope I can spend some evenings this week on this. Or are you actually preparing a PR with this code @SimonLeibfritz? Then I'm happy to leave it to you. My learning curve is still quite steep here.

[Dieschdel]

Nah @jeroenvlek I didn't yet prepare a PR or something along that line. I gladly help to expand this amazing project, but I literally started using this project 2 days ago (with some prior Rust knowledge though), so I have little to no experience on what would be a good solution. So go ahead if you have a good idea on how to integrate this :D.

Some issues I still see with the example as stated above:
My code example still misses a lot of features and feels kind of hacky in my opinion.
It does not properly integrate into the rest of the lib, and only works for 8bit (so far).
It also feels wrong to have a dedicated Pwm struct (or package) without allowing for other usages of the hardware PWM (e.g., switch between Fast and phase corrected mode, etc.).

I'm furthermore not really happy with the unsafe nature of analog_write. Not only does it not block/consume the pin in question (even though the PWM overwrites every state assigned to the pin), but it does not even require mutability for pwm.
I recall that similar problems are normally solved using a split like method (e.g., tokio io split splitting read and write into separate objects.)
This function would consume the original object (and both pins), in favor of two separate independent objects, each containing access to one (instead of both) channels. The resulting workflow would look somewhat like this.

let dp = Peripherals::take().unwrap();
let pins = pins!(dp);

let pwm = dp.TC0.into_pwm(CS0_A::PRESCALE_256);
let (pin5, pin6) = pwm.into_split(pins.d5, pins.d6)

pin5.analog_write(255);
pin6.analog_write(128);

I tried to do this in the first place, but quickly realized, that splitting any TC is not possible, since itself is part of the Peripherals object (and thus making destructing/moving impossible). We would probably have to refactor parts of the original object to achieve this workflow.

Another option, I thought about, is to call analog_write via pwm without the pin context. I just really dislike that syntax :(.

let dp = Peripherals::take().unwrap();
let pins = pins!(dp);

let pwm = dp.TC0.into_pwm(CS0_A::PRESCALE_256);

// enums should be different for each pwm
pwm.analog_write(Pwm0Pins::PIN5, 255);

[jeroenvlek]

Well, I'm first just going to create an example using Simon's code here, because also simultaneously wiring everything up at the avr-hal-generic, atmega-hal and arduino-hal levels through macros is too complex to get anything started.

It also feels wrong to have a dedicated Pwm struct (or package) without allowing for other usages of the hardware PWM (e.g., switch between Fast and phase corrected mode, etc.).

True, that's exactly why it wasn't implemented yet, as @Rahix mentioned higher up this thread, but now we opted for a simple pwm module first. Once we have the right abstractions in place adding the other modes should be straightforward.

On a side note: What editor are you all using? Vscode + rust-analyzer does not like this project, and that doesn't help gaining an overview.. (I've got nightly-2021-01-07 in my toolchain)

PS: There's also a Pwm trait in embedded-hal is compatibility with that desired?
PPS: What is the "Fully Qualified Syntax" of Pins::PD5 and Pins::PD6? :)

[Rahix]

On a side note: What editor are you all using? Vscode + rust-analyzer does not like this project, and that doesn't help gaining an overview.. (I've got nightly-2021-01-07 in my toolchain)

Neovim. rust-analyzer seems to work on this code for me.

PS: There's also a Pwm trait in embedded-hal is compatibility with that desired?

For now I wouldn't bother. This code is subject to change in embedded-hal at the moment anyway so let's wait until the dust has settled over there before worrying here.

PPS: What is the "Fully Qualified Syntax" of Pins::PD5 and Pins::PD6? :)

I'm not sure? I suppose this was from an error message? Can you maybe show it in full so I can take a look?

[jeroenvlek]

Too early for a PR so I'll just drop it here. Once this compiles I'll add the implementations for D9, D10, D11 based on the old pwm.rs from the atmega-hal. I do get the necessity for macros here and different arms that implement different Prescalers, that will be the step after getting it working. Going bottom-up this time.

/*!
 * Pulse Width Modulation on pins D9, D10, D11 according to project 4 "Color Mixing Lamp" 
 * in the Arduino Uno book. See page 54 for the circuit.
 */
#![no_std]
#![no_main]

use panic_halt as _;
use arduino_hal::pac::TC0;
use arduino_hal::port::Pins;
use arduino_hal::port::Pin;
use arduino_hal::port::mode;
use avr_device::atmega328p::tc0::tccr0b::CS0_A;

pub struct Pwm<TC> {
    pub inner: TC,
}

pub trait IntoPwm<CSX, TC> {
    fn into_pwm(self, prescale: CSX) -> Pwm<TC>;
}

impl IntoPwm<CS0_A, TC0> for TC0 {
    fn into_pwm(self, clock_select: CS0_A) -> Pwm<TC0> {
        self.tccr0a.write(|w| w.wgm0().pwm_phase().com0a().match_clear().com0b().match_clear());
        self.tccr0b.write(|w| w.cs0().bits(clock_select as u8));

        return Pwm::<TC0> { inner: self };
    }
}

pub trait PwmWriteOps<TC> {
    fn pwm_write(&self, pwm: &Pwm<TC>, value: u8);
}

impl PwmWriteOps<TC0> for Pin<mode::Output, Pins::PD5> {
    fn pwm_write(&self, pwm: &Pwm<TC0>, value: u8) {
        pwm.inner.ocr0b.write(|w| unsafe { w.bits(value) });
    }
}

impl PwmWriteOps<TC0> for Pin<mode::Output, Pins::PD6> {
    fn pwm_write(&self, pwm: &Pwm<TC0>, value: u8) {
        pwm.inner.ocr0a.write(|w| unsafe { w.bits(value) });
    }
}

#[arduino_hal::entry]
fn main() -> ! {
    let dp = arduino_hal::Peripherals::take().unwrap();
    let pins = arduino_hal::pins!(dp);

    let mut green_output = pins.d9.into_output();
    let mut red_output = pins.d10.into_output();
    let mut blue_output = pins.d11.into_output();
    

    loop {
        
        arduino_hal::delay_ms(800);
    }
}

And the error:

[jvlek@rick-sanchez arduino-uno]$ cargo build --bin uno-pwm-color-mixing
   Compiling arduino-uno-examples v0.0.0 (/home/jvlek/dev/embedded_rust/avr-hal/examples/arduino-uno)
error[E0223]: ambiguous associated type
  --> examples/arduino-uno/src/bin/uno-pwm-color-mixing.rs:36:45
   |
36 | impl PwmWriteOps<TC0> for Pin<mode::Output, Pins::PD5> {
   |                                             ^^^^^^^^^ help: use fully-qualified syntax: `<arduino_hal::Pins as Trait>::PD5`

error[E0223]: ambiguous associated type
  --> examples/arduino-uno/src/bin/uno-pwm-color-mixing.rs:42:45
   |
42 | impl PwmWriteOps<TC0> for Pin<mode::Output, Pins::PD6> {
   |                                             ^^^^^^^^^ help: use fully-qualified syntax: `<arduino_hal::Pins as Trait>::PD6`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0223`.
error: could not compile `arduino-uno-examples`

To learn more, run the command again with --verbose.

I haven't gotten to the Advanced Traits topic in the Rust book yet :)

[Rahix]

Oh, I see. Well first of all your code needs be something like

use arduino_hal::hal::port as hal_port;

impl PwmWriteOps<TC0> for Pin<mode::Output, hal_port::PD5> { }

But that isn't how it is supposed to be. It looks like we are missing re-exports of all the pin types in arduino-hal. Here is the port module from arduino-hal (= arduino_hal::port, with the pin types missing): https://rahix.github.io/avr-hal/arduino_hal/port/index.html and here is the port module from the atmega-hal (= arduino_hal::hal::port): https://rahix.github.io/avr-hal/atmega_hal/port/index.html

This needs to be fixed as well...