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.

Sign up for GitHub

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

Jump to bottom

Improve split between USART's receiver and transmitter #210

Merged
merged 5 commits into from
Feb 18, 2020
Merged

Improve split between USART's receiver and transmitter #210

merged 5 commits into from
Feb 18, 2020

Conversation

hannobraun
Copy link
Member

No description provided.

@hannobraun
Refactor the way TXDAT's address is retrieved
3a3eb5c 
This change doesn't make a whole lot of sense by itself, but it is
necessary for something else I'm working on (see next commit). Since
that commit is already big enough, I've extracted this change into a
separate one.
@hannobraun
Improve split between USART's Rx and Tx
89ddb6c 
It was already possible to split a USART peripheral into a receiver and
transmitter, but that feature was mostly useless. Both still borrowed
the original USART, which severely limited the use cases. It wasn't
possible to move a receiver or transmitter somewhere else, without also
moving the USART or moving the USART into a `static`.

This commit changes this, by creating a strong split that doesn't
require any borrowing. `Rx` and `Tx` can just be moved out of `USART`
and used independently. To do anything with `USART` that would affect
either of them (disabling it, for example), they have to be moved back
in.

The `RegProxy` stuff got a bit crazy here, but I wasn't able to find a
simpler solution. The only alternative I can think of is not use
`RegProxy` and instead unsafely access the peripheral everywhere it is
used.
@hannobraun
Implement various traits on USART
fbf9ffb 
This is a convenience feature that makes functionality accessible on
`USART` directly, as long as `Rx` or `Tx` haven't been moved out.
@hannobraun
Simplify examples
ee70ed9
@hannobraun
Copy link
Member Author

Please don't merge. I'm having problems downstream, possibly related to receiving. I'll investigate.

@hannobraun
Copy link
Member Author

Okay, whatever my problem is, I don't think it's related to this pull request. Now everything works again. Since the whole thing happened at the same time GDB/OpenOCD were acting up, I'm inclined to blame those two.

I'll keep an eye out, but for now, I don't think there's anything wrong with this pull request. Feel free to review/merge.

@david-sawatzke
Copy link
Member

I really dislike the magic numbers for the TXDAT address.
What do you think about using the REGISTER_BLOCK in the Instance:

const REGISTER_BLOCK: *const pac::usart0::RegisterBlock = <pac::$instance>::ptr();

and getting the address itself inside end_addr:

unsafe { &(*I::REGISTER_BLOCK).txdat as *const _ as *mut u8 }

?

With that, you could remove all other additions to the trait, which would simplify the code quite a bit and be worth it. (In my opinion, certainly not a blocker)

LGTM otherwise

@timokroeger
Copy link

I also use the method described by david in the EFM32PG12-HAL.
Actually all that is required is this trait with ptr() method which acts as super trait for the peripheral instance.

@hannobraun
Copy link
Member Author

Thank you for the suggestions, both of you. I've pushed a new commit that gets rid of the magic numbers.

@david-sawatzke

With that, you could remove all other additions to the trait, which would simplify the code quite a bit and be worth it. (In my opinion, certainly not a blocker)

I might be misunderstanding what you're saying, but I think the other additions to the trait are required because of RegProxy. At least I can't think of a way to get rid of them, without getting rid of RegProxy.

We could just get rid of RegProxy, of course, but then we'd need an unsafe block everywhere we're using a register. As far as I can tell, this is what all other HALs are doing. Maybe we should just do that, as the safety of RegProxy is dubious anyway (the constructor should be unsafe).

RegProxy is useful to "move" registers somewhere else, but in cases where multiple APIs use the same registers (like here), it probably doesn't provide and advantage. In fact, it's probably more error-prone than unsafe everywhere, as it hides what's going on, and what's required to keep everything sound.

But in any case, I'd prefer to merge this pull request as-is and continue this discussion elsewhere (I've opened #211). Should we decide to get rid of RegProxy, I'm happy to do the leg-work of removing it and simplifying the code currently using it.

@hannobraun hannobraun mentioned this pull request Feb 18, 2020
Open
david-sawatzke
david-sawatzke approved these changes Feb 18, 2020
View reviewed changes
Copy link
Member

@david-sawatzke david-sawatzke left a comment

Choose a reason for hiding this comment

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

Looks good now!

@david-sawatzke david-sawatzke merged commit 3ad71ea into lpc-rs:master Feb 18, 2020
@hannobraun hannobraun deleted the usart branch February 19, 2020 06:21
@hannobraun hannobraun mentioned this pull request Feb 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-sawatzke david-sawatzke david-sawatzke approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hannobraun @david-sawatzke @timokroeger