Skip to content

Use tock-registers rather than volatile-register and clean up API #6

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

qwandor
Copy link
Contributor

@qwandor qwandor commented Apr 6, 2022

This PR does quite a few things.

  1. The biggest change is using tock-registers rather than volatile-register. tock-registers is used by the cortex-a crate, so it makes sense for users on Cortex A at least to have fewer dependencies. tock-registers itself has no dependencies, so we're trading three dependencies (bitflags, volatile-register and vcell) for one. It also makes for nicer and somewhat more typesafe code, as tock-registers has macros to generate types (including enums) for bit fields.
  2. I've removed some of the public methods from the MmioUart8250 struct which were providing direct access to registers. If clients need functionality not provided by the wrapper methods then that functionality should be added to this crate, rather than having clients directly access registers.
  3. I've marked MmioUart8250::new as unsafe. The caller needs to make sure that the address being passed in really is the base address of appropriate MMIO registers, and not an unmapped address, or the address of some other memory. Otherwise it would let safe code read and write arbitrary memory, which is unsound.
  4. I added some tests.

@qwandor
Copy link
Contributor Author

qwandor commented Apr 6, 2022

Oh right, and tock-registers does currently require nightly, but that should change when Rust 1.61 is released in May, which is planned to stabilise const_fn_trait_bound.

@qwandor qwandor force-pushed the registers branch 2 times, most recently from 5f0211b to 78e5165 Compare April 6, 2022 15:52
@duskmoon314
Copy link
Member

Wow, this seems very reasonable and it is a breaking change.

I’m not familiar with tock-registers and I don’t know whether it is a better alternative to volatile-register. The rust embedded book and cortex-mare using volatile-register or its dependency vcell, so I used it at the beginning of this crate. Maybe it is better but I prefer a stable choice for this crate. Thus I would like to discuss this after the stabilization.

@qwandor
Copy link
Contributor Author

qwandor commented Apr 7, 2022

Fair, I'll try to split out the other changes into a separate PR so we can get that in in the meantime.

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