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

Unstable generation results #36

Open
igiona opened this issue Sep 23, 2024 · 3 comments
Open

Unstable generation results #36

igiona opened this issue Sep 23, 2024 · 3 comments

Comments

@igiona
Copy link
Contributor

igiona commented Sep 23, 2024

I'm experiencing some strange behaviour from the chiptool.

Repro

Checkout https://github.com/igiona/nrf-pac/tree/TestFlakyChiptool

cd TEST 
chiptool generate --svd nrf52840.svd --transform nrf52840.yaml
rustfmt lib.rs
git diff

You can commit these changes, repeat the steps above, and you will see other changes coming out similar to:

diff --git a/TEST/lib.rs b/TEST/lib.rs
index d02724a..89dc42e 100644
--- a/TEST/lib.rs
+++ b/TEST/lib.rs
@@ -490,40 +490,40 @@ pub mod aar {
                 Enable(0)
             }
         }
-        #[doc = "Enable interrupt"]
+        #[doc = "Disable interrupt"]
         #[repr(transparent)]
         #[derive(Copy, Clone, Eq, PartialEq)]
         pub struct Inten(pub u32);
         impl Inten {
-            #[doc = "Write '1' to enable interrupt for event END"]
+            #[doc = "Write '1' to disable interrupt for event END"]
             #[inline(always)]
             pub const fn end(&self) -> bool {
                 let val = (self.0 >> 0usize) & 0x01;
                 val != 0
             }
-            #[doc = "Write '1' to enable interrupt for event END"]
+            #[doc = "Write '1' to disable interrupt for event END"]
             #[inline(always)]
             pub fn set_end(&mut self, val: bool) {
                 self.0 = (self.0 & !(0x01 << 0usize)) | (((val as u32) & 0x01) << 0usize);
             }
-            #[doc = "Write '1' to enable interrupt for event RESOLVED"]
+            #[doc = "Write '1' to disable interrupt for event RESOLVED"]
             #[inline(always)]
             pub const fn resolved(&self) -> bool {
                 let val = (self.0 >> 1usize) & 0x01;
                 val != 0
             }
-            #[doc = "Write '1' to enable interrupt for event RESOLVED"]
+            #[doc = "Write '1' to disable interrupt for event RESOLVED"]
             #[inline(always)]
             pub fn set_resolved(&mut self, val: bool) {
                 self.0 = (self.0 & !(0x01 << 1usize)) | (((val as u32) & 0x01) << 1usize);
             }
-            #[doc = "Write '1' to enable interrupt for event NOTRESOLVED"]
+            #[doc = "Write '1' to disable interrupt for event NOTRESOLVED"]
             #[inline(always)]
             pub const fn notresolved(&self) -> bool {
                 let val = (self.0 >> 2usize) & 0x01;
                 val != 0
             }

The doc-string doesn't stay constant and keep "toggling" every tool run.

Expected behaviour

The generated code stays constant after each run

Debugging notes

I did not invest too much time in debugging. What for now I can say is:
If you search the SVD file for <name>PAYLOAD</name>, you will find only 2 entries: one for clear and one for set, with the 2 descriptions that are "flickering" in the generated lib.rs.

-            #[doc = "Write '1' to disable interrupt for event PAYLOAD"]
+            #[doc = "Write '1' to enable interrupt for event PAYLOAD"]
             #[inline(always)]
             pub const fn payload(&self) -> bool {
                 let val = (self.0 >> 2usize) & 0x01;
                 val != 0
             }
-            #[doc = "Write '1' to disable interrupt for event PAYLOAD"]
+            #[doc = "Write '1' to enable interrupt for event PAYLOAD"]
             #[inline(always)]
             pub fn set_payload(&mut self, val: bool) {
                 self.0 = (self.0 & !(0x01 << 2usize)) | (((val as u32) & 0x01) << 2usize);
             }

The tool seems to pick the doc-string "randomly" the doc-string, whereas the function code stays constant.

Maybe someone of you has already experienced this?

@Dirbaio
Copy link
Member

Dirbaio commented Sep 23, 2024

yeah it's a known problem, i've seen this before.

My guess is some HashMap somewhere that causes the randomness, switching to BTreeMap should fix it.

@igiona
Copy link
Contributor Author

igiona commented Sep 23, 2024

Okay, I'll see if I can find the time to fix this.

Interestingly enough, the rp-pac doesn't seem to suffer from the same issue.

@igiona
Copy link
Contributor Author

igiona commented Sep 29, 2024

@Dirbaio I looked into this issue, and indeed it turned out being a HashMap/Set problem.

I fixed the MergeTransform case here https://github.com/igiona/chiptool/tree/Fix-Issue36, and I added also a minimal test case.
I guess that one would want to change all occurrences of HashMap/Set with BTreeMap/Set, or at least most of them.

While digging into this issue, I wondered weather the current code is generating the docstring that one would actually want (at least in the merge-transformation)...

In the minimal test-case in the branch above, we have 2 registers (INTENSET, INTENCLR) that get merged into a single one (Inten).

Wile merging, the description of the first to-be-merged register will be taken, resulting in:

        impl Inten {
            #[doc = "Write '1' to enable interrupt for event HFCLKSTARTED"]
            #[inline(always)]
            pub const fn hfclkstarted(&self) -> bool {
                let val = (self.0 >> 0usize) & 0x01;
                val != 0
            }
            #[doc = "Write '1' to enable interrupt for event HFCLKSTARTED"]
            #[inline(always)]
            pub fn set_hfclkstarted(&mut self, val: bool) {
                self.0 = (self.0 & !(0x01 << 0usize)) | (((val as u32) & 0x01) << 0usize);
            }
        }

The "in-memory" bit-representation of the register can't actually know what value one needs to set as val or what action one performs, because this depends on which actual register this struct is going to be written into.
So picking "the first field-description" it's likely to be wrong in most cases.

I either would proposed to:

  • Don't generated a docstring at all, in case of a merge-transoformation, or something like "look into this and that register"
  • Generate a more verbose docstring like
#[doc = "When used on Clock.intenset(): Write '1' to enable interrupt for event HFCLKSTARTED
When used on Clock.intenclr(): Write '1' to disable interrupt for event HFCLKSTARTED"]

Let me know your opinion on this ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants