Unify arithmetic implementation of field registers and add unit tests.

[gendx]

Pull Request Overview

While reviewing #1661 (comment), it came up that there is some amount of code duplication in the arithmetic operations on various register types.

This pull request unifies the implementation, by adding new functions to the Field and FieldValue types, and making the other types use these. Unit tests are also added for these unified functions.

Some formulas are also simplified:

• (value << shift) & (mask << shift) => (value & mask) << shift
• (val & (field.mask << field.shift)) >> field.shift != T::zero() => val & (field.mask << field.shift) != T::zero()

Testing Strategy

This pull request was tested by Travis-CI.

TODO or Help Wanted

• matches_any(&self, field: FieldValue<T, R>) -> bool doesn't makes a lot of sense to me, because it doesn't use any "value" of the field. It could be matches_any(&self, field: Field<T, R>) -> bool instead. But then it's the same as the existing is_set(&self, field: Field<T, R>) -> bool. Should matches_any be removed? Tock doesn't use a lot of matches_any.
• ReadWrite and InMemoryRegister have the same implementation. Is that intended? I wonder why volatile read/write are needed for InMemoryRegister - given that such "register" is not MMIO. Tock doesn't use a lot of InMemoryRegister.
• This pull request could be extended by defining new traits to further remove code duplication. However, this would mean more refactoring to expose these traits, and a breaking change for the published crate (https://crates.io/crates/tock-registers).
  Here is a draft of that:

trait ReadAble<T: IntLike, R: RegisterLongName> {
    /// Get the raw register value
    fn get(&self) -> T;

    /// Read the value of the given field
    fn read(&self, field: Field<T, R>) -> T {
        field.read(self.get())
    }

    /// Read value of the given field as an enum member
    fn read_as_enum<E: TryFromValue<T, EnumType = E>>(&self, field: Field<T, R>) -> Option<E> {
        field.read_as_enum(self.get())
    }

    ...
}

trait WriteAble<T: IntLike, R: RegisterLongName> {
    /// Set the raw register value
    fn set(&self, value: T);

    /// Write the value of one or more fields, overwriting the other fields with zero
    fn write(&self, field: FieldValue<T, R>) {
        self.set(field.value);
    }
}

trait ReadWriteAble<T: IntLike, R: RegisterLongName>: ReadAble<T, R> + WriteAble<T, R> {
    /// Write the value of one or more fields, maintaining the value of unchanged fields via a
    /// provided original value, rather than a register read.
    fn modify_no_read(&self, original: LocalRegisterCopy<T, R>, field: FieldValue<T, R>) {
        self.set(field.modify(original.get()));
    }

    ...
}

// ==========
// ReadWrite

impl<T: IntLike, R: RegisterLongName> ReadAble<T, R> for ReadWrite<T, R> {
    fn get(&self) -> T {
        unsafe { ::core::ptr::read_volatile(&self.value) }
    }
}

impl<T: IntLike, R: RegisterLongName> WriteAble<T, R> for ReadWrite<T, R> {
    fn set(&self, value: T) {
        unsafe { ::core::ptr::write_volatile(&self.value as *const T as *mut T, value) }
    }
}

impl<T: IntLike, R: RegisterLongName> ReadWriteAble<T, R> for ReadWrite<T, R> {}

// ==========
// ReadOnly

impl<T: IntLike, R: RegisterLongName> ReadAble<T, T> for ReadOnly<T, R> {
    fn get(&self) -> T {
        unsafe { ::core::ptr::read_volatile(&self.value) }
    }
}

// ==========
// WriteOnly

impl<T: IntLike, R: RegisterLongName> WriteAble<T, R> for WriteOnly<T, R> {
    fn set(&self, value: T) {
        unsafe { ::core::ptr::write_volatile(&self.value as *const T as *mut T, value) }
    }
}

Documentation Updated

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

Formatting

• Ran make formatall.

[bradjc]

* `matches_any(&self, field: FieldValue<T, R>) -> bool` doesn't makes a lot of sense to me, because it doesn't use any "value" of the field. It could be `matches_any(&self, field: Field<T, R>) -> bool` instead. But then it's the same as the existing `is_set(&self, field: Field<T, R>) -> bool`. Should `matches_any` be removed? Tock doesn't use a lot of `matches_any`.

This is a good observation, and arguably the implementation of matches_any isn't quite correct. It seems like it should also return true if RegisterField::CLEAR matches.

* `ReadWrite` and `InMemoryRegister` have the same implementation. Is that intended? I wonder why volatile read/write are needed for `InMemoryRegister` - given that such "register" is not MMIO. Tock doesn't use a lot of `InMemoryRegister`.

InMemoryRegister uses volatile because the underlying memory might be mapped to a DMA peripheral as well. The different name signals that it is not a normal register.

[bradjc]

Looks good to me, but I'm not really a steward of this code, so my approval shouldn't weigh too heavily.

[brghena]

@andre-richter if you've got the time, it would be good to get your thoughts on this as someone using the registers interface externally.

[andre-richter]

I won’t have time to adapt to breaking changes Immediately , but that’s also not an issue since we have the previous versions on crates.io until I can adapt.

I guess a bump to 6.0.0 makes sense for this due to the breakage.

[gendx]

I won’t have time to adapt to breaking changes Immediately , but that’s also not an issue since we have the previous versions on crates.io until I can adapt.

I guess a bump to 6.0.0 makes sense for this due to the breakage.

As of now, the changes should not be breaking - the API is simply extended with more functions for Field and FieldValue. It is also made more efficient - in particular I noticed that the simpler arithmetic for is_set (one less right shift in the source code) actually reduces binary size for the hifive1 board at least (I did some manual analysis to confirm that - but the best would be to fix #1701 to know the effect on all the boards). So the compiler somehow didn't optimize that on that platform.

So I think we can merge the current changes as a 5.x update, and then in a separate PR explore the traits I proposed for a 6.x version.

[gendx]

Also, @andre-richter can you confirm that the following unit-test gating works for you?

#[cfg(not(feature = "no_std_unit_tests"))]
#[cfg(test)]

[andre-richter]

@gendx, misread the breaking part, sorry.

I pulled your patch locally and pointed a local register-rs crate copy to it, which I then hooked into a build of one of my kernels at https://github.com/rust-embedded/rust-raspberrypi-OS-tutorials/tree/master/14_exceptions_part2_peripheral_IRQs

Everything compiled and booted, and the unit-test gate also worked.

[ppannuto]

This looks good; I'd say it can be merged right after the Tock 1.5 release.

---

For the new traits, if I understand correctly, they remove code duplication in this library, at the expense of slightly more verbose code (extra imports) in the users of the library. I'm not sure that's the right tradeoff to make?

[gendx]

For the new traits, if I understand correctly, they remove code duplication in this library, at the expense of slightly more verbose code (extra imports) in the users of the library. I'm not sure that's the right tradeoff to make?

It's a bit unfortunate that traits have to be manually imported for their functions to be available in Rust. The idiomatic way for crates to solve this problem is to provide some kind of prelude namespace with the relevant traits, that dependents can import in a simple statement as use foo::prelude::*;.

However, Tock has a hard ban on wildcard imports (despite no such support in a Rust compiler flag). IMO the verbosity trade-off is more caused by this choice than by some specific traits. If we choose to reject any wildcard imports, then in exchange we accept more verbosity in imports.

[ppannuto]

Yeah, maybe this is the "I'm a luddite with no IDE speaking" [or as a person who reviews code in a browser, or via email, or any of the numerous places where tooling can't solve this problem], but the that's a terrible idiom :).

Wildcard imports generally break the idea of "I'm reading this code and there's this magic thing, where did it come from?" Not anywhere else in this file? Oh, let me go search the pile of import *'s. I feel pretty strongly that import * is an anti-feature in any language.

---

Style aside, I guess the question is whether we get any material benefit from the new traits for users of the library? I think that interface is much more important, and imposing small costs on the implementation of a library to make its usage easier / cleaner is generally a worthwhile tradeoff.

[gendx]

I don't have a strong opinion on the import * pattern, although I also err on the side of explicit imports rather than wildcards. The main drawback I see is in refactoring code (and cargo fix unfortunately doesn't support fixing imports rust-lang/cargo#6278). On the verbosity side, I don't think there's much drawback of more verbose imports, as the import section is a block separated from the rest of the code.

Regarding the benefits of an implementation for their users, I don't think they are limited to the API surface. A simpler implementation is also more robust to refactoring, easier to unit test, etc. So the benefits to the users of such a library is a better tested, more robust, more reliable code with fewer bugs. Especially in the long-term of the software engineering life-cycle (less likely that someone adding code in 6 months introduces or fixes a bug or performance regression/improvement for only one of the register types).

If the "cost" is more traits to imports, I think it's totally reasonable. And if that's too high of a cost we can also reconsider the wildcard imports and/or add some weight in the balance for rust-lang/cargo#6278 to be fixed.

[ppannuto]

Yeah, I guess if the "cost" is just trait imports, that's pretty minimal overhead. I'd be more reticent if it required changes at use sites, but I don't think that would be the case here.

[gendx]

Yeah, I guess if the "cost" is just trait imports, that's pretty minimal overhead. I'd be more reticent if it required changes at use sites, but I don't think that would be the case here.

The functions would be the same. Just moved to traits instead of being duplicated in each of the types.

[ppannuto]

Okay, I think status is this PR is pending 1.5 release, then ready to merge.

Then a subsequent PR for the trait updates makes sense.

[bradjc]

bors r+

[bors]

This PR was included in a batch that successfully built, but then failed to merge into master (it was a non-fast-forward update). It will be automatically retried.