DMA buffer swap logic

[jordens]

This PR (into the dma branch) rewrites the next_transfer() and next_transfer_with() implementations.

• CurrentBuffer enums renamed to be more accurate and peripheral-matching.
• Added a few stream methods to expose the dbm/ct bits in joint semantics (also faster).
• Using usize for the addresses instead of u32.
• The non-double buffer and the double buffer code paths are unified.
• It poisons the inactive buffer address to convert silent corruption into DMA/bus errors (@adamgreig 's idea).
• With that, I'd consider next_transfer_with() safe. It's not DMA overrun safe in general but it is safe from any concurrent buffer access by user and DMA since the buffer the user has access to is always out of reach of the DMA (by poisoning or implicit disable). Certainly safe in the sense that there is nothing that the user can do to make it safer or less safe.
• Overrun errors due to due to DMA hitting the poison or writing to the active address will not go unnoticed (user needs to clear, handle, and restart).
• Errors due to next_transfer{,_with} being invoked late (DMA wrap around) can still go unnoticed. I don't think there is anything we can do about it since the peripheral will happily wrap around unattended (AFAWK). But they now are guaranteed to not lead to corruption within a buffer.
• next_transfer() just calls next_transfer_with() with triple buffering (one active, one inactive, and one user).
• fence() added.

Pending:

• This is untested on hardware.
• BDMA is not done.
• Returning a/the buffer on error might make sense.

@thalesfragoso @richardeoin @ryan-summers comments appreciated.

[thalesfragoso]

I haven't looked at all the PR yet, but one thing you could change is to use fence instead of compiler_fence + dmb, that way you avoid a function call (cortex_m::asm::dmb is never inlined on stable).

[jordens]

The cortex-m assembly is being inlined even on stable for a while now.

[thalesfragoso]

The cortex-m assembly is being inlined even on stable for a while now.

I don't think so, as far as I know, inlined assembly calls in stable was only made possible recently in cortex-m 0.7.0 and only when using linker-plugin-lto. As of today, I don't think there is a single PAC/HAL using cortex-m 0.7, which means that people can't really use 0.7 together with those crates, also, the use of linker-plugin-lto isn't widespread. (See rust-embedded/cortex-m#259 and rust-embedded/cortex-m#139)

And btw, this is the exactly use case of fence, it will insert a DMB instruction.

[ryan-summers]

In general, I think these are nice improvements and increase maintainability. There's still a race condition that I would like to resolve though. Comments inline

[ryan-summers]

Inadvertently approved when I requested changes

[jordens]

I don't think so, as far as I know, inlined assembly calls in stable was only made possible recently in cortex-m 0.7.0 and only when using linker-plugin-lto.

Ack. It felt to me like it has been there for much longer. Changed to fence().

[ryan-summers]

I think one addition to an nb::Result would be appropriate - other than that, we should likely test this on hardware, and pending that, it looks good to me otherwise.

[ryan-summers]

Instead of adding a NotReady, can we use an nb::Result::WouldBlock? I think it's essentially being used in the same way - a user needs to wait for the transfer to finish first.

[jordens]

I think that's useful. But this error should also carry the unused buffer (otherwise that's lost). The alternative would be to have another closure that gets the unused buffer in case of error, making transfer_next_with() somewhat like Result::map_or_else().

[ryan-summers]

That's a good point, but this PR doesn't support returning the unused buffer on failure anyways

[jordens]

That regression is on the TODO list above.

[ryan-summers]

It seems to me like this is something we should be doing for all users, since the DMA memory is guaranteed to be in a cache-able memory location. We should do an MVA invalidate for all addresses in the buffer if the d-cache is enabled, as the comment here indicates.

Note that there seems to be some issue here because cortex-m requires ownership of the SCB for invalidate_dcache_by_address() and cache lines must be 32-byte aligned, which means that DMA buffers must be cache-aligned when the d-cache is enabled as well.

Now that I've written all of this, it may be worth breaking out into a separate PR to add support for d-cache-enabled implementations.

https://docs.rs/cortex-m/0.7.0/cortex_m/peripheral/struct.SCB.html#method.invalidate_dcache_by_address

[ryan-summers]

It looks like that would also restrict us to using buffers that are a length that is a multiple of the cache size.

[ryan-summers]

Note that there's a lot of other cache operations that we aren't supporting (such as cleaning the active buffer to commit it to memory before starting a DMA write), so this isn't necessarily the only place where cache operation support needs to be added. Let's leave this for the future though and add a disclaimer that DMA does not support the D-cache being enabled.

[richardeoin]

Looks good to me, with the caveat that I haven't tried it on hardware either. Thanks for the improvements u32 -> usize and fence.

I'd say this can be merged into the dma branch as soon as you're happy with it, since I see the whole dma branch as experimental/unstable.

[ryan-summers]

I have tested this on hardware and verified that it functions as intended.

[richardeoin]

I've also successfully tested this on hardware. @jordens could you mark this as "ready for review" / let me know merging it into #153 is OK. We can continue any discussion there.