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

UART DMA #535

Merged
merged 10 commits into from
Dec 30, 2022
Merged

UART DMA #535

merged 10 commits into from
Dec 30, 2022

Conversation

9names
Copy link
Member

@9names 9names commented Dec 24, 2022

Add DMA support for UART peripheral.
Tested with basic loopback example - works fine for single characters but explodes if you send many at once (you can reproduce this by holding a key, so it's not even that fast)

@9names
Copy link
Member Author

9names commented Dec 24, 2022

Now it's not even doing as well as before - it either sends nothing or gibberish.

@9names
Copy link
Member Author

9names commented Dec 24, 2022

Okay, the swapped dreq is what was causing the failures.
I think we're still not clearing some fault state on startup though - once things go weird they stay that way until the reset button is pressed.

@9names 9names changed the title WIP: UART DMA UART DMA Dec 24, 2022
@9names
Copy link
Member Author

9names commented Dec 24, 2022

Yep, while this code works we're definitely hitting some DMA issues with it.
If you run the example from a hard reset it works fine.
If you then launch it again via probe-run, it stops at the first DMA write.
If you break, then then launch it yet again via probe-run, it starts spitting out the binary (which makes all my terminal programs very upset).

@9names 9names marked this pull request as ready for review December 24, 2022 11:54
The existing code called reset_bringup() on split, which worked from a
hard-reset but failed on a soft reset (eg - reset via debugger).
Added a reset_bring_down call before reset_bringup to ensure that DMA is in
a sensible state before using it.
@9names
Copy link
Member Author

9names commented Dec 24, 2022

We weren't resetting the DMA peripheral during init.
It worked the first time because the peripheral was in reset after hard-reset, but not after a soft reset.
The UART DMA demo now works reliably

@jannic
Copy link
Member

jannic commented Dec 25, 2022

We weren't resetting the DMA peripheral during init.
It worked the first time because the peripheral was in reset after hard-reset, but not after a soft reset.

IIRC probe-run doesn't reset the device after flashing, it just jumps to the entry point. This may be a bug in probe-run.

@9names
Copy link
Member Author

9names commented Dec 25, 2022

All the other peripherals were already doing

self.reset_bring_down(resets);
self.reset_bring_up(resets);

on startup to ensure they're in a good state.

DMA and PLL are presently the only things that only do

self.reset_bring_up(resets);

@jannic
Copy link
Member

jannic commented Dec 25, 2022

All the other peripherals were already doing

self.reset_bring_down(resets);
self.reset_bring_up(resets);

Just a side note: I wonder if we need a way to skip this reset. After all, the watchdog can be configured to not reset the whole chip but only parts of it. There are probably applications where you'd want to keep some peripherals running when the watchdog fires.

For now, I'd prefer to keep things consistent and reset all peripherals on initialization. (PLL might be tricky - is it possible to get to that point while some clocks are already configured to run from PLL?)

@9names
Copy link
Member Author

9names commented Dec 25, 2022

Just a side note: I wonder if we need a way to skip this reset

The C SDK and examples do not reset most of the peripherals, which suggests that this should be possible.
The only use-case I can think of currently is the dapper-mime 2nd core mode, where you need to avoid reinitialising the entire chip in order to keep core1 communicating via USB.

@@ -238,7 +240,7 @@ impl<B: WriteBuffer> WriteTarget for B {
/// transfers involving peripherals commonly have to wait for data to be available or for available
/// space in write queues. This type defines whether the sink or the source shall pace the transfer
/// for peripheral-to-peripheral transfers.
#[derive(Clone, Copy)]
#[derive(Clone, Copy, Debug)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd likely also be nice to have it derive defmt when available.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should probably add more derives than that to the public-facing types too. Nice catch

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this and the DMAError struct to use

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]

@9names 9names merged commit 33dd1c6 into rp-rs:main Dec 30, 2022
@9names 9names mentioned this pull request Jul 12, 2023
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.

None yet

3 participants