Skip to content
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

return raw value from write/modify #873

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

burrbull
Copy link
Member

The idea from @AdinAck . See #859

This looks not so beautiful like #738, but almost not breaking and allows to transfer data between writes with unsafe.

    let bits = rcc.apb1rstr().modify(|_, w| w.usart2rst().clear_bit());
    rcc.apb1rstr().write(|w| unsafe { // save on 1 read operation
        w.bits(bits).usart2rst().set_bit()
    });

@AdinAck
Copy link

AdinAck commented Oct 20, 2024

This is certainly similar to my idea but not quite the same.

This does not permit arbitrary return types.

I have been using the ability to return arbitrary return types to enforce type-state validity for HALs like so:

let state = reg.modify(|_, w| {
    State::set(w.some_field())
});

Full example

@burrbull
Copy link
Member Author

Ok. After you've wrote I understand even less.
What is State::set? Constructor of State?
What it takes? Value of register or value of field?
What does some_field() do? Does it change a field somehow?

I cannot to chain field more?

@AdinAck
Copy link

AdinAck commented Oct 20, 2024

What is State::set? Constructor of State?

Yes, a type-state corresponds to the value of a bitfield.

What it takes? Value of register or value of field?

set takes a mutable reference to the exact bitfield of a register it represents the state of.

What does some_field() do? Does it change a field somehow?

some_field was a placeholder name for some imaginary bitfield in some register.

Imagine a hypothetical peripheral which can only be enabled or disabled:

struct ImaginaryPeripheral<State: PowerState> {
    rb: IMGPERIPH,
    state: State,
}

If I wanted a ImaginaryPeripheral<Enabled> I would need to convert ImaginaryPeripheral<Disabled> into that. (Just like conversion of pin modes: PB3<Analog> into PB3<Input<PullUp>>)

So I could write a function for conversion:

impl<State: PowerState> ImaginaryPeripheral<State> {
    fn freeze<NewState: PowerState>(self) -> ImaginaryPeripheral<NewState> {
        let new_state = self.rb.reg().modify(|_, w| { // peripheral has a single register called "reg"
            NewState::set(w.power()) // power type-state only needs the power bitfield of this register
        });

        ImaginaryPeripheral {
            rb: self.rb,
            state: new_state,
        }
    }
}

The peripheral type holds instances of type-states to prove that the type-state was in fact set.

The definition of set for Disabled would say, set the bit to 0, and the definition of set for Enabled would set the bit to 1.

All of this is just an example of my reason for wanting to return arbitrary values from the register write/modify closure. It is conceivable other use-cases exist, but this is mine.

@AdinAck
Copy link

AdinAck commented Oct 20, 2024

Looking at your example, I think I see what you're going for, and that's definitely a different problem than the one I am trying to solve.

@burrbull
Copy link
Member Author

Traditional solution for your example.

Stable Rust:

struct ImaginaryPeripheral<State: PowerState> {
    rb: IMGPERIPH,
    state: State, // or even PhantomData<State>
}
trait PowerState {
   const STATE: StateEnum; // generated by svd2rust
}
struct Enabled;
impl PowerState for Enabled {
   const STATE: StateEnum = StateEnum::Enabled;
}

self.rb.reg().modify(|_, w| w.power().variant(NewState::STATE));
ImaginaryPeripheral {
    rb: self.rb,
    state: NewState,
}

Nightly Rust:

#![feature(adt_const_params)]

struct ImaginaryPeripheral<const State: StateEnum> {
    rb: IMGPERIPH,
}

self.rb.reg().modify(|_, w| w.power().variant(State));
ImaginaryPeripheral::<NewState> {
    rb: self.rb,
}

@burrbull
Copy link
Member Author

Anyway I suggest you to discuss this in Tuesday meeting.

@AdinAck
Copy link

AdinAck commented Oct 20, 2024

Yeah that is how HALs primarily look right now, and in fact the version of the example I linked earlier that made it into stm32g4xx-hal looks like this.

The problem I have with it is that the line:

self.rb.reg().modify(|_, w| w.power().variant(NewState::STATE));

could be commented out and the program would still compile no problem.

More complex peripherals have unintuitive type-state relations where errors can slip through easily. I'm trying to eliminate these kinds of errors.

I like knowing that any and all undesired behavior is limited to set implementations, which are trivial to implement.

@AdinAck
Copy link

AdinAck commented Oct 20, 2024

Anyway I suggest you to discuss this in Tuesday meeting.

Ok I still don't know what the procedure is for this but I will try. Thanks!

@burrbull
Copy link
Member Author

could be commented out and the program would still compile no problem.
More complex peripherals have unintuitive type-state relations where errors can slip through easily. I'm trying to eliminate these kinds of errors.

Ok. It's clearer now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants