Skip to content

improve derive for registers (breaking) #118

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
Merged
merged 8 commits into from
Aug 20, 2022
Merged

improve derive for registers (breaking) #118

merged 8 commits into from
Aug 20, 2022
+238 −72

Conversation

burrbull
Copy link
Member

@burrbull burrbull commented Aug 18, 2022 •
edited
Loading

Closes #73, #116

burrbull and others added 5 commits August 19, 2022 07:52
@burrbull
_derive register (Rust)
c0df912
@burrbull
skip derived
c6daaee
@burrbull
derive (Python) initial
bd760e5
@mciantyre @burrbull
Allow renames of derived peripherals
0faef6f 
This commit enables two features (F):

F1. We're permitted to rename a peripheral that has a derivedFrom
attribute.
F2. When we rename a peripheral that has derivations, svd patch will
automatically update all derivations to use the new name.

These behaviors support renames of peripheral hierarchies. Suppose a
chip has two USB peripherals: USB_OTG1 and USB_OTG2. USB_OTG2 derives
from USB_OTG1. Before this commit, renaming these peripherals to USB1
and USB2 looks like this:

  _modify:  # Step 1: Rename the base peripheral USB_OTG1 => USB1.
    _peripherals:
      USB_OTG1:
        name: USB1
  _delete:  # Step 2: Delete USB_OTG2, because it can't be renamed.
    - USB_OTG2
  _add:     # Step 3: Add USB2, derived from USB1, with values from the SVD.
    USB2:
      derivedFrom: USB1
      baseAddress: 0x4042C000
      addressBlock: # Re-define address block
      interrupts:  # Re-define interrupts

Step 2 is required because modifications don't apply to derivedFrom
peripherals (line 180 in README.md). Step 3 means we duplicate SVD
information which was otherwise correct. This could be cumbersome when
trying to rename many instances.

(I wasn't able to figure out any other way to achieve this rename. Let
me know if this is already supported using other svdtool directives.)

F1 makes it easier to achieve the rename. There's no need for steps 2
and 3. When combined with F2, we write

  _modify:
    _peripherals:
      USB_OTG1:
        name: USB1
      USB_OTG2:
        name: USB2

F2 is a convenience. Without it, it's possible to use _derive directives
to fix the names. (You can't use _rebase though; _rebase needs both
peripherals to exist.) But _derive seems to focus on collapsing
independently-defined peripherals, which is not what we're doing when
changing peripheral names.
@burrbull
rename derived (rust)
76a2885
@burrbull
Copy link
Member Author

@mciantyre Could you test both Python and Rust svdtools?

@burrbull burrbull marked this pull request as ready for review August 19, 2022 09:21
@burrbull
Copy link
Member Author

svd2rust now supports register deriving, but svdtools doesn't
@adamgreig what your thoughts?

burrbull
burrbull commented Aug 19, 2022
View reviewed changes
src/patch/peripheral.rs
Comment on lines +380 to +382
self.get_register(rderive)
.ok_or_else(|| anyhow!("register {} not found", rderive))?;

Copy link
Member Author

Choose a reason for hiding this comment

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

Also 1 question. Should we do this check?
It doesn't support cross-peripheral deriving.

@mciantyre
Copy link
Contributor

Looks good for my purposes. See 0faef6f's message to understand original and new approaches.

Python

  • ✅ Original renaming approach still works.
  • ✅ New naming approach works.

Rust

  • ✅ Original renaming approach still works.
  • ✅ New naming approach works.

mciantyre added a commit to imxrt-rs/svdtools that referenced this pull request Aug 19, 2022
@mciantyre
Accept a YAML file, not an SVD file, when patching
d4331ef 
A UI nitpick I noticed while testing rust-embedded#118. The Rust-based svdtools patch
help output suggests that the user provides an SVD file:

  $ svdtools patch --help
  svdtools-patch
  Patches an SVD file as specified by a YAML file

  USAGE:
      svdtools patch <SVD_FILE>

  ARGS:
      <SVD_FILE>    Path to input SVD file

  OPTIONS:
      -h, --help    Print help information

If we actually provide an SVD file, the tool errors.

  $ svdtools patch ../ral/svd/imxrt1176_cm7.svd
  [2022-08-19T12:47:48Z ERROR svdtools::cli] mapping values are not allowed in this context at line 11 column 24

Compare this with the Python documentation, which suggests that the user
provides a YAML file, not an SVD file.

  svd patch --help
  Usage: svd patch [OPTIONS] YAML_FILE

    Patches an SVD file as specified by a YAML file

  Options:
    --help  Show this message and exit.
@adamgreig
Copy link
Member

This looks good to me. I propose we merge this first, then fix conflicts in #89 and merge that too, then release a new svdtools (despite the stm32-rs and rp2040 tests failing at that point) and get PRs merged in those repos as soon as possible afterwards to use the new version. Does that sound OK?

@burrbull
Copy link
Member Author

OK

@burrbull burrbull merged commit bb884ab into master Aug 20, 2022
@bors bors bot deleted the errors branch August 20, 2022 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

derive_register is copy
3 participants
@burrbull @mciantyre @adamgreig