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

Refactor K20 to use ioreg! #129

Merged
merged 5 commits into from Oct 19, 2014

Conversation

Projects
None yet
3 participants
@bgamari
Contributor

bgamari commented Aug 8, 2014

No description provided.

@bgamari

This comment has been minimized.

Contributor

bgamari commented Aug 8, 2014

@bharrisau you may be interested in this. It hasn't been tested yet however.

@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Aug 8, 2014

Hmm, the brace alignment to make way for the doc comments is disappointing.

@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Aug 8, 2014

And there will need to be some thinking in how to handle SIM. It changes between devices with different peripheral loadouts. Might end up having to take an ID and converting it to a register and bit pair.

@bgamari

This comment has been minimized.

Contributor

bgamari commented Aug 8, 2014

@bharrisau I agree concerning the brace alignment issue and was thinking of implementing inner docstrings (e.g. //!) as a way to fix this.

@bgamari

This comment has been minimized.

Contributor

bgamari commented Aug 8, 2014

@bharrisau My only experience with the K20 series is with the MK20DX128. It's possible (likely?) that the SIM block changes in different family members. If you have any insights here I'd love to hear.

@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Aug 8, 2014

Better wait for @farcaller, style is his thing ;)

@@ -6,6 +6,7 @@
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0

This comment has been minimized.

@farcaller

farcaller Aug 8, 2014

Member

You lost your cursor :)

@@ -37,10 +43,38 @@ pub fn plugin_registrar(reg: &mut Registry) {
reg.register_macro("ioregs", macro_ioregs);
}

pub fn get_reg_dump_root(cx: &ExtCtxt) -> Option<Path> {
for mi in cx.cfg.iter() {
match mi.node {

This comment has been minimized.

@farcaller

farcaller Aug 8, 2014

Member

I'd actually prefer to use if block ant let (xx,yy) = unpacking to this match block, it's quite hard to parse it.

This comment has been minimized.

@bgamari

bgamari Aug 8, 2014

Contributor

Oops, I actually hadn't necessarily intended on including this patch in this set. The patch is intended to allow users to export register definitions for use with external tooling (e.g. gdb) but at the moment the output format is not terribly useable.

@farcaller

This comment has been minimized.

Member

farcaller commented Aug 8, 2014

The brace alignment is truly quite a mess, also, "in-line" apidocs are //!, not ///, no? I'd prefer something like

/// Port control register
0x0 => reg32 pcr[32] {
  /// Pull direction select
  0 => ps {
    0 => PULL_DOWN,
    1 => PULL_UP
  }
...
@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Aug 8, 2014

If parsing isn't an issue, that matches what we have done elsewhere.

@bgamari

This comment has been minimized.

Contributor

bgamari commented Aug 8, 2014

@farcaller I agree that the brace alignment isn't great. I'll admit that I'm not terribly familiar with this aspect of Rust's docstring syntax, //! sounds like what I had intended to implement. I do worry, however, that placing field docstrings on separate lines is going to greatly bloat the format, essentially doubling the size. Perhaps this would be acceptable?

/// Port control register
0x0 => reg32 pcr[32] {
  0 => ps {                //! Pull direction select
    0 => PULL_DOWN,
    1 => PULL_UP
  }

It will take a bit of parsing acrobatics, but I think it should be doable.

@farcaller

This comment has been minimized.

Member

farcaller commented Aug 8, 2014

placing field docstrings on separate lines is going to greatly bloat the format

Same problem as docstrings in struct definitions :-(

Having support for both /// and //! would be nice.

@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Aug 8, 2014

If you hadn't done such a good job of implementing the DSL I was going to suggest a syntax that let the compiler handle all the parsing using item_decorators and item_modifiers.

#[zinc_block]
pub struct Foo {
  /// BAR register
  #[offset(0)] bar: Bar
}

#[zinc_register(u32)]
struct Bar {
  /// Pull direction select
  #[bit(0)] ps: bool,
  #[bits(1, 2)] #[ro] baz: BazEnum
}

We could revisit it if the maintenance on the parser gets too much.

@bgamari

This comment has been minimized.

Contributor

bgamari commented Aug 14, 2014

@bharrisau somehow I missed your comment until now. I hadn't known about item decorators until you mentioned them but even now that I do, I still weakly believe that a dedicated parser is the right way to do things. Using Rust syntax here strongly detracts from the readability and verifiability of the code. Especially given how (relatively) simple the parser is, I think it's worth the code.

In other news, I intend on fixing the comment situation (implementing proper inner and outer docstrings) this weekend, if not earlier.

@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Aug 14, 2014

Yes, the iroeg parser provides a much much nicer language than doing it in Rust with attributes. It just comes down to the decision between maintaining our parser, or letting Rust handle it. In your PR, the parser was only about half the code anyway the rest was generation of the ioreg stuff.

As I'm not the one maintaining it, I prefer the DSL.

@bgamari

This comment has been minimized.

Contributor

bgamari commented Aug 14, 2014

I quickly worked inner doc comment support into the parser this afternoon and updated the k20 branch. Let me know how this looks. Not that I'm not entirely sure that the use of inner doc comments after non-block items is sane.

@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Aug 18, 2014

Reminder to remove all the ioreg changes from this PR.

hacknbot added a commit that referenced this pull request Aug 18, 2014

Merge pull request #129 from bgamari/ioreg-k20
Refactor K20 to use ioreg!

Reviewed-by:

hacknbot added a commit that referenced this pull request Aug 18, 2014

Merge pull request #129 from bgamari/ioreg-k20
Refactor K20 to use ioreg!

Reviewed-by:

hacknbot added a commit that referenced this pull request Aug 18, 2014

Merge pull request #129 from bgamari/ioreg-k20
Refactor K20 to use ioreg!

Reviewed-by:

hacknbot added a commit that referenced this pull request Aug 18, 2014

Merge pull request #129 from bgamari/ioreg-k20
Refactor K20 to use ioreg!

Reviewed-by:

hacknbot added a commit that referenced this pull request Aug 19, 2014

Merge pull request #129 from bgamari/ioreg-k20
Refactor K20 to use ioreg!

Reviewed-by:

hacknbot added a commit that referenced this pull request Aug 19, 2014

Merge pull request #129 from bgamari/ioreg-k20
Refactor K20 to use ioreg!

Reviewed-by:

@bgamari bgamari force-pushed the bgamari:ioreg-k20 branch 5 times, most recently from dc48f73 to 36cdf1d Aug 28, 2014

@bgamari bgamari force-pushed the bgamari:ioreg-k20 branch 2 times, most recently from 04edc59 to c9a95ff Oct 19, 2014

@bgamari bgamari force-pushed the bgamari:ioreg-k20 branch from c9a95ff to a60c9e2 Oct 19, 2014

bgamari added a commit that referenced this pull request Oct 19, 2014

Merge pull request #129 from bgamari/ioreg-k20
Refactor K20 to use ioreg!

@bgamari bgamari merged commit 1c90cad into hackndev:master Oct 19, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details

@farcaller farcaller removed the in progress label Oct 19, 2014

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