Implement async for lcd_cam i8080

[liebman]

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

• I have updated existing examples or added new ones (if applicable).
• I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
• My changes were added to the CHANGELOG.md in the proper section.
• My changes are in accordance to the esp-rs API guidelines

Extra:

• I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

Description

Implement async i8080

Testing

new HIL tests
Local Hub75 application

[liebman]

One issue is that I8080::new() takes the TX part of a channel and this does not carry the Async marker and that means that there is no way prove that the TX channel passed is appropriate. The quick fix would be to have I8080::new() take the entire Channel.

[Dominaezzz]

I've had this one in my mental backlog but I'm glad someone else has gotten to it first.

I love that you've kept the blocking method around even when the driver is in "async" mode, so the new send_dma_async is like an optional extra.

One issue is that I8080::new() takes the TX part of a channel and this does not carry the Async marker and that means that there is no way prove that the TX channel passed is appropriate. The quick fix would be to have I8080::new() take the entire Channel.

When it comes to one shot DMA transfers, where you provide a filled buffer upfront and you just wait for the transfer to finish. You don't actually need an async DMA channel (and should probably avoid it maybe) to make the driver async, you just need to use the peripheral's (LCD_CAM) own interrupt. LCD_TRANS_DONE or something like this.
Some other drivers in the hal (like SPI) asynchronously wait for the DMA to finish, then blockingly drain the peripheral's FIFO, which is undesirable (I'm drafting a PR to fix it).
You can avoid that problem here by just using the LCD_CAM's interrupts directly.
(The channel async marker is more useful for circular transfers)

Since the LCD_CAM only has one interrupt handler (shared between the LCD and Camera), you'll have to move the new_async method up to the LcdCam struct and decide the async-ness there. I imagine the struct would look like this.

struct LcdCam<'d, DM> {
    lcd: Lcd<'d, DM>,
    cam: Cam<'d>,
}

I'm not too sure what to do with the shared INT_ENA register but since there's no set_interrupt_handler method on the LcdCam, I think you can skip the use of a critical section for now. Whoever adds async support to the Camera driver can worry about that 🙂.

[liebman]

Maybe we can do something similar to the way interrupts are handled in gpio::IO?

[Dominaezzz]

You mean the bit about allowing the user to setup their own handler even in async mode?

[Dominaezzz]

Code looks great!

[liebman]

You mean the bit about allowing the user to setup their own handler even in async mode?

Yes and this could extend to a by default implementation that creates seperate handlers for Camera and Lcd.

[Dominaezzz]

You mean the bit about allowing the user to setup their own handler even in async mode?

Yes and this could extend to a by default implementation that creates seperate handlers for Camera and Lcd.

My only qualm against it is having to come up with some kind of contract about what the user handler is allowed to do.
i.e. Don't clear x interrupt bits, don't disable the interrupt, etc.
I also worry about the contract changing (the hal isn't stable yet) in such a way that users don't notice their interrupt handler is now broken after upgrading the hal in their Cargo.toml.
And also #1525 which I think could use more Send consideration.

I'd rather this sort of thing wait until the interrupt story in the HAL is more fleshed out.

[liebman]

I added tests to make sure that:

• a blocking i8080 works with a blocking DMA channel
• a blocking i8080 works with an async DMA channel
• an async i8080 works with a blocking DMA channel
• an async i8080 works with an async DMA channel

When the TX and RX parts of a DMA channel carry the Mode then this can be removed and we can enforce Blocking with Blocking and Async with Async.

[Dominaezzz]

LGTM

Ideally send_dma_async should call self.tear_down_send() if the future it returns is cancelled but this hal in general doesn't support cancellation so I consider this optional.

[bjoernQ]

LGTM - I don't have the HW for it but maybe would be worth to at least re-test the blocking implementation example (@JurajSadel ?)

For the future would be good to have a way to have more checks in the test but at least we test it's not freezing or crashing the system

[JurajSadel]

LGTM, thanks!

[liebman]

rebased and update for ReadBuffer change

[jessebraham]

Thanks!

[liebman]

Looks like the CI failure was a timeout to api.github.com