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

First pass at implementing derivedFrom. #190

Closed
wants to merge 3 commits into from

Conversation

nsabovic
Copy link
Contributor

Trying to fix #181, this is the first pass at implementing derivedFrom. These also the very first lines I've ever written in Rust, so be gentle.

This patch requires rust-embedded/svd#49.

With this patch, I get a bit further in generating the peripherals for SAM D10.

@jamesmunns
Copy link
Member

Hey @nsabovic, sorry there hasn't been any response yet. I'll try to review and test this over the next few days, and also follow rust-embedded/svd#49 to see if anything happens there.

Thanks for contributing!

Copy link
Contributor

@wez wez left a comment

Choose a reason for hiding this comment

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

This seems to do the job, except for the (totally forgivable) rust newbie mistake with the semicolons I mentioned inline.

With that fixed, its generating code like this for me with the atsamd21 svd:

impl Deref for SERCOM4 {
    type Target = sercom0::RegisterBlock;
    fn deref(&self) -> &sercom0::RegisterBlock {
        unsafe { &*SERCOM4::ptr() }
    }
}

// an error in that case
(Ident::new(&*base.to_sanitized_snake_case()), true)
let base = if derive_regs {
Ident::new(&*p_derivedfrom.unwrap().name.to_sanitized_snake_case());
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to remove the ; from the end of the line here and in the else branch below because they're causing base to have the type () rather than the Ident type that you're constructing here, and that causes things to explode because the generated code comes out like this:

impl Deref for SERCOM4 {
    type Target = ( ) :: RegisterBlock ;
...
``

@wez
Copy link
Contributor

wez commented Mar 15, 2018

@nsabovic are you planning to do something similar for registers?

@wez
Copy link
Contributor

wez commented Mar 15, 2018

FWIW, I'd vote for inlining the helper from rust-embedded/svd#49 into this PR and it will be easier to land and integrate just a single PR; PR dependencies are a PITA :)

fn derive_peripheral(p: &Peripheral, other: &Peripheral) -> Peripheral {
    let mut derived = p.clone();
    if derived.group_name.is_none() && other.group_name.is_some() {
        derived.group_name = other.group_name.clone();
    }
    if derived.description.is_none() && other.description.is_some() {
        derived.description = other.description.clone();
    }
    if derived.registers.is_none() && other.registers.is_some() {
        derived.registers = other.registers.clone();
    }
    if derived.interrupt.is_empty() {
        derived.interrupt = other.interrupt.clone();
    }
    derived
}

@wez
Copy link
Contributor

wez commented Mar 15, 2018

Over in rust-embedded/svd#50 I've added parsing out the <register derivedFrom= attribute, and together with this PR, I have some code on top that expands derived registers: https://github.com/wez/svd2rust/tree/derivedFrom.

That resolves the bad sizing issue that @kevinmehall mentioned in #181 (comment). Actually, I think all three of those points have now been addressed and we just need to review, clean things up and land them.

@nsabovic
Copy link
Contributor Author

@wez thanks for catching that 👍

Registers, clusters and fields can all derive from others, would be nice to make a more general solution. My chip's SVD (SAM D10) doesn't have <register derivedFrom=""> in the SVD, so I spaced out on that part of the spec completely...

...but it does have alternateCluster and alternateRegister, so I'd love to have those—the API with unions, does it require using unsafe all over the place? I was thinking, since my chip has an alternate register for, say PER register in dither5 mode, to have in the impl of PER register a function called as_dither5(), which would give you a dither5 view of the same register, while also taking a reference so you can't access it in both modes at the same time. Seems rustier, and also capitalizes on the fact that there is a default register, and the alternate register. What do you think?

...which were preventing the correct mod being generated for derived
registers.
@mathk mathk added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 21, 2019
@mathk
Copy link

mathk commented Feb 21, 2019

Ping from triage: This need to be review. Also merge/rebase is needed.

@mathk mathk added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Feb 21, 2019
@Disasm
Copy link
Member

Disasm commented Mar 16, 2019

Ping from triage: @nsabovic could you rebase your changes?

@Disasm Disasm requested a review from jamesmunns March 16, 2019 16:36
@ryankurte ryankurte requested a review from a team as a code owner March 17, 2019 23:36
@ryankurte
Copy link

bors try

bors bot added a commit that referenced this pull request Mar 17, 2019
@ryankurte ryankurte self-assigned this Mar 17, 2019
@bors
Copy link
Contributor

bors bot commented Mar 17, 2019

try

Build failed

Copy link

@ryankurte ryankurte left a comment

Choose a reason for hiding this comment

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

looks like a few things need to be fixed after rebase (sorry @nsabovic!)

@@ -76,10 +76,10 @@ pub fn render(
     }
 
     // erc: *E*ither *R*egister or *C*luster
-    let ercs = p.registers.as_ref().map(|x| x.as_ref()).unwrap_or(&[][..]);
+    let ercs: Vec<Either<Register, Cluster>> = p.registers.as_ref().map(|x| x.as_ref()).unwrap_or(&[][..]);
 
     let registers: &[&Register] = &util::only_registers(&ercs)[..];
-    let clusters = util::only_clusters(ercs);
+    let clusters = util::only_clusters(&ercs);
 
     // No `struct RegisterBlock` can be generated
     if registers.is_empty() && clusters.is_empty() {
@@ -90,7 +90,7 @@ pub fn render(
 
     // Push any register or cluster blocks into the output
     let mut mod_items = vec![];
-    mod_items.push(register_or_cluster_block(ercs, defaults, None, nightly)?);
+    mod_items.push(register_or_cluster_block(&ercs, defaults, None, nightly)?);
 
     // Push all cluster related information into the peripheral module
     for c in &clusters {

all_peripherals.iter().find(|x| x.name == *s)
});

let p_merged = p_derivedfrom.map(|x| p_original.derive_from(x));

Choose a reason for hiding this comment

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

where does the Peripheral::derive_from(&mut self, p: Peripheral) method come from? (this might have got disappeared during rebase..?)

@michalfita
Copy link

Does anyone has working branch with stuff for supporting derivedFrom? I tried @wez's and @nsabovic's and both fail to build.

@therealprof
Copy link
Contributor

@nsabovic Sorry for the long delay. Are you still interested in working on this? If so would you mind bringing the PR into a working state?

@wez
Copy link
Contributor

wez commented May 23, 2019

#256 is the successor to this PR

@nsabovic
Copy link
Contributor Author

I am not really set up to look at this right now. If you need it urgently, I'm not the best bet.

@burrbull burrbull mentioned this pull request Jul 26, 2019
@adamgreig
Copy link
Member

Closing in preference of now merged #319 which is built on this and #256.

@adamgreig adamgreig closed this Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing peripherals in the generated file
9 participants