Skip to content
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.

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

Use AsMut/AsRef-like deref instead of unions #218

Closed
Emilgardis opened this issue May 14, 2018 · 6 comments
Closed

Use AsMut/AsRef-like deref instead of unions #218

Emilgardis opened this issue May 14, 2018 · 6 comments

Comments

@Emilgardis
Copy link
Member

Emilgardis commented May 14, 2018

Currently, svd2rust supports overlapping registers via the cli flag --nightly. This will generate an union of all registers that overlap each other.

Since it is not expected that non-Copy fields in unions will be stabilized soon (see tracking issue rust-lang/rust#32836), another system for access would prove beneficial for library maintainers and users wanting to access overlapping registers using stable rust.

cc @wez @japaric

related #16

@wez
Copy link
Contributor

wez commented May 15, 2018

Yeah, makes sense. Referencing this example from atsamd21:

https://docs.rs/atsamd21g18a/0.2.1/atsamd21g18a/tc3/union.RegisterBlock.html

instead of the union generate something like this:

pub struct RegisterBlock {}

impl RegisterBlock {
    fn as_count8_mut(&mut self) -> &mut COUNT8 {
        unsafe {
            &mut *((self as *mut Self) as *mut COUNT8)
        }
    }

    fn as_count8_ref(&self) -> &COUNT8 {
        unsafe {
            &*((self as *const Self) as *const COUNT8)
        }
    }

    fn as_count16_mut(&mut self) -> &mut COUNT16 {
        unsafe {
            &mut *((self as *mut Self) as *mut COUNT16)
        }
    }

   // as_count16_ref(), as_count32_mut(), as_count32_ref() here
}

I've found that in atsamd21-hal I'd often make a safe accessor for the union anyway, so this removes some boilerplate for me.

@wez
Copy link
Contributor

wez commented May 15, 2018

(caffeine kicks in) or are you suggesting that we literally implement AsMut/AsRef? That would avoid figuring out naming for accessor methods

@wez
Copy link
Contributor

wez commented May 15, 2018

I can play around with this, since I think I'm the only person currently with a crate that requires and is using this :)

@Emilgardis
Copy link
Member Author

Emilgardis commented May 15, 2018

I'm pretty sure having functions providing the variants is the most ergonomic instead of using AsRef/AsMut (as those doesn't take a type in the function).

// Definition of register block
struct RegisterBlock {
    reg1: CCMR,
    ...
}

struct CCMR {
    ...
}

struct CCMR_INPUT {
    ...
}

struct CCMR_OUTPUT {
    ...
}

assert!(reg.is::<CCMR>());

// using AsRef and AsMut for CCMR_*
let input: &mut CCMR_INPUT = reg.as_mut();
input.write(...);

// using functions in CCMR
reg.as_input().write(...);

However, another approach, which I find the most convincing, would be to have traits that are syntactic sugar for the AsRef and AsMut route and possible define it in bare-metal. That would make the following possible:

use bare_metal::AsMutRegister; // TODO: better trait and fn name

reg.as_mut_register::<CCMR_INPUT>().write(...);

// and

fn foo<I: AsMut<CCMR_INPUT>>(mut input: I) {
    input.as_mut().write(...);
}

foo(reg);

EDIT:: fixed some mistakes in fn foo

wez added a commit to wez/svd2rust that referenced this issue Jan 3, 2019
This removes any observable side effect of the --nightly switch by
removing the use of unions and the untagged_unions feature gate.
Unions are replaced with accessor functions that return the appropriate
register block reference.

Here's a playground link that shows that the pointer calculation looks
reasonable:

https://play.integer32.com/?version=stable&mode=debug&edition=2018&gist=cd56444abc03e6781e526bbddc7082bc

This commit is a breaking change.

This is based on this WIP PR branch:
rust-embedded#256

It implements the easiest standalone portion of this issue:
rust-embedded#218

and also makes accessing the unions "safe" too, which is requested here:
rust-embedded#230
@wez
Copy link
Contributor

wez commented Jan 4, 2019

I have some WIP in https://github.com/wez/svd2rust/tree/no_unions
It happens to look like the suggestion in #230

@burrbull burrbull mentioned this issue Jul 13, 2019
burrbull pushed a commit to burrbull/svd2rust that referenced this issue Jul 19, 2019
This removes any observable side effect of the --nightly switch by
removing the use of unions and the untagged_unions feature gate.
Unions are replaced with accessor functions that return the appropriate
register block reference.

Here's a playground link that shows that the pointer calculation looks
reasonable:

https://play.integer32.com/?version=stable&mode=debug&edition=2018&gist=cd56444abc03e6781e526bbddc7082bc

This commit is a breaking change.

This is based on this WIP PR branch:
rust-embedded#256

It implements the easiest standalone portion of this issue:
rust-embedded#218

and also makes accessing the unions "safe" too, which is requested here:
rust-embedded#230
@burrbull burrbull mentioned this issue Jul 19, 2019
bors bot added a commit that referenced this issue Jul 19, 2019
318: No unions3 r=therealprof a=burrbull

Implemented by @wez this PR replace untagged unions with functions for access to alternate registers.

Closes #230, #218, stm32-rs/stm32-rs#149 .

cc @therealprof

Co-authored-by: Wez Furlong <[email protected]>
@burrbull burrbull mentioned this issue Jul 19, 2019
@adamgreig
Copy link
Member

I believe this is now solved by #318.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants