Add generalised spi driver for clockless LEDs by mlandauer · Pull Request #112 · ahdinosaur/blinksy

mlandauer · 2026-10-07T02:10:07Z

This addresses #12

The implementation is currently pretty long winded for what it actually does (to my taste anyway). Most of this is due to hoops we're having to jump through to calculate buffer sizes at compile time (See #107)

Documentation is still to come but I thought showing this to you earlier rather than later was probably a good idea!

I haven't changed anything to make it easier to build the examples (there are 2 now) in blinksy-esp. Happy to add that if it makes sense. The new example isn't actually building against blinksy-esp but rather is building against blinksy but is esp specific. If there's a better place to put that example...

I'm somewhat surprised at how large this PR has become. It certainly was not my intention. I don't like the way the blocking and async code is duplicated and I don't like that there are two buffer sizes that have to be calculated. The builder makes it slightly less messy.

The usage of the new api goes something like this:

const SPI_FREQ_HZ: u32 = clockless_spi_ideal_frequency_hz::<Led>(150);

// Create spi that implements embedded_hal::spi::SpiBus trait or embedded_hal_async::spi::SpiBus (if using async feature)
// and set its frequency to SPI_FREQ_HZ
// let spi = ...

let writer = ClocklessSpiBuilder::default()
       .with_spi(spi)
       .with_freq_hz(SPI_FREQ_HZ)
       .with_buffer_size::<{ clockless_spi_buffer_size::<Led, SpiDma<Async>, _>(Layout::PIXEL_COUNT, SPI_FREQ_HZ) }>()
       .with_pulse_size::<{ clockless_spi_pulse_size::<Led, SpiDma<Async>, _>(SPI_FREQ_HZ) }>()
       .with_async()
       .build();

This addresses ahdinosaur#12 The implementation is currently pretty long winded for what it actually does (to my taste anyway). Most of this is due to hoops we're having to jump through to calculate buffer sizes at compile time (See ahdinosaur#107) Documentation is still to come

mlandauer · 2026-10-07T02:11:11Z

@ahdinosaur it would be great if you could let me know if this on the right track before I finish it off by adding documentation

mlandauer · 2026-10-07T02:12:30Z

And I managed to completely fail to mention that I explained the motivation behind this in #12 (comment)

ahdinosaur

Hi, thank you for this! 💜

Overall this looks great. I feel this definitely matches the vibes of the existing Blinksy code. I like your thinking in #12 (comment), thank you for sharing. I also feel like you "get" Blinksy, which gives me a good feeling.

I wouldn't worry about how many lines this adds. Yes there's some boilerplate, but I think that's okay.

I had a few nitpick comments, sorry, you can do with that as you want. 🤷‍♀️

ahdinosaur · 2026-10-07T23:36:37Z

+> {
+    spi: Spi,
+    freq_hz: Freq,
+    _led: PhantomData<Led>,


I don't usually _ the PhantomData (e.g. https://github.com/ahdinosaur/blinksy/blob/main/blinksy/src/control.rs#L245 ), but also it doesn't really matter.

ahdinosaur · 2026-10-07T23:39:56Z

+use crate::driver::ClocklessSpiAsync;
+use crate::markers::{Async, Blocking};
+
+pub struct ClocklessSpiBuilder<


Good to see that you understood my builder pattern! 👍

It might be a lot of code, but it's mostly just boilerplate so I don't mind.

ahdinosaur · 2026-10-07T23:43:11Z

+    Led,
+    SpiWord,
+    Freq,
+    Dm,


What's a Dm?

Is this what I called Exec? https://github.com/ahdinosaur/blinksy/blob/main/blinksy/src/control.rs#L249. Might be best to be consistent.

Ohh, Dm comes from the esp-hal crate and how they handle this same situation. Yeah I'd be keen to be consistent with Blinksy, even if I do use Dm here: https://github.com/ahdinosaur/blinksy/blob/main/esp/blinksy-esp/src/rmt.rs#L129

ahdinosaur · 2026-10-07T23:45:01Z

+    Led: ClocklessLed,
+{
+    pub fn build(self) -> ClocklessSpi<BUFFER_SIZE, PULSE_SIZE, Led, Spi, SpiWord> {
+        assert!(BUFFER_SIZE > 0, "set buffer_size before calling build");


ahdinosaur · 2026-10-07T23:48:07Z

+use bitvec::{array::BitArray, order::Msb0, slice::BitSlice};
+use core::marker::PhantomData;
+
+pub const fn freq_hz_to_duration_ns(freq_hz: u32) -> u32 {


Generally I like to order exports in a file like a house's intimacy gradient: most public at the top, most private at the bottom. These are all public, but I reckon the helpers should go at the bottom.

Also 🤷‍♀️ , this file is more a collection of related things, so not a clear entrance. Feel free to ignore me.

But also, do these functions need to be pub? Why not pub(crate)?

ahdinosaur · 2026-10-08T00:03:45Z

+    duration_ns_to_freq_hz(max_clock_period_ns)
+}
+
+fn encode_spi_buffer<


Should go to the bottom.

ahdinosaur · 2026-10-08T00:11:49Z

+    .with_dma(p.DMA_CH0)
+    .with_buffers(dma_rx_buf, dma_tx_buf)
+    .into_async();
+    let writer = ClocklessSpiBuilder::default()


This all looks good. 👍

https://github.com/ahdinosaur/blinksy/blob/main/blinksy/src/control.rs#L249

Also put more public stuff at the top of the file

mlandauer · 2026-10-08T21:42:46Z

Thanks @ahdinosaur for the feedback. All good suggestions. I've hopefully addressed them and added some documentation.

Looks like I've broken CI. ugh.

mlandauer · 2026-10-08T21:44:45Z

I keep getting caught out by this. Keep forgetting that code in docs gets run. Silly me.

mlandauer · 2026-10-08T21:52:31Z

@ahdinosaur hopefully good for you to look over again now

ahdinosaur

This looks great, thanks!

Minor docs feedback. 📝

ahdinosaur · 2026-10-10T08:55:49Z

+use bitvec::{array::BitArray, order::Msb0, slice::BitSlice};
+use core::marker::PhantomData;
+
+/// Represents the timing (in number of SPI bits) to encode zero, one and reset LED signals


Oxford comma please 😅

Definitely not my natural writing style but I'm happy to oblige ;-)

ahdinosaur · 2026-10-10T08:57:54Z

+///
+/// # Type Arguments
+///
+/// - `Led` - The LED protocol implementation (must implement ClocklessLed)


Can this (and the next lines) link to the trait(s)?

ahdinosaur · 2026-10-10T09:02:02Z

 categories.workspace = true

 [dependencies]
+# radium 0.7 is a dependency of the latest released version of bitvec which breaks on esp32. So using an unreleased


We won't be able to publish a new crate version until this is resolved. Although I haven't been publishing new crate versions lately, but that's on me.

This was a bit of show-stopper I think because it doesn't look like we could depend on a bitvec release for that fix getting done anytime soon. It's annoying.

Anyway, I've rejigged things to not depend on bitvec. It does tidy up the public interfaces just a tiny amount too even though what's under the hood is now more messy.

function

destination

mlandauer · 2026-10-11T04:03:34Z

@ahdinosaur hopefully ready for another look over

state

mlandauer · 2026-10-11T05:53:50Z

I keep thinking I'm done and then I find a bug I hadn't spotted before. See the last commit 96c13f1

ahdinosaur · 2026-10-12T02:10:17Z

Merged, thanks @mlandauer ! 🎉

mlandauer · 2026-10-12T04:43:42Z

Thank you @ahdinosaur!

ahdinosaur reviewed Oct 8, 2026

View reviewed changes

mlandauer added 17 commits October 9, 2026 03:50

Update Cargo.lock in blinksy-esp after blinksy update

b7cf662

Rename PhantomData fields to match convention used in the crate

5b5f405

Rename for consistency with

612d46b

https://github.com/ahdinosaur/blinksy/blob/main/blinksy/src/control.rs#L249

Make it easier to build examples in blinksy-esp

246e6d4

Don't make things public that don't need to be

0239768

Also put more public stuff at the top of the file

Move helper function to the bottom of the file

eaa9437

Add docs for clockless_spi_buffer_size

606bf08

Add docs for clockless_spi_pulse_size

36bd5cf

Add docs for builder

a752fe6

Add link to the spi driver in the clockless docs

aea4ce4

First rough pass at documentation for ClocklessSpi

2868cc3

document Timing

3ebf906

Add rough docs for ClocklessSpiAsync

d07323e

Add documentation for frequency calculation

96c68d7

Remove warn(missing_docs)

17f1fd5

Rename struct

fb003f2

Improve "how this works" section

8412e6f

Code in added documentation can't be run unfortunately

c919176

mlandauer changed the title ~~WIP: Add generalised spi driver for clockless LEDs~~ Add generalised spi driver for clockless LEDs Oct 8, 2026

Add link to new spi driver on crate level documentation

45534dd

ahdinosaur reviewed Oct 10, 2026

View reviewed changes

mlandauer added 2 commits October 11, 2026 08:01

Small documentation fixes

03433be

More concise way of expressing the bitvec trait bound

479d652

mlandauer added 13 commits October 11, 2026 08:59

Simplify slightly by passing BitSlice rather than BitArray to encode

2951d91

function

Confine the bit mangling inside encode_spi_buffer

e658231

Fix buffer size calculation if led or spi word is not u8

8facc5e

First rough implementation of copying bits around

7e8cb6a

Add function to just copy a bit of a word

8607d57

Add some simple tests and fix bug

a6509a1

Use the new function to simplify

9a69872

Start abstracting a slice of bits

fef2a4d

Make it easier getting the saved pulse code bits

6e8e21d

Tiny refactor on the way to generalising copy_bits_msb

e05114f

Refactor interface of copy_bits_msb so there's just a source and a

bdecf97

destination

Add a little note about alignment for the copy function

eb256d3

Reorder only

d9d3ca9

Send a reset to the LED before the first write to ensure it's in a known

96c13f1

state

ahdinosaur merged commit 3b71e11 into ahdinosaur:main Oct 12, 2026
6 checks passed

mlandauer deleted the clockless-spi-driver-rebased branch October 12, 2026 04:43

ahdinosaur mentioned this pull request Oct 15, 2026

Add support for clockless SPI #12

Closed

Conversation

mlandauer commented Oct 7, 2026

Uh oh!

mlandauer commented Oct 7, 2026

Uh oh!

mlandauer commented Oct 7, 2026

Uh oh!

ahdinosaur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlandauer commented Oct 8, 2026

Uh oh!

mlandauer commented Oct 8, 2026

Uh oh!

mlandauer commented Oct 8, 2026

Uh oh!

ahdinosaur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlandauer commented Oct 11, 2026

Uh oh!

mlandauer commented Oct 11, 2026

Uh oh!

Uh oh!

ahdinosaur commented Oct 12, 2026

Uh oh!

mlandauer commented Oct 12, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants