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

Create crate for Tock Registers #984

Merged
merged 4 commits into from Jun 21, 2018

Conversation

Projects
None yet
6 participants
@brghena
Copy link
Contributor

brghena commented Jun 12, 2018

Pull Request Overview

This PR creates a separate crate for the Tock register interface (named tock-regs) and moves the code to tools/tock-register-interface/. While this change affects many files throughout Tock, almost all of them are just changes to the import statement. Additionally, Cargo.toml files have changed to reference the new crate, and documentation has been updated.

The driver for this change is to allow other projects to use or extend Tock registers. In order to support this, the mask and shift fields of Field as well as the mask and value fields of FieldValue are made public.

An example of using this crate in an external project can be found here based off of examples from @niklasad1 and @andre-richter. This example both uses Tock registers and extends them with additional functionality. Unfortunately, the first build of this example repo takes a considerable amount of time (several minutes) because using the Tock registers crate requires a clone of the entire Tock repo. Subsequent builds are quick, however.

Closes #930.

Testing Strategy

Compiling all boards.

TODO or Help Wanted

None

Documentation Updated

  • Kernel: Updated the relevant files in /docs, or no updates are required.
  • N/A Userland: Added/updated the application README, if needed.

Formatting

  • Ran make formatall.
@andre-richter

This comment has been minimized.

Copy link
Contributor

andre-richter commented Jun 12, 2018

Awesome, thanks a lot for the effort. Unfortunately, currently not much free time for hobby projects on my side, but I will definitely check this out once time permits.

pub mod regs;

#[macro_use]
pub mod macros;

This comment has been minimized.

@niklasad1

niklasad1 Jun 12, 2018

Member

So the only thing I would want is to export the structs directly here by:

pub use regs::{ReadOny, WriteOnly, LocalRegisterCopy, Field, FieldValue}

Then if I use this crate it's enough to

extern crate tock_regs;
use tock_regs::ReadWrite;

Instead of

extern crate tock_regs;
use tock_regs::regs::ReadWrite;

This comment has been minimized.

@brghena

brghena Jun 12, 2018

Author Contributor

That seems like a good suggestion. I will do that.

@@ -293,8 +290,8 @@ impl<R: RegisterLongName> From<LocalRegisterCopy<u32, R>> for u32 {
/// Specific section of a register.
#[derive(Copy, Clone)]
pub struct Field<T: IntLike, R: RegisterLongName> {
mask: T,
shift: u32,
pub mask: T,

This comment has been minimized.

@niklasad1

niklasad1 Jun 12, 2018

Member

Why are these fields made pub?

This comment has been minimized.

@brghena

brghena Jun 12, 2018

Author Contributor

So that other users can extend the register system. For example: https://github.com/brghena/tock-register-example/blob/master/src/cpu_regs.rs#L16

It is annoying that they have to public though. Do you have any suggestions on a better way to do this?

This comment has been minimized.

@niklasad1

niklasad1 Jun 14, 2018

Member

Ok, I see. I don't think it possible to leak private items without getters.

@niklasad1
Copy link
Member

niklasad1 left a comment

Looks good 👍

use kernel::common::StaticRef;
use tock_regs::regs::{ReadOnly, ReadWrite};

This comment has been minimized.

@bradjc

bradjc Jun 12, 2018

Contributor

I think it would be better to have the kernel crate depend on the new tock_regs crate, and re-export the regs they way they are now so we don't need all of these changes.

This comment has been minimized.

@brghena

brghena Jun 13, 2018

Author Contributor

What's the rationale for re-exporting them through the kernel? I'm happy to make the change, but I'm not sure why it would be preferable compared to directly referencing the tock_regs crate. Do we expect all libraries to be re-exported through the kernel in Tock?

This comment has been minimized.

@bradjc

bradjc Jun 13, 2018

Contributor

It would mean not having to change all these files every time we do this (since there is a motion to move the cells too), and it seems like all of these helper functions should come from the same place (and I think it makes sense to stick with them coming from the kernel trait).

This comment has been minimized.

@niklasad1

niklasad1 Jun 13, 2018

Member

@bradjc

Do you mean that only the kernel imports the crate(tock_regs) and then exports as pub?

For example in lib.rs:

extern crate tock_regs;

pub use tock_regs;

Then others just use use kernel::tock_regs::ReadOnly?!

This comment has been minimized.

@bradjc

bradjc Jun 13, 2018

Contributor

No, the kernel lib.rs does whatever it has to so that chips use kernel::common::regs::ReadOnly as they do now.

This comment has been minimized.

@phil-levis

phil-levis Jun 13, 2018

Collaborator

I agree with Brad that centralizing the dependencies within Tock (by re-exporting through the kernel crate) is a good idea.

This comment has been minimized.

@niklasad1

niklasad1 Jun 14, 2018

Member

Ah, makes sense!

@bradjc
Copy link
Contributor

bradjc left a comment

Idea: export tock_regs through the kernel.

@phil-levis
Copy link
Collaborator

phil-levis left a comment

I agree with @bradjc suggestion to re-export.

@alevy alevy force-pushed the seperate-registers branch from d3509fa to b4b210c Jun 19, 2018

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jun 19, 2018

Better @phil-levis and @bradjc? Now none of the crates have changed at all, except kernel which re-exports tock-regs and the Cargo.lock files which now include tock-regs as a sub-dependency through kernel.

@bradjc
Copy link
Contributor

bradjc left a comment

Looks good

@alevy alevy dismissed stale reviews from bradjc and themself via 2bfcb3e Jun 19, 2018

@brghena

This comment has been minimized.

Copy link
Contributor Author

brghena commented Jun 19, 2018

This looks good to me. Thanks so much for making those changes Amit!!

brghena added some commits May 11, 2018

Make Tock Register Interface its own crate
The only real changes here are making the tock-register-interface crate,
and the `Cargo.toml` and `lib.rs` file connections to it in `arch/` and
`chips/`.

Tested on Hail, where the Hail app still Hails.

@brghena brghena force-pushed the seperate-registers branch from 2bfcb3e to 9294bad Jun 19, 2018

@brghena

This comment has been minimized.

Copy link
Contributor Author

brghena commented Jun 19, 2018

(rebased on master again since we had all the pulls today)

@brghena brghena referenced this pull request Jun 19, 2018

Merged

A directory for crates #1006

2 of 2 tasks complete
#![no_std]

extern crate tock_regs;

pub use tock_regs::{register_bitfields, register_bitmasks};

This comment has been minimized.

@niklasad1

niklasad1 Jun 19, 2018

Member

Is it really worth introducing another unstable feature just for using extern macros over #[macro_use(register_bitfields, register_bitmasks)]?

OT: I don´t like this syntax for using macros confuses me with ordinary imports but but .........

@niklasad1
Copy link
Member

niklasad1 left a comment

Ok, grumble on using #![feature(use_extern_macros)] because we are not consistent through the codebase. In all other places, we are using #[macro_use]

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jun 19, 2018

@niklasad1

on using #![feature(use_extern_macros)] because we are not consistent through the codebase. In all other places, we are using #[macro_use]

Don't they serve different purposes? I thought #![feature(use_extern_macros)] was the only way to export macros imported from a different crate, while #[macro_use] only works when importing macros from a sub-module.

@niklasad1

This comment has been minimized.

Copy link
Member

niklasad1 commented Jun 20, 2018

@alevy

Yes, you right never mind (clearly haven´t done my homework)
But, a super module can’t import a submodule’s macros

Addressed concern

@bradjc bradjc added the last-call label Jun 21, 2018

@phil-levis
Copy link
Collaborator

phil-levis left a comment

My one comment is that if it isn't already there, putting Shane's name somewhere in the documentation would be useful. This was less important when it was internal but if the goal is for other people to use it too giving credit in the files is good.

@bradjc bradjc dismissed stale reviews from phil-levis, niklasad1, alevy, and themself via da8cfca Jun 21, 2018

@bradjc

bradjc approved these changes Jun 21, 2018

@bradjc bradjc merged commit 435f4d3 into master Jun 21, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@bradjc bradjc deleted the seperate-registers branch Jun 21, 2018

@andre-richter

This comment has been minimized.

Copy link
Contributor

andre-richter commented Jun 24, 2018

Thanks for carving this out!

Any plans to upload it to crates.io with a license? Would make it a bit easier to include it and reuse some snippets where needed in one's own crates :)

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jun 25, 2018

Would pulling it from the gihub repo in cargo.toml not work for now? I'm not sure how everyone feels but I don't think we are ready to support that as a component on crates.io.

@andre-richter

This comment has been minimized.

Copy link
Contributor

andre-richter commented Jun 25, 2018

Not at all, that is perfectly fine. The question was more towards the licensing. But I overlooked the license files in the tock root, and thought the project is unlicensed. My bad, nevermind 😅.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment