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

Don't Merge: Initial working commit of an async uarte #40

Closed
wants to merge 6 commits into from

Conversation

jamesmunns
Copy link
Member

@jamesmunns jamesmunns commented Jan 3, 2019

This branch is using (a hacked up version of) bbqueue to act as a SPSC.

Won't build because it relies on some hacked up repos I needed to get it working, but worth sharing anyway.

Edit: This is now based on the released 0.2.1 version of BBQueue.

…PSC.

Won't build because it relies on some hacked up repos I needed to get it working,
but worth sharing anyway.
@jamesmunns
Copy link
Member Author

Note: This branch uses (essentially) the 0.0.2 release of BBQueue. I'll update this PR later today to split the hacked up interface into a new file (uarte::UarteAsync or so), so that this can live side by side with the existing blocking Uarte.

Copy link
Contributor

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

I left some comments in response to your question in #11. This is not a full review.

By the way, how familiar are you with Jorge's article about DMA? I don't recall enough of the details to know whether it will be helpful here, but I've immersed myself in it in the past and believe it is a good approach. (For reference, here's my own partial implementation.)

nrf52-hal-common/src/uarte.rs Outdated Show resolved Hide resolved
nrf52-hal-common/src/uarte.rs Outdated Show resolved Hide resolved
nrf52-hal-common/src/uarte.rs Outdated Show resolved Hide resolved
@jamesmunns
Copy link
Member Author

By the way, how familiar are you with Jorge's article about DMA? I don't recall enough of the details to know whether it will be helpful here, but I've immersed myself in it in the past and believe it is a good approach. (For reference, here's my own partial implementation.)

I'm not terribly familiar, unfortunately. This is approaching the problem in a different way though (I think), leaning less heavily on futures style abstractions, and rather being a more straightforward "stream" implementation. I think it could definitely be possible to impl Reader and Writer with this.

To be honest, I'm open to having multiple parallel versions of each peripheral, perhaps abstracting some of the common behavior to a central mod.rs. Since each "style" of peripheral requires ownership of the -pac peripheral, we don't have to worry about the user trying to mix and match.


/// Return the raw interface to the underlying UARTE peripheral
pub fn free(self) -> T {
self.periph
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 disable interrupts and close any open DMA transactions here, and we should maybe disable the UARTE peripheral before giving it back.

}


pub struct Pins {
Copy link
Member Author

Choose a reason for hiding this comment

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

Stuff like this should probably move to a shared mod.rs between uarte.rs and uarte_async.rs.

use crate::uarte_async::DmaState::*;
let mut stat = Idle;

::core::mem::swap(&mut stat, &mut DMA_STATUS);
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 add a note here that this is "safe" to do, since we are in interrupt condition, and I don't think it is possible for interrupts to be re-entrant.

Err(_) => panic!("queue error"),
};

let sz = min(255, rgrant.buf.len());
Copy link
Member Author

Choose a reason for hiding this comment

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

This should not be 255, but instead something that makes sense for both the 832 and 840


// Set up the DMA write
uart.txd.ptr.write(|w|
// We're giving the register a pointer to the stack. Since we're
Copy link
Member Author

Choose a reason for hiding this comment

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

Comments are no longer true

@hannobraun
Copy link
Contributor

@jamesmunns

By the way, how familiar are you with Jorge's article about DMA? I don't recall enough of the details to know whether it will be helpful here, but I've immersed myself in it in the past and believe it is a good approach. (For reference, here's my own partial implementation.)

I'm not terribly familiar, unfortunately. This is approaching the problem in a different way though (I think), leaning less heavily on futures style abstractions, and rather being a more straightforward "stream" implementation. I think it could definitely be possible to impl Reader and Writer with this.

To be honest, I'm open to having multiple parallel versions of each peripheral, perhaps abstracting some of the common behavior to a central mod.rs. Since each "style" of peripheral requires ownership of the -pac peripheral, we don't have to worry about the user trying to mix and match.

Thanks for the explanation. I agree that parallel approaches are probably the right way to go at this point. Maybe one approach will "win" and we can deprecate some or all of the others in time, but right now too much is in flux, I think. Rust's async story is still developing and it's not clear to me how that will affect nb/embedded-hal and HALs in general.

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.

2 participants