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

ioreg syntax extension #108

Merged
merged 9 commits into from Aug 7, 2014

Conversation

Projects
None yet
6 participants
@bgamari
Copy link
Contributor

commented Jul 17, 2014

I'll need to write something more coherent here but here is the continuation of the ioreg work originally discussed in #90. I've restructured and cleaned up the builder code substantially and getter and setter objects have been implemented. Note that this currently requires some compiler bits that have yet to be merged to compile. You can find rustdoc output for the code generated by a simple example here. More on this later when it's not as late.

I would apply the proper label to indicate this is work in progress but it seems I don't have permission to do so.

Remaining work

  • Examine options for reducing documentation clutter
@bgamari

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2014

@bharrisau I've updated the rustdoc upload, feel free to have a look. The FTM_STATUS register has set_to_clear flags.

use super::super::node;

/// A visitor to build accessor functions for each register struct
pub struct BuildAccessors<'a, 'b, 'c> {

This comment has been minimized.

Copy link
@farcaller

farcaller Jul 17, 2014

Member

Any reason not to unify lifetimes here?

This comment has been minimized.

Copy link
@bgamari

bgamari Jul 17, 2014

Author Contributor

Vladimir Pouzanov notifications@github.com writes:

+/// A visitor to build accessor functions for each register struct
+pub struct BuildAccessors<'a, 'b, 'c> {

Any reason not to unify lifetimes here?

I played around with these for quite a while to make the borrow checker
happy. That being said, things have changed substantially since then, so
perhaps they can now be unified. I'll have a look later.

let docstring = match field.docstring {
Some(d) => {
let s = "*[".to_string() + access_tag.to_string()
+ "]* " + token::get_ident(d.node).get();

This comment has been minimized.

Copy link
@farcaller

farcaller Jul 17, 2014

Member

to_string() calls here makes quite a mess, I'd suggest using format!()

This comment has been minimized.

Copy link
@bgamari

bgamari Jul 17, 2014

Author Contributor

Vladimir Pouzanov notifications@github.com writes:

  •  let s = "*[".to_string() + access_tag.to_string()
    
  •    + "]\* " + token::get_ident(d.node).get();
    

to_string() calls here makes quite a mess, I'd suggest using format!()

Done.

None => vec!(),
};

quote_item!(

This comment has been minimized.

Copy link
@farcaller

farcaller Jul 17, 2014

Member

Please keep cx on the same line if you do calls to quote_*, that adds to the readability of quoted context.

This comment has been minimized.

Copy link
@bgamari

bgamari Jul 17, 2014

Author Contributor

Vladimir Pouzanov notifications@github.com writes:

  • quote_item!(

Please keep cx on the same line if you do calls to quote_*, that
adds to the readability of quoted context.

Agreed and done.

DUMMY_SP,
ast::TyFixedLengthVec(u8_ty,
self.cx.expr_uint(DUMMY_SP, length)));
Spanned {

This comment has been minimized.

Copy link
@farcaller

farcaller Jul 17, 2014

Member

there's respan for that.

This comment has been minimized.

Copy link
@bgamari

bgamari Jul 17, 2014

Author Contributor

Vladimir Pouzanov notifications@github.com writes:

  •    Spanned {
    

there's respan for that.

Good point; that's a nice cleanup. I've propagated this throughout the
patch.

@farcaller

This comment has been minimized.

Copy link
Member

commented Jul 17, 2014

I've added a few notes on the code. I will have some more on the style later on when this settles a bit, but overall it's very nice, great job!

@bgamari

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2014

@farcaller, @bharrisau what do we think about peripheral instantiation? I'm tempted to punt on bit-banding support for now as I don't have a lot of time recently and I'd like to get this merged eventually.

@bharrisau

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2014

I'll throw bit-banding in later. The easiest is to use weak linkage of the bitband memory address (and I'll be able to do a memory map macro when I get a chance that will generate that).

What do you mean by peripheral instantiation? As in, generating the #[link_name="armmem_SYSTICK"] pub static SYSTICK: SYSTICKReg line? I'd leave that out, and we can do it in platformtree or a memory map macro. (I'd like to shift the device specific stuff one level higher than HAL - memory map and pin map. Leave HAL to handle the vendor/architecture specific stuff)

@bgamari

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2014

@bharrisau That sounds reasonable enough.

Unfortunately it seems like something has broken with the recent rustc/zinc changes. After rebasing I'm seeing the following unhelpful build failure,

rustc --opt-level 2 -Z no-landing-pads --cfg cfg_mcu_has_spi --cfg mcu_lpc17xx --cfg arch_cortex_m3 --target thumbv7m-linux-eabi -Ctarget-cpu=cortex-m3 -C relocation_model=static      -L /opt/exp/zinc/build  --out-dir /opt/exp/zinc/build  /opt/exp/zinc/src/main.rs 
/opt/exp/zinc/src/main.rs:1:1: 1:1 error: expected ident, found `=`
/opt/exp/zinc/src/main.rs:1 // Zinc, the bare metal stack for rust.
@bharrisau

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2014

If I had to guess - there is an '=' used incorrectly in a macro.

@bharrisau

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2014

Maybe somewhere you are calling .expect_ident?

@farcaller

This comment has been minimized.

Copy link
Member

commented Jul 30, 2014

No, this is rust-lang/rust#15972. Build with -O0.

@farcaller

This comment has been minimized.

Copy link
Member

commented Jul 30, 2014

(I've seen the same problem in #116)

@bgamari

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2014

@farcaller it seems like the fix has made it upstream. I'm building a new rustc as we speak. Well done tracking this one down, by the way.

@bgamari

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2014

This morning I brushed off these patches, rebased them, and did a bit more porting of K20. I found a few bugs and cleaned up the documentation a bit further in the process. At this point I feel pretty good about merging so folks can finally start using this in anger.

What do people think?

// limitations under the License.

use syntax::ast;
use syntax::ast::{P};

This comment has been minimized.

Copy link
@farcaller

farcaller Aug 6, 2014

Member

Remove {/} here and on next line.


impl<'a, 'b, 'c> node::RegVisitor for BuildAccessors<'a, 'b, 'c> {
fn visit_prim_reg(&mut self, path: &Vec<String>, reg: &node::Reg,
_width: node::RegWidth, fields: &Vec<node::Field>) {

This comment has been minimized.

Copy link
@farcaller

farcaller Aug 6, 2014

Member

Why not just _?

}

fn build_field_accessors<'a>(cx: &'a ExtCtxt, path: &Vec<String>,
reg: &node::Reg, field: &node::Field)

This comment has been minimized.

Copy link
@farcaller

farcaller Aug 6, 2014

Member

Please offset the repeating lines at 4 spaces (relative to fn start) here and everywhere.

This comment has been minimized.

Copy link
@bgamari

bgamari Aug 6, 2014

Author Contributor

I'll admit that I'm not a huge fan of this aspect of the style guidelines as it makes it much less clear where the function's signature ends and the body begins. That being said, I've made the changes.

This comment has been minimized.

Copy link
@thehydroimpulse

thehydroimpulse Aug 6, 2014

fyi, the rust style guide is to align subsequent arguments with the first argument. https://github.com/rust-lang/rust-guidelines/blob/master/style/whitespace.md#line-wrapping (not sure if that's against the Zinc style guide or not)

fn frobnicate(a: Bar, b: Bar,
              c: Bar, d: Bar)
              -> Bar {
    ...
}

This comment has been minimized.

Copy link
@farcaller

farcaller Aug 6, 2014

Member

Thinking of it, I just don't like the "dangling" -> part, really. And I like how

fn func(arg1,
    arg2) -> Whatever {
  let first_line = 1;
}

makes a clear visual distinction on where the body starts. Let's move this into ML maybe? No sense to make a major restyling work because of that.

This comment has been minimized.

Copy link
@farcaller

farcaller Aug 6, 2014

Member

Yes, and the same is actually in google c++ style guide so I don't know what's wrong with me :-) keep it as is.

This comment has been minimized.

Copy link
@bgamari

bgamari Aug 6, 2014

Author Contributor

@farcaller just to make sure we are on the same page, what style would you prefer here?

This comment has been minimized.

Copy link
@farcaller

farcaller Aug 6, 2014

Member

Keep it as it is now. However, if you can fit everything in one line given that line starts with 4-space indent — that would be preferred.

This comment has been minimized.

Copy link
@bgamari

bgamari Aug 6, 2014

Author Contributor

Alright.

// limitations under the License.

use syntax::ast;
use syntax::ast::{P};

This comment has been minimized.

Copy link
@farcaller

farcaller Aug 6, 2014

Member

ditto.

// See the License for the specific language governing permissions and
// limitations under the License.

use std::gc::{Gc};

This comment has been minimized.

Copy link
@farcaller

farcaller Aug 6, 2014

Member

ditto.

-> Option<P<ast::Item>> {
match field.ty.node {
node::EnumField { variants: ref variants, .. } => {
// FIXME: We construct a path, then only take the last

This comment has been minimized.

Copy link
@farcaller

farcaller Aug 6, 2014

Member

that's FIXME(bgamari)

quote_method!(cx,
$doc_attr
pub fn $fn_name<'a>(&'a mut self, new_value: $field_ty)
-> &'a mut $setter_ty {

This comment has been minimized.

Copy link
@farcaller

farcaller Aug 6, 2014

Member

4 spaces here.

$doc_attr
pub fn $fn_name<'a>(&'a mut self, new_value: $field_ty)
-> &'a mut $setter_ty {
self.value |= (self.value & ! $mask) | ((new_value as $unpacked_ty) & $mask) << $shift;

This comment has been minimized.

Copy link
@farcaller

farcaller Aug 6, 2014

Member

and 2 spaces here.

}

/// Size of registers of register group in bytes
pub fn regs_size(regs: Gc<Vec<Reg>>) -> uint {

This comment has been minimized.

Copy link
@farcaller

farcaller Aug 6, 2014

Member

Any reason to use Gc, not Rc?

This comment has been minimized.

Copy link
@bgamari

bgamari Aug 6, 2014

Author Contributor

I'm following rustc's model and using Gc for now. Really this probably should just be P to make things easier when rustc transitions to Rc.

This comment has been minimized.

Copy link
@farcaller

farcaller Aug 6, 2014

Member

Can you please switch to Rc whenever you don't interact with rustc/syntax? That was the suggestion I got on #rust, and it seems to be quite reasonable.

This comment has been minimized.

Copy link
@bgamari

bgamari Aug 6, 2014

Author Contributor

Sure.

This comment has been minimized.

Copy link
@bgamari

bgamari Aug 6, 2014

Author Contributor

Actually, this means that I'll need to change a lot of box<Gc>s to Rc::new(...). I could do this, but it seems like it might be better to switch to Rc once box works with it as syntactically it's much cleaner.

This comment has been minimized.

Copy link
@farcaller

farcaller Aug 6, 2014

Member

Ok then, that's not a big deal.


pub trait RegVisitor {
/// Path includes name of `Reg` being visited
fn visit_prim_reg<'a>(&'a mut self, _path: &Vec<String>, _reg: &'a Reg,

This comment has been minimized.

Copy link
@farcaller

farcaller Aug 6, 2014

Member

Why all the _?

This comment has been minimized.

Copy link
@bgamari

bgamari Aug 6, 2014

Author Contributor

Because they're not used.

This comment has been minimized.

Copy link
@farcaller

farcaller Aug 6, 2014

Member

Yes, sorry. I mean: why using those in a trait?

This comment has been minimized.

Copy link
@bgamari

bgamari Aug 6, 2014

Author Contributor

Do you mean why do I define dummy visitors?

This comment has been minimized.

Copy link
@farcaller

farcaller Aug 6, 2014

Member

facepalm didn't notice the default implementation, sorry, all that makes sense now.

src/main.rs Outdated
@@ -57,6 +58,8 @@ pub mod os;

// TODO(farcaller): clean up when fixed.
#[cfg(not(test))]
mod std {
pub mod std {

This comment has been minimized.

Copy link
@farcaller

farcaller Aug 6, 2014

Member

How is this used?

This comment has been minimized.

Copy link
@bgamari

bgamari Aug 6, 2014

Author Contributor

I actually can't remember at this point but things seem to build without it; I'll revert it.

@farcaller

This comment has been minimized.

Copy link
Member

commented Aug 6, 2014

Thanks for all the work, looks nice.

I've left a few comments in for you. Also, can you split the macro part and the new ioreg implementations to separate PRs?

@bgamari

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2014

I almost forgot: My tree currently has deny(missing_doc) commented out as many of the register definitions don't have documentation. In general, I think we should accept a slightly lower standard of documentation for the register interfaces than the high-level library interfaces. The register interfaces have fewer users and frankly I think it's reasonable to expect that you'll need to datasheet by your side when you are poking at registers.

Would there be any objection to my adding allow(missing_doc) annotations to items produced by ioregs!?

@farcaller

This comment has been minimized.

Copy link
Member

commented Aug 6, 2014

Would there be any objection to my adding allow(missing_doc) annotations to items produced by ioregs!?

I'm fine with that. It might be useful to add a reference to where in the datasheet you can find the register description, but those fluctuate quite as well. Maybe chapter number / name would be fine?

@bgamari

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2014

@farcaller as far as I can tell things look good for merge.

@errordeveloper

This comment has been minimized.

Copy link
Member

commented Aug 6, 2014

@bgamari great stuff! I don't seem to see any unit tests here, so I suppose you are intending to add them in a separate pull request...

@bgamari

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2014

@errordeveloper there is still a bit of work to be done on the documentation and testing side of things but I'd like to get this merged to get people using it and lessen the maintenance burden.

bgamari added some commits Aug 7, 2014

Rakefile: Fix build_docs rule
Need to define library path; not sure how this worked before.
let clear_mask: $unpacked_ty = $clear as $unpacked_ty;
if self.mask != 0 {
let v: $unpacked_ty = self.reg.value.get() & ! clear_mask & ! self.mask;
self.reg.value.set(self.value | v);

This comment has been minimized.

Copy link
@bharrisau

bharrisau Aug 7, 2014

Contributor

As some future work, will just double check how the generated ASM for this looks.

_ => low_bit as uint,
};

// TODO(bgamari): Do we want to enforce an order here?

This comment has been minimized.

Copy link
@bharrisau

bharrisau Aug 7, 2014

Contributor

Yes? Until we have a reason for reverse bit order (maybe for fields that we need to reverse the bits for?).

@bharrisau

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2014

Wow, awesome work. I haven't had a chance to have a thorough look (and I probably wont until my contract finishes at the end of this month).

Can someone land a PR that renames all the current ioreg! to ioreg_old! to make room for this one?

@farcaller

This comment has been minimized.

Copy link

commented on bf1fe45 Aug 7, 2014

r+

@farcaller

This comment has been minimized.

Copy link
Member

commented Aug 7, 2014

Ok, let's land this.

@hacknbot

This comment has been minimized.

Copy link
Contributor

commented on bf1fe45 Aug 7, 2014

saw approval from farcaller
at bgamari@bf1fe45

This comment has been minimized.

Copy link
Contributor

replied Aug 7, 2014

merging bgamari/zinc/ioreg-rebased = bf1fe45 into auto

This comment has been minimized.

Copy link
Contributor

replied Aug 7, 2014

bgamari/zinc/ioreg-rebased = bf1fe45 merged ok, testing candidate = 6f6bb5b

This comment has been minimized.

Copy link
Contributor

replied Aug 7, 2014

Merge sha 6047409 is stale.

This comment has been minimized.

Copy link
Contributor

replied Aug 7, 2014

merging bgamari/zinc/ioreg-rebased = bf1fe45 into auto

This comment has been minimized.

Copy link
Contributor

replied Aug 7, 2014

merging bgamari/zinc/ioreg-rebased = bf1fe45 into auto

This comment has been minimized.

Copy link
Contributor

replied Aug 7, 2014

bgamari/zinc/ioreg-rebased = bf1fe45 merged ok, testing candidate = 5955517

This comment has been minimized.

Copy link
Contributor

replied Aug 7, 2014

Merge sha 93fc12a is stale.

This comment has been minimized.

Copy link
Contributor

replied Aug 7, 2014

merging bgamari/zinc/ioreg-rebased = bf1fe45 into auto

This comment has been minimized.

Copy link
Contributor

replied Aug 7, 2014

bgamari/zinc/ioreg-rebased = bf1fe45 merged ok, testing candidate = 7bcf91c

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

Merge pull request #108 from bgamari/ioreg-rebased
ioreg syntax extension

Reviewed-by: farcaller

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

Merge pull request #108 from bgamari/ioreg-rebased
ioreg syntax extension

Reviewed-by: farcaller

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

Merge pull request #108 from bgamari/ioreg-rebased
ioreg syntax extension

Reviewed-by: farcaller

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

Merge pull request #108 from bgamari/ioreg-rebased
ioreg syntax extension

Reviewed-by: farcaller

farcaller added a commit that referenced this pull request Aug 7, 2014

Merge pull request #108 from bgamari/ioreg-rebased
Added new ioreg syntax extension

@farcaller farcaller merged commit 51e2abc into hackndev:master Aug 7, 2014

1 of 2 checks passed

default running tests for candidate 7bcf91c89c19d77908a735dbc6c477e89aca9bf7
Details
continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.