Support PSRAM in DmaTxBuf

[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.
• I have added necessary changes to user code to the Migration Guide.
• 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

This PR adds support for PSRAM in DmaTxBuf allowing DMA transfers out from PSRAM.

Some observations:

• it seems to only work with octal PSRAM
• there seem to be some edge cases where
  • changing the CHUNK_SIZE makes it work or fail
  • changing the BUFFER_SIZE makes it work or fail

Testing

• I added an example
• I added a hardware test that passes

[bugadani]

(and 100% fails on an esp32s3 with psram-2m)

Lovely, maybe it has something to do with the reduced bandwidth of QSPI vs OSPI?

[liebman]

(and 100% fails on an esp32s3 with psram-2m)

Lovely, maybe it has something to do with the reduced bandwidth of QSPI vs OSPI?

Actually I think it was because one of my modules only has quad psram-2m and I had it configured for opsram-2m

example seems to work now

[Dominaezzz]

LGTM fwiw

[jessebraham]

Thanks, LGTM!

[bugadani]

Thanks, provided that the tests pass, this looks good to me, too.

[liebman]

?? Its 32. `DmaBufBlkSize::Size32` Where do you see 1?

…

On Tue, Sep 24, 2024 at 3:06 PM Dániel Buga ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In hil-test/tests/spi_half_duplex_write_psram.rs <#2161 (comment)>: > + Context { + spi, + pcnt_unit: pcnt.unit0, + pcnt_source: mosi_loopback, + } + } + + #[test] + #[timeout(3)] + fn test_spi_writes_are_correctly_by_pcnt(ctx: Context) { + const DMA_BUFFER_SIZE: usize = 4; + const DMA_ALIGNMENT: DmaBufBlkSize = DmaBufBlkSize::Size32; + const DMA_CHUNK_SIZE: usize = 4096 - DMA_ALIGNMENT as usize; + + let (_, descriptors) = dma_descriptors_chunk_size!(0, DMA_BUFFER_SIZE, DMA_CHUNK_SIZE); + let buffer = dma_alloc_buffer!(DMA_BUFFER_SIZE, DMA_ALIGNMENT as usize); Is alignment = 1 intentional here? — Reply to this email directly, view it on GitHub <#2161 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANWPXEBWTLMBAL4OQEBEODZYHO6HAVCNFSM6AAAAABOIDUVYGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMRWGU3DKNJUHE> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[bugadani]

I mixed up the two enums. 🤷‍♂️ There likely should be only one, and we should convert it differently depending on the place of use.

[bugadani]

I don't think we do any validation here, so in theory, could we simplify all this? Could we, instead of erroring if block_size is None, assume the strictest alignment requirement, and use that for our chunk size? If yes, do we even need manually specifying block_size? The difference between 4080 and 4032 byte chunks sounds mostly negligible to me.

[Dominaezzz]

I don't think we do any validation here

Isn't this the validation? Oh you mean that we don't check that the buffer aligns to the block size? That's because it's TX, fair it could be skipped.

imo the user should be able to choose and the choice should be explicit. Using bigger block sizes has bandwidth consequences for PSRAM.

Would moving the psram support to a different type get out of your way?

[bugadani]

Would moving the psram support to a different type get out of your way?

This isn't exactly in my way, I was just wondering. But I missed the bandwidth part of the question so I'm fine with the rest :)