Register array in RegisterBlock #121

kjetilkjeka · 2017-07-05T17:33:02Z

I guess this pull request is dropping a bit out of the sky, but I realized I needed the feature sooner rather than later so decided to just go for it. When it's merged it should fix #114

Some refactoring was done to allow for integration register arrays, the most significant are

expand function only takes 1 register instead of slice. This means that the function has to be called for every register that should be expanded. This was done so a register can be conditionaly expanded depending on if it's possible to convert to a Rust array or not.
register_to_rust method was factored out from register_block. Mainly so register block wouldn't grow very large with lots of nested control flow.
ExpandedRegister was removed, the functions only use Register struct now. This was because if not it would require conversion functions when registers was not expanded. The struct would also need changing as it would only have to support arrays.
Since the ExpandedRegisters was dropped the expand function now writes %s(i) (where i is the index) to the names of the expanded registers. This is used later on to resolve name and type.
regex is a dependecy, this is used to replace %s(i) with the index. Adding this dependecy might not be desired? and if not I can reimplement it with some logic that iterates over the string.

I've tested with S32K144.svd and it seems to work perfectly with that file.

As mentioned in #114 i do hope that the method of using svd array information instead of unrolling arrays after is acceptable.

kjetilkjeka · 2017-07-05T17:36:20Z

Also have a look at line 399 at generate.rs

I guess that is only valid as long as there are 4 byte (32 bit) registers. However I'm not sure how it can be generalized over register lengths?

japaric

Thanks for the PR, @kjetilkjeka.

It seems reasonable to only translate register arrays to a Rust array in the case of compact (no hole) arrays.

I left several comments about the implementation.

I guess that is only valid as long as there are 4 byte (32 bit) registers. However I'm not sure how it can be generalized over register lengths?

I did not understand this constraint. Why are 4 byte registers special here? Shouldn't the check be register.size == dim_increment? That would check if each array element has reserved space apart from the rergister.

japaric · 2017-07-05T22:14:09Z

src/generate.rs

+                    let mut previous = 0;
+
+                    let get_number_option = |element_option: Option<&String>| -> Option<usize> {element_option
+                                                                        .and_then(|element| match element.parse::<usize>() {


this match element.parse() is equivalent to element.parse().ok()

japaric · 2017-07-05T22:14:39Z

src/generate.rs

+                                                                        })
+                    };
+
+                    let mut index_convertible_temp = match get_number_option(index_iter.next()) {


this match is equivalent to LHS == Some(0)

japaric · 2017-07-05T22:17:18Z

src/generate.rs

+                    for element in index_iter {
+                        if !index_convertible_temp { break; }
+
+                        index_convertible_temp = match get_number_option(Some(element)) {


this get_number_option(Some(element)) would be simpler as element.parse().ok()

japaric · 2017-07-05T22:17:44Z

src/generate.rs

+                    for element in index_iter {
+                        if !index_convertible_temp { break; }
+
+                        index_convertible_temp = match get_number_option(Some(element)) {


the match can probably be replaced by LHS == Some(previous + 1)

japaric · 2017-07-05T22:24:58Z

src/generate.rs

+        match *register {
+            Register::Single(ref _into) => registers_expanded.push(register.clone()),
+            Register::Array(ref _into, ref array_info) => {
+                let index_convertible = if let Some(ref indexes) = array_info.dim_index {


Could you add a comment about what this does? It wasn't entirely clear from reading the contents. Is it checking that indexes == 0..info.dim? In that case this may be simpler to write this as indexes.iter().eq(0..info.dim).

japaric · 2017-07-05T22:41:47Z

src/util.rs

    out
 }

+
+pub fn sort_by_offset(mut registers: Vec<Register>) -> Vec<Register> {


I don't think this routine is complex enough that should be a function.

japaric · 2017-07-05T22:42:48Z

src/util.rs

-/// the register arrays have been expanded.
-pub fn expand(registers: &[Register]) -> Vec<ExpandedRegister> {
+/// and turn in into alist of registers where the register arrays have been expanded.
+pub fn expand(register: &Register) -> Vec<Register> {


Given that this will now only be used with Register::Array it may be better not to pass &Register but rather the contens of the Register::Array variant

japaric · 2017-07-05T22:46:18Z

src/util.rs

-                    info.name.replace("[%s]", "")
+            for (idx, i) in indices.iter().zip(0..) {
+                let name = if has_brackets {
+                    info.name.replace("[%s]", format!("%s({})", idx).as_str())


why do we want to keep the "%s" in this stage? Is it going to be used afterwards? Why not just directly replace it with the formatted idx at this point?

japaric · 2017-07-05T22:46:28Z

src/util.rs

                } else {
-                    info.name.replace("%s", "")
+                    info.name.replace("%s", format!("%s({})", idx).as_str())


same question as above

japaric · 2017-07-05T22:51:42Z

src/generate.rs

+                    || {
+                        format!("Register {} has no `size` field", register.name)
+                    },
+                )? * array_info.dim / 8,


nit: I know that it's unrelated to this PR but could you replace the 8 by a BITS_PER_BYTE constant?

kjetilkjeka · 2017-07-06T20:54:41Z

I agree that %s(N) is hacky. But the ExpandedRegister didn't work either as there was no support for array. I made a new struct extending syn::Field (by composition) adding size, offset and description. This makes perfect sense in my head as we're converting from svd::Register to rust fields (which syn::Field represents). Another bonus is the ability to use to_tokens instead of formating manually. Hope you agree with this reasoning? This made some refactoring necessary inside util but I think it makes more sense as it is now.

I guess that is only valid as long as there are 4 byte (32 bit) registers. However I'm not sure how it can be generalized over register lengths?

I did not understand this constraint. Why are 4 byte registers special here? Shouldn't the check be register.size == dim_increment? That would check if each array element has reserved space apart from the rergiste

You're not suppose to understand that, was just me beeing stupid. Should be fixed now.

Most of your comments were considering the conditions for expanding. I've cleaned them up a whole lot. I think it's readable to the point making more comment than the one outside the function unnecessary. But since I wrote the code 2 hours ago I'm quite biased, say if you still want me to add a comment there.

I think I've addressed all the issues you mentioned except "Given that this will now only be used with Register::Array it may be better not to pass &Register but rather the contens of the Register::Array variant". I was a bit reluctant to change this out because:
-The similar function pub fn convert_svd_register(register: &svd::Register) -> syn::Field is used with svd::Register making my mental model of how to use these functions more consistent

That expanding a Single register is allowed and gives exactly one registers makes sense (atleast in my head)
One more argument gives the ability to make logic errors by passing non corresponding info and array_info
Anyway It's not a big deal but just think about it, if you still want it changed after, no problem.

homunkulus · 2017-07-06T22:34:08Z

☔ The latest upstream changes (presumably c7aed0a) made this pull request unmergeable. Please resolve the merge conflicts.

kjetilkjeka · 2017-07-07T13:25:19Z

Rebase onto japaric/master to resolve merge conflicts and include bare_metal features

homunkulus · 2017-07-07T19:57:47Z

☔ The latest upstream changes (presumably 82ebef2) made this pull request unmergeable. Please resolve the merge conflicts.

…ndedRegister extension

…llable on single registers instead of the whole slice

…ly contains numeric indexes and it starts on 0 and contains every element in the range. Then it is convertable to a rust array

…verted to a rust array

… of spliting on [ had a bug when the index was in the middle of the name.

…ork on the new struct RegisterBlockField. Instead of creating a brand new type representing a rust field it's an extension (by composition) of the syn::Field to also give information about offset, size and description. This means that the util extend function now extend to a vector of syn::Field.

…ould have been in the first place)

…logic

… array is that it's packed in memory, not that fields are 32 bits

… not

kjetilkjeka · 2017-07-08T13:49:03Z

Rebased onto 0.11.1 to make mergeable

kjetilkjeka · 2017-07-12T12:58:45Z

@japaric What's the status on svd2rust at them moment? Now that the bare metal changes are adopted will this PR be next in line or should I expect others merged before it?

kjetilkjeka · 2017-08-22T19:37:52Z

@japaric I guess you didn't have the time to look at this PR last time we talked about it.

Do you have an updated estimate of when you will be able to review this PR?

japaric

Sorry for taking so long to come back to this.

I've requested two changes: one is required to have svd2rust not regress in terms of documentation rendering. Please fix them and I'll merge this.

Do you know if any of the devices in the test suite is exercising this change? If none is could you add your SVD file to the test suite?

japaric · 2017-09-14T20:18:08Z

src/generate.rs

@@ -461,69 +460,135 @@ pub fn peripheral(
    Ok(())
 }

+struct RegisterBlockField {
+    field: syn::Field,
+    description: Ident,


This is not a identifier; it should be a String.

japaric · 2017-09-14T20:18:34Z

src/generate.rs

            i += 1;
        }

        let comment = &format!(
            "0x{:02x} - {}",
            register.offset,
-            util::respace(&register.info.description)
+            register.description,


Please revert this change or the documentation will have random newlines in it. As in: keep the util::respace.

kjetilkjeka · 2017-09-15T17:02:25Z

Thank you for the review @japaric

I've requested two changes: one is required to have svd2rust not regress in terms of documentation rendering. Please fix them and I'll merge this.

This should be fixed now.

Do you know if any of the devices in the test suite is exercising this change? If none is could you add your SVD file to the test suite?

MK02F12810.svd should exercise this change (amongst other) for this register.

    <register>
      <dim>8</dim>
      <dimIncrement>0x4</dimIncrement>
      <dimIndex>0,1,2,3,4,5,6,7</dimIndex>
      <name>TAGVDW0S%s</name>
      <description>Cache Tag Storage</description>
      <addressOffset>0x100</addressOffset>
      <size>32</size>
      <access>read-write</access>
      <resetValue>0</resetValue>
      <resetMask>0xFFFFFFFF</resetMask>
      <fields>
        <field>
          <name>valid</name>
          <description>1-bit valid for cache entry</description>
          <bitOffset>0</bitOffset>
          <bitWidth>1</bitWidth>
          <access>read-write</access>
        </field>
        <field>
          <name>tag</name>
          <description>14-bit tag for cache entry</description>
          <bitOffset>5</bitOffset>
          <bitWidth>14</bitWidth>
          <access>read-write</access>
        </field>
      </fields>
    </register>
    <register>

The diff for this register (between kjetilkjeka/svd2rust/master and kjetilkjeka/svd2rust/field_array) looks like the following:

12303c12303,12317
<         pub tagvdw0s: [TAGVDW0S; 8],
---
>         pub tagvdw0s0: TAGVDW0S,
>         #[doc = "0x104 - Cache Tag Storage"]
>         pub tagvdw0s1: TAGVDW0S,
>         #[doc = "0x108 - Cache Tag Storage"]
>         pub tagvdw0s2: TAGVDW0S,
>         #[doc = "0x10c - Cache Tag Storage"]
>         pub tagvdw0s3: TAGVDW0S,
>         #[doc = "0x110 - Cache Tag Storage"]
>         pub tagvdw0s4: TAGVDW0S,
>         #[doc = "0x114 - Cache Tag Storage"]
>         pub tagvdw0s5: TAGVDW0S,
>         #[doc = "0x118 - Cache Tag Storage"]
>         pub tagvdw0s6: TAGVDW0S,
>         #[doc = "0x11c - Cache Tag Storage"]
>         pub tagvdw0s7: TAGVDW0S,

This means that the doc string is the same as it used to be for tagvdw0s0. "#[doc = 0x100 - Cache Tag Storage]"

I've also manually inspected the other registers in this file where the new code path is exercised and they are equivalent to the previous example.

The previous example is also equivalent to the .svd file I tested during devlopment S32K144.svd

I can, of course, manually inspect more files? But I've also tested it with the different (allowed) uses of dimIncrement and dimIndex manually. So I believe the next step should be to confirm that all the .svd files in the test suite will be compiled without error.

japaric · 2017-09-19T19:57:30Z

Thanks, @kjetilkjeka.

@homunkulus r+

homunkulus · 2017-09-19T19:57:31Z

📌 Commit 52cf267 has been approved by japaric

homunkulus · 2017-09-19T19:57:34Z

⌛ Testing commit 52cf267 with merge 1f3542f...

Register array in RegisterBlock I guess this pull request is dropping a bit out of the sky, but I realized I needed the feature sooner rather than later so decided to just go for it. When it's merged it should fix #114 Some refactoring was done to allow for integration register arrays, the most significant are - expand function only takes 1 register instead of slice. This means that the function has to be called for every register that should be expanded. This was done so a register can be conditionaly expanded depending on if it's possible to convert to a Rust array or not. - register_to_rust method was factored out from register_block. Mainly so register block wouldn't grow very large with lots of nested control flow. - ExpandedRegister was removed, the functions only use Register struct now. This was because if not it would require conversion functions when registers was not expanded. The struct would also need changing as it would only have to support arrays. - Since the ExpandedRegisters was dropped the expand function now writes ```%s(i)``` (where i is the index) to the names of the expanded registers. This is used later on to resolve name and type. - regex is a dependecy, this is used to replace ```%s(i)``` with the index. Adding this dependecy might not be desired? and if not I can reimplement it with some logic that iterates over the string. I've tested with [S32K144.svd](https://github.com/kjetilkjeka/s32k144.rs/blob/master/src/S32K144.svd) and it seems to work perfectly with that file. As mentioned in #114 i do hope that the method of using svd array information instead of unrolling arrays after is acceptable.

homunkulus · 2017-09-20T01:18:57Z

☀️ Test successful - status-appveyor, status-travis
Approved by: japaric
Pushing 1f3542f to master...

kjetilkjeka changed the title ~~Field array~~ Register array in RegisterBlock Jul 5, 2017

japaric reviewed Jul 5, 2017

View reviewed changes

kjetilkjeka force-pushed the field_array branch from 487512e to 8d5563c Compare July 7, 2017 13:24

kjetilkjeka added 19 commits July 8, 2017 14:50

Extracted sort from expand into its own function

8ed24a1

Made expand work directly on Register structure instead of using Expa…

c47764d

…ndedRegister extension

Refactored out the piece of code that formats the rust field

dd8c1b1

Made it possible to only expand certain registers by making expand ca…

806f722

…llable on single registers instead of the whole slice

Implemented conversion from Array type register to rust array

63d1af8

Wrote the condition that checks if the array is convertable. If it on…

6110797

…ly contains numeric indexes and it starts on 0 and contains every element in the range. Then it is convertable to a rust array

Fixed bug where offset was calculated incorrectly with register arrays

e8b5035

Require that the svd array represented a packed memory area to be con…

244b06c

…verted to a rust array

Used regex to replace %s on expanded fields, the previous used hacked…

f0e85fd

… of spliting on [ had a bug when the index was in the middle of the name.

Added function for converting svd registers into rust fields

a24a245

Removed everything relating to regex as it's not needed (and never sh…

fc238af

…ould have been in the first place)

Refactored out register size to avoid duplication of error detection …

09f191e

…logic

Added comment explaining when registers will be expanded

db90f78

Added BITS_PER_BYTE constant

aff4410

Fixed coding error. A condition for converting from svd array to rust…

d3c0632

… array is that it's packed in memory, not that fields are 32 bits

Cleaned up conditions that find out if register should be expanded or…

3d3cd94

… not

Swaped out magic number 8 by BITS_PER_BYTE

5d1a0f7

Made include self style consistent with the rest of the project

26849d6

kjetilkjeka force-pushed the field_array branch from 45d1340 to 26849d6 Compare July 8, 2017 13:47

kjetilkjeka mentioned this pull request Aug 22, 2017

LPC82x: Wrong offsets in GPIO_PORT peripheral #137

Open

japaric suggested changes Sep 14, 2017

View reviewed changes

kjetilkjeka added 2 commits September 15, 2017 18:23

Changed description type to String from Ident

1ee0872

Added respace to avoid breaking docs

52cf267

homunkulus merged commit 52cf267 into rust-embedded:master Sep 20, 2017

homunkulus mentioned this pull request Sep 20, 2017

Implemented new functions to format literals as seperated hex idents #134

Merged

Register array in RegisterBlock #121

Register array in RegisterBlock #121

Uh oh!

Conversation

kjetilkjeka commented Jul 5, 2017

Uh oh!

kjetilkjeka commented Jul 5, 2017

Uh oh!

japaric left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjetilkjeka commented Jul 6, 2017

Uh oh!

homunkulus commented Jul 6, 2017

Uh oh!

kjetilkjeka commented Jul 7, 2017

Uh oh!

homunkulus commented Jul 7, 2017

Uh oh!

kjetilkjeka commented Jul 8, 2017

Uh oh!

kjetilkjeka commented Jul 12, 2017

Uh oh!

kjetilkjeka commented Aug 22, 2017

Uh oh!

japaric left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjetilkjeka commented Sep 15, 2017

Uh oh!

japaric commented Sep 19, 2017

Uh oh!

homunkulus commented Sep 19, 2017

Uh oh!

homunkulus commented Sep 19, 2017

Uh oh!

homunkulus commented Sep 20, 2017

Uh oh!

Uh oh!