Rename `matches_any()` to `any_matching_bits_set()`, implement new `matches_any()` #3336

hudson-ayers · 2022-11-28T17:03:13Z

Pull Request Overview

The current implementation of matches_any() does not implement the functionality the name implies. This PR renames the existing implementation to a name which better describes its functionality, and introduces a new matches_any() function (with a different interface) that actually correctly implements the functionality suggested by the name.

This PR also adds several tests to the tock-registers test suite to verify the new version works as expected, and removes a feature gate on a feature that no longer exists for the crate which was preventing some of the tock-registers test suite from being run as part of cargo test.

#3311 (comment) contains a complete description of the issues with the current method.

Fixes #3311

Testing Strategy

This pull request was tested by cargo test.

TODO or Help Wanted

This pull request still needs feedback on method names

Documentation Updated

README.md updated.

Formatting

Ran make prepush.

bradjc · 2022-11-28T18:31:08Z

Need to update readme.

lschuermann

Looks good except for the pending README update. Thanks @hudson-ayers!

hudson-ayers · 2022-11-28T20:09:13Z

README updated

bradjc

Fixing this is a little confusing.

If we look at what is probably a very old use of this API (in SAM4L USART), it makes it more clear what matches_any() was trying to do do:

        let ints_active = self.registers.imr.matches_any(
            Interrupt::RXBUFF::SET
                + Interrupt::TXEMPTY::SET
                + Interrupt::TIMEOUT::SET
                + Interrupt::PARE::SET
                + Interrupt::FRAME::SET
                + Interrupt::OVRE::SET
                + Interrupt::TXRDY::SET
                + Interrupt::RXRDY::SET,
        );

...

        if !(rx_active || tx_active || ints_active || is_panic) {
            pm::disable_clock(self.clock);
        }

Beside my point about matches_all() no longer being consistent, do we actually want two functions? It seems like what we want from an API point of view is the new matches_any(), and to update all current uses to the new version.

For example, this code in apollo3 stimer looks suspicious:

    fn is_running(&self) -> bool {
        let regs = self.registers;
        regs.stcfg.any_matching_bits_set(STCFG::CLKSEL::XTAL_DIV2)
    }

Particularly when given what start() does:

    fn start(&self) -> Result<(), ErrorCode> {
        // Set the clock source
        self.registers.stcfg.write(STCFG::CLKSEL::XTAL_DIV2);
        Ok(())
    }

Shouldn't the is_running() check be regs.stcfg.matches_all([STCFG::CLKSEL::XTAL_DIV2])?

I can see an efficiency argument for including any_matching_bits_set(), but I'm worried that if we can't restrict it to only single bit fields then it can be used in ways it shouldn't be and lead to very strange behavior.

bradjc · 2022-11-28T20:07:32Z

libraries/tock-register-interface/src/interfaces.rs

-    fn matches_any(&self, field: FieldValue<Self::T, Self::R>) -> bool {
-        field.matches_any(self.get())
+    fn any_matching_bits_set(&self, field: FieldValue<Self::T, Self::R>) -> bool {<
8000
/span>

            
+        field.any_matching_bits_set(self.get())
    }

    #[inline]
    /// Check if all specified parts of a field match
    fn matches_all(&self, field: FieldValue<Self::T, Self::R>) -> bool {
        field.matches_all(self.get())


It seems like matches_all() needs to change as well. This is now all_matching_bits_set(), right?

matches_all() is actually already correct -- since it was already checking for an exact match across the entire passed FieldValue, it works correctly for multi-bit fields and for matching on cleared bits as well as set bits.

For convenience here are the two implementations:

/// Check if any of the bits covered by the mask for this /// `FieldValue` are set in the passed value #[inline] pub fn any_matching_bits_set(&self, val: T) -> bool { val & self.mask != T::zero() } /// Check if all specified parts of a field match #[inline] pub fn matches_all(&self, val: T) -> bool { val & self.mask == self.value }

matches_all() actually looks at self.value() and makes sure that the passed value, when masked, matches it. So the problem in #3311 does not apply.

Ok good point. Seems like if 8000 there is only one FieldValue, then it should be .matches().

Yeah, that makes sense to me, but I don't think the current name is really wrong ... not sure how to balance that with a desire not to modify the API of tock-registers more than needed, especially since it is the one library with non-Tock downstream users

hudson-ayers · 2022-11-28T20:41:36Z

Beside my point about matches_all() no longer being consistent, do we actually want two functions? It seems like what we want from an API point of view is the new matches_any(), and to update all current uses to the new version.

For example, this code in apollo3 stimer looks suspicious:
    fn is_running(&self) -> bool {
        let regs = self.registers;
        regs.stcfg.any_matching_bits_set(STCFG::CLKSEL::XTAL_DIV2)
    }

Yeah, good call, this is incorrect, because CLKSEL is a multi-bit field.

Shouldn't the is_running() check be regs.stcfg.matches_all([STCFG::CLKSEL::XTAL_DIV2])?

For the interfaces currently defined in this PR, the correct change would be either regs.stcfg.matches_all(STCFG::CLKSEL::XTAL_DIV2) or regs.stcfg.matches_any([STCFG::CLKSEL::XTAL_DIV2])

I can see an efficiency argument for including any_matching_bits_set(), but I'm worried that if we can't restrict it to only single bit fields then it can be used in ways it shouldn't be and lead to very strange behavior.

Yeah, the reason I included it is for efficiency -- when just matching on a bunch of single bit fields, it is much more efficient than the new matches_any(), which has to loop through each passed field. I don't think there is an easy way to enforce that it can only be called with a FieldValue composed of multiple single-bit FieldValues added together though.

hudson-ayers · 2022-11-29T18:04:54Z

Pushed an update to fix that stimer example (which was broken before this PR, to be clear)

hudson-ayers · 2022-11-29T18:30:51Z

I would be onboard with removing any_matching_bits_set() and just forcing users to use the less-efficient, more correct matches_any() if people prefer that. If someone really needs the efficient implementation it is easy enough to handwrite

    // current approach
    // let any_bits_set_efficient = self.regs.srp.any_matching_bits_set(TX::INT::SET + RX::INT::SET);
  
    // new approach if we remove `any_matching_bits_set()`
    let any_bits_set_efficient = self.regs.srp.get() & (TX::INT::SET + RX::INT::SET).mask() != 0;

hudson-ayers · 2022-12-15T23:54:06Z

Any thoughts on these two approaches? I personally am in favor of what is currently in the PR, but don't feel too strongly

bradjc · 2022-12-16T18:18:56Z

What I still feel like is swapping one quirk for another is that reg.any_matching_bit_set(INTERRUPT::BIT1::CLR + INTERRUPT::BIT2::CLR) works the same as reg.any_matching_bit_set(INTERRUPT::BIT1::SET + INTERRUPT::BIT2::SET). This API really shouldn't use FieldValues it should use Fields, but we can't do concatenation on Fields (INTERRUPT::BIT1 + INTERRUPT::BIT2 doesn't compile) so if we want this efficient functionality we need to use FieldValues.

Can we & in the FieldValue value? That way it 1) forces the more intuitive use case (::SET rather than ::CLR) and 2) makes the adjective "set" apply to both the FieldValue and the underlying register value, rather than just one (leaving the caller to understand which applies).

#[inline]
    pub fn any_matching_bits_set(&self, val: T) -> bool {
        val & self.mask & self.value != T::zero()
    }

We could also call it any_set()

           if interrupts.any_set(
                Status::BUSERR::SET
                    + Status::SMBPECERR::SET
                    + Status::SMBTOUT::SET
                    + Status::ORUN::SET
                    + Status::URUN::SET,
            ) {...}

hudson-ayers · 2022-12-16T23:56:14Z

What I still feel like is swapping one quirk for another is that reg.any_matching_bit_set(INTERRUPT::BIT1::CLR + INTERRUPT::BIT2::CLR) works the same as reg.any_matching_bit_set(INTERRUPT::BIT1::SET + INTERRUPT::BIT2::SET).

Can we & in the FieldValue value? That way it 1) forces the more intuitive use case (::SET rather than ::CLR) and 2) makes the adjective "set" apply to both the FieldValue and the underlying register value, rather than just one (leaving the caller to understand which applies).
#[inline]
    pub fn any_matching_bits_set(&self, val: T) -> bool {
        val & self.mask & self.value != T::zero()
    }
We could also call it any_set()
           if interrupts.any_set(
                Status::BUSERR::SET
                    + Status::SMBPECERR::SET
                    + Status::SMBTOUT::SET
                    + Status::ORUN::SET
                    + Status::URUN::SET,
            ) {...}

Yeah, I think that seems like a reasonable approach. We will have to re-write the unit tests for the function, as this change breaks the current unit tests, but I find the current unit tests somewhat incomprehensible anyway

lschuermann · 2022-12-23T21:09:20Z

This API really shouldn't use FieldValues it should use Fields, but we can't do concatenation on Fields (INTERRUPT::BIT1 + INTERRUPT::BIT2 doesn't compile) so if we want this efficient functionality we need to use FieldValues.

@bradjc I mean, presumably we could override the bitwise-OR operator for Fields such that we could do something like the following?

interrupts.any_set(
    Status::BUSERR
    | Status::SMBPECERR
    | Status::SMBTOUT
    | Status::ORUN
    | Status::URUN
)

Although this may also make it more confusing, as we don't know specifically which values we're using (presumably this would be T::max() & self.mask).

We could also call it any_set()

Not a fan of that as long as we're accepting any T, given that nothing's stopping a user from composing arbitrary things as the function argument, or just using raw integer types. Then "any" looses all meaning.

hudson-ayers · 2023-01-02T20:55:51Z

This API really shouldn't use FieldValues it should use Fields, but we can't do concatenation on Fields (INTERRUPT::BIT1 + INTERRUPT::BIT2 doesn't compile) so if we want this efficient functionality we need to use FieldValues.

@bradjc I mean, presumably we could override the bitwise-OR operator for Fields such that we could do something like the following?
interrupts.any_set(
    Status::BUSERR
    | Status::SMBPECERR
    | Status::SMBTOUT
    | Status::ORUN
    | Status::URUN
)
Although this may also make it more confusing, as we don't know specifically which values we're using (presumably this would be T::max() & self.mask).

Yeah, I think that overriding the bitwise OR operator is probably going to make things more confusing, I would vote against that.

We could also call it any_set()

Not a fan of that as long as we're accepting any T, given that nothing's stopping a user from composing arbitrary things as the function argument, or just using raw integer types. Then "any" looses all meaning.

In practice, I think it was a mistake to make most of the functions in fields.rs pub, instead of pub(crate). It really only makes sense for these functions to be called by the outer functions on the Readable/Writable traits. Those functions do not accept arbitrary T anyway, which would resolve that concern. Would you like any_set() if the implementation in fields.rs was pub(crate)?

lschuermann · 2023-01-04T11:45:01Z

In practice, I think it was a mistake to make most of the functions in fields.rs pub, instead of pub(crate). It really only makes sense for these functions to be called by the outer functions on the Readable/Writable traits. Those functions do not accept arbitrary T anyway, which would resolve that concern.

Yes, I agree with that. This function seem destined for misunderstanding their implementation and do not constrain the input types well to limit it to a reasonable subset of possible behaviors.

Would you like any_set() if the implementation in fields.rs was pub(crate)?

I like &ing self.value() in its implementation as proposed by @bradjc, but I'm not sold on the any_set name. I think something more explicit like any_matching_bits_set is much more expressive.

As a side note, something that made me skittish when seeing the example above was the use of the + operator. Even though I'd consider myself very familiar with the library, my intuition was that presumably something like Status::BUSERR::SET must return a T (instead of our FieldValue), and then + would only be equal to a bitwise-or if these were distinct, non-overlapping fields. Considering the usual add-operator semantics, it seems that overloading Add instead of BitOr on FieldValue was a mistake.

The current implementation of matches_any() does not implement the functionality the name implies. This commit renames the existing implementation to a name which better describes its functionality, and introduces a new matches_any() function (with a different interface) that actually correctly implements the functionality suggested by the name. This commit also adds several tests to the tock-registers test suite to verify the new version works as expected, and removes a feature gate on a feature that no longer exists for the crate which was preventing some of the tock-registers teste suite from being run as part of `cargo test`.

hudson-ayers · 2023-01-18T00:15:17Z

I rebased this PR, and made the following updates:
2. FieldValue::any_matching_bits_set() now &'s in self.value, forcing the more intuitive use case (i.e. my_interrupts.any_matching_bits_set(MY_FIELD::CLEAR) will never return true).
3. Updated the tests to reflect this functionality change

I did not make the following changes:

I did not change the name to any_set() based on Leon's pushback.
I did not make FieldValue::any_matching_bits_set() pub(crate), since it turns out that litex_registers.rs has to be able to call it. This bums me out a bit, but I didn't see an easy fix for this that did not involve moving much of the logic from litex_registers into tock_registers. Still, it doesn't really feel right that we are basing the interface to tock_registers off the peculiarities of litex_registers, especially in this case where the function being public makes it much more likely to be used incorrectly in place of the correct method on the Field struct

hudson-ayers · 2023-02-07T19:47:48Z

ping? @lschuermann @bradjc

bradjc

I've stood in the way long enough!

bradjc · 2023-03-08T22:28:37Z

bors r+

bors · 2023-03-08T22:56:26Z

Build succeeded:

github-actions bot added nrf Change pertains to the nRF5x family of MCUs. sam4l Change pertains to the SAM4L MCU. tock-libraries This affects libraries supported by the Tock project labels Nov 28, 2022

lschuermann previously approved these changes Nov 28, 2022

View reviewed changes

hudson-ayers dismissed lschuermann’s stale review via e761468 November 28, 2022 20:02

hudson-ayers force-pushed the fix_matches_any branch from 3939b1f to e761468 Compare November 28, 2022 20:02

bradjc reviewed Nov 28, 2022

View reviewed changes

hudson-ayers force-pushed the fix_matches_any branch from e761468 to 00c1595 Compare November 29, 2022 18:04

hudson-ayers force-pushed the fix_matches_any branch from 00c1595 to 52d20eb Compare January 18, 2023 00:06

hudson-ayers force-pushed the fix_matches_any branch from 52d20eb to 480ee65 Compare January 18, 2023 00:13

alistair23 approved these changes Feb 9, 2023

View reviewed changes

bradjc approved these changes Mar 8, 2023

View reviewed changes

bors bot merged commit 2ab8466 into tock:master Mar 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Rename `matches_any()` to `any_matching_bits_set()`, implement new `matches_any()` #3336

Rename `matches_any()` to `any_matching_bits_set()`, implement new `matches_any()` #3336

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Rename matches_any() to any_matching_bits_set(), implement new matches_any() #3336

Rename matches_any() to any_matching_bits_set(), implement new matches_any() #3336

Uh oh!

Conversation

Uh oh!

Pull Request Overview

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Rename `matches_any()` to `any_matching_bits_set()`, implement new `matches_any()` #3336

Rename `matches_any()` to `any_matching_bits_set()`, implement new `matches_any()` #3336