rust-embedded / svd2rust

Generate Rust register maps (`struct`s) from SVD files
Apache License 2.0
697 stars 150 forks source link

Implement the ability to work with `W` outside of `write` #708

Open AndreySmirnov81 opened 1 year ago

AndreySmirnov81 commented 1 year ago

I want for example instead of this

use stm32f1::stm32f103 as pac;
let periph = pac::Peripherals::take().unwrap();
let usart1 = periph.USART1;
// ...
usart1.cr1.write(|w| {
    w.ue().enabled();
    w.m().m9();
    w.pce().enabled();
    w.ps().even();
    w.te().enabled();
    w.tcie().enabled();
    w.re().disabled(); // <- First write
    w
});
// ...
usart1.cr1.write(|w| {
    w.ue().enabled();
    w.m().m9();
    w.pce().enabled();
    w.ps().even();
    w.te().enabled();
    w.tcie().enabled();
    w.re().enabled(); // <- Modified
    w
});
// ...
usart1.cr1.write(|w| {
    w.ue().enabled();
    w.m().m9();
    w.pce().enabled();
    w.ps().even();
    w.te().enabled();
    w.tcie().enabled();
    w.re().disabled(); // <- Modified
    w
});

to do so

use stm32f1::stm32f103 as pac;
let periph = pac::Peripherals::take().unwrap();
let usart1 = periph.USART1;
// ...
const USART_BASE_CFG: pac::usart1::cr1::W = pac::usart1::cr1::INITIAL_VALUE
    .ue().enabled()
    .m().m9()
    .pce().enabled()
    .ps().even()
    .te().enabled()
    .tcie().enabled()
    .re().disabled();
// ...
usart1.cr1.write(USART_BASE_CFG);
// ...
usart1.cr1.write(USART_BASE_CFG.re().enabled());
// ...
usart1.cr1.write(USART_BASE_CFG.re().disabled());

Or for example like this

let mut w = pac::usart1::cr1::INITIAL_VALUE;
w = w
   .ue().enabled()
   .m().m9()
   .pce().enabled()
   .ps().even()
   .te().enabled()
   .tcie().enabled()
   .re().disabled();
usart1.cr1.write(w);

Is it possible to change svd2rust to allow this?

adamgreig commented 1 year ago

Thanks for suggesting this!

We discussed it at the end of today's meeting (here) though didn't really come to any conclusions yet.

I think one problem is that even if we added a const W like your INITIAL_VALUE (which is fairly easy to do, though may affect compile times a bit), it's not clear how you could pass it in. The write() method takes a closure that returns a reference to the W, so for example something like this doesn't work:

let mut w: stm32f405::tim2::cr1::W  = unsafe { core::mem::transmute(0) };
w.opm().enabled();
tim2.cr1.write(|_| w.cen().enabled());
error[E0597]: `w` does not live long enough
  --> src/main.rs:33:24
   |
33 |     tim2.cr1.write(|_| w.cen().enabled());
   |                    --- ^----------------
   |                    |   |
   |                    |   borrowed value does not live long enough
   |                    |   returning this value requires that `w` is borrowed for `'static`
   |                    value captured here
...
43 | }
   | - `w` dropped here while still borrowed

Making a whole new function that takes W directly isn't a great option as it seems very likely impact compile times for something that's not likely to see much use. There are some workarounds, like you could use bits() on the W to load in the common fields and then just set the extra fields you want, or write your own wrapper around write() that also accepts a closure, but first sets your common fields before calling the closure. None of that's ideal though; maybe someone else will have a better idea of a way to include this.

It's worth noting that chiptool, an svd2rust fork, does already support this feature by only having one type for register values, so you can easily modify it and then pass it to write().

AndreySmirnov81 commented 1 year ago

Thank you for considering the proposal!

Let me clarify a little how I imagine it:

  1. make W copyable
  2. add method write_value(w: W)
  3. add constants INITIAL_VALUE: W
  4. make methods for getting field writers and their method as const fn (I'm not sure it's possible right now)

After that, instead of

write(|w| w.f1().a1().f2().a2());

you can write

write_value(*pac::peripheral::register::INITIAL_VALUE.f1().a1().f2().a2());

or for example

let mut val = pac::peripheral::register::INITIAL_VALUE;
val.f1().a1();
val.f2().a2();
write_value(val);

const VAL: pac::peripheral::register::W = *pac::peripheral::register::INITIAL_VALUE.f1().a1().f2().a2();
write_value(VAL);

Instead of

reset();

you can write

write_value(pac::peripheral::register::INITIAL_VALUE);