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

Add support for USART hardware flow control #269

Merged
merged 8 commits into from Aug 7, 2020
Merged

Add support for USART hardware flow control #269

merged 8 commits into from Aug 7, 2020

Conversation

hannobraun
Copy link
Member

No description provided.

pub fn disable_cts_throttling(
self,
) -> (Tx<I, Enabled<W>, NoThrottle>, Function) {
interrupt::free(|_| {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if you're planning to change that later, but this currently isn't needed, since we're only changing the cfg register in the Usart enable function and in Tx stuff. I'd also prefer if users have the option to supply the cs separately, in case they know better (e.g. when using rtic) & not disabling interrupts via magic, but there are definitely cons to that approach as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Even if we could do without a critical section here, I don't think we should, unless we're very sure the API is no longer going to change much. It's way too easy to make a change elsewhere and invalidate the assumptions made here.

Having the user supply the critical section token is an interesting idea. I'll look into it later. I'm a bit concerned how the move required by this API is going to interact with an interrupt::free closure at the call site, but I think it shouldn't be a problem.

This is necessary to support repeated enabling and disabling of that
function. Otherwise the SWM function could not be returned again without
runtime overhead.
They are no longer unused.
It is no longer unused.
@hannobraun
Copy link
Member Author

Rebased on top of master. No changes were necessary.

@david-sawatzke: I thought a bit about your suggestion to have the user pass the critical section token. I think that's definitely better, but it's one more thing that complicates the API and needs to be documented. I then considered to accept the token in an Option, to ease the burden on the user, but couldn't quite decide whether that's a good pattern or over-engineering.

In the end, I took the easy way out and decided to leave it as is (you already approved it, after all). I would welcome a future improvement here, but think for now it is fine. The critical section is really short, so the impact is minimal anyway.

@hannobraun hannobraun merged commit 5fbe54b into lpc-rs:master Aug 7, 2020
@hannobraun hannobraun deleted the usart-flow-control branch August 7, 2020 10:30
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

2 participants