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.

Sign up for GitHub

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

Jump to bottom

Fall back to caption from module if instance caption is missing #15

Merged
merged 1 commit into from
Nov 24, 2020
Merged

Fall back to caption from module if instance caption is missing #15

merged 1 commit into from
Nov 24, 2020

Conversation

trembel
Copy link
Contributor

@trembel trembel commented Nov 23, 2020 •
edited
Loading

This should solve #5, s.t. on a missing caption in the <instance ...> element, the caption of the parent <module ...> is used.

Signed-off-by: trembel silvano.cortesi@hotmail.com

@trembel
[MOD] Fallback to <module ...> caption, if <instance ...> has not…
ea93f0b 
… caption.

Signed-off-by: trembel <silvano.cortesi@hotmail.com>
@trembel
Copy link
Contributor Author

trembel commented Nov 23, 2020

I'm thus unsure if this already works. @Rahix can you please guide me on testing this PR (it compiles, but I've not tested it on an atdf)?

@Rahix
Copy link
Owner

Rahix commented Nov 23, 2020

Hey,

thanks for starting work on this issue!

My usual workflow for hacking atdf2svd is as follows: I use the "old"/released version of the tool to generate an SVD for a certain chip that is known to have the issue in question. Then, I do changes and generate a new SVD with the patched version of atdf2svd. Using diff, vimdiff, or meld, I compare old SVD against new SVD, to check that only the things I wanted to have change have actually changed and whether those changes look like intended.

In your case, I think you should expect to see a few peripherals with missing descriptions to gain one in the diff output.

@Rahix Rahix linked an issue Nov 23, 2020 that may be closed by this pull request
Fall back to caption from module if instance caption is missing #5
Closed
@Rahix Rahix changed the title topic-5: Fall back to caption from module if instance caption is missing Fall back to caption from module if instance caption is missing Nov 23, 2020
@Rahix Rahix added the enhancement New feature or request label Nov 23, 2020
@trembel
Copy link
Contributor Author

trembel commented Nov 23, 2020 •
edited
Loading

Thanks that helps!
Two questions:

  • What should be used if even the module does not contain a caption? Just the name of the module? -- EDIT -- as far as i know this is only the case in the below scheme..
  • There seems to be an error on the parsing of this interrupts fields: the module-instance is neglected. How should that be handled?
    attiny412.atdf:
      <interrupts>
        <interrupt index="1" module-instance="CRCSCAN" name="NMI"/>
        <interrupt index="2" module-instance="BOD" name="VLM"/>
        <interrupt index="3" module-instance="PORTA" name="PORT"/>
        <interrupt index="6" module-instance="RTC" name="CNT"/>
        <interrupt index="7" module-instance="RTC" name="PIT"/>
        <interrupt index="8" module-instance="TCA0" name="LUNF"/>
        <interrupt index="8" module-instance="TCA0" name="OVF"/>
        <interrupt index="9" module-instance="TCA0" name="HUNF"/>
        <interrupt index="10" module-instance="TCA0" name="CMP0"/>
        <interrupt index="10" module-instance="TCA0" name="LCMP0"/>
        <interrupt index="11" module-instance="TCA0" name="CMP1"/>
        <interrupt index="11" module-instance="TCA0" name="LCMP1"/>
        <interrupt index="12" module-instance="TCA0" name="CMP2"/>
        <interrupt index="12" module-instance="TCA0" name="LCMP2"/>
        <interrupt index="13" module-instance="TCB0" name="INT"/>
        <interrupt index="14" module-instance="TCD0" name="OVF"/>
        <interrupt index="15" module-instance="TCD0" name="TRIG"/>
        <interrupt index="16" module-instance="AC0" name="AC"/>
        <interrupt index="17" module-instance="ADC0" name="RESRDY"/>
        <interrupt index="18" module-instance="ADC0" name="WCOMP"/>
        <interrupt index="19" module-instance="TWI0" name="TWIS"/>
        <interrupt index="20" module-instance="TWI0" name="TWIM"/>
        <interrupt index="21" module-instance="SPI0" name="INT"/>
        <interrupt index="22" module-instance="USART0" name="RXC"/>
        <interrupt index="23" module-instance="USART0" name="DRE"/>
        <interrupt index="24" module-instance="USART0" name="TXC"/>
        <interrupt index="25" module-instance="NVMCTRL" name="EE"/>
      </interrupts>

attiny412.svd:

      <interrupt>
        <name>NMI</name>
        <value>1</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>VLM</name>
        <value>2</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>PORT</name>
        <value>3</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>CNT</name>
        <value>6</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>PIT</name>
        <value>7</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>LUNF</name>
        <value>8</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>HUNF</name>
        <value>9</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>CMP0</name>
        <value>10</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>LCMP0</name>
        <value>10</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>CMP1</name>
        <value>11</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
...

@trembel
Copy link
Contributor Author

trembel commented Nov 23, 2020

For the AVR128DA its even stranger:

      <interrupt>
        <name>NMI</name>
        <value>1</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>VLM</name>
        <value>2</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>CNT</name>
        <value>3</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>PIT</name>
        <value>4</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>CCL</name>
        <value>5</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>LUNF</name>
        <value>7</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>HUNF</name>
        <value>8</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>CMP0</name>
        <value>9</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>LCMP0</name>
        <value>9</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>CMP1</name>
        <value>10</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>LCMP1</name>
        <value>10</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>CMP2</name>
        <value>11</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>LCMP2</name>
        <value>11</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>OVF</name>
        <value>14</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>TRIG</name>
        <value>15</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>TWIS</name>
        <value>16</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>TWIM</name>
        <value>17</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>RESRDY</name>
        <value>24</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>WCMP</name>
        <value>25</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>ZCD</name>
        <value>26</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>PTC</name>
        <value>27</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>PORT</name>
        <value>34</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>EE</name>
        <value>35</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>INT</name>
        <value>36</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>RXC</name>
        <value>37</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>DRE</name>
        <value>38</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>TXC</name>
        <value>39</value>
        <description>&lt;TBD></description>
      </interrupt>
      <interrupt>
        <name>AC</name>
        <value>40</value>
        <description>&lt;TBD></description>
      </interrupt>

      <interrupts>
        <interrupt index="1" module-instance="CRCSCAN" name="NMI"/>
        <interrupt index="2" module-instance="BOD" name="VLM"/>
        <interrupt index="3" module-instance="RTC" name="CNT"/>
        <interrupt index="4" module-instance="RTC" name="PIT"/>
        <interrupt index="5" module-instance="CCL" name="CCL"/>
        <interrupt index="6" module-instance="PORTA" name="PORT"/>
        <interrupt index="7" module-instance="TCA0" name="LUNF"/>
        <interrupt index="7" module-instance="TCA0" name="OVF"/>
        <interrupt index="8" module-instance="TCA0" name="HUNF"/>
        <interrupt index="9" module-instance="TCA0" name="CMP0"/>
        <interrupt index="9" module-instance="TCA0" name="LCMP0"/>
        <interrupt index="10" module-instance="TCA0" name="CMP1"/>
        <interrupt index="10" module-instance="TCA0" name="LCMP1"/>
        <interrupt index="11" module-instance="TCA0" name="CMP2"/>
        <interrupt index="11" module-instance="TCA0" name="LCMP2"/>
        <interrupt index="12" module-instance="TCB0" name="INT"/>
        <interrupt index="13" module-instance="TCB1" name="INT"/>
        <interrupt index="14" module-instance="TCD0" name="OVF"/>
        <interrupt index="15" module-instance="TCD0" name="TRIG"/>
        <interrupt index="16" module-instance="TWI0" name="TWIS"/>
        <interrupt index="17" module-instance="TWI0" name="TWIM"/>
        <interrupt index="18" module-instance="SPI0" name="INT"/>
        <interrupt index="19" module-instance="USART0" name="RXC"/>
        <interrupt index="20" module-instance="USART0" name="DRE"/>
        <interrupt index="21" module-instance="USART0" name="TXC"/>
        <interrupt index="22" module-instance="PORTD" name="PORT"/>
        <interrupt index="23" module-instance="AC0" name="AC"/>
        <interrupt index="24" module-instance="ADC0" name="RESRDY"/>
        <interrupt index="25" module-instance="ADC0" name="WCMP"/>
        <interrupt index="26" module-instance="ZCD0" name="ZCD"/>
        <interrupt index="27" module-instance="PTC" name="PTC"/>
        <interrupt index="28" module-instance="AC1" name="AC"/>
        <interrupt index="29" module-instance="PORTC" name="PORT"/>
        <interrupt index="30" module-instance="TCB2" name="INT"/>
        <interrupt index="31" module-instance="USART1" name="RXC"/>
        <interrupt index="32" module-instance="USART1" name="DRE"/>
        <interrupt index="33" module-instance="USART1" name="TXC"/>
        <interrupt index="34" module-instance="PORTF" name="PORT"/>
        <interrupt index="35" module-instance="NVMCTRL" name="EE"/>
        <interrupt index="36" module-instance="SPI1" name="INT"/>
        <interrupt index="37" module-instance="USART2" name="RXC"/>
        <interrupt index="38" module-instance="USART2" name="DRE"/>
        <interrupt index="39" module-instance="USART2" name="TXC"/>
        <interrupt index="40" module-instance="AC2" name="AC"/>
      </interrupts>

Also how do you deal with duplicate entries in the atdf (they are "synonyms", e.g. the same register entry), e.g. the double index 7, 9 or 10

@trembel
Copy link
Contributor Author

trembel commented Nov 23, 2020 •
edited
Loading

Also sometimes the description of the module are "missleading", e.g. if there are multiple instances of a peripheral. Would it be desired to append the instance name to the description (i.e. "Analog Comparator AC0")?
old_new

@Rahix
Copy link
Owner

Rahix commented Nov 23, 2020

What should be used if even the module does not contain a caption? Just the name of the module? -- EDIT -- as far as i know this is only the case in the below scheme..

Hmm, I'd say we shouldn't do anything in that case. The description can be added later via a patch in avr-device where a much more descriptive text can be passed in instead of an attempt at rescuing the sub-standard ATDF contents ...

There seems to be an error on the parsing of this interrupts fields: the module-instance is neglected. How should that be handled?

Can you open a separate issue for discussing that? I suspect this might be similar to #4 and we need to add logic to account for these additional elements in the ATDF.

Also how do you deal with duplicate entries in the atdf (they are "synonyms", e.g. the same register entry), e.g. the double index 7, 9 or 10

Ugh ... We need to warn and only add one, I don't think SVD can express this situation. But should be double checked ...

Do you know if this is actual interrupt "re-use" or did they just duplicate them for "fun"?

Also sometimes the description of the module are "missleading", e.g. if there are multiple instances of a peripheral. Would it be desired to append the instance name to the description (i.e. "Analog Comparator AC0")?

IMO it is fine like it currently looks in your diff. You'll always have the peripheral name next to its description so I'd argue it is easy to understand what is going on ...

Rahix
Rahix approved these changes Nov 23, 2020
View reviewed changes
Copy link
Owner

@Rahix Rahix left a comment

Choose a reason for hiding this comment

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

From my side this change looks good and I'd merge it as it is right now :)

@Rahix
Copy link
Owner

Rahix commented Nov 23, 2020

Can you remove the PR draft state please?

This was referenced Nov 23, 2020
Closed
Closed
@trembel trembel marked this pull request as ready for review November 23, 2020 15:49
@Rahix Rahix merged commit 4f8450c into Rahix:master Nov 24, 2020
@Rahix
Copy link
Owner

Rahix commented Nov 24, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rahix Rahix Rahix approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fall back to caption from module if instance caption is missing
2 participants
@trembel @Rahix