Rahix / avr-hal

embedded-hal abstractions for AVR microcontrollers
Apache License 2.0
1.32k stars 223 forks source link

Weird issue with two u8 ranged loops #569

Closed burumdev closed 3 months ago

burumdev commented 3 months ago

I wrote this program to control an RGB LED module and it should more or less seamlessly cycle around different colors. I used simple PWM for this. The platform I use is Arduino UNO with atmega328pb chip. And the PWM setup is:

    let timer0 = Timer0Pwm::new(dp.TC0, Prescaler::Prescale64);
    let timer2 = Timer2Pwm::new(dp.TC2, Prescaler::Prescale64);

    let mut led_rgb = LedRGB {
        red_output: pins.d11.into_output().into_pwm(&timer2),
        green_output: pins.d6.into_output().into_pwm(&timer0),
        blue_output: pins.d5.into_output().into_pwm(&timer0),
    };

Problem is after the first loop that represents a reversed ramp it gets stuck on the second loop that is the ascending ramp. I can't figure out what's wrong with it:

    let delay_ms = 10;

    loop {
        for x in (0..=255).rev() {
            led_rgb.red_output.set_duty(x);
            led_rgb.green_output.set_duty(255 - x);
            let blue: u8 = if x >= 128 { 128 - (255 - x) } else { 127 - x };
            led_rgb.blue_output.set_duty(blue);

            ufmt::uwriteln!(&mut serial, "LOOP 1: RED: {}, GREEN: {}, BLUE: {}", x, 255 - x, blue).unwrap_infallible();

            arduino_hal::delay_ms(delay_ms);
        }

        for y in 0..=255 {
            led_rgb.red_output.set_duty(y);
            led_rgb.green_output.set_duty(255 - y);
            let blue: u8 = if y >= 128 { 128 - (255 - y) } else { 127 - y };
            led_rgb.blue_output.set_duty(blue);

            ufmt::uwriteln!(&mut serial, "LOOP 2: RED: {}, GREEN: {}, BLUE: {}", y, 255 - y, blue).unwrap_infallible();

            arduino_hal::delay_ms(delay_ms);
        }
    }

When I follow the console output, I see the first loop and the values are correct, then it jumps to second loop and values are again correct, but the program gets stuck on the second loop repeating it again and again forever.

When I remove the = sign from the second range loop ie:

for y in 0..255 instead of for y in 0..=255

It works as expected with alternating pattern of loops, but the ramp doesn't reach 255 value and the result is one value less than required.

When I test the code with 0..=255 on regular PC Rust on Linux I get the alternating pattern of loops working as expected.

CoolSlimbo commented 3 months ago

Hey, just produced a minimum version of what you provided, can you attempt to run this (tested on an Arduino UNO)

#![no_std]
#![no_main]

use arduino_hal::prelude::*;
use panic_halt as _;

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

    loop {
        for x in (0..=255).rev() {
            ufmt::uwriteln!(&mut serial, "L1 {}", x).unwrap_infallible();
            arduino_hal::delay_ms(10);
        }
        for x in 0..=255 {
            ufmt::uwriteln!(&mut serial, "L2 {}", x).unwrap_infallible();
            arduino_hal::delay_ms(10);
        }
    }
}

(Created from a new project generated with cargo generate --git https://github.com/Rahix/avr-hal-template.git)

burumdev commented 3 months ago

Hi @CoolSlimbo .

Good idea. I tried commenting out everything as you suggested and it started working fine. But as soon as I activate the line that sets red LED's duty: led_rgb.red_output.set_duty(x); it starts to get stuck in the second loop again.

Then I comment the red LED line and activate green and blue LED lines, then everything is fine. Loops are alternating.

One difference between red LED and the others is that red uses Timer2Pwm and others use Timer0Pwm.

I also tried to use the other pin d3 or PD3 that Timer2Pwm uses and result is the same. Loop gets stuck with Timer2Pwm and set_duty().

One thing I should also add is the red LED set_duty line in the first loop doesn't cause any problems. If I comment out the second and the first remains, then the loops still alternate as expected.

For anyone interested I post the complete code that triggers the issue down below:

#![no_std]
#![no_main]

use arduino_hal::prelude::_unwrap_infallible_UnwrapInfallible;
use arduino_hal::hal::port::{ PB3, PD5, PD6 };
use arduino_hal::simple_pwm::{ Timer0Pwm, Timer2Pwm, Prescaler, IntoPwmPin };
use arduino_hal::port::{ Pin, mode };

use panic_halt as _;

struct LedRGB {
    red_output: Pin<mode::PwmOutput<Timer2Pwm>, PB3>,
    green_output: Pin<mode::PwmOutput<Timer0Pwm>, PD6>,
    blue_output: Pin<mode::PwmOutput<Timer0Pwm>, PD5>,
}

#[arduino_hal::entry]
fn main() -> ! {
    let dp = arduino_hal::Peripherals::take().unwrap();
    let pins = arduino_hal::pins!(dp);
    let timer0 = Timer0Pwm::new(dp.TC0, Prescaler::Prescale64);
    let timer2 = Timer2Pwm::new(dp.TC2, Prescaler::Prescale64);

    let mut serial = arduino_hal::default_serial!(dp, pins, 57600);

    let mut led_rgb = LedRGB {
        red_output: pins.d11.into_output().into_pwm(&timer2),
        green_output: pins.d6.into_output().into_pwm(&timer0),
        blue_output: pins.d5.into_output().into_pwm(&timer0),
    };

    led_rgb.red_output.enable();
    led_rgb.green_output.enable();
    led_rgb.blue_output.enable();

    let delay_ms = 10;

    loop {
        for x in (0..=255).rev() {
            led_rgb.red_output.set_duty(x);
            led_rgb.green_output.set_duty(255 - x);
            let blue: u8 = if x >= 128 { 128 - (255 - x) } else { 127 - x };
            led_rgb.blue_output.set_duty(blue);

            ufmt::uwriteln!(&mut serial, "LOOP 1: RED: {}, GREEN: {}, BLUE: {}", x, 255 - x, blue).unwrap_infallible();

            arduino_hal::delay_ms(delay_ms);
        }

        // This loop gets stuck and loops forever, but the first loop and this one should alternate
        for y in 0..=255 {
            // Commenting out below line will make the code work as expected.
            led_rgb.red_output.set_duty(y);
            // LEDs connected to Timer0Pwm work fine. Loops will alternate when I use them
            led_rgb.green_output.set_duty(255 - y);
            let blue: u8 = if y >= 128 { 128 - (255 - y) } else { 127 - y };
            led_rgb.blue_output.set_duty(blue);

            ufmt::uwriteln!(&mut serial, "LOOP 2: RED: {}, GREEN: {}, BLUE: {}", y, 255 - y, blue).unwrap_infallible();

            arduino_hal::delay_ms(delay_ms);
        }
    }
}
CoolSlimbo commented 3 months ago

Have you tried with a different timer to Timer2?

burumdev commented 3 months ago

Trying with Timer1Pwm it has the same bug. Tried changing prescaler values to 8, 256, 1024 etc. no effect there either.

burumdev commented 3 months ago

Though I tried one more thing and got interesting result:

        for y in 0..=255 {
            //led_rgb.red_output.set_duty(y);
            // LEDs connected to Timer0Pwm work fine. Loops will alternate when I use them
            led_rgb.green_output.set_duty(y);
            let blue: u8 = if y >= 128 { 128 - (255 - y) } else { 127 - y };
            led_rgb.blue_output.set_duty(blue);

            ufmt::uwriteln!(&mut serial, "LOOP 2: RED: {}, GREEN: {}, BLUE: {}", y, 255 - y, blue).unwrap_infallible();

            arduino_hal::delay_ms(delay_ms);
        }

led_rgb.green_output.set_duty(y); I changed this line to set the duty cycle directly to y counter value on green_output which uses Timer0Pwm and the bug appeared again.

So at this point setting the duty cycle to 255 directly after an ascending ramp of values triggers the bug, regardless of the PWM source for that pin. Very strange.

CoolSlimbo commented 3 months ago

That is. I'll have to dig around to find my RGB LED, and I'm about to help with another issue. I'll take your last full posted example and see what I can make of it on my Arduino and LED.

Also, for future code snippets, please add rs after the backticks to highlight it in Rust syntax. (I.e. ```rs)

burumdev commented 3 months ago

Then what I did shocked the civilized world:

            led_rgb.green_output.set_duty(255 - y);
            let blue: u8 = if y >= 128 { 128 - (255 - y) } else { 127 - y };
            led_rgb.blue_output.set_duty(blue);
            led_rgb.red_output.set_duty(y);

I restored the code to it's original bugging version, and simply moved led_rgb.red_output.set_duty(y); to the end of the lines that set the RGB led duty cycles and VOILA! It started to work as expected! Hope this helps find a solution. It looks like a compiler bug creating bad machine code to me.

Thank you for looking into this. Cheers!

CoolSlimbo commented 3 months ago

That's what it could be.

I guess I might fill my night with some figuring out a) what tf AVR assembly looks like, and b) figuring out how it results.

Fell free to close the issue, but I'll post updates on what I find.

CoolSlimbo commented 3 months ago

So, I attempted to run your code provided here, however, mine seemed to have run fine, without any issue, looping back and fourth between the two loops successfully.

So, to determine the issue that may be present, can I ask what toolchain version you are running on, and what commit your arduino_hal is pinned to, in your Cargo.toml?

burumdev commented 3 months ago

Sure. Here it is:

[dependencies.arduino-hal]
git = "https://github.com/rahix/avr-hal"
rev = "3e362624547462928a219c40f9ea8e3a64f21e5f"
features = ["arduino-uno"]

And rust-toolchain.toml content is:

[toolchain]
channel = "nightly-2024-03-22"
components = ["rust-src"]
profile = "minimal"
CoolSlimbo commented 3 months ago

Okay... very confused now.

The code example you said wasn't working, works fine for me on my UNO, and we have matching HAL's and toolchains...

This could've just been a single time fluke of some form with your compiler being special. Since I can't reproduce on a fresh install, and you've resolved it yourself, I don't see a reason as to why we have to keep this open.

If anyone has this issue in the future, and comes to this issue, feel free to open it up again.

(Free to close it now buru)

burumdev commented 3 months ago

Strange indeed. I also tried the same code with an original UNO board and result is the same. I'll close this issue now as it seems related to my GCC and GCC-AVR setup.

My GCC version is 10.2.1 and GCC-AVR is 5.4.0+Atmel3.6.2 on a Debian 11 system. So if anyone else bumps into this bug these might help. Finally can you post the versions of your GCC and GCC-AVR packages? Thanks a lot.

CoolSlimbo commented 3 months ago

Rocking avr-gcc 14.1.0.

So could be a difference between platforms building on, or our avr-gcc versions, as it seems.

burumdev commented 3 months ago

I finally updated my Debian system from version 11 to 12. Which comes with:

GCC version 12.2 avr-gcc version 5.4.0+Atmel3.6.2 (Same as debian 11 version)

This update totally resolved the issue. So the bug was probably related to GCC version. If anybody comes across this behavior of loops not conforming, I recommend updating the development environment.

Thanks.