Implement I2S #232

dacut · 2023-04-07T00:42:17Z

This implements I2S standard mode, as a first run fix for #205. It does not yet implement PDM, TDM, or PCM modes.

There is an issue here that I'd like comments on. Currently, once a GPIO pin is passed into esp_idf_hal::i2s::config::StdGpioConfigBuilder, it cannot be reused, even after the driver is dropped. This seems to fit into the intended design of Peripheral, as documented here:

When writing a HAL, the intended way to use this trait is to take impl Peripheral<P = ..> in the HAL’s public API (such as driver constructors), calling .into_ref() to obtain a PeripheralRef, and storing that in the driver struct.
.into_ref() on an owned T yields a PeripheralRef<'static, T>. .into_ref() on an &'a mut T yields a PeripheralRef<'a, T>.

But this is a surprise to @ivmarkov in this comment and doesn't feel natural to me, either.

Vollbrecht

Thank you for writing this driver ! Nice that you got it working ❤️
I have a couple of questions about the heavy use of generics for the pins. Did you had a problem somewhere i didn't see why this generic approach was needed? I would think if the Configs just take Options for things the user don't need a lot of this can be reduced.
I left a couple of comments but keep in mind a maybe totally wrong about thinks.

Cargo.toml

src/i2s.rs

Vollbrecht · 2023-04-07T11:59:00Z

to give an concrete example how i would have made it without generics on the gpiostruct ->

struct StdGpioConfig {
    Bclk: PeripheralRef<'static, AnyOutputPin>,
    Din: Option<PeripheralRef<'static, AnyInputPin>>,
    Dout: Option<PeripheralRef<'static, AnyOutputPin>>,
}

impl StdGpioConfig {
    pub fn new(
        Bclk: PeripheralRef<'static, impl OutputPin + 'static>,
        Din: Option<PeripheralRef<'static, impl InputPin + 'static>>,
        Dout: Option<PeripheralRef<'static, impl OutputPin + 'static>>,
    ) -> Self {
        let Bclk = Bclk.map_into::<AnyOutputPin>();
        let Din = match Din {
            Some(Din) => Some(Din.map_into::<AnyInputPin>()),
            None => None,
        };
        let Dout = match Dout {
            Some(Dout) => Some(Dout.map_into::<AnyOutputPin>()),
            None => None,
        };
        Self { Bclk, Din, Dout }
    }
}

with the .pin() method you always have access to the underlying gpio number

ivmarkov · 2023-04-07T12:23:02Z

to give an concrete example how i would have made it without generics on the gpiostruct ->

struct StdGpioConfig {
    Bclk: PeripheralRef<'static, AnyOutputPin>,
    Din: Option<PeripheralRef<'static, AnyInputPin>>,
    Dout: Option<PeripheralRef<'static, AnyOutputPin>>,
}

impl StdGpioConfig {
    pub fn new(
        Bclk: PeripheralRef<'static, impl OutputPin + 'static>,
        Din: Option<PeripheralRef<'static, impl InputPin + 'static>>,
        Dout: Option<PeripheralRef<'static, impl OutputPin + 'static>>,
    ) -> Self {
        let Bclk = Bclk.map_into::<AnyOutputPin>();
        let Din = match Din {
            Some(Din) => Some(Din.map_into::<AnyInputPin>()),
            None => None,
        };
        let Dout = match Dout {
            Some(Dout) => Some(Dout.map_into::<AnyOutputPin>()),
            None => None,
        };
        Self { Bclk, Din, Dout }
    }
}

with the .pin() method you always have access to the underlying gpio number

I wouldn't hurry up with this. Two problems:

PeripheralRef is not an API and should not appear anywhere in public APIs
'static does not work

I'll follow up with alternative suggestions tmr.

Vollbrecht · 2023-04-07T12:46:19Z

to give an concrete example how i would have made it without generics on the gpiostruct ->

struct StdGpioConfig {
    Bclk: PeripheralRef<'static, AnyOutputPin>,
    Din: Option<PeripheralRef<'static, AnyInputPin>>,
    Dout: Option<PeripheralRef<'static, AnyOutputPin>>,
}

impl StdGpioConfig {
    pub fn new(
        Bclk: PeripheralRef<'static, impl OutputPin + 'static>,
        Din: Option<PeripheralRef<'static, impl InputPin + 'static>>,
        Dout: Option<PeripheralRef<'static, impl OutputPin + 'static>>,
    ) -> Self {
        let Bclk = Bclk.map_into::<AnyOutputPin>();
        let Din = match Din {
            Some(Din) => Some(Din.map_into::<AnyInputPin>()),
            None => None,
        };
        let Dout = match Dout {
            Some(Dout) => Some(Dout.map_into::<AnyOutputPin>()),
            None => None,
        };
        Self { Bclk, Din, Dout }
    }
}

with the .pin() method you always have access to the underlying gpio number

I wouldn't hurry up with this. Two problems:

* `PeripheralRef` is not an API and should not appear anywhere in public APIs

* `'static` does not work

I'll follow up with alternative suggestions tmr.

yeah should have put Peripheral there .. so more something like --->

struct StdGpioConfig<'d> {
    Bclk: PeripheralRef<'d, AnyOutputPin>,
    Din: Option<PeripheralRef<'d, AnyInputPin>>,
    Dout: Option<PeripheralRef<'d, AnyOutputPin>>,
}

impl<'d> StdGpioConfig<'d> {
    pub fn new(
        Bclk: impl Peripheral<P = impl OutputPin > + 'd,
        Din: Option<impl Peripheral<P = impl InputPin > + 'd>,
        Dout: Option<impl Peripheral<P = impl OutputPin > + 'd>,
    ) -> Self {
        let Bclk = Bclk.into_ref().map_into::<AnyOutputPin>();
        let Din = match Din {
            Some(Din) => Some(Din.into_ref().map_into::<AnyInputPin>()),
            None => None,
        };
        let Dout = match Dout {
            Some(Dout) => Some(Dout.into_ref().map_into::<AnyOutputPin>()),
            None => None,
        };
        Self { Bclk, Din, Dout }
    }
}

dacut · 2023-04-07T15:50:07Z

Thanks for the comments @Vollbrecht and @ivmarkov. I'll wait for @ivmarkov's suggestions before proceeding, though will fix Config.toml to revert my accidental local commit.

Vollbrecht · 2023-04-07T15:52:28Z

in the other drivers like spi we just consume the Peripheral and dont store it after config is finishd. So this is something he might be playing into but we will see .

ivmarkov · 2023-04-09T06:39:02Z

in the other drivers like spi we just consume the Peripheral and dont store it after config is finishd. So this is something he might be playing into but we will see .

The fact that we do not (need to) store the pins does not mean we consume them.

src/i2s.rs

dacut · 2023-04-09T18:45:59Z

Thanks for all the comments. I'm looking into incorporating them now.

dacut · 2023-04-09T22:27:17Z

Ugh. Ok, I realized why it's problematic initialize the channels in the driver ::new() path (and thus take pin arguments, etc.). This moves the channels from the REGISTERED to the READY state, which means you can't register interrupt handlers on them. This is crucial for handling any streaming activity on the DMA.

The alternative is to add an interrupt handler argument in there, too, but... the parameters are getting messy. I don't like having the pins taken by position, either, since it's easy to mix them up (taking them in the config by name made this nice at the expense of generic hell). There are other things that people might want to do to the channel while it's in the REGISTERED state before it moves to the READY state, too.

dacut · 2023-04-13T01:09:22Z

Ok, I refactored this based on all the comments. This sample code has also been updated to work again.

The CI workflows won't pass until there's a new release of esp-idf-sys with esp-rs/esp-idf-sys@ce83a37 in there.

dacut · 2023-04-20T19:32:37Z

Whew. Ok, I think this is ready for re-review, @ivmarkov and @Vollbrecht.

After a lot of testing with code in https://github.com/dacut/test-esp-sound, I realized that trying to do lifetimes with the callbacks was next to impossible. You quickly end up with self-referential lifetimes between the driver and the callback, and that's just a mess. It's far better to pass the callback into the driver and let it own it. It does mean the callback needs to be 'static, however.

Alas, my testing uncovered fundamental issues with the callback mechanism in ESP-IDF 5.0.x. It looks like this is broken because there's a queue that needs to be messaged to mark DMA buffers as available for reuse, but this queue is private. I've marked this as a bug in the commentary.

Without writing to the DMA buffers directly, however, I'm experiencing pops, likely due to the delay between a DMA transfer completes and the copy into the next DMA buffer is ready. You can hear an example in this video, which comes from this sample code for a SparkFun Thing Plus.

Vollbrecht · 2023-04-21T14:11:10Z

Alas, my testing uncovered fundamental issues with the callback mechanism in ESP-IDF 5.0.x. It looks like this is broken because there's a queue that needs to be messaged to mark DMA buffers as available for reuse, but this queue is private. I've marked this as a bug in the commentary.

Can you broadly speaking elaborate if this only affecting a part that is only available in idf v.5.x or - is there an equivalent present in this wrapper api for this functionality using idf v4.4 ?

I will have a look in the next days, if i can get it running on my c3 hardware and play with it a bit.

dacut · 2023-04-21T16:31:26Z

Looks like the equivalent callback functionality isn't even available in 4.4. The generic write code is almost identical, however, so something must have been working; I can't imagine Espressif have not had working audio-out with ESP-IDF all these years.

I do see a note in the 4.4 docs about using the APLL clock source for "high frequency clock applications." I've been using the default to the 160 MHz clock. I didn't imagine that "high frequency" would mean 48 kHz audio rates (1.5 MHz bit clock), but I wonder if this applies to the I2S MClk (which I'm not using, but the I2S peripheral might be)? That's running at 256x sample rate by default (12.288 MHz).

I'll give this a shot when I get back to my test setup.

dacut · 2023-04-22T00:21:30Z

Switching to APLL didn't help, but switching to a triangle wave output fixed this issue. The problem was actually in my sine wave computation; I had code that addressed the discontinuity at the edge of the DMA buffer length, but forgot to add that back in when I rewrote this. You're hearing the discontinuity in the sine wave, not anything to do with the I2S code.

Please go ahead and review. I will add 4.4 compatibility in a separate PR.

dacut · 2023-04-25T03:50:54Z

I do have v4.4 and PDM/TDM support now in a separate branch, but it's a lot more to review. (https://github.com/dacut/esp-idf-hal/tree/i2s-modes) I'll hold off on that PR until you get a chance to look at this updated PR.

dacut · 2023-06-03T19:14:31Z

Let me know if you need help reviewing this and would like to have a virtual meeting to go over it.

ivmarkov · 2023-06-04T09:08:19Z

Very good thank you!
I'm currently on travel, but give me some time to review next week.

I definitely want i2s to be merged. Selfish reasons - I have a project that needs it. :)

ivmarkov · 2023-06-07T07:15:42Z

@dacut Fork https://github.com/dacut/esp-idf-hal/tree/i2s-modes looks quite good I must say! I might have some minor comments/suggestions only. Let me take a deeper look today/tmr...

dacut · 2023-06-07T17:14:11Z

@ivmarkov Would you like me to merge it into this PR? I was holding off because I thought it might be too much to review at once.

ivmarkov · 2023-06-07T17:41:50Z

Merging? I thought your other branch is in fact superseding this PR?

dacut · 2023-06-07T18:02:15Z

It builds on top of this PR (it's based off the i2s branch). I was keeping it separate just for sanity, but if you're already looking, I'll just merge i2s-modes into i2s. I think it requires one more change to esp-idf-sys as well.

ivmarkov · 2023-06-07T18:31:01Z

Please do. I was quickly comparing against the esp-idf-hal master branch anyway.

dacut · 2023-06-07T19:47:55Z

Alright, i2s is now equivalent to i2s-modes. (Sorry for the delay; meetings + had to remember which machine I was last hacking on...)

This change includes the I2S headers for ESP-IDF v4.4, allowing the Rust I2S driver being considered in this pull request to build on that version of ESP-IDF. esp-rs/esp-idf-hal#232

dacut · 2023-06-07T19:52:12Z

This change in esp-idf-sys is required to build on v4.4:
esp-rs/esp-idf-sys#207

The current HEAD is sufficient for building on v5.x.

This change includes the I2S headers for ESP-IDF v4.4, allowing the Rust I2S driver being considered in this pull request to build on that version of ESP-IDF. esp-rs/esp-idf-hal#232

This isn't strictly necessary in alloc mode, but it makes the code easier to use.

Channels aren't used, so pools aren't needed (and the types aren't valid).

dacut · 2023-06-14T17:59:36Z

@ivmarkov, when you get a chance, can you fire off another CI check? I believe I've fixed all of the fixable issues (that is, everything except the compiler error): https://github.com/dacut/esp-idf-hal/actions/runs/5264225899/jobs/9515249060

Making it no_std friendly required a bit of a work to avoid the use of Box: it uses a static pool for the channels in that case (to keep them pinned so the pointers are stable for the SDK).

Also, if you have a link for the compiler issue, let me know. I didn't see it in esp-rs/rust. Thanks!

ivmarkov · 2023-06-15T19:09:35Z

LGTM, final look over the weekend, and then we likely merge!

ivmarkov

Forget about the minor Debug nits. The main two questions that needs addressing are:

We are leaking memory when alloc is enabled, right? Is this how things are supposed to be?
Given all the equilibristics for implementing a no-alloc callback-safe version, why do we have the alloc variants? Can't we just remove them, which would take care of removing the memory leaks. This can also make the code safer, as you don't have to pass around and keep *mut raw pointers. You can reduce the unsafe and pass around &'static mut references instead.

src/i2s/pcm.rs

src/i2s/pdm.rs

ivmarkov · 2023-06-18T15:42:08Z

src/i2s.rs

+            callback: None,
+        };
+
+        Box::leak(Box::new(channel))


This memory seems to be never reclaimed? Also, why do we need this impl, igven that you have a no-alloc impl anyway, which can be used regardless of whether the alloc module is used or not?

Argh, you're correct. I was reclaiming it from a Box::from_raw elsewhere, but that code went away.

To sum up: I wasn't aware that no_std was even an option until the CI tooling failed with the Box with this enabled. I put the no-alloc option in at the last minute to make it happy.

My main concern is this eats up size_of::<I2sChannel<dyn I2sRxCallback/I2sTxCallback>> * 2 (32) bytes of RAM (*4 == 64 for ESP32), even for folks who don't use I2S, and there's not a lot of it (160 kB).

Should I add in the Box::from_raw code, or eat the 32/64 bytes of RAM for everyone?

What I usually do in code elsewhere is that the "callback" code is only available when the alloc feature is enabled. Otherwise, the user just does not have the option to use callbacks.

In practice this is not a problem, because using no_std + allocator with esp-idf-* is more of a "hygiene" thing (as in don't overuse the Rust STD API if you don't need it) rather than a solution to an actual problem. Further, using no_std without alloc is even less of an actual use case, as ESP-IDF itself is heavily allocator-based, so the fact that the Rust wrappers on top won't allocate is not solving anything other than putting some discipline and forcing the Rust driver implementor to think where allocations are really needed.

So maybe you could leave the alloc code only then.

Now, I realized your code has an even bigger problem - the "callback" stuff - the way you implemented it - is viral, because you demand that the channel itself (I2s(Rx|Tx)Channel) has a stable memory location. This - in turn - means that the channels - if you leave only the Box-ing code and remove the static mut MaybeUninit tricks - will be boxed. But then, if the users compile without alloc enabled, she would lose not just the option to set callbacks, but in fact the whole i2s code!

Putting the whole driver under an alloc requirement then is also an option. BUT: digging even further, I think the crux of the problem is that your I2s(Rx|Tx)Callback trait has a stronger API than necessary - it provides to the user
a &mut reference to the I2s(Rx|Tx)Channel instance - which - I think - is not really necessary? The trait (or closure) should just get an indication of what that channel it is (port number) and the event itself. The user_ctx should be
the closure itself, so that the user can close over whatever state she wants. Not to mention that providing a &mut reference to the channel is unsound in the first place, as you get mutable aliasing in that way (use might be already inside read/write or other functions of the channel that take a &mut ref to itself when the callback ISR triggers).

Moreover, setting the callback should be marked with unsafe, because the callback is executed in an ISR context and as such is extremely dangerous - for one, users cannot use the Rust STD API inside.

My suggestion:

Leave only the alloc based code, remove the static mut tricks

Demote the callback from an I2s(RX|TX)Callback instance to just F: FnMut(...)

Parameterize the set_callback methods by F and not the whole channel

Pass everything needed - channel port number, what type of event this is and the event itself as parameters to the closure

User does not get access to the channel instance from inside the closure

Store the F inside a double box (using two boxes of indirection is important with closures, see the callback code everywhere else)

Demote the tx and rx channels to regular members of the driver

Last but not least - mark the set_callback methods as unsafe, because the closure will be executed from an ISR context

Finally, and if you are wondering, for what the callbacks might be useful - the one useful feature is implementing async APIs on top of that driver. The callbacks will just awake the executor, while the read/write APIs will then be called with a timeout of 0.

@dacut Let me know what you think. If these changes feel as too much work for you, I can also try to implement them.
It is just that I'll be able to test it earliest in July.

Ah, I think I see how the callbacks are supposed to work now. (Ignore my deleted comment.) Instead of handling the DMA event in the ISR, you're just supposed to wake up a task to handle the DMA event.

I'm used to platforms where the DMA has to be handled in the ISR (acknowledged at the very least) or the entire subsystem goes awry.

Will look at this tonight or tomorrow. (Work got busy...)

@ivmarkov Can you clarify why double-boxing is used? (Note that it's missing in pcnt.rs, but present in gpio.rs and timer.rs). I would expect a Pin<Box<...>> instead of Box<Box<...>>.

ivmarkov · 2023-06-18T15:42:17Z

src/i2s.rs

+            callback: None,
+        };
+
+        Box::leak(Box::new(channel))


ivmarkov · 2023-06-19T06:41:31Z

src/i2s.rs

+            callback: None,
+        };
+
+        Box::leak(Box::new(channel))


What I usually do in code elsewhere is that the "callback" code is only available when the alloc feature is enabled. Otherwise, the user just does not have the option to use callbacks.

In practice this is not a problem, because using no_std + allocator with esp-idf-* is more of a "hygiene" thing (as in don't overuse the Rust STD API if you don't need it) rather than a solution to an actual problem. Further, using no_std without alloc is even less of an actual use case, as ESP-IDF itself is heavily allocator-based, so the fact that the Rust wrappers on top won't allocate is not solving anything other than putting some discipline and forcing the Rust driver implementor to think where allocations are really needed.

So maybe you could leave the alloc code only then.

Now, I realized your code has an even bigger problem - the "callback" stuff - the way you implemented it - is viral, because you demand that the channel itself (I2s(Rx|Tx)Channel) has a stable memory location. This - in turn - means that the channels - if you leave only the Box-ing code and remove the static mut MaybeUninit tricks - will be boxed. But then, if the users compile without alloc enabled, she would lose not just the option to set callbacks, but in fact the whole i2s code!

Putting the whole driver under an alloc requirement then is also an option. BUT: digging even further, I think the crux of the problem is that your I2s(Rx|Tx)Callback trait has a stronger API than necessary - it provides to the user
a &mut reference to the I2s(Rx|Tx)Channel instance - which - I think - is not really necessary? The trait (or closure) should just get an indication of what that channel it is (port number) and the event itself. The user_ctx should be
the closure itself, so that the user can close over whatever state she wants. Not to mention that providing a &mut reference to the channel is unsound in the first place, as you get mutable aliasing in that way (use might be already inside read/write or other functions of the channel that take a &mut ref to itself when the callback ISR triggers).

Moreover, setting the callback should be marked with unsafe, because the callback is executed in an ISR context and as such is extremely dangerous - for one, users cannot use the Rust STD API inside.

My suggestion:

Leave only the alloc based code, remove the static mut tricks

Demote the callback from an I2s(RX|TX)Callback instance to just F: FnMut(...)

Parameterize the set_callback methods by F and not the whole channel

Pass everything needed - channel port number, what type of event this is and the event itself as parameters to the closure

User does not get access to the channel instance from inside the closure

Store the F inside a double box (using two boxes of indirection is important with closures, see the callback code everywhere else)

Demote the tx and rx channels to regular members of the driver

Last but not least - mark the set_callback methods as unsafe, because the closure will be executed from an ISR context

Finally, and if you are wondering, for what the callbacks might be useful - the one useful feature is implementing async APIs on top of that driver. The callbacks will just awake the executor, while the read/write APIs will then be called with a timeout of 0.

Add Debug derivations where missing. Add Default derivations to enums now that MSRV supports them.

dacut · 2023-06-22T23:54:58Z

Oof... As I implement some of the suggestions, I'm recalling why some of the complexity had to be mirrored into Rust. The C SDK doesn't pass the port into the callback; only the SDK channel handle. Removing the Rust channel from the callback loses the I2S port as a result.

You can't get the I2S port from an i2s_chan_handle_t in an ISR context. It's an opaque pointer; getting this requires a call to i2s_channel_get_info(), which isn't marked as ISR safe. Looking at the code, it's not even "pinky-promise" ISR safe as it loops through various globals to find the channel handle and locks a mutex to copy data over.

We can either store the port number alongside the callback, or just deduce it from the pointer value.

@ivmarkov

This makes the rx/tx callbacks into functions instead of traits per @ivmarkov's recommendations. This helps with a number of ergonomic issues with the code (particularly around channels and lifetimes).

This mode was only supported in the v4 driver series, and not all that well at that. This doesn't appear to be a standard I2S operating mode anyway.

dacut · 2023-06-23T01:47:10Z

I've done a quick first-round implementation of these changes, but not tested them yet. I need to see if I can change the dyn FnMut() into a proper trait at some level (or, rather why it wasn't compiling when I had that syntax in there).

ivmarkov · 2023-06-23T05:49:19Z

Looks much better. A few nits:

Rather than pushing it up to the user to provide Box<dyn FnMut>, we should ask the user for an <F: FnMut(...) -> bool + 'static> which we - in the driver - turn into a Box<dyn FnMut>. Neither is better over the other, it is just that at other places we do the F thing, so this change suggestion is only for symmetry with the rest of the code
You don't have to store the Box<dyn FnMut> thing in a static context - you can store it directly in the driver / in the tx/rx channels. At the places where we store it in a static context we do it because there is no other option, if I remember it correctly. I.e. ISR_RX_HANDLERS is maybe unnecessary?
There is no unsubscribe method. I guess we need one? Either unsubscribe, or - as we do it in other places - subscribe should provide a Subscription struct that will unsubscribe when you drop it. I'm for unsubscribe as it is a simpler approach

Finally w.r.t. double-boxing - you are doing it already, kind of. Or in a way, the user - with the current implementation - would do the first Box, and then you - in UnsafeCallback - do the second Box. The problem with the first box (Box) is simply that it is a fat pointer, and hence it cannot be cast to e.g. void*, that's all.

ivmarkov · 2023-06-23T05:50:20Z

Hey @dacut the changes above ^^^ have become quite big. How about me merging what we have already, and then you provide the final cleanups/fixes as a separate PR?

ivmarkov · 2023-06-23T05:52:48Z

@dacut I've merged it. Thanks so much for your patience and doing all the little adjustments I've asked for.

What is remaining is this. If you could open a separate PR for it?

dacut · 2023-06-23T07:30:21Z

@ivmarkov Will do.

I was running into issues with removing the Box<dyn ...> bit out from the user side for some reason that I was still looking into (rustc wasn't happy about making the trait a dyn), which was probably related to my workspace state at the time.

ivmarkov · 2023-06-23T07:57:42Z

It should be always possible to do let handler: Box<dyn FnMut(...)> = Box::new(f), where f: F where F: FnMut(...).

This makes a few changes suggested in the last bit of esp-rs#232: * Trait functions take impl FnMut instead of Box<dyn FnMut>. * Callback bookkeeping is no longer static (like the GPIO callbacks); instead, it's stored with the channel. * Added rx/tx_unsubscribe() methods to the channels. Also, fixed the naming of the I2sRxEvent/I2sTxEvent enums to be more inline with the brevity of Rust in general: * DataReceived -> RxDone * ReceiveOverflow -> RxOverflow * DataTransmitted -> TxDone * TransmitOverflow -> TxOverflow

dacut · 2023-06-23T19:57:12Z

Yep, have a new PR coming your way once my CI runs and verifies I've not done something else stupid.

This makes a few changes suggested in the last bit of esp-rs#232: * Trait functions take impl FnMut instead of Box<dyn FnMut>. * Callback bookkeeping is no longer static (like the GPIO callbacks); instead, it's stored with the channel. * Added rx/tx_unsubscribe() methods to the channels. Also, fixed the naming of the I2sRxEvent/I2sTxEvent enums to be more inline with the brevity of Rust in general: * DataReceived -> RxDone * ReceiveOverflow -> RxOverflow * DataTransmitted -> TxDone * TransmitOverflow -> TxOverflow

* Ergonomic changes for I2S callbacks. This makes a few changes suggested in the last bit of #232: * Trait functions take impl FnMut instead of Box<dyn FnMut>. * Callback bookkeeping is no longer static (like the GPIO callbacks); instead, it's stored with the channel. * Added rx/tx_unsubscribe() methods to the channels. Also, fixed the naming of the I2sRxEvent/I2sTxEvent enums to be more inline with the brevity of Rust in general: * DataReceived -> RxDone * ReceiveOverflow -> RxOverflow * DataTransmitted -> TxDone * TransmitOverflow -> TxOverflow * Fix no-std, no-alloc builds. Fix leak introduced by changing the UnsafeCallback Box<Box<...>> to a *mut Box<...> (which isn't dropped automatically).

* Ergonomic changes for I2S callbacks. This makes a few changes suggested in the last bit of esp-rs/esp-idf-hal#232: * Trait functions take impl FnMut instead of Box<dyn FnMut>. * Callback bookkeeping is no longer static (like the GPIO callbacks); instead, it's stored with the channel. * Added rx/tx_unsubscribe() methods to the channels. Also, fixed the naming of the I2sRxEvent/I2sTxEvent enums to be more inline with the brevity of Rust in general: * DataReceived -> RxDone * ReceiveOverflow -> RxOverflow * DataTransmitted -> TxDone * TransmitOverflow -> TxOverflow * Fix no-std, no-alloc builds. Fix leak introduced by changing the UnsafeCallback Box<Box<...>> to a *mut Box<...> (which isn't dropped automatically).

dacut mentioned this pull request Apr 7, 2023

i2s support? #205

Closed

Vollbrecht requested changes Apr 7, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

src/i2s.rs Outdated Show resolved Hide resolved

src/i2s.rs Outdated Show resolved Hide resolved

src/i2s.rs Outdated Show resolved Hide resolved

src/i2s.rs Outdated Show resolved Hide resolved

dacut force-pushed the i2s branch from 3bbcc47 to 47b350e Compare April 7, 2023 15:51

ivmarkov requested changes Apr 9, 2023

View reviewed changes

dacut mentioned this pull request Jun 7, 2023

Allow I2S builds on v4.4. esp-rs/esp-idf-sys#207

Merged

dacut added 5 commits June 13, 2023 23:40

A few cfg fixes for clippy.

caf8dd4

Make unsafe decls match for create_rx/tx.

de5feb5

This isn't strictly necessary in alloc mode, but it makes the code easier to use.

Add missing field in clone().

5e76ea6

Actually put comm_fmt in clone().

d7050d9

Don't create pools on v4.4.

718e42b

Channels aren't used, so pools aren't needed (and the types aren't valid).

dacut force-pushed the i2s branch from d3e0688 to 718e42b Compare June 14, 2023 17:53

ivmarkov requested changes Jun 18, 2023

View reviewed changes

ivmarkov requested changes Jun 19, 2023

View reviewed changes

Derive Debug, Default.

e829ec8

Add Debug derivations where missing. Add Default derivations to enums now that MSRV supports them.

dacut added 2 commits June 22, 2023 18:35

Make callbacks functions.

6df65cc

This makes the rx/tx callbacks into functions instead of traits per @ivmarkov's recommendations. This helps with a number of ergonomic issues with the code (particularly around channels and lifetimes).

Remove PCM compression support.

458ad52

This mode was only supported in the v4 driver series, and not all that well at that. This doesn't appear to be a standard I2S operating mode anyway.

dacut added 3 commits June 22, 2023 18:51

I2S_PORT_COUNT, UnsafeCallback unused in 4.4.

7255e21

Remove duplicate cfg block.

436064b

Use core::slice instead of std::slice.

48058da

ivmarkov merged commit de7dee4 into esp-rs:master Jun 23, 2023

dacut mentioned this pull request Jun 23, 2023

Ergonomic changes for I2S callbacks. #269

Merged

Implement I2S #232

Implement I2S #232

Conversation

dacut commented Apr 7, 2023

Vollbrecht left a comment

Choose a reason for hiding this comment

Vollbrecht commented Apr 7, 2023 • edited Loading

ivmarkov commented Apr 7, 2023

Vollbrecht commented Apr 7, 2023

dacut commented Apr 7, 2023

Vollbrecht commented Apr 7, 2023

ivmarkov commented Apr 9, 2023

dacut commented Apr 9, 2023

dacut commented Apr 9, 2023

dacut commented Apr 13, 2023 • edited Loading

dacut commented Apr 20, 2023

Vollbrecht commented Apr 21, 2023

dacut commented Apr 21, 2023

dacut commented Apr 22, 2023 • edited Loading

dacut commented Apr 25, 2023

dacut commented Jun 3, 2023

ivmarkov commented Jun 4, 2023

ivmarkov commented Jun 7, 2023

dacut commented Jun 7, 2023

ivmarkov commented Jun 7, 2023

dacut commented Jun 7, 2023

ivmarkov commented Jun 7, 2023

dacut commented Jun 7, 2023

dacut commented Jun 7, 2023

dacut commented Jun 14, 2023

ivmarkov commented Jun 15, 2023

ivmarkov left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacut commented Jun 22, 2023

dacut commented Jun 23, 2023

ivmarkov commented Jun 23, 2023

ivmarkov commented Jun 23, 2023

ivmarkov commented Jun 23, 2023

dacut commented Jun 23, 2023

ivmarkov commented Jun 23, 2023

dacut commented Jun 23, 2023

Vollbrecht commented Apr 7, 2023 •

edited

Loading

dacut commented Apr 13, 2023 •

edited

Loading

dacut commented Apr 22, 2023 •

edited

Loading

ivmarkov left a comment •

edited

Loading