Skip to content

Disjoint arrays #662

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

Merged
merged 3 commits into from
Nov 7, 2022
Merged

Disjoint arrays #662

merged 3 commits into from
Nov 7, 2022

Conversation

n8tlarsen
Copy link
Contributor

Solution 2 for #660 which pre-parses ERC type names and provides expansion information to the render functions. Works great with a few SVDs from the Renesas RA DFP which tend to use disjoint arrays quite frequently.

@n8tlarsen n8tlarsen requested a review from a team as a code owner October 6, 2022 00:29
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @reitermarkus (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools labels Oct 6, 2022
Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

also needs a cargo fmt

@burrbull
Copy link
Member

burrbull commented Oct 7, 2022

Let's make release before merging this. @Emilgardis ?

@n8tlarsen n8tlarsen requested a review from Emilgardis October 7, 2022 15:22
@Emilgardis
Copy link
Member

Let's make release before merging this. @Emilgardis ?

Yes lets!

@burrbull
Copy link
Member

burrbull commented Oct 7, 2022

rebase, please

@n8tlarsen
Copy link
Contributor Author

I know re-writing history that is publicly available is generally bad practice, but there were a number of conflicts with master that needed to be resolved in order to rebase.

@Emilgardis
Copy link
Member

I know re-writing history that is publicly available is generally bad practice

we generally approve of doing that in prs, especially since thats when you have the opportunity, feel free to squash the commits, for now it's fine though as we're still reviewing this

@n8tlarsen
Copy link
Contributor Author

I found an issue with the check_erc_reprs function when there are registers that use derivedFrom intermingled with registers that could be matched to a disjoint array

@n8tlarsen
Copy link
Contributor Author

I also think the include_info fields of Repr could be simplified by instead utilizing the derived_from field of an erc, which I'd like to do before merging. Should I move this PR to a draft or is it ok for now?

@Emilgardis
Copy link
Member

convert it to a draft if you feel there's more to be done 👍🏼

@n8tlarsen n8tlarsen marked this pull request as draft October 7, 2022 21:26
@n8tlarsen
Copy link
Contributor Author

Hey everyone I reworked this to take advantage of the deriveFrom system and I think the logic is a bit easier to follow. Essentially instead of selectively including the register info, I'm allowing regex matched registers to implicitly deriveFrom others. I need to update some of the comments yet, but I'm running into an issue where derived registers each append a pub use <derived_name> in the library, causing a a bunch of rustc error[E0255]: the name <derived_name> is defined multiple times. Here's an example elc.rs.txt generated from R7FA2A1AB.svd.

@n8tlarsen n8tlarsen force-pushed the disjoint-array branch 4 times, most recently from 3390a42 to e3c240a Compare October 18, 2022 20:18
@n8tlarsen n8tlarsen marked this pull request as ready for review October 19, 2022 02:24
@Emilgardis Emilgardis requested a review from a team October 19, 2022 16:20
@n8tlarsen
Copy link
Contributor Author

I think the derive info method is preferable as it adds the least complexity. Let me know and I can squash out the old Representation method.

@burrbull
Copy link
Member

burrbull commented Nov 6, 2022

yet one rebase on master, please

@burrbull
Copy link
Member

burrbull commented Nov 6, 2022

Could you also show examples of source -> generated code?

@n8tlarsen
Copy link
Contributor Author

Happy to share some examples. Here's a patched source from the Renesas FSP r7fa2a1ab.svd.patched. Patch addressed some enumeratedValues issues only. The file includes both explicit deriveFrom keys as well as some registers which the PR identifies as ImplicitDerive. The first interesting generated file contains instances handling ImplicitDerive cases: elc.rs. This file is quite long but illustrates good handling of multiple deriveFrom keys where the registers are arrays and the PR renames them to avoid name collisions: pfs.rs. Specifically one instance of P40%sPFS derives from P000PFS while another derives from P407PFS. You can find other sources in PAC project I'm working on: https://github.com/ra-rs/ra. I'm just waiting on this PR to push PACs for many of the SVDs.

@burrbull
Copy link
Member

burrbull commented Nov 7, 2022

ok. LGFM now.
Fix changelog

@burrbull
Copy link
Member

burrbull commented Nov 7, 2022

Could you also check issues: #283, #169 and #357 ?

burrbull
burrbull previously approved these changes Nov 7, 2022
@n8tlarsen
Copy link
Contributor Author

After merging, this might be a good opportunity to improve/expand the CI a bit by adding tests against one or more of the Renesas SVDs I've referenced since they exercise this feature extensively. Not sure how that's accomplished but I'm happy to help.

@n8tlarsen
Copy link
Contributor Author

Fixed a comment typo when I did a find/replace:

-    // Locate conflicting regions; we'll need to use unions to derive_infoesent them.
+    // Locate conflicting regions; we'll need to use unions to represent them.

@n8tlarsen
Copy link
Contributor Author

Also cleaned up / reverted some debug items:

Click to expand
--- a/src/generate/peripheral.rs
+++ b/src/generate/peripheral.rs
@@ -1227,7 +1227,6 @@ fn render_ercs(
             RegisterCluster::Register(reg) => {
                 trace!("Register: {}, DeriveInfo: {}", reg.name, derive_info);
                 let mut rpath = None;
-                let before_name = reg.name.to_string();
                 if let DeriveInfo::Implicit(rp) = derive_info {
                     rpath = Some(rp.clone());
                 } else {
@@ -1236,14 +1235,14 @@ fn render_ercs(
                         rpath = derive_register(reg, &dpath, path, index)?;
                     }
                 }
+                let reg_name = &reg.name;
                 let rendered_reg =
                     register::render(reg, path, rpath, index, config).with_context(|| {
                         let descrip = reg.description.as_deref().unwrap_or("No description");
                         format!(
-                            "Error rendering register\nName: {before_name}\nDescription: {descrip}"
+                            "Error rendering register\nName: {reg_name}\nDescription: {descrip}"
                         )
                     })?;
-                reg.name = before_name;
                 mod_items.extend(rendered_reg)
             }
         }

@burrbull
Copy link
Member

burrbull commented Nov 7, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 7, 2022

Build succeeded:

@bors bors bot merged commit e109523 into rust-embedded:master Nov 7, 2022
@n8tlarsen n8tlarsen deleted the disjoint-array branch November 7, 2022 20:29
@n8tlarsen n8tlarsen restored the disjoint-array branch November 7, 2022 20:30
@n8tlarsen n8tlarsen deleted the disjoint-array branch November 7, 2022 20:33
@Emilgardis Emilgardis mentioned this pull request Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants