Skip to content

Add missing type hints in fields codegen#869

Merged
Urhengulas merged 2 commits intoknurling-rs:mainfrom
apohrebniak:derive-type-hints
Oct 16, 2024
Merged

Add missing type hints in fields codegen#869
Urhengulas merged 2 commits intoknurling-rs:mainfrom
apohrebniak:derive-type-hints

Conversation

@apohrebniak
Copy link
Copy Markdown
Contributor

I noticed that having

#[derive(defmt::Format)]
struct Foo {
  u16: u16,
  u32: u32,
  u64: u64,
  u128: u128,
}

cargo expand generates:

impl defmt::Format for Foo
where
    u64: defmt::Format,
    u128: defmt::Format,
{
    fn _format_tag() -> defmt::Str {
        {
            defmt::export::make_istr({
                #[link_section = ".defmt.{\"package\":\"examples\",\"tag\":\"defmt_derived\",\"data\":\"Foo {{ u16: {=u16:?}, u32: {=u32:?}, u64: {=?:?}, u128: {=?:?} }}\",\"disambiguator\":\"6889995364419830437\",\"crate_name\":\"stm32f411x_scsi_bbb\"}"]
                #[export_name = "{\"package\":\"examples\",\"tag\":\"defmt_derived\",\"data\":\"Foo {{ u16: {=u16:?}, u32: {=u32:?}, u64: {=?:?}, u128: {=?:?} }}\",\"disambiguator\":\"6889995364419830437\",\"crate_name\":\"stm32f411x_scsi_bbb\"}"]
                static S: u8 = 0;
                &S as *const u8 as u16
            })
        }
    }
    fn _format_data(&self) {
        match self {
            Self {
                u16,
                u32,
                u64,
                u128,
            } => {
                defmt::export::u16(u16);
                defmt::export::u32(u32);
                defmt::export::fmt(u64);
                defmt::export::fmt(u128);
            }
        }
    }
}

the important bit here is that there're missing type hints for u64 and u128. Therefore

    let foo = Foo {
        u16: 0xCAFE,
        u32: 0xCAFE,
        u64: 0xCAFE,
        u128: 0xCAFE,
    };
    defmt::info!("{:#X}", foo);

outputs INFO Foo { u16: 0xCAFE, u32: 0xCAFE, u64: 51966, u128: 51966 }.

This PR adds those missing hints, so the output is correct:
INFO Foo { u16: 0xCAFE, u32: 0xCAFE, u64: 0xCAFE, u128: 0xCAFE }

@apohrebniak apohrebniak marked this pull request as ready for review September 29, 2024 16:16
Copy link
Copy Markdown
Member

@Urhengulas Urhengulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@jonathanpallant
Copy link
Copy Markdown
Contributor

Is this a breaking change? We're altering how a struct is encoded over the wire using our default formatter, and changing how the values get formatted by default on the host side. Although arguably, we're just fixing a mistake and it should always have been like this.

@Urhengulas
Copy link
Copy Markdown
Member

Is this a breaking change? We're altering how a struct is encoded over the wire using our default formatter, and changing how the values get formatted by default on the host side. Although arguably, we're just fixing a mistake and it should always have been like this.

I don't think that this is breaking. We do not change the wire format. And all derive(defmt::Format) that worked before will still work.

@Urhengulas Urhengulas added the breaking change fix / feature / improvement involves a breaking change and needs to wait until next minor version label Oct 16, 2024
@Urhengulas Urhengulas enabled auto-merge October 16, 2024 02:34
@Urhengulas Urhengulas added this pull request to the merge queue Oct 16, 2024
Merged via the queue into knurling-rs:main with commit afebfc5 Oct 16, 2024
@apohrebniak apohrebniak deleted the derive-type-hints branch October 16, 2024 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change fix / feature / improvement involves a breaking change and needs to wait until next minor version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants