Non blocking smart led

[robamu]

No description provided.

[tschundler]

I finally got around to some testing tonight. It only seems to only work with 2 of 6 ws28xx variants.

On my esp32, I measure about 25us between pixels and about 55us total.

From some research like https://wp.josh.com/2014/05/13/ws2812-neopixels-are-not-so-finicky-once-you-get-to-know-them/amp/ and the various datasheets, depending on the chip, the last time might be ~6us, ~50us, or ~250us

on the other 4, only one LED lights. So if this is accepted, it needs to be accepted with the caveat that it depends heavily on which LED are used Incidentally among those not working are what I'm pretty sure are genuine WorldSemi LEDs.

But... exciting news, I am able to drive two sets of the type that work in parallel using a join!

(don't mind the bad grounding causing ghost signals)

But... here, the time between pixels is closer to 34us

I should go back to exploring double buffering in RMT async writes. Supposedly there's an ability now to force code into RAM to run faster?

[tschundler]

one more very odd thing

with a buffer size of 65, I can only send 62 pixels of data. Otherwise I get BufferSizeExceeded. To send 64, I need to make a 67 pixel buffer.

#[main]
async fn main(spawner: Spawner) {
    let peripherals = esp_hal::init(esp_hal::Config::default());
    esp_println::logger::init_logger_from_env();

    esp_println::println!("Init!");

    let timg0 = TimerGroup::new(peripherals.TIMG0);
    esp_hal_embassy::init(timg0.timer0);

    let io = Io::new(peripherals.GPIO, peripherals.IO_MUX);

    let led_pin = io.pins.gpio18;
    let l2_pin = io.pins.gpio23;

    let rmt = Rmt::new_async(peripherals.RMT, 80.MHz()).unwrap();

    let rmt_buffer = smartLedBuffer!(67);
    let rmt2_buffer = smartLedBuffer!(67);

    let mut l1 = asynch::SmartLedAdapterAsync::new(rmt.channel0, led_pin, rmt_buffer);
    let mut l2 = asynch::SmartLedAdapterAsync::new(rmt.channel1, l2_pin, rmt2_buffer);

    esp_println::println!("Start!");

    let mut ofs = 0;
    loop {
        let data = {
            let mut i = 0;
            [(); 64].map(|_| {
                i += 1;
                hsv2rgb(Hsv {
                    hue: (((i * 4) + ofs + 60) % 255) as u8,
                    sat: 240,
                    val: 10,
                })
            })
        };

        let f1 = l1.write(data[0..64].iter().cloned());
        let f2 = l2.write(data[0..64].iter().cloned());
        //let (o1, o2) = embassy_futures::join::join(f1, f2).await;
        let o1 = f1.await;
        let o2 = f2.await;
        o1.unwrap_or_else(|e| {
            esp_println::println!("Error1: {:?}", e);
        });
        o2.unwrap_or_else(|e| {
            esp_println::println!("Error2: {:?}", e);
        });

        ofs = (ofs + 1) % 255;
        esp_println::print!(".");
        Timer::after(Duration::from_millis(10)).await;
    }
}

changing the above to SmartLedBuffer!(64); will return errors when running.

[lulingar]

Any reason why to exclude esp-hal 0.21.1?

[robamu]

hmm, no. I just wonder whether <=0.21 or <=0.21.* would be cleaner/better (or even does the job)

[robamu]

https://docs.rs/semver/latest/semver/enum.Op.html The first way works :)

[lulingar]

Heh I see! It seems a bit misleading before a good Samaritan points you to the SemVer docs 😉

[lulingar]

Oh it seems something is missing, this bit me:

error: failed to select a version for `esp-hal`.
    ... required by package `esp-hal-smartled v0.13.0`
    ... which satisfies dependency `esp-hal-smartled = "^0.13.0"` of package `test-xtensa-nostd v0.1.0 (/home/being/code/practice/rust/test-xtensa-nostd)`
versions that meet the requirements `^0.20.0` are: 0.20.1

the package `esp-hal` links to the native library `esp-hal`, but it conflicts with a previous package which links to `esp-hal` as well:
package `esp-hal v0.21.1`
    ... which satisfies dependency `esp-hal = "^0.21.1"` of package 

My relevant lines on Cargo.toml are:

esp-hal = { version = "0.21.1", features = ["esp32s3"] }
esp-hal-smartled = { version = "0.13.0", features = ["esp32s3"] }

[robamu]

I just fixed the dependency specifier to also allow higher versions.. Can you try 0.14? I also bumped the minor version of the crate itself.

[lulingar]

It worked fine! I had to set in Cargo.toml:

esp-hal-smartled = { git = "https://github.com/robamu/esp-hal-community.git", rev = "3562306", features = ["esp32s3"] }

[robamu]

On the second problem: the async API requires a different and slightly larger buffer size because each transfer must be 0 terminated. I have not created an async macro to declare that buffer yet, but i provided a const function which calculates the required buffer size.

It might be a reasonable idea to also add an async example?

[horho77]

It might be a reasonable idea to also add an async example?

async example sounds like a great idea 👍

[robamu]

I can try to add an example which is similar to the existing one, except it uses the async API.

[lulingar]

I can try to add an example which is similar to the existing one, except it uses the async API.

Here's mine:

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

use core::{ptr::addr_of_mut, str::{self, FromStr}};

use alloc::string::String;

extern crate alloc;
use esp_alloc as _;

use esp_backtrace as _;
use esp_hal::{
    cpu_control::{CpuControl, Stack as CPUStack},
    gpio::{AnyPin, Input, Io, Level, Output, Pull},
    prelude::*,
    rmt::Rmt,
    timer::{
        timg::TimerGroup, AnyTimer
    },
    Async,
};

use esp_hal_embassy::Executor;
use embassy_executor::Spawner;
use embassy_time::{Delay, Duration, Instant, Timer};

use esp_println::println;

use esp_hal_smartled::{
    smart_led_buffer,
    asynch::SmartLedAdapterAsync,
};
use smart_leds::{
    brightness, gamma,
    hsv::{hsv2rgb, Hsv},
};


#[main]
async fn main(spawner: Spawner) -> ! {
    esp_println::logger::init_logger_from_env();

    let peripherals = esp_hal::init({
        let mut config = esp_hal::Config::default();
        config.cpu_clock = CpuClock::Clock240MHz;
        config
    });

    let timg0 = TimerGroup::new(peripherals.TIMG0);
    let timer1: AnyTimer = timg0.timer1.into();
    esp_hal_embassy::init(timer1);

    let mut cpu_control = CpuControl::new(peripherals.CPU_CTRL);
    let io = Io::new(peripherals.GPIO, peripherals.IO_MUX);

    let led_pin = io.pins.gpio48;
    let freq = 80.MHz();
    let rmt = Rmt::new_async(peripherals.RMT, freq).unwrap();
    let rmt_buffer = smart_led_buffer!(1);
    let led = SmartLedAdapterAsync::new(rmt.channel0, led_pin, rmt_buffer);

    let mut color = Hsv {
        hue: 0,
        sat: 255,
        val: 255,
    };
    let mut data;

    loop {
        for hue in 0..=255 {
            color.hue = hue;
            // Convert from the HSV color space (where we can easily transition from one
            // color to the other) to the RGB color space that we can then send to the LED
            data = [hsv2rgb(color)];
            // When sending to the LED, we do a gamma correction first (see smart_leds
            // documentation for details) and then limit the brightness to 10 out of 255 so
            // that the output it's not too bright.
            if let Err(e) = led.write(brightness(gamma(data.iter().cloned()), 20)).await {
                log::error!("Driving LED: {:?}", e);
            }
            Timer::after(Duration::from_millis(10)).await;
        }
    }
}

[Be-ing]

smart-leds-trait just added an async version of the trait, though it hasn't been released yet:

smart-leds-rs/smart-leds-trait#14
smart-leds-rs/smart-leds-trait#15

[robamu]

looks interesting. I'll rebase this PR :)

[robamu]

What do you think about removing the asynch module and simply keeping the *Async variants in the same module?

[Be-ing]

smart-leds-trait 0.3.1 has been released with the SmartLedsWriteAsync trait.

[robamu]

I need to rebase this anyway.. I can look whether I can already integrate this.

[robamu]

I now implemented the new SmartLedsWriteAsync trait for the Async structure. I also removed the asynch submodule, and new new API is now placed in the root. This is more in line how it is commonly done for other asynch APIs.

[MabezDev]

I don't think this needs a _async postfix?

[MabezDev]

Actually do we need this it all, it's a duplicate of the other function isn't it?

[robamu]

yes, if there is no submodule, it needs a distinct name.

[MabezDev]

It's a shame we have to duplicate this, I think if Rmt in esp-hal was updated to use the Async/Blocking mode parameters, we could have one smartled driver which could work in blocking or async mode.

[MabezDev]

If we're not going to change the Rmt peripheral, I suggest that we try and share as much code between the two async and blocking adapters as possible

[robamu]

Yes, it is a bit of duplication. That's why I tried to extract as much common logic as possible into free functions or re-usable functions. I think the subtle differences of those structs made it difficult to unify them. Having two distincts structs makes it explicit on type level which one is used though. It might also be that the flexibility of having two structs makes it easier to accomodate (external) changes in the future

[Be-ing]

You may be able to get rid of the code duplication using the bisync crate. I just did this for the embedded-sdmmc crate and I'm quite happy with how the code turned out using bisync.

[robamu]

I also added an asynchronous example now. Haven´t tested it on hardware yet though. It is really similar to the hello_rgb example.

[robamu]

I am currently updating my small lightstrip project, so I hope I can test this again soon :)

[kaspar030]

We used this on an Ulanzi Pixel Clock with a lot of (attention gathering) success on Chemitzer Linux Tage. 🙂
It seems rock solid.

[robamu]

that's nice, so this was verified again :) needs to be rebased again though

[BlinkyStitt]

This looks like exactly what I need for my current project. Anything I can help with to get this merged in?

[robamu]

I updated this to the most recent esp-hal.. maybe we should wait until the v1 release? there were differences between the latest beta version and the upstream again. I don't think it works in its current form due to trait bound issues on the pin.

[robamu]

I will update this, just waiting for the update for the project files, it is kind of painful to work without them

[robamu]

I wonder what this is, trying to compile the async example:

error: linking with `rust-lld` failed: exit status: 1
  |
  = note:  "rust-lld" "-flavor" "gnu" "/tmp/rustcTA5e0N/symbols.o" "<59 object files omitted>" "--as-needed" "-Bstatic" "/home/rmueller/Rust/esp-hal-community/esp-hal-smartled/target/riscv32imac-unknown-none-elf/debug/deps/{libesp_hal_embassy-0b570ad2ccdcbe31.rlib,libstatic_cell-2fe59a31e8f78c59.rlib,libembassy_executor-196bff06069a43a8.rlib,libsmart_leds-3f75fd4ede618e7e.rlib,libesp_hal_smartled-a3ea92e38901e2ad.rlib,libsmart_leds_trait-a604cb46675aa2a9.rlib,librgb-fc2138c8ab3dfa9e.rlib,libesp_hal-333444a37969506e.rlib,libembedded_can-b9fe4f377ed1cd0f.rlib,librand_core-649b74d2bdd1b284.rlib,libufmt_write-4b219053f0868c4d.rlib,libfugit-c57e125a9136b376.rlib,libgcd-75f78a47cc6ded6a.rlib,libembassy_embedded_hal-f8e6cd2fccfd0ed9.rlib,libembassy_time-100f4b9db317a8e2.rlib,libembassy_time_driver-3af4d7a0de091818.rlib,libembassy_futures-f92de4155a8254b9.rlib,libembedded_storage_async-04cf31a3227f3a6c.rlib,libembedded_storage-21c58fc38769de16.rlib,libembedded_hal-424a9e9c45a58ce5.rlib,libvoid-9cad67ac0ed01aaa.rlib,libnb-570cb8537b8c7a7f.rlib,libchrono-d35a563323106706.rlib,libnum_traits-c3c08c7f10bbc1c8.rlib,libembassy_sync-c0ba62720071d255.rlib,libfutures_util-4870ba19dfbbd0e2.rlib,libpin_project_lite-b54fa9bacd7a6895.rlib,libfutures_task-2a3b7b57fe26ef75.rlib,libpin_utils-767783140774eb37.rlib,libfutures_core-f5973be1f36c664f.rlib,libfutures_sink-771e8a53c31f1186.rlib,libembedded_io_async-81f3e71899145521.rlib,libheapless-ce22fe139a4b4f91.rlib,libhash32-21bb8fde374278b8.rlib,libbyteorder-d979f2953ef7d250.rlib,libbitflags-0b9ab7067d6069a0.rlib,libnb-0b491629be2a3667.rlib,libbitfield-b0ef1c64dabde8d7.rlib,libesp_config-d3cc4dc8c8530809.rlib,libcfg_if-3ab91fc94e10801e.rlib,libbytemuck-9eddb76f309b053c.rlib,libesp32c6-5fec569ce6b06d0a.rlib,libvcell-6f683f7881d6c8de.rlib,libembedded_io-5d70309093d948d5.rlib,libenumset-1bf2f2c2cd5aa154.rlib,libembedded_hal_async-edc1bc5fb2e8fa41.rlib,libstrum-0ff07aaea6a69cc5.rlib,libesp_riscv_rt-29ee066acf154428.rlib,libriscv-cb8806e4f964e57c.rlib,libriscv_pac-8aad88270ef81da3.rlib,libembedded_hal-52028cfd21c291a1.rlib,libesp_backtrace-0834b3259d067ed4.rlib,libesp_println-d84f3260855be6f5.rlib,libcritical_section-c0dd728f05931daf.rlib,libportable_atomic-26ccc9f29d4ae4f5.rlib}.rlib" "<sysroot>/lib/rustlib/riscv32imac-unknown-none-elf/lib/{librustc_std_workspace_core-*,libcore-*,libcompiler_builtins-*}.rlib" "-Bdynamic" "-z" "noexecstack" "-L" "/home/rmueller/Rust/esp-hal-community/esp-hal-smartled/target/riscv32imac-unknown-none-elf/debug/build/esp-hal-d06c557878c8f3cf/out" "-L" "/home/rmueller/Rust/esp-hal-community/esp-hal-smartled/target/riscv32imac-unknown-none-elf/debug/build/esp32c6-25d5d06b111af851/out" "-o" "/home/rmueller/Rust/esp-hal-community/esp-hal-smartled/target/riscv32imac-unknown-none-elf/debug/examples/hello_rgb_async-f58f20f05f1ab989" "--gc-sections" "-Tlinkall.x" "-Tlinkall.x"
  = note: some arguments are omitted. use `--verbose` to show all linker arguments
  = note: rust-lld: error: /home/rmueller/Rust/esp-hal-community/esp-hal-smartled/target/riscv32imac-unknown-none-elf/debug/build/esp-hal-d06c557878c8f3cf/out/memory.x:22: region 'RAM' already defined
          >>>     RAM : ORIGIN = 0x40800000 , LENGTH = 0x6E610
          >>>                                          ^


error: could not compile `esp-hal-smartled` (example "hello_rgb_async") due to 1 previous error

esp-hal-community/esp-hal-smartled on  non-blocking-smart-led [$?] is 📦 v0.15.0 via 🦀 v1.86.0 took 30s
❯

probably just some setup issue

[jamessizeland]

hmm yeah I'll pull your branch and take a look myself

[robamu]

i actually also wanted to test this on an esp32c6 devboard I have here, just rebased as well.

[robamu]

Just reveived the same error on a different machine, building the hello_rgb example.. can you re-produce this?

[MabezDev]

I'll dismiss my review shortly, as I don't want to block progress here.

Just an FYI, @wisp3rwind is working on a pretty radical RMT refactor. In particular the latest PR has some interesting changes which should make supporting blocking and async smartled stuff easier: https://github.com/esp-rs/esp-hal/pull/3505/files#diff-a18a0af13c09debca19ef19cda6411ea68747fb7fd9ef38a6a3c57e4a7005fb6R702.

Hopefully that link works correctly, but if it doesn't here is the snippet I was referring to:

/// RMT Channel Creator
pub struct ChannelCreator<Dm, const CHANNEL: u8, Cap>
where
    Dm: crate::DriverMode,
{

Channel creator will be generic over the mode (Blocking/Async), which should eventually reduce some boiler plate here.

Not strictly relevant to this PR anymore

[jamessizeland]

can you sync this branch again? Its missing the .cargo changes

[jamessizeland]

I've added the .cargo/config.toml files and run this with cargo esp32c3 -p esp-hal-smartled --example hello_rgb_async and it works nicely for me! I'll try it on my c6 board too now.

[jamessizeland]

yep working. I think if you just merge in the latest changes it'll work.

[jamessizeland]

works on my devices. Just needs main merging in first

[wisp3rwind]

I'll dismiss my review shortly, as I don't want to block progress here.

Just an FYI, @wisp3rwind is working on a pretty radical RMT refactor. In particular the latest PR has some interesting changes which should make supporting blocking and async smartled stuff easier: https://github.com/esp-rs/esp-hal/pull/3505/files#diff-a18a0af13c09debca19ef19cda6411ea68747fb7fd9ef38a6a3c57e4a7005fb6R702.

Hopefully that link works correctly, but if it doesn't here is the snippet I was referring to:

/// RMT Channel Creator
pub struct ChannelCreator<Dm, const CHANNEL: u8, Cap>
where
    Dm: crate::DriverMode,
{

Channel creator will be generic over the mode (Blocking/Async), which should eventually reduce some boiler plate here.

FWIW, completely over-engineering smart LED driving is how I got there.

What I have locally is a completely interrupt-driven async RMT writer, which uses an Encoder type (similar to the IDF driver) to directly generate pulse codes from RGB(WW) arrays in the ISR and write it to the RMT channel RAM, without any intermediate buffers that would contain pulse codes.

I'll attempt to upstream all of this, but it's going to take a while: In particular, the ISR case raises a bunch of issues regarding safety/soundness of the code (because the future needs to take ownership of the buffer/Encoder for this to be sound on drop of leak of the future), and I don't think how I designed it is ideal yet. I also tried an async implementation where the buffer is refilled on poll of the RmtTxFuture, but that didn't seem to be fast enough and led to glitches.

[robamu]

Rebased on upstream again.

@wisp3rwind @MabezDev That sounds really cool. I have a question: I actually had to re-configure the RMT output pin as input to avoid phantom current through the data pin when my smart led strip was off. maybe the esp RMT channel structure can keep a type erased flex/any pin instance in the future so we can enable stuff like temporarily re-configuring as input, and later re-configuring a pin as a RMT pin?

@jamessizeland thanks a lot for testing. I am still wondering what kind of issue I am looking at here.. I cleaned the repo multiple times as well.. which rust version are you using?

[robamu]

and it just disappeared :D