SPI RX/TX DMA and move to embedded-dma (continuation of #165)

[korken89]

Continuation of #165

Closes #123

[korken89]

Working example added now, the following trace is from an logic analyzer:

Which also prints (with MISO connected to GND):

buf pre: 0x[f0, aa, 0, ff, f]
buf post: 0x[0, 0, 0, 0, 0]

And the following with MISO to VCC:

buf pre: 0x[f0, aa, 0, ff, f]
buf post: 0x[ff, ff, ff, ff, ff]

I think this PR is ready for review!

[MathiasKoch]

I am not sure i can provide much feedback on this, as i don't even have any hardware that uses SPI. I can do a small test of the UART part of it, when i get the time.

Other than that, it looks like a huge improvement in ergonomics, just from reading through it.

[korken89]

Thanks for having a look!
If you can give the UART a go it would be great, I have been testing the SPI extensively so I think we are on the clear there.

And I must agree, the ergonomics significantly improved! :D

[David-OConnor]

FYI, I started with embedded-dma, and ended up going with plain arrays to keep complexity down.

[korken89]

@David-OConnor Thanks for the input! And interesting choice of API.

How come you chose to not use embedded-dma? I assume you need the same type bounds yourself to not tread into UB land (as we did before this PR).
A problem I see if you were to go with pain arrays, is how would one use owning buffers with the APIs?
Maybe you have a different use-case in mind?

[David-OConnor]

I'm on the fence on this and may eventually switch back, but it seems like these traits complicate user code without adding much value. Ie, I'm leaving it to the user to make sure the array's memory location is managed properly.

For example, you could have a mutable buffer set up in the main function, and pass it as a mutable reference to the read_dma method of a peripheral struct. With buf being the mutable reference, the HAL passes buf.as_mut_ptr() as u16 as the memory address, and buf.len() as u16 as the ndtr value. Writing DMA is similar, but without mutability. This keeps both user code, and HAL code simple and transparent.

This API is still very much subject to change, but I'm keeping complexity to a minimum until specific abstractions (like embedded-dma) become valuable enough to be worth the complexity they add.

[David-OConnor]

Some overall thoughts overall after reading the code.

1: This is a solid addition, and improves the library by adding important functionality. Merging as-is would be great; could clean up in future PRs.

2: There are a number of abstractions here between the spi and dma modules, which results in jumping around between different parts of these files for what amounts to a set of linear steps, described by the reference manuals. This has no impact on code execution, and enables some useful abstractions. It makes code review (ie looking for bugs and places to improve) and maintenance tougher.

3: Related to 2: You could document the reference manual steps using comments (perhaps verbatim) quotes, to improve maintainability.

[korken89]

Thanks for the feedback!

I'll add notes to the RM, these will indeed help.

On the jumping between files, this is unfortunately the old structure of the HAL, would one want to change this one would need to do a major redesign of the internals.
I think we are approaching that this could be worth it for easier readability - I agree on that the jumping is not easy to follow, plus the heavy use of macros instead of generics causes e.g. rust-analyzer to not be able to follow the code...

[korken89]

@David-OConnor I added references to the RM, is there something more we should add on that front?

[David-OConnor]

Looks great man!

[korken89]

Super! I just need to test run the changes on HW so nothing weird sneaked in.

[korken89]

I did some testing and mostly it looks good, but check the CS pin in the following image.

This is generated with the following code, and I am not sure why it returns early so the pin can de-assert.

    let buf = unsafe {
        static mut BUF: [u8; 5] = [0xf0, 0xaa, 0x00, 0xff, 0x0f];
        &mut BUF
    };

    rprintln!("buf pre: 0x{:x?}", &buf);
    
    cs.set_low().ok();
    let transfer = dma_spi.transfer(buf);
    let (buf, dma_spi) = transfer.wait();
    cs.set_high().ok();

    rprintln!("buf post: 0x{:x?}", &buf);

The RX and TX DMA needs to say they have read/sent all data before wait() returns.
Any ideas what can cause the earlier return?

[korken89]

I have checked a bit more, and the SPIs busy flag is also de-asserted before the transfer completes.
I will have to start combing the RM and checking erratas now.

[korken89]

I have now finished analyzing this and it's up to specification and by design as far as I understand. All internal flags happen with the sampling instant.
SPI allows deassert of the CS line, as long as all flanks with data are out (in either direction), which we adhere to.
Moreover, I have tested this driver on an ADXL357 accelerometer, LIS3DHH accelerometer, and an ICM20602 IMU - all which work fine.

After all this testing I'd like to say that this is ready for merge.
Ping @MathiasKoch and @David-OConnor - any objections?

[David-OConnor]

Looks great!

[MathiasKoch]

I think it look great! Awesome job on this! It's a huge step in the right direction! 👌

[korken89]

Thanks for all the reviews!