Common traits for identical IP blocks

[jameysharp]

I want to be able to write generic functions that can operate on any instance of the same peripheral across a vendor's product line, or any instance of the same peripheral within a given chip.

For a concrete example: as far as I know, ST Micro uses the same bxCAN IP on every chip in the STM32 line that has a CAN peripheral, and on some chips they have multiple copies of the bxCAN peripheral. So I'd like to be able to write generic code that uses a bxCAN peripheral, independent of which STM32 product you're building for and which instance of the peripheral you're using.

I think a lot of the relevant information can be inferred directly from the .svd files, by checking whether two peripherals have identical register definitions, ignoring reset values. But I can't decide how much manual intervention makes sense.

What are your thoughts?

[japaric]

This sound a bit like the derivedFrom feature that SVD files have, except that this would have to operate across different files. I think it makes sense to have some sort of inheritance between files. For example:

Suppose that some peripherals (e.g. TIM7) are common to several STM32 devices so those could be factored out in a common file, e.g. stm32_common.svd then a particular SVD could refer to that common file:

<!-- stm32f100xx.svd -->
<peripherals>
  <peripheral derivedFrom="stm32_common.svd TIM7">
    <name>TIM7</name>
    <baseAddress>0xdeadbeef</baseAddress>
  </peripheral>
  ..
</peripherals>

Then svd2rust would generate something like this:

extern crate stm32_common;

pub const TIM7: Peripheral<stm32_common::tim7::RegisterBlock> = unsafe { Peripheral::new(0xdead_beef) };

Where stm32_common is the crate generated by svd2rust from stm32_common.svd. Then you could write your library generically against stm32_common and be able to use derived crates like stm32f100xx as the types would match.

The thing is that SVD files don't support this syntax, and to make this work we would have to modify SVD files to indicate the "file inheritance". So I'm not sure how to proceed here.

Perhaps we could have some external file that specifies that the peripheral TIM7 of stm32f100xx is derived from the peripheral TIM7 of stm32_common. That would make svd2rust ignore stm32f100xx.svd's information about TIM7 and generate code like above.

[jameysharp]

I really like your external-file proposal, to avoid having to modify the vendor-provided files. It doesn't have to completely ignore the original SVD, though. For one thing, it could check that the API for the common version is a superset of the API it would have generated, perhaps adding enumerated values but not changing anything else, as a sanity check.

Also, ideally I'd want the reset value for each register taken from the vendor definition, not the common one. This matters even within one chip, not just across a product line. For example, on STM32F411, GPIOA and GPIOB are not derivedFrom any other GPIO block, but all of the GPIO blocks are identical except for reset value. Since reset value doesn't affect the API from the user's point of view, ideally it wouldn't force different types either.

For convenience, it'd be nice to also build a tool that generates an initial common.svd and external map from a directory full of SVD files for a particular product line, by automatically identifying register layouts that are equivalent. I think that's an entirely automatable process.

Thanks for thinking about this! I feel my thoughts are more clear already from your feedback. 😁

[idubrov]

As noted above, being able to have common interface would be useful not only across different devices, but also across different peripherals in the same device.

Moreover, some peripherals could have different features, but still share some common functionality.

For example, take STM32 timers. There are three types of timers: "basic", "general-purpose" and "advanced". All three share features of "basic" timer. "general-purpose" and "advanced" have few more features (like "slave mode").

Library that uses quadrature encoder feature, for example, might be coded against interface that is common between "general-purpose" and "advanced" timers (since "basic" timer does not support quadrature encoders). Some other library might be coded against interface that is common across all three types of timers. Motor control application would only work with "advanced" timer.

As for the common SVD file approach, would SVD "duck typing" work here? You can make it that any peripheral that matches some definition from common SVD would automatically implement traits generated from that SVD. Something along these lines.

Similar question is that how well common interfaces based on low-level registers would work? For example, GPIO ports have similar features in STM32F051 vs STM32F103 (pull-ups/downs, input/output, BRR/BSRR registers), but configuration registers are completely different. I don't really like HAL library, but, I think, this is the problem it tries to solve.

[japaric]

I think in the case of timers peripheral inheritance would work. Basically the general purpose timer (e.g. TIM2 would derive from the basic timer (e.g. TIM7). In the SVD this would be represented by having <peripheral derivedFrom="TIM7"> for TIM2 plus the <peripheral> tree would include the registers that TIM2 has but TIM7 hasn't. In the svd2rust generated code the change would be that tim7::RegisterBlock would be able to deref to tim2::Registerblock. Finally the end user would be able to write APIs like this:

fn do_something_with_basic_timer(tim: &tim7::RegisterBlock} { .. }

do_something_with_basic_timer(&TIM7);
do_something_with_basic_timer(&TIM2);

This hasn't been implemented (I think there's an issue open about this) but it's something that SVDs support.

but configuration registers are completely different.

Peripheral inheritance only handles the case where the registers of a peripheral are a subset of other peripheral's. For this GPIO case you would have to write a HAL.

[idubrov]

Is it possible to inherit the whole peripheral and then add an extra field to some inherited register? For example, "basic" timer would not have field CKD (clock division) in CR1, but "general-purpose" timer would.

[japaric]

The SVD spec says

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

In your case if TIM2 defines a CR1 <register> tree then it must define all the fields of CR1. At that point TIM7.CR1 and TIM2.CR1 have different types so maybe my idea of using Deref would have be more complicated to implement that I expected (or not implementable in the way I depicted above).

[jameysharp]

@idubrov, good point about duck typing. I can't imagine a circumstance where two peripherals with identical registers would need different types. So maybe the external file mapping definitions back to the common SVD is unnecessary.

Regarding your other two points, I think they're important questions, maybe with the same answer. Is it enough to hand-write traits and implementations to cover the commonalities between peripherals that are not identical but have overlapping functionality? Regarding STM32 timers, I'm imagining that the crate generated by svd2rust for the common SVD would supplement the auto-generated Rust module with a handwritten module of traits and their implementations for the common peripherals.

@japaric, as long as the fields in corresponding registers are a subset, and fields that are in both are at the same offsets, then a Deref implementation based on mem::transmute should work fine, I think?

[japaric]

as long as the fields in corresponding registers are a subset, and fields that are in both are at the same offsets, then a Deref implementation based on mem::transmute should work fine, I think?

Yes that would work. Just pointing out that the derivedFrom feature does overrides instead of "append this stuff to the base". This makes the subset check trickier because you have to recursively check that all uses of derivedFrom are being used to extend the base, and not to override some of its properties.

And I forgot to comment on this before:

by checking whether two peripherals have identical register definitions, ignoring reset values

It is required that the reset values match otherwise register.reset() would behave differently on each device. Also all write operations start from the reset value: the methods of the W proxy modify that reset value, and the modified value is written into the register. Again, if the reset values don't match then register.write(|w| w.bitfield().bits(1)) would behave differently.

[jameysharp]

While I agree that both .reset and .write need to behave differently on peripherals with different reset values, that has no effect on their API—they are source-code compatible. So I'd still like to be able to write code that is generic over different instances of a peripheral even if those instances have different reset states. The Rust compiler would wind up producing different binary code, but the source code needn't change. If the differing reset values are in fields that are meaningful to the generic code, then it should set them explicitly.

[japaric]

If the differing reset values are in fields that are meaningful to the generic code, then it should set them explicitly.

I think this sets the bar too high. Now the author of the generic code needs to (a) check every single device that their code might be used against while writing the implementation, or (b) write the implementation very defensively making sure to never use reset() and to set all the fields when using write() without compiler assistance. If they forget even one field in some write call they will have possibly introduced a bug in their code that will go unnoticed during their testing until it blows up in someone else's face. IMO, if we want to expose generic code where the reset value is unknown then the API should reflect that and (a) not expose reset and (b) somehow force the caller of write to set all the register's fields.

[jameysharp]

That's a reasonable position. I'm not sure I agree, but I'd be happy to see a version of svd2rust that addresses my other concerns without being generic over reset values. Once we have experience with some level of generic API, we'll be better able to evaluate whether it makes sense to be generic over reset values too.

So, what would you suggest for next steps?

[japaric]

Tentative next steps:

• Come up with a definition for "peripheral equality". When two peripherals are
  equal svd2rust can automatically use the current "derivedFrom" logic. But when
  are two peripherals considered equal?

  • My first estimate would be: peripheral names don't matter, base addresses
    don't matter, all the registers must be "equal" -- this register equality
    can leave off documentation (<description> fields). I consider that reset
    values of each register must match in value as well but the OP disagrees.
    Interrupt information would likely be left off from the analysis as it
    doesn't matter on which peripheral that information appears (*)

• Put this definition to test. IMO, the easiest way would be to write a tool
  that uses the same SVD parser that svd2rust uses and looks for identical
  peripherals within a single SVD file . The SVD files @ryankurte linked in
  Device family HALs in rust wg#33 seem like a good test suite: it seems that e.g. USART1
  and USART0 are identical; the tool should confirm this.

  • At this point we could simplify some SVD files by using "derivedFrom" in
    places where it was not previously used. The tool would indicate where
    "derivedFrom" can be used.

• Once the definition is in good shape the tool could be extended to search for
  equivalent peripherals across different SVD files. It would good to run the
  tool across a bunch of SVD files that roughly belong to the same device
  family, e.g. all the STM32F1 devices, to get an idea of how similar those
  devices are. Running the tool across all the stm32 devices would be
  interesting as well. Is there any shared peripherall across all those?

• The next step would be to decide how to deal with equivalent peripherals
  across different SVD files in svd2rust. The case where N devices share the
  same peripheral seems easy to handle with a single common crate and reexports,
  but if there are 3 or 4 variants of a peripheral across, let's say, 20 SVD
  files then how should the common crates be organized?

  • As I elaborated in
    Device family HALs in rust wg#33 (comment) I
    consider that reducing the number of types and having device crates as
    re-exports of a common crate is the proper way to deduplicate code. But the
    trait approach should also be explored.

  • This experimentation even could be done "out of tree" by manually creating a
    SVD file with common definitions (by copy pasting the definitions from other
    files) and then running svd2rust on it: that would be the common crate. Then
    other devices crates can be simply written as reexports of the common crate,
    at least for the simplest cases.

It would good to run the
tool across a bunch of SVD files that roughly belong to the same device
family, e.g. all the STM32F1 devices, to get an idea of how similar those
devices are.

@pftbest collected some similar information about MSP430 devices (see #122 (comment)). Maybe they can share some insights about how they collected that information?

(*) derivedFrom leaves off interrupt information. See below:

    <peripheral derivedFrom="DMA1">
      <name>DMA2</name>
      <baseAddress>0x40020400</baseAddress>
      <interrupt>
        <name>DMA2_Channel1</name>
        <description>DMA2 Channel1 global interrupt</description>
        <value>56</value>
      </interrupt>
      <interrupt>
        <name>DMA2_Channel2</name>
        <description>DMA2 Channel2 global interrupt</description>
        <value>57</value>
      </interrupt>
      <interrupt>
        <name>DMA2_Channel3</name>
        <description>DMA2 Channel3 global interrupt</description>
        <value>58</value>
      </interrupt>
      <interrupt>
        <name>DMA2_Channel4_5</name>
        <description>DMA2 Channel4 and DMA2 Channel5 global
        interrupt</description>
        <value>59</value>
      </interrupt>
    </peripheral>