cannot _derive from a register that is affected by a _cluster modification

[simpkins]

I'm trying to make a patch to clean up the peripherals in the ESP32 USB0 peripheral. It has registers for several endpoints that need to be clustered, and I also want to make some of the registers derive from other registers. svdtools currently doesn't appear to allow me to both apply clustering and make derivedFrom modifications at the same time.

If I add a _derive section, these changes appear to take be applied before the _cluster section. When the _derive changes are made it checks that the derivedFrom register names are valid. However, these register names are later changed by the _cluster operation, which makes svdtools generate an invalid SVD files since the derivedFrom fields refer to the old names. I can't use the new names in the _derive section, since svdtools rejects them as invalid since they don't exist yet when the check is made.

In theory I suppose svdtools should track all derivedFrom attributes throughout the SVD file, and update them correctly whenever it renames/clusters/derives a register that is the destination of an existing derivedFrom attribute. This seems possibly more complicated than it is worth, however. Alternatively, it would be nice if there were an easy way to force svdtools to skip the register name check in _derive, so I could specify the desired derivedFrom name without svdtools complaining because it doesn't exist yet.

Here is a with more details about what I would like to accomplish https://github.com/esp-rs/esp-pacs/pull/213/files/e36f054438d1a2a44e81f24eea4efae80ea861e0#r1550642332

[burrbull]

In theory I suppose svdtools should track all derivedFrom attributes throughout the SVD file, and update them correctly whenever it renames/clusters/derives a register that is the destination of an existing derivedFrom attribute.

Yeah. Maybe.

Could you first try 2 other ways to _derive after _cluster:

1. Use _derive inside _cluster like:

   PER1:
   _cluster:
   CNAME%s:
      REG1: {}
      REG2: {}
      _derive:
          REG3: REG1

2. Split on 2 peripheral rule blocks (with different headers which match same peripheral). Workaround for command order:

   PER1:
   _cluster:
   CNAME%s:
      REG1: {}
      REG2: {}
   "PER[1]":
   "CNAME%s":
      _derive:
          REG3: REG1

[simpkins]

Thanks! It looks like the first approach (putting _derive inside _cluster) does work in the simple case. It doesn't work in my situation, but it's for a somewhat unrelated reason; the _derive check doesn't handle absolute identifier names (anything with a . in it).

In my case I need to create 2 separate clusters, and I want some of the registers in one cluster to derive from registers in the other cluster. However, it should be easier to just fix the _derive name check to handle absolute identifier names. I'll try working on a diff to do this.

I did create a hacky workaround that just allows bypassing the destination register name here. If I can fix the handling of absolute names I shouldn't need this, but the test case in the diff does have small example showing more exactly what I'm trying to do: https://github.com/simpkins/svdtools/commit/c0e072ba679aaa7c7f6b7a04d1c3b15b690489de

[burrbull]

the _derive check doesn't handle absolute identifier names (anything with a . in it).

Hmm. It's really weird.

[simpkins]

Handling absolute identifier names in _derive looks like it will be rather tricky. Since we are modifying one of the elements of the device, you can't pass in a reference to the device in order to look up other registers in other peripherals/clusters.

One option would be to have patch operations always return a new object rather than mutating a peripheral/cluster in place. This way you can pass in the existing device, and the register name validation could try to look up the object in the current peripheral if the name appears to refer to this peripheral, or in the wider device if not. This gets sort of complicated when handling clusters, however, since you have to check if the name refers to something in the current cluster, something else in the current peripheral being operated on, or something not in this peripheral. This also would have some performance overhead since we have to copy each object rather than patching it in place, although that might not really be a big deal in practice.

In the short term, I'm sort of inclined to just skip validating the destination register name if it contains .

It looks like the derive_register() code does contain some logic to try and fix up derivedFrom references in the same peripheral/cluster as the register that is now being derived-from. This tries to find references that point to the current register and update them to point to the new derivedFrom destination. However, it doesn't handle derivedFrom references in other peripherals/clusters.

[burrbull]

What if just disable this check for all absolute pathes? (by presence of ".")

[simpkins]

Yeah, that sounds good to me. (It's what I proposed in my comment above.)

I've got a PR almost ready that I can submit to do this.

[simpkins]

Closing this out since the suggestion to use _derive inside _cluster works, and the other issue I was having with absolute names has also been resolved. Thanks for your help!