phip1611 / ws2818-rgb-led-spi-driver

Simple, educational driver for WS28xx RGB LED (chains) written in Rust. It uses SPI device for timing. Needs Linux and works definitely on Raspberry Pi.
MIT License
19 stars 6 forks source link

Possible to remove CPU-intensive spinlock? #9

Closed stonewareslord closed 2 years ago

stonewareslord commented 2 years ago

The spinlock in sleep_busy_waiting_ms takes 100% CPU power from one core of the pi. I am trying to replace it with something more efficient, with no success.

I tried replacing it with thread::sleep, but all the LEDs turn white, and I cannot figure out why that might be. The only difference I know of for these two methods is that the spinlock has ~2microsecond overhead while thread::sleep has ~100microsecond overhead. Full test commit: https://github.com/stonewareslord/ws2818-rgb-led-spi-driver/commit/f69279b76c670275762ab49d7235d43c93819501; summary of change:

diff --git a/examples/src/bin/moving-pixel.rs b/examples/src/bin/moving-pixel.rs
--- a/examples/src/bin/moving-pixel.rs
+++ b/examples/src/bin/moving-pixel.rs
@@ -31,6 +34,28 @@ fn main() {
         adapter.write_encoded_rgb(&data).unwrap();

         i = (i + 1) % num_leds;
-        sleep_busy_waiting_ms(1000 / 10); // 100ms / 10Hz
+        thread::sleep(Duration::from_millis(ms));
     }
 }

I could maybe get that we need to fill the SPI buf at a consistent rate, but adding randomness to the sleep time has no effect (spinlock still works, thread::sleep still doesn't). Full test commit: https://github.com/stonewareslord/ws2818-rgb-led-spi-driver/commit/b12346ef4e131dca38ac64d693bff6e81ca739a8; summary of change:

diff --git a/examples/src/bin/moving-pixel.rs b/examples/src/bin/moving-pixel.rs
index e39faed..4d4447a 100644
--- a/examples/src/bin/moving-pixel.rs
+++ b/examples/src/bin/moving-pixel.rs
@@ -34,7 +36,7 @@ fn main() {
         adapter.write_encoded_rgb(&data).unwrap();

         i = (i + 1) % num_leds;
-        sleep_busy_waiting_ms(1000 / 10); // 100ms / 10Hz
+        sleep_busy_waiting_ms(1000 / 10) + rng.gen_range(0, 10); // 100ms / 10Hz
     }
 }

Is there some kind of timing issue I am not aware of here? I would expect the loop time after write_encoded_rbg should not matter and the only side effects of both of these functions is the sleep time.

P.S. Thank you for the amazing library!

phip1611 commented 2 years ago

Hi @stonewareslord well, good question. At this point, after adapter.write_encoded_rgb(&data).unwrap();, it should be indeed irrelevant, what kind of sleep/wait-function is used. Maybe I have later time to look into this.

stonewareslord commented 2 years ago

Thank you. You can just clone my repo and run cargo run --bin moving-pixel to test.

Maybe this is just some weird issue with my pi? I tried combining the two -- using thread::sleep for half the time and spinlock for the other half, and the lights break if I sleep for 50ms or more, but not <= 40ms. I didn't commit this, but if you wanted to try that, here's the diff:

diff --git a/examples/src/bin/moving-pixel.rs b/examples/src/bin/moving-pixel.rs
index 4d4447a..7cb5a2c 100644
--- a/examples/src/bin/moving-pixel.rs
+++ b/examples/src/bin/moving-pixel.rs
@@ -38,19 +38,18 @@ fn main() {
         i = (i + 1) % num_leds;
         let ms = 1000 / 10 + rng.gen_range(0, 10); // 100ms / 10Hz
         let before = Instant::now(); // For printing time this takes
+        let target_time = Instant::now().add(Duration::from_millis(ms));

         // Using thread::sleep() - DOES NOT WORK - lights turn solid white
-        thread::sleep(Duration::from_millis(ms));
+        // Works when duration <= 40ms; breaks when duration >= 50ms
+        thread::sleep(Duration::from_millis(40));

         // Original code - WORKS FINE
-        /*
-        let target_time = Instant::now().add(Duration::from_millis(ms));
         loop {
             if Instant::now() >= target_time {
                 break;
             }
         }
-        */

         let after = Instant::now();
phip1611 commented 2 years ago

I hope I can look into this next week - sorry for the delay

stonewareslord commented 2 years ago

Thank you for looking at it if you have time!

For anyone else having this issue, for now you can use threads to spinlock in the background:

let (tx, rx) = channel::<String>();

thread::spawn(move || {
  loop {
    if let Ok(message) = rx.try_recv() {
      // Do something
    }

    // Working code with spinlock
  }
});

tx.send("green")?;

Still has 100% CPU core load, of course.

On 07/25/ , Philipp Schuster wrote:

I hope I can look into this next week - sorry for the delay

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/phip1611/ws2818-rgb-led-spi-driver/issues/9#issuecomment-886247597

phip1611 commented 2 years ago

I finally found some time to investigate this. I can reproduce it

With thread::sleep OS gives control to another thread temporarily. I guess in this time, the SPI devices does odd stuff. I try to find a way to investigate.. if it is a linux bug or a bug in the spidev crate or whatever..

Type1J commented 2 years ago

This isn't a Linux bug. It's the SPI clock varying with the CPU. Try this: If you're on an RPi3 add to /boot/config.txt:

core_freq=250

If you're on an RPi4 add to /boot/config.txt:

core_freq=500
core_freq_min=500

Reboot, and you should be able to use std::thread::sleep() without any LEDs turning white.

Type1J commented 2 years ago

I think that this is actually GPU speed, but allowing it to vary breaks several serial interfaces when not using a clock line, including SPI. The Raspberry Pi docs say to use these paramters to be able to use the serial interfaces.

phip1611 commented 2 years ago

I didn't really fix something here but added a note to README and the code https://github.com/phip1611/ws2818-rgb-led-spi-driver/commit/012db26780f9c5437f0300f5fe77904cb7a8f0dc