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

F4 uart derivation interrupt fixes, add uart field definitions for f413/f423 #754

Closed

Conversation

tim-seoss
Copy link
Contributor

This PR corrects a regression caused by #713 and cleans up similar problems across other f4 parts, as well as correcting some incorrect interrupt definitions.

stm32-rs#713 fixed the UART4 peripheral
on f413 so that it didn't derive from the USART (which is a superset of
the UART), but didn't add the UART register definitions, causing
problems for downstream crates, by effectively removing some register
definitions.
Expands stm32-rs#713 to include uart5,
and also corrects a faulty interrupt definition which was pulled in from
f405.
Several SVDs derived their UART 7,8,9,10 peripherals from a USART
instead of a UART.
UARTs 7,8,9,10 have incorrect interrupt definitions.  For 7,8 copy the
definitions from f469 instead.  Switch to using the f469 definitions for
UART4,5 too (these are identical to the f405 ones, but in the f405 svd,
UART7,8 also have incorrect interrupt definitions, so switch all UARTs
to copy from  f469 for for simplicity).
@newAM newAM added the stm32f4 label Jul 5, 2022
tim-seoss added a commit to tim-seoss/stm32f4xx-hal that referenced this pull request Jul 5, 2022
tim-seoss added a commit to tim-seoss/stm32f4xx-hal that referenced this pull request Jul 5, 2022
Due to svd bugs, some UART peripherals were defined as USARTs.  This
commit relies on PAC changes made in:
stm32-rs/stm32-rs#754
@tim-seoss tim-seoss changed the title F4 uart derivation interrupt fixes F4 uart derivation interrupt fixes, add uart field definitions for f413/f423 Jul 6, 2022
@tim-seoss
Copy link
Contributor Author

n.b. This CI failure seems to be due to a difference in the output of the Python and Rust versions of svdtools (the Python version was first in my path, so was taking precedence).

@adamgreig
Copy link
Member

They have different binary names (svd for Python, svdtools for Rust), the SVDTOOLS environment variable tells Make which one to use but defaults to Rust now. Are you able to reproduce the error locally using plain make or SVDTOOLS=svdtools make?

@tim-seoss
Copy link
Contributor Author

Hi, some time ago I'd previously switched to the Python version (using a symlink, because I didn't spot the env variable) whilst I was debugging another problem. The problem is reproducible here when using the Rust version.

Is it expected that both the Python and Rust versions should behave identically?

@adamgreig
Copy link
Member

Ah, that explains it! For correct input they should both generate the same output, but the Rust version is stricter about validity checking (or rather: has new validity checks that Python didn't have, so the rules about what's valid are stricter).

@tim-seoss
Copy link
Contributor Author

tim-seoss commented Jul 6, 2022

I've hacked the input file, so that it both the Python and Rust versions run without complaint, but still produce different output...

The minimal hacked down version of stm32f413.yaml which reproduces this is:

_svd: ../svd/stm32f413.svd

# The UART peripherals are erroneously derived from a fully-featured USART
# (with extra features, and register fields which UARTs lack).
_delete:
  UART4:
  UART5:

_copy:
  UART4:
    from: ../svd/stm32f405.svd:UART4
  UART5:
    from: ../svd/stm32f405.svd:UART5

The Python version is outputting this (which is what I expect):

    <peripheral derivedFrom="UART4">
      <name>UART5</name>
      <baseAddress>0x40005000</baseAddress>
      <interrupt>
        <name>UART5</name>
        <description>UART5 global interrupt</description>
        <value>53</value>
      </interrupt>
    </peripheral>

Whilst the Rust version is giving:

<peripheral>
      <name>UART5</name>
      <baseAddress>0x40005000</baseAddress>
      <interrupt>
        <name>USART4</name>
        <description>UART 4 global interrupt</description>
        <value>52</value>
      </interrupt>
    </peripheral>

The derivedFrom is missing entirely - the one I want is the correct one in the unpatched stm32f405.svd, but there's also an incorrect one in stm32f413.svd, yet the output has neither.

The interrupt is wrong - it's kept the one from stm32f413.svd (despite the _delete), instead of using the correct one from stm32f405.svd.

Does this reproducer look OK to open a bug against svdtools, or am I doing something wrong ?

@tim-seoss
Copy link
Contributor Author

BTW, would you prefer that I tackle the svdtools issue, or instead workaround it using a different (less concise) fix instead so that the regression can be addressed sooner?

@burrbull
Copy link
Contributor

burrbull commented Jul 6, 2022

I think the issue is due to concurrent rules where different sequence of operations cause different result.

@tim-seoss
Copy link
Contributor Author

tim-seoss commented Jul 12, 2022

This revised stm32f413.yaml (with the _delete removed) appears to produce the same results (svd and svdtools output still differ in the same way):

_svd: ../svd/stm32f413.svd

# The UART peripherals are erroneously derived from a fully-featured USART
# (with extra features, and register fields which UARTs lack).
_copy:
  UART4:
    from: ../svd/stm32f405.svd:UART4
  UART5:
    from: ../svd/stm32f405.svd:UART5

... so I think this is a bug rather than undefined behaviour. I'll try and take a look later this week.

Comment on lines 213 to +216
UART4:
UART5:
UART7:
UART8:
Copy link
Contributor

Choose a reason for hiding this comment

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

there must be array of strings, not a dict of dicts

burrbull pushed a commit to stm32-rs/stm32f4xx-hal that referenced this pull request Aug 14, 2022
Due to svd bugs, some UART peripherals were defined as USARTs.  This
commit relies on PAC changes made in:
stm32-rs/stm32-rs#754
bors bot added a commit that referenced this pull request Aug 22, 2022
762: F4 uart derivation interrupt fixes r=adamgreig a=burrbull

retry of #754 

cc `@tim-seoss`


Co-authored-by: Andrey Zgarbul <zgarbul.andrey@gmail.com>
burrbull pushed a commit to stm32-rs/stm32f4xx-hal that referenced this pull request Sep 28, 2022
Due to svd bugs, some UART peripherals were defined as USARTs.  This
commit relies on PAC changes made in:
stm32-rs/stm32-rs#754
@burrbull
Copy link
Contributor

@newAM
Should close. Fixed by #762

@newAM
Copy link
Member

newAM commented Dec 27, 2022

Sounds good!

@newAM newAM closed this Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants