rust-embedded / rust-embedded/svd2rust

Proposal: Generate proxy structs for registers

Open
#213 15 comments 3 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
857
Forks
164
PR merge metrics
No merged PRs in 30d

Description

Introduction

svd2rust generates two structs to represent a peripheral:

  1. The RegisterBlock, that resides at the memory location where the registers are mapped.
  2. A proxy struct that deref's to the RegisterBlock.

This is useful, because it means that the proxy struct can be moved and owned, allowing us to build safe abstractions on top of it.

I suggest that much in the same way, it would be useful to generate proxy structs for single registers, allowing us to move and own those registers.

TL;DR

Assume a peripheral named PERIPHERAL with a register named REG. Generate the current register struct (peripheral::REG, the one with the VolatileCell) as peripheral::reg::Register instead. Generate a proxy struct that re-uses the old name (peripheral::REG) that basically looks like the peripheral proxy struct (PERIPHERAL), but dereferences to peripheral::reg::Register.

Motivation

Suppose we have a peripheral that can be logically split into multiple parts, each of which has full control over a set of registers. We might want to write an API that looks something like this:

struct PERIPHERAL {
    part_a: PartA,
    part_b: PartB,
}


struct PartA { ... }

impl PartA {
    // methods that access register set A
}


struct PartB { ... }

impl PartB {
    // methods that access register set B
}

Right now there are several ways to implement such an API, each of which has drawbacks:

  1. Have PERIPHERAL borrow the peripheral struct. Give PartA and PartB references to their respective registers. This works fine, but it means we need lifetime parameters everywhere (no big deal), and we can't easily put Peripheral into a static.
  2. Don't give PartA and PartB anything, but have them access the registers in an unsafe way. This allows us to write a nice API, but under the hood it's harder to get right.
  3. Give the peripheral struct to a third struct, and pass that into PartA and PartB whereever they need to access registers. This is the way to go if there actually needs to be some synchronization when accessing registers, but if the register two sets are truly independent, this is unnecessary, cumbersome, and limits the API user's designs (because that third struct needs to be available whereever PartA and PartB live).

If we had proxy structs for registers, in the same way we have proxy structs for peripherals, we could just give those to PartA and PartB, allowing us to provide a nice API that doesn't require too much unsafe to implement. It could look something like this:

struct PERIPHERAL {
    part_a: PartA,
    part_b: PartB,
}

impl PERIPHERAL {
    fn new(peripheral: raw::PERIPHERAL) -> Self {
        PERIPHERAL {
            part_a: PartA {
                reg_a1: peripheral.reg_a1,
                reg_a2: peripheral.reg_a2,
            },
            part_b: PartB {
                reg_b1: peripheral.reg_b1,
                reg_b2: peripheral.reg_b2,
            },
    }
}


struct PartA {
    reg_a1: REG_A1,
    reg_a2: REG_A2,
}

impl PartA {
    // methods that access register set A
}


struct PartB {
    reg_b1: REG_B1,
    reg_b2: REG_B2,
}

impl PartB {
    // methods that access register set B
}

Design

Suppose we have a peripheral called PERIPHERAL with a register called REG. Currently, svd2rust would generate the following:

  1. The proxy struct PERIPHERAL.
  2. The register block peripheral::RegisterBlock.
  3. The struct peripheral::REG (containing the VolatileCell).
  4. The module peripheral::reg, containing all the other code related to that register.

I propose making the following changes:

  1. Generate the currently existing register struct (with the VolatileCell) as peripheral::reg::Register instead.
  2. Generate the new proxy struct peripheral::REG, that basically looks like PERIPHERAL, except it deref's to peripheral::reg::Register.
  3. Remove PERIPHERAL's PhantomData field. Instead add the proxy structs for all registers there.

Partial Implementation

I have a partial implementation of this proposal: branch, diff

This branch implements my suggestions 1. and 2., but not 3. I ran into some trouble adding the register proxy fields to the peripheral proxy struct, and got the distinct feeling that the code needs some thourough refactoring to do this right. I don't have time for this right now, so I opted to post this proposal first, to see if we can reach agreement first.

I created experimental branches of lpc82x and lpc82x-hal to test this partial implementation.

Open Questions

If this were implemented, do we still need RegisterBlock? It seems to me that it becomes redundant.

Feedback

Please leave your feedback to let me know what you think. If we can agree that this change would be worthwile, I'll try to allocate the time to implement it.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reviewing the proposed design and the partial register-proxies branch and diff, especially how register and peripheral proxy structs are generated. Resolve the open question about whether RegisterBlock remains necessary and determine how the peripheral proxy should own register proxies. Done means an agreed design is implemented and validated against the referenced lpc82x experiment.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
embedded-iot, tooling
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.