Handle overrides in element group derivation

[scootermon]

I have an SVD file with two instances of a peripheral. The second peripheral contains the same registers, but instead of 128 it only has 64. The SVD file uses derivedFrom in the second peripheral to inherit these registers but overrides the <dim> element. To my surprise, svd2rust generated arrays with the same number of elements for both instances.
Well, after following the trail I ended up here and the reason wasn't hard to find.

I saw #283 and the follow up #286, but there's little to no reasoning attached to these PRs.

Looking at the spec here I really don't see a reason why the fields from dimElementGroup shouldn't also be overridden.
The derivedFrom description reads

Specify the register name from which to inherit data. Elements specified subsequently override inherited values.

I can't see a statement about the dimElementGroup elements somehow being exempt.

[adamgreig]

Thanks for the PR and sorry for the delay in reviewing! LGTM but I don't understand why the CI workflows haven't run and there doesn't seem to be the usual option to approve running them. Maybe if you run another force push it will run/allow approving to run?

Otherwise all seems OK so we could merge through the merge queue which will run CI too. @burrbull / @Emilgardis ?

[burrbull]

Unfortunately the spec does not fully describe behavior during derive. I not sure about correctness of code neither before PR not after (I afraid that partial derive of dim group could cause tag conflicts). I need see real example what was working wrong and how it should work.

[burrbull]

I have an SVD file with two instances of a peripheral.

@scootermon show this file, please. Or part of the file.

[burrbull]

Suggested change

	derived.dim_index = derived.dim_index.or_else(|| dim_index.clone());
	derived.dim_name = derived.dim_name.or_else(|| dim_name.clone());
	derived.dim_array_index = derived.dim_array_index.or_else(|| dim_array_index.clone());
	derived.dim_index = derived.dim_index.or_else(|| {
	dim_index
	.as_ref()
	.and_then(|di| (*dim == derived.dim).then(|| di.clone()))
	});
	derived.dim_name = derived.dim_name.or_else(|| dim_name.clone());
	derived.dim_array_index = derived.dim_array_index.or_else(|| {
	dim_array_index
	.as_ref()
	.and_then(|dai| (*dim == derived.dim).then(|| dai.clone()))
	});

We should derive dim_index only when same size.

[scootermon]

I have an SVD file with two instances of a peripheral.

@scootermon show this file, please. Or part of the file.

@burrbull, unfortunately, I cannot share the file or significant portions from it. Here's a reduced version just to illustrate the pattern:

<!-- CAN0 -->
<register>
  <name>eRAM[%s]</name>
  <description>Embedded RAM</description>
  <size>32</size>
  <access>read-write</access>
  <dim>128</dim>
  <dimIncrement>4</dimIncrement>
  <!-- [Other elements omitted] -->
</register>

<!-- CAN1 (this one is complete!) -->
<register derivedFrom="CAN0.eRAM[%s]">
  <name>eRAM[%s]</name>
  <description>Embedded RAM</description>
  <dim>64</dim>
  <dimIncrement>4</dimIncrement>
</register>

The CAN1 peripheral only has half the amount of "RAM" registers, but the fields are otherwise identical.

[scootermon]

For reference, I found the following in the SVDSuite project:

https://github.com/ARMify-Project/SVDSuite/blob/fb45cbd1f3cef157a715f4de28ecc2e548cae47e/tests/svd/development/algorithm.svd#L82-L83

and the corresponding test assertions:

https://github.com/ARMify-Project/SVDSuite/blob/fb45cbd1f3cef157a715f4de28ecc2e548cae47e/tests/test_process/test_development.py#L145-L197

From a cursory glance I believe the svdconv implementation is consistent with this as well, but the C++ isn't exactly easy to follow and I didn't test it myself.

[Emilgardis]

Really strange that CI hasn't run.
Might be because this pr is based on inomotech-foss:master instead of a non-main branch

I agree with being hesitant to this change. Does the svd pass through svdconv?

[scootermon]

Really strange that CI hasn't run. Might be because this pr is based on inomotech-foss:master instead of a non-main branch

I think it's some weirdness with the CI not being approved in time. I rebased the PR so you should be able to approve the workflow run again. You can also see the run from the forked repo here: https://github.com/inomotech-foss/svd/actions/runs/20753031060

[scootermon]

I agree with being hesitant to this change. Does the svd pass through svdconv?

@Emilgardis I ran ./svdconv test.svd --outdir scratch --generate=header (svdconv version 3.3.49) and it generated the following header:

typedef struct {                                /*!< CAN0 Structure                                                            */
  /* [SNIP] */
  __IM  uint32_t  RESERVED4[11];
  __IOM uint32_t  CAN0_eRAM[128];               /*!< Embedded RAM                                                              */
  __IM  uint32_t  RESERVED5[384];
  /* [SNIP] */
} CAN0_Type;

typedef struct {                                /*!< CAN1 Structure                                                            */
  /* [SNIP] */
  __IM  uint32_t  RESERVED4[11];
  __IOM uint32_t  CAN1_eRAM[64];                /*!< Embedded RAM                                                              */
  __IM  uint32_t  RESERVED5[448];
  /* [SNIP] */
} CAN1_Type;

So it definitely understands the <dim> override.

I went ahead and crafted a minimal SVD file to reproduce this. You can find it along with the generated header file here: https://gist.github.com/scootermon/68828d5bc4f19b06dfb3a0b1e473d155

[burrbull]

@scootermon

LGTM
But:

1. I don't understand why old code does not work for your example. Unless there's something important hidden under <!-- [Other elements omitted] -->
2. You have not answer on my previous suggestion.

[scootermon]

2. You have not answer on my previous suggestion.

Apologies, @burrbull. I first wanted to settle whether there's even a chance for this PR to get merged before going into the details.

1. I don't understand why old code does not work for your example. Unless there's something important hidden under <!-- [Other elements omitted] -->

No, there's nothing important hidden there, it's just a lot more registers that aren't relevant to the issue.
Running the current svd2rust version on the SVD file from my gist (https://gist.github.com/scootermon/68828d5bc4f19b06dfb3a0b1e473d155#file-test-svd) generates incorrect code.
It generates both struct CAN0 and struct CAN1 with eram: [Eram; 128], but the can1 instance doesn't have 128 instances of the ERAM register, only 64!