feat(stm32): Add DMA based, ring-buffer based rx uart, v3 by rmja · Pull Request #1404 · embassy-rs/embassy

rmja · 2023-04-27T09:05:06Z

This PR replaces #1150. Comparing to that PR, this one has the following changes:

The implementation now aligns with the new stm32 dma module, thanks @Dirbaio!
Calls to read() now returns on either 1) idle line, or 2) ring buffer is at most half full. This is different from the previous pr, which would return a lot of 1 byte reads. Thank you @chemicstry for making me realize that it was actually not what I wanted. This is accomplished using half-transfer completed and full-transfer completed interrupts. Both seems to be supported on both dma and bdma.

The implementation still have the issue mentioned here: #1150 (comment)

Regarding the todos here: #1150 (comment). I have removed the exposure of ndtr from dma::RingBuffer to the uart so that the uart now simply calls ringbuf::reload_position() to align the position within the ring buffer to that of the actual running dma controller. BDMA and GPDMA is not implemented. I do not have any chips with those dma controllers, so maybe someone else should to this so that it can be tested.

The saturate_serial test utility inside tests/utils has an --idles switch which can be used to saturate the uart from a pc, but with random idles.

Because embassy-stm32 now can have tests, we should probably run them in ci. I do this locally to test the DmaRingBuffer: cargo test --no-default-features --features stm32f429ig.

cc @chemicstry @Dirbaio

xoviat · 2023-04-30T19:24:44Z

These changes are needed to fix the build: rmja/embassy@99ade50. Other than that, I'm not pleased that we'll have a buffered.rs and an rx_ringbuffered.rs file in the uart mod. Perhaps a better name would be buffered_v2.rs? I also think it's unfortunate that write was not implemented, but that's understandable.

Anyway, the ringbuffer.rs is pub(crate), which is good; it can be modified internally to add bdma and gpdma later. And the implementation appears to be correct, and it is tested. So I don't have really anything to add there.

Dirbaio · 2023-04-30T19:35:04Z

I'm working on adding BDMA+GPDMA right now, so no need to feature-gate it.

About naming: I've been thinking about it, and I think we need to make the buffered uart the "default", and name the non-buffered one "raw". See the discussions for the embedded-hal serial traits, where "buffered vs unbuffered" was discussed and the conclusion was most of the people are better off with the buffered impls. rust-embedded/embedded-hal#349

So it could be something like:

embassy_stm32::uart -> DMA-buffered (the one added in this PR)
embassy_stm32::uart::irq_driven -> IRQ-buffered (the current "buffered uart")
embassy_stm32::uart::raw -> raw, unbuffered

I think we should do it in a follow-up PR though: this one has been delayed enough, it's RX-only for now, and the change should be done in rp, nrf at the same time.

Dirbaio · 2023-04-30T21:37:03Z

embassy-stm32/src/dma/dma.rs

+        let dma = self.regs();
+        let channel_num = self.num();
+        let index = self.index();
+        // Manually process tcif in case transfer was completed and we are in a higher priority task.


All the existing drivers already have this "problem", Making the interrupt a lower priority than the task that uses it is not supported. I wouldn't take measures to try to mitigateit.

The reason I made this was that I at some point called get_complete_count() from a uart interrupt, and the priority relation between the uart and the dma controller is not strict. I can see now that I do not do this anymore, and so this could probably be removed. But it would open for future issues if someone accidently called get_complete_count() from a uart interrupt.

Dirbaio · 2023-05-01T16:07:43Z

embassy-stm32/src/dma/dma.rs

+
+    // First, figure out if tcif is set without a cs.
+    if isr_reg.read().tcif(channel_num % 4) && cr_reg.read().tcie() {
+        // Make tcif test again within a cs to avoid race when incrementing complete_count.


what race is this avoiding?

EDIT: I guess it was due to the separate process_tcif call. I've removed it, so I think this is not needed anymore.

The race is not possible if it is guaranteed that the DMA interrupt always have the highest priority.

Dirbaio · 2023-05-01T16:21:01Z

removed process_tcif in DMA.
Added BDMA support
Feature-gated ringbuffer on chips with GPDMA for now. Circular mode in GPDMA is not trivial 😓. I'm fairly confident it can be done with the current architecture though, unlike with the previous one. It just requires learning how the crazy linked list descriptors work.
cleaned up test so that it passes in HIL.

bors try

bors · 2023-05-01T16:31:21Z

try

Build failed:

all

Dirbaio · 2023-05-01T19:15:30Z

let's see if HIL tests pass. 🙏

bors r+

1404: feat(stm32): Add DMA based, ring-buffer based rx uart, v3 r=Dirbaio a=rmja This PR replaces #1150. Comparing to that PR, this one has the following changes: * The implementation now aligns with the new stm32 dma module, thanks `@Dirbaio!` * Calls to `read()` now returns on either 1) idle line, or 2) ring buffer is at most half full. This is different from the previous pr, which would return a lot of 1 byte reads. Thank you `@chemicstry` for making me realize that it was actually not what I wanted. This is accomplished using half-transfer completed and full-transfer completed interrupts. Both seems to be supported on both dma and bdma. The implementation still have the issue mentioned here: #1150 (comment) Regarding the todos here: #1150 (comment). I have removed the exposure of ndtr from `dma::RingBuffer` to the uart so that the uart now simply calls `ringbuf::reload_position()` to align the position within the ring buffer to that of the actual running dma controller. BDMA and GPDMA is not implemented. I do not have any chips with those dma controllers, so maybe someone else should to this so that it can be tested. The `saturate_serial` test utility inside `tests/utils` has an `--idles` switch which can be used to saturate the uart from a pc, but with random idles. Because embassy-stm32 now can have tests, we should probably run them in ci. I do this locally to test the DmaRingBuffer: `cargo test --no-default-features --features stm32f429ig`. cc `@chemicstry` `@Dirbaio` Co-authored-by: pennae <github@quasiparticle.net> Co-authored-by: Rasmus Melchior Jacobsen <rmja@laesoe.org> Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>

bors · 2023-05-01T19:32:03Z

Build failed:

all

Dirbaio · 2023-05-01T19:42:53Z

bors r+

bors · 2023-05-01T19:55:00Z

Merge conflict.

…ed yet.

Dirbaio · 2023-05-01T20:43:36Z

bors r+

1404: feat(stm32): Add DMA based, ring-buffer based rx uart, v3 r=Dirbaio a=rmja This PR replaces #1150. Comparing to that PR, this one has the following changes: * The implementation now aligns with the new stm32 dma module, thanks `@Dirbaio!` * Calls to `read()` now returns on either 1) idle line, or 2) ring buffer is at most half full. This is different from the previous pr, which would return a lot of 1 byte reads. Thank you `@chemicstry` for making me realize that it was actually not what I wanted. This is accomplished using half-transfer completed and full-transfer completed interrupts. Both seems to be supported on both dma and bdma. The implementation still have the issue mentioned here: #1150 (comment) Regarding the todos here: #1150 (comment). I have removed the exposure of ndtr from `dma::RingBuffer` to the uart so that the uart now simply calls `ringbuf::reload_position()` to align the position within the ring buffer to that of the actual running dma controller. BDMA and GPDMA is not implemented. I do not have any chips with those dma controllers, so maybe someone else should to this so that it can be tested. The `saturate_serial` test utility inside `tests/utils` has an `--idles` switch which can be used to saturate the uart from a pc, but with random idles. Because embassy-stm32 now can have tests, we should probably run them in ci. I do this locally to test the DmaRingBuffer: `cargo test --no-default-features --features stm32f429ig`. cc `@chemicstry` `@Dirbaio` Co-authored-by: Rasmus Melchior Jacobsen <rmja@laesoe.org> Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>

bors · 2023-05-01T21:07:24Z

Build failed:

all

…0kb)

Dirbaio · 2023-05-01T21:35:55Z

bors r+

bors · 2023-05-01T21:55:52Z

Build succeeded:

all

rmja mentioned this pull request Apr 27, 2023

feat(stm32): Add DMA based, ring-buffer based rx uart #1150

Closed

Dirbaio reviewed Apr 30, 2023

View reviewed changes

Dirbaio reviewed May 1, 2023

View reviewed changes

Dirbaio force-pushed the stm32-uart-buffered-rx-v3 branch from 119aa3a to f987949 Compare May 1, 2023 16:18

bors bot added a commit that referenced this pull request May 1, 2023

Try #1404:

ae9887c

Dirbaio force-pushed the stm32-uart-buffered-rx-v3 branch 2 times, most recently from 80e9923 to 371f3ef Compare May 1, 2023 17:54

rmja and others added 8 commits May 1, 2023 22:42

Ring-buffered uart rx with one-period overrun detection

4945579

Do not disable dma request when idle line is detected

4ea6662

Support overflow detection for more than one ring-period

fc268df

Remove unused import

7757405

Actually clear idle flag

4584303

stm32/dma: remove separate process_tcif.

14e0090

stm32/bdma: add ringbuffer support.

25864ae

stm32/uart: feature-gate ringbuffer out when using gpdma, not support…

96e8a7d

…ed yet.

Dirbaio force-pushed the stm32-uart-buffered-rx-v3 branch from 4890aa6 to 4efc3d5 Compare May 1, 2023 20:43

Dirbaio force-pushed the stm32-uart-buffered-rx-v3 branch from 4efc3d5 to 980205a Compare May 1, 2023 21:17

stm32/dma: solve overlapping impl on DmaCtrl on stm32h7

00cde67

Dirbaio added 3 commits May 1, 2023 23:20

stm32/test: add real defmt timestamp

1806422

stm32/test: cleanup ringbuffer test, exit on success (transferring 10…

7601779

…0kb)

stm32/test: fix race condition in uart_dma.

a1d4530

Dirbaio force-pushed the stm32-uart-buffered-rx-v3 branch from 980205a to a1d4530 Compare May 1, 2023 21:35

bors bot merged commit 6096f0c into embassy-rs:master May 1, 2023

Comments

Conversation

rmja commented Apr 27, 2023

Uh oh!

xoviat commented Apr 30, 2023

Uh oh!

Dirbaio commented Apr 30, 2023

Uh oh!

Dirbaio Apr 30, 2023

Choose a reason for hiding this comment

Uh oh!

rmja May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dirbaio May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmja May 1, 2023

Choose a reason for hiding this comment

Uh oh!

Dirbaio commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors bot commented May 1, 2023

try

Uh oh!

Dirbaio commented May 1, 2023

Uh oh!

bors bot commented May 1, 2023

Uh oh!

Dirbaio commented May 1, 2023

Uh oh!

bors bot commented May 1, 2023

Uh oh!

Dirbaio commented May 1, 2023

Uh oh!

bors bot commented May 1, 2023

Uh oh!

Dirbaio commented May 1, 2023

Uh oh!

bors bot commented May 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rmja May 1, 2023 •

edited

Loading

Dirbaio May 1, 2023 •

edited

Loading

Dirbaio commented May 1, 2023 •

edited

Loading