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

Register Write Limitation #859

Closed
AdinAck opened this issue Aug 26, 2024 · 12 comments
Closed

Register Write Limitation #859

AdinAck opened this issue Aug 26, 2024 · 12 comments

Comments

@AdinAck
Copy link
Contributor

AdinAck commented Aug 26, 2024

Moved from stm32-rs-nightlies

I ran into an issue when writing to a register where I wanted to return a value from the write, like this:

let val = reg.write(|w| {
    T::set(w.field())
});

But, the signature of write forbids this:

pub fn write<F, T>(&self, f: F)
    where
        F: FnOnce(&mut REG::Writer) -> &mut W<REG>,

So I'm curious, why not change the signature to:

pub fn write<F, T>(&self, f: F) -> T
    where
        F: FnOnce(&mut REG::Writer) -> T

By implementing it like this:

pub fn write<F, T>(&self, f: F) -> T
where
    F: FnOnce(&mut W<REG>) -> T,
{
    let mut writer = W {
        bits: REG::RESET_VALUE & !REG::ONE_TO_MODIFY_FIELDS_BITMAP
            | REG::ZERO_TO_MODIFY_FIELDS_BITMAP,
        _reg: marker::PhantomData,
    };
    let result = f(&mut writer);

    self.register.set(writer.bits);

    result
}

Is there a drawback to this?

@burrbull
Copy link
Member

#738 (comment)

@AdinAck
Copy link
Contributor Author

AdinAck commented Aug 26, 2024

I don't believe my proposal is related to that. I don't want to return the writer, in fact, the proposed body of write forbids T ever being W or &mut W since bits is moved out of the writer for register.set, no?

@AdinAck
Copy link
Contributor Author

AdinAck commented Aug 26, 2024

Ok yes I believe I set up an analogous setup in rust playground:

#[derive(Debug)]
struct Bits(u32);

#[derive(Debug)]
struct W {
    bits: Bits,
    _reg: (),
}

fn consume_bits(Bits(num): Bits) {
    println!("bits: {}", num);
}

fn write<F, T>(f: F) -> T
where
    F: FnOnce(&mut W) -> T,
{
    let mut writer = W { bits: Bits(0), _reg: () };
    
    let result = f(&mut writer);
    
    consume_bits(writer.bits);
    
    result
}

fn main() {
    let outside = write(|_w| 0xaa);
    // let outside = write(|w| w); <- does not compile
    println!("outisde: {:?}", outside);
}

So I don't believe this introduces any complications as mentioned in #738.

@burrbull
Copy link
Member

Sorry for waiting. Looks interesting. I think you could nominate it for next meeting: rust-embedded/wg#800

@AdinAck
Copy link
Contributor Author

AdinAck commented Oct 20, 2024

No problem, you have a lot oh your hands. I've never done that before so I don't know the proper procedure.

@burrbull
Copy link
Member

burrbull commented Oct 22, 2024

Do your suggestion require changing of signatures of write/modify only or field writers too?

In other words can we use traditional field modifying system simultaneously with yours?

@AdinAck
Copy link
Contributor Author

AdinAck commented Oct 22, 2024

Just write and modify, all the field writers work as is!

@AdinAck
Copy link
Contributor Author

AdinAck commented Oct 22, 2024

I'm going to turn this issue into a PR so exactly what I'm proposing is visible.

@burrbull
Copy link
Member

burrbull commented Oct 22, 2024

I understand what you want, but I not sure what is better:

  • replace existent write and modify or
  • add additional write and modify with new signature and other names

I prefer second as less breaking.

@AdinAck
Copy link
Contributor Author

AdinAck commented Oct 22, 2024

Ah I hadn't even thought of that, two good options!

I agree, it took me like 3 hours to convert all write and modify call sites in the stm32g4xx-hal, wouldn't want others to go through that.

I'll still make a PR but as an addition rather than a change.

@AdinAck
Copy link
Contributor Author

AdinAck commented Oct 24, 2024

Resolved by #874.

@AdinAck AdinAck closed this as completed Oct 24, 2024
@burrbull
Copy link
Member

burrbull commented Nov 7, 2024

@AdinAck I've released current state of stm32-rs G4 as stm32g4-staging

You could use it as:

[dependencies.stm32g4]
package = "stm32g4-staging"
version = "0.17.0"

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

No branches or pull requests

2 participants