Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate I8080 driver to a move based API #2191

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Dominaezzz
Copy link
Contributor

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.
  • I have added necessary changes to user code to the Migration Guide.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

Similar to #1856 but for the I8080 driver.

Starting a transfer now moves the driver into the transfer object. This allows you to store the transfer object in a static (for interrupts) or in a struct/enum.

Starting a transfer now takes a DmaTxBuffer rather than a plain slice. This allows you to bring your own DMA data structure, and also allows you to prepare the next transfer whilst one is already in progress.

This PR also adds support for cancelling a transfer on the spot.

Future enhancements

  • Refactor the I8080Transfer (and the SpiDmaTransfer) struct into a common DmaTransfer struct like so, I8080Transfer<'d, DmaChannel0, Blocking, BUF> -> DmaTransferRx<'d, DmaChannel0, BUF, I8080<'d, Blocking>>.
  • Update the example to use more interesting DmaTxBuffer implementation that does RLE for the frame buffer.
  • Enhance DmaTxBuf to take a generic param which would allow more primitive types. e.g. DmaTxBuf<u16>, DmaTxBuf<embedded_graphics::Rgb565>, etc.

cc @JurajSadel to run the example again, this is the last one (but no promises 🙂).
cc @liebman to review as well as this should allow you to easily use PSRAM with this driver after #2161 lands.

Reviewer note: Don't let the diff of the diff in the migration guide confuse you.

Testing

Ran the example and HIL tests

esp-hal/src/lcd_cam/lcd/i8080.rs Show resolved Hide resolved
examples/src/bin/lcd_i8080.rs Show resolved Hide resolved
examples/src/bin/lcd_i8080.rs Show resolved Hide resolved
hil-test/tests/lcd_cam_i8080.rs Show resolved Hide resolved
esp-hal/src/lcd_cam/lcd/i8080.rs Show resolved Hide resolved
@bugadani
Copy link
Contributor

What do you think about introducing with_buffer that would return a simplified API, like SpiDma -> SpiDmaBus? Is this in the plans, or am I missing something why it's not feasible?

@Dominaezzz
Copy link
Contributor Author

What do you think about introducing with_buffer that would return a simplified API, like SpiDma -> SpiDmaBus? Is this in the plans, or am I missing something why it's not feasible?

Can do. I have a comment in the example explaining why I didn't but I can add a really simple wrapper.

@bugadani
Copy link
Contributor

Can do. I have a comment in the example explaining why I didn't but I can add a really simple wrapper.

Mmm I guess if there are common variants we might want to do it, but don't spend time just to humour me if I don't understand something :)

@Dominaezzz
Copy link
Contributor Author

The blocking case is somewhat straight forward but for the async case, when the user drops the future, should I cancel the transfer or let it complete in the background like SPI? I'll probably skip async for now.

On second thought, I'd rather skip it. This driver isn't too popular I think and the few folks that use it can copy the bus implementation in the example and tweak it to their needs.

@Dominaezzz Dominaezzz mentioned this pull request Sep 18, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants