Skip to content

Conversation

@koutoftimer
Copy link

@koutoftimer koutoftimer commented Feb 10, 2021

32u4 require closer look. Not sure what to do with10 bit counter. Other chips verified with datasheets, at least timer1 on 328p works like a charm with 16 bit counter.

related to #134

Note: 32u4 require closer look. Not sure what to do with
10 bit counter.
@Rahix
Copy link
Owner

Rahix commented Feb 10, 2021

As I wrote in the other issue, this goes against what the TimerPwm abstraction is currently trying to archive: It is supposed to provide a unified interface for using any compatble pin for PWM without caring too much about the details.

That is the reason why all timers are configured for a MAX of 255. This way, no matter what pin you're using, the set_duty() method always gets a value from 0 to 255 representing 0 to full duty cycle.

If we were to change this as you propose here, downstream users would need to know what timer they are using for a specific pin and whether this timer is 8 or 16 (or 10) bits wide. I think this is not acceptable for an abstraction which tries to hide exactly those details and allow homogeneously treating all PWM pins the same.

As I mentioned over in #132, the way forwards is a separate abstraction (or even multiple) for the different applications of the timer peripherals. Also ref. #44.


at least timer1 on 328p works like a charm with 16 bit counter.

Are you sure? The timer is still configured for mode 5 (= "Fast PWM, 8-bit") and will wrap at 0xFF:

tim.tccr1a.modify(|_, w| w.wgm1().bits(0b01));
tim.tccr1b.modify(|_, w| {
w.wgm1().bits(0b01);

Thus, calling set_duty() with a value above 255 will just lead to saturating behavior.

@Rahix Rahix added hal-generic Related to MCU generic parts of avr-hal hal-impl labels Feb 10, 2021
@koutoftimer
Copy link
Author

koutoftimer commented Feb 10, 2021

@Rahix Code I was using

#![feature(llvm_asm)]

#![no_std]
#![no_main]
#![feature(abi_avr_interrupt)]

use arduino_uno::{
    delay_ms, prelude::*,
};
use panic_halt as _;

const MAX_CYCLES: u16 = 40_000; // 50 Hz

// SG 90 servo -90..90 angle mapping to 50..250 byte values
fn duty(value: u8) -> u16 {
    20 * value as u16
}

#[arduino_uno::entry]
fn main() -> ! {
    let pd = arduino_uno::Peripherals::take().unwrap();
    let mut pins = arduino_uno::Pins::new(pd.PORTB, pd.PORTC, pd.PORTD);
    let mut serial = arduino_uno::Serial::new(
        pd.USART0, pins.d0, pins.d1.into_output(&mut pins.ddr),
        9600.into_baudrate(),
    );
    let mut timer1 = arduino_uno::pwm::Timer1Pwm::new(
        pd.TC1, arduino_uno::pwm::Prescaler::Prescale8);
    // FIXME: can't export `avr_device::interrrupt::free`
    {
        let timer = unsafe { &*arduino_uno::pac::TC1::ptr() };
        timer.tccr1a.modify(|_, w| w.wgm1().bits(0b10));
        timer.tccr1b.modify(|_, w| w.wgm1().bits(0b11));
        timer.icr1.modify(|_, w| unsafe { w.bits(MAX_CYCLES) });
    }

    let mut servo = pins.d9.into_output(&mut pins.ddr).into_pwm(&mut timer1);
    servo.set_duty(duty(150));
    servo.enable();

    ufmt::uwriteln!(&mut serial, "Hello from Arduino!\r").void_unwrap();

    loop {
        let b: u8 = (serial.read_byte() - '0' as u8) * 100 +
                (serial.read_byte() - '0' as u8) * 10 +
                (serial.read_byte() - '0' as u8);
        servo.set_duty(duty(b));
        ufmt::uwriteln!(&mut serial, "duty set to {}\r", b).void_unwrap();
    }
}

Moving discussion to appropriate issues.

@Rahix Rahix changed the base branch from main to old July 11, 2021 09:46
@Rahix Rahix added old This issue/pr targets the old version of avr-hal and removed hal-impl labels Jul 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hal-generic Related to MCU generic parts of avr-hal old This issue/pr targets the old version of avr-hal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants