Skip to content

[WIP] DO NOT MERGE: Code reduction #303

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

Closed
wants to merge 5 commits into from
Closed

Conversation

therealprof
Copy link
Contributor

Reduce code by eliminating duplication and boilerplating

@therealprof therealprof requested a review from a team as a code owner July 3, 2019 20:44
@rust-highfive
Copy link

r? @ryankurte

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools labels Jul 3, 2019
@therealprof
Copy link
Contributor Author

Those two changes reduce the file size of a cargo fmted STM32F042 SVD lib.rs from 7893118 to 5911219 and compile time from 26.57s/24.28s to 17.00s/16.95s (unoptimized/optimized).

@therealprof
Copy link
Contributor Author

Now down to 5097551 bytes and 14.43s.

@therealprof
Copy link
Contributor Author

CC @adamgreig @jamesmunns @japaric

Interested to learn what others think. Changes should be API compatible with what we have now but stm32-rs would need some adjustments.

It would also be possible to move all of that code (and more) into a generic memory-mapped crate allowing for far more features and custom implementations of a memory mapped register interface but for now I wanted to keep things simple to play around with some changes.

@burrbull
Copy link
Member

burrbull commented Jul 6, 2019

reset_value() should be marked as const fn or replaced with
pub const RESET_VALUE: W = W { bits: #rv };

@therealprof therealprof added the S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. label Jul 8, 2019
@therealprof
Copy link
Contributor Author

@burrbull I guess there're a few non-breaking uncontroversial changes which could still be applied. But at the moment this is not going to fly; it's not particularly pretty but also breaks the HAL impls due to the requirement to import the traits.

I'm still looking into ways to generically express the individual registers in a way that would allow us have a default trait impl for read, write and modify which would yield the largest reduction in code since those are completely identical. But I haven't found the key to do that yet.

@therealprof therealprof changed the title [WIP] Code reduction [WIP] DO NOT MERGE: Code reduction Jul 8, 2019
@burrbull
Copy link
Member

burrbull commented Jul 8, 2019

I also made many experiments with traits to make default implementation and could not find good solution of the issue. The main problem is access to bits self.register.set(w.bits);

The only way I could reduce code size is using macros.

@burrbull
Copy link
Member

burrbull commented Jul 10, 2019

This is the best I could think of.

Traits could be moved to vcell crate:

pub trait FromBits<T> {
    fn from_bits(bits: T) -> Self;
}

pub trait ResetValue {
    #[doc = r" Reset value of the register"]
    const RESET_VALUE: Self;
}

/// Reads the contents of the register
pub trait ReadRegister<R, T>: core::ops::Deref<Target=VolatileCell<T>>
where
    R: FromBits<T>,
    T: Copy,
{
    fn read(&self) -> R {
        R::from_bits((*self).get())
    }
}

/// Writes to the register
pub trait WriteRegisterWithReset<W, T>: core::ops::Deref<Target=VolatileCell<T>>
where
    W: ResetValue + core::ops::Deref<Target=T>,
    T: Copy,
{
    #[inline]
    fn write<F>(&self, f: F)
    where
        F: FnOnce(&mut W) -> &mut W,
    {
        let mut w = W::RESET_VALUE;
        f(&mut w);
        (*self).set(*w);
    }
}

/// Writes to the register
pub trait WriteRegisterWithZero<W, T>: core::ops::Deref<Target=VolatileCell<T>>
where
    W: FromBits<T> + core::ops::Deref<Target=T>,
    T: Copy+Default,
{
    #[inline]
    fn write<F>(&self, f: F)
    where
        F: FnOnce(&mut W) -> &mut W,
    {
        let mut w = W::from_bits(T::default());
        f(&mut w);
        (*self).set(*w);
    }
}

/// Writes the reset value to the register
pub trait ResetRegister<W, T>: WriteRegisterWithReset<W, T>
where
    W: ResetValue + core::ops::Deref<Target=T>,
    T: Copy,
{
    fn reset(&self) {
        self.write(|w| w)
    }
}

/// Modifies the contents of the register
pub trait ModifyRegister<R, W, T>: core::ops::Deref<Target=VolatileCell<T>>
where
    R: FromBits<T>,
    W: FromBits<T> + core::ops::Deref<Target=T>,
    T: Copy,
{
    #[inline]
    fn modify<F>(&self, f: F)
    where
        for<'w> F: FnOnce(&R, &'w mut W) -> &'w mut W
    {
        let bits = (*self).get();
        let r = R::from_bits(bits);
        let mut w = W::from_bits(bits);
        f(&r, &mut w);
        (*self).set(*w);
    }
}

Macros for code reduction:

#[macro_export]
macro_rules! deref_cell {
    ($REG:ty, $T:ty) => {
        impl core::ops::Deref for CR1 {
            type Target = vcell::VolatileCell<$T>;
            fn deref(&self) -> &Self::Target {
                &self.register
            }
        }
    }
}

#[macro_export]
macro_rules! w {
    ($T:ty) => {
        pub struct W {
            bits: $T,
        }

        impl core::ops::Deref for W {
            type Target = $T;
            fn deref(&self) -> &Self::Target {
                &self.bits
            }
        }

        impl crate::FromBits<$T> for W {
            fn from_bits(bits: $T) -> Self {
                Self { bits }
            }
        }
    }
}

#[macro_export]
macro_rules! r {
    ($T:ty) => {
        pub struct R {
            bits: $T,
        }

        impl crate::FromBits<$T> for R {
            fn from_bits(bits: $T) -> Self {
                Self { bits }
            }
        }
    }
}

Usage:

pub struct CR1 {
    register: vcell::VolatileCell<u32>,
}
crate::deref_cell!(CR1, u32);

pub mod cr1 {
    crate::w!(u32);
    crate::r!(u32);

    impl vcell::ResetValue for W {
        const RESET_VALUE: Self = Self {bits: 0};
    }
    impl vcell::WriteRegisterWithReset<W, u32> for super::CR1 {}
    impl vcell::ResetRegister<W, u32> for super::CR1 {}
    impl vcell::ReadRegister<R, u32> for super::CR1 {}
    impl vcell::ModifyRegister<R, W, u32> for super::CR1 {}
}

or

pub mod cr1 {
    crate::w!(u32);
    crate::r!(u32);

    impl vcell::WriteRegisterWithZero<W, u32> for super::CR1 {}
    impl vcell::ReadRegister<R, u32> for super::CR1 {}
    impl vcell::ModifyRegister<R, W, u32> for super::CR1 {}
}

@burrbull
Copy link
Member

#258

@burrbull
Copy link
Member

You forgot about 1bit-wise variants.

@burrbull burrbull mentioned this pull request Jul 11, 2019
@burrbull
Copy link
Member

New implementation: #306

@therealprof
Copy link
Contributor Author

@burrbull That is very interesting! Lets see whether @japaric is happy to take the vcell change or whether we have to move it someplace else but I like your use of traits; feel free to move more of my code around if you see the chance; I'm not particularly fond of the set...bits functions, they're rather hacky.

@burrbull
Copy link
Member

I would bring set_bits_ back to svd2rust, but I don't know how to access to them:
crate:: works if functions on top level.

But in this case they must be used from subpackage: https://github.com/stm32-rs/stm32-rs/blob/master/stm32f0/src/lib.rs

@therealprof
Copy link
Contributor Author

@burrbull Maybe it'd be possible to use traits and default impls by using the trick here: https://github.com/dtolnay/inherent

@burrbull
Copy link
Member

It depends on proc_macro2. It will take time to compile dependencies.
If use procedural macros, we can use traits+derive as I proposed in #285 (of course with some changes).

@therealprof
Copy link
Contributor Author

@burrbull I did not necessarily mean the crate but the idea to avoid having to use the crates implementating the traits everywhere. We're already already generating code in svd2rust so it'd be simple to incorporate the inherent behaviour, though I'm not sure it is worth it.

@burrbull
Copy link
Member

Incorporate where?
Into svd2rust (I don't see how it can help)
or in generated code?

@therealprof
Copy link
Contributor Author

In the generated code.

@burrbull
Copy link
Member

I've applied cargo expand on example in inherent repo and what I get:

#![feature(prelude_import)]
#![no_std]
#[prelude_import]
use ::std::prelude::v1::*;
#[macro_use]
extern crate std as std;
mod types {
    use inherent::inherent;
    trait Trait {
        fn f(self);
    }
    pub struct Struct;
    impl Struct {
        pub fn f(self) {
            <Self as Trait>::f(self)
        }
    }
    impl Trait for Struct {
        fn f(self) {}
    }
}
fn main() {
    types::Struct.f();
}

it added

impl Struct {
   ...
}

@therealprof
Copy link
Contributor Author

Yep, I was think along the lines of adding traits for register Read, Write, ... whatever and impls for that traits and those inherent style proxy functions hiding away the trait impls for the outside. Not sure it's worth it in terms of code size and/or compile time though.

@burrbull
Copy link
Member

burrbull commented Jul 22, 2019

Looks like inherent doesn't support default impls in traits. So it useless for us.

@therealprof
Copy link
Contributor Author

I'm still looking at the mechanism, not this particular crate.

@burrbull
Copy link
Member

I have idea:
https://gist.github.com/6bd0d6a20a52c0704c767c8bb3813c6a

The main change is to use generic writer/reader structures (AKA W, R) instead of
generating them in each module.

@therealprof
Copy link
Contributor Author

Nice!

@burrbull
Copy link
Member

In any case, we must release svd2rust as is (or with minimal changes) and think how to improve it in future.

@therealprof
Copy link
Contributor Author

Yes, we should do a release soon...

@burrbull
Copy link
Member

burrbull commented Jul 22, 2019

@burrbull
Copy link
Member

burrbull commented Jul 25, 2019

@burrbull
Copy link
Member

burrbull commented Aug 2, 2019

I made draft of WProxy. See from 200 line:
https://gist.github.com/fb6ecf0f0b9c23e9d1d1ea42637cc7f9
This variant with inline offset. As we saw it require some additional memory in debug.

With const generics it will be:

    ///Writer Proxy
    pub struct WProxy<'a, U, REG, N, FI, const: O: u8> {
        w: &'a mut W<U, REG>,
        _field: marker::PhantomData<(FI, N)>,
    }

@burrbull
Copy link
Member

burrbull commented Aug 3, 2019

I tried to use associated size type where it's possible and got very nice code:
https://github.com/burrbull/svd2rust/tree/associated
but firmware size increased ~5% for both release and debug build. 😢

@therealprof
Copy link
Contributor Author

Let's close this superseded PR.

@therealprof therealprof closed this Aug 6, 2019
@therealprof therealprof deleted the branch/code-reduction branch December 31, 2019 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants