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

regs/macro.rs: add optional conversion from value into bit field enum member #1019

Merged
merged 8 commits into from Jun 26, 2018

Conversation

Projects
None yet
4 participants
@eupn
Copy link
Contributor

eupn commented Jun 22, 2018

Pull Request Overview

This pull request adds a function:

pub fn read_as_enum<E: TryFromValue<T, EnumType=E>>(&self, field: Field<T, R>) -> Option<E>

for types ReadOnly<T, R> and ReadWrite<T, R> of regs.rs.

This function enables optional conversion from arbitrary integer value of a bitfield into one of the possible enumerated values specified for the bitfield.

Since for some bit fields there can be a set of enumerated values associated with it, it would be great if one could read a bit field and match over those values.

For example, consider following register and its bit fields:

register_bitfields![
CLK_CTRL [
    /* ---%< -snip- %<--- */

    /// Select SYS_CLK source
    SYS_CLK_SEL OFFSET(30) NUMBITS(2) [
        /// 32M internal clock
        _32MInternalClock = 0,
        /// external crystal clock
        ExternalCrystalClock = 1,
        /// 32K clock
        _32KClock = 2
    ]
],
]

If one wants to read this register and do something for each or some of its possible enumerated values, one could write like this:

let clk_sel_val = syscon.clk_ctrl.read(CLK_CTRL::SYS_CLK_SEL);
match clk_sel_val {
    0 => do_stuff_with_32mhz_int(),
    1 => do_stuff_with_32mhz_ext(),
    2 => do_stuff_with_32khz(),

    _ => panic!(""),
}

For now, one can only read bitfield's value as an integer, and to match on possible values of bitfield it is required to know exact integer representation (iotw, variant number) of those values. Needless to say that such approach can easily lead to a bug.

As an alternative, there are match_all() or match_any() functions that can be used to check whether bitfield contains a value:

let clk_sel_val = syscon.clk_ctrl.read(CLK_CTRL::SYS_CLK_SEL);

if syscon.clk_ctrl.matches_all(CLK_CTRL::SYS_CLK_SEL::_32MInternalClock) {
    do_stuff_with_32mhz_int();
} else if syscon.clk_ctrl.matches_all(CLK_CTRL::SYS_CLK_SEL::ExternalCrystalClock) {
    do_stuff_with_32mhz_ext();
} else if syscon.clk_ctrl.matches_all(CLK_CTRL::SYS_CLK_SEL::_32KClock) {
    do_stuff_with_32khz();
} else {
    // Invalid value read from bit field
    panic!("")
}

But this approach tends to produce a hard-to-read chain of if and else, doesn't exploit powerful match machinery of Rust and won't work well for bit fields with more than one or two possible enumerated values.

This pull request is extending a register_bitfield![] macro and Read(Only/Write)<T, R> types such that it is possible to read bitfield value and perform a match over it as enum:

let clk_sel_val = syscon.clk_ctrl.read_as_enum(CLK_CTRL::SYS_CLK_SEL);

match clk_sel_val {
    Some(CLK_CTRL::SYS_CLK_SEL::Value::_32MInternalClock) => do_stuff_with_32mhz_int(),
    Some(CLK_CTRL::SYS_CLK_SEL::Value::ExternalCrystalClock) => do_stuff_with_32mhz_ext(),
    Some(CLK_CTRL::SYS_CLK_SEL::Value::_32KClock) => do_stuff_with_32khz(),

    None => panic!(""),
}

or:

let clk_sel_val = syscon.clk_ctrl.read_as_enum(CLK_CTRL::SYS_CLK_SEL);

if let Some(clk_sel_val) = clk_sel_val {
    match clk_sel_val {
        CLK_CTRL::SYS_CLK_SEL::Value::_32MInternalClock => do_stuff_with_32mhz_int(),
        CLK_CTRL::SYS_CLK_SEL::Value::ExternalCrystalClock => do_stuff_with_32mhz_ext(),
        CLK_CTRL::SYS_CLK_SEL::Value::_32KClock => do_stuff_with_32khz(),
    }
} else {
      // Invalid value read from bit field
      panic!("");
}

Testing Strategy

This pull request was tested by make allboards

TODO or Help Wanted

n/a

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make formatall.

@eupn eupn changed the title regs/macro.rs: add optional conversion from value into bitfield enum member regs/macro.rs: add optional conversion from value into bit field enum member Jun 22, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jun 22, 2018

Do you think it is possible to combine these two lines?

let clk_sel_val = syscon.clk_ctrl.read(CLK_CTRL::SYS_CLK_SEL);
let clk_sel_val = CLK_CTRL::SYS_CLK_SEL::Value::try_from(clk_sel_val);

maybe something like

let clk_sel_val = syscon.clk_ctrl.read_as(CLK_CTRL::SYS_CLK_SEL);

?

@eupn

This comment has been minimized.

Copy link
Contributor

eupn commented Jun 22, 2018

@bradjc

Do you think this is possible to combine these two lines

Yes, I can do it with generic function, like this in ReadOnly<T, R>, ReadWrite<T, R> types:

pub fn read_as_enum<E: TryFromValue<T, EnumType=E>>(&self, field: Field<T, R>) -> Option<E>

And then by implementing TryFromValue trait for bitfield's Value enums, we can read register's bitfield value as a enum in single call:

let clk_sel_val = syscon.clk_ctrl.read_as_enum::<CLK_CTRL::SYS_CLK_SEL::Value>(CLK_CTRL::SYS_CLK_SEL);

Rust compiler will infer all type parameters for that function so one doesn't have to specify enum type with turbo-fish syntax, that's pretty cool:

let clk_sel_val = syscon.clk_ctrl.read_as_enum(CLK_CTRL::SYS_CLK_SEL);

These changes done in the follow-up commits.

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jun 22, 2018

let clk_sel_val = syscon.clk_ctrl.read_as_enum(CLK_CTRL::SYS_CLK_SEL);

That's pretty awesome. Looks good to me, but could you please add the function to the readme?

@bradjc bradjc added the P-Upkeep label Jun 24, 2018

@phil-levis
Copy link
Collaborator

phil-levis left a comment

I like this a lot -- I've definitely had to fight against this limitation in converting to Register.

@bradjc bradjc referenced this pull request Jun 25, 2018

Merged

tools: fix build-all-docs script #1030

2 of 2 tasks complete

eupn added some commits Jun 22, 2018

regs/macro.rs: add try_from for bitfield enum
This commit will add a function `try_from` for any bitfield created with register_bitfield![] macro. This function enables optional conversion from arbitrary integer value into one of the possible enumerated values specified for the bitfield.
regs/README.rs: added read_as_enum() and matching
Added info about read_as_enum() function and matching into Reading and Matching sections.

@bradjc bradjc force-pushed the eupn:patch-2 branch from 5024e08 to d81c6b5 Jun 26, 2018

keep `use` formatting consistent
Doesn't look like rustfmt is smart enough to enforce this in macros yet.
@ppannuto
Copy link
Member

ppannuto left a comment

This is great! Thanks 👍

@bradjc bradjc merged commit 660672d into tock:master Jun 26, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@eupn eupn deleted the eupn:patch-2 branch Jun 26, 2018

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