Skip to content

[Embassy-rp] more defmt derives#6373

Open
vic1707 wants to merge 2 commits into
embassy-rs:mainfrom
vic1707:embassy-rp-more-defmt
Open

[Embassy-rp] more defmt derives#6373
vic1707 wants to merge 2 commits into
embassy-rs:mainfrom
vic1707:embassy-rp-more-defmt

Conversation

@vic1707

@vic1707 vic1707 commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Hi, quick PR with a few more #[cfg_attr(feature = "defmt", derive(defmt::Format))] in embassy-rp

Disclaimer: I originally only targeted the uart config structs/enums but had a few tokens laying around so asked an llm (gpt5.5) to try and find other cases where such derive could be useful. Hope that's ok

Comment thread embassy-rp/src/spi.rs
Comment on lines +43 to +64
#[cfg(feature = "defmt")]
impl defmt::Format for Config {
fn format(&self, f: defmt::Formatter) {
let phase = match self.phase {
Phase::CaptureOnFirstTransition => "CaptureOnFirstTransition",
Phase::CaptureOnSecondTransition => "CaptureOnSecondTransition",
};
let polarity = match self.polarity {
Polarity::IdleLow => "IdleLow",
Polarity::IdleHigh => "IdleHigh",
};

defmt::write!(
f,
"Config {{ frequency: {=u32}, phase: {}, polarity: {} }}",
self.frequency,
phase,
polarity,
);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not derive this?

And also, even if implemented manually, it should use istr instead of native strings

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

because Phase & Polarity don't implement Format in embedded_hal_02, we can't use Debug2Format either as both enums don't have a Debug impl.
They only have #[derive(Clone, Copy, PartialEq, Eq)]

Good catch for istr thx 👍

@vic1707

vic1707 commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

@diondokter
build's failing because fixed's defmt feature only appeared in 1.30.0 (currently using 1.28.0).
Is the project open to a bump of that dependency (in a separate PR)?

@diondokter

Copy link
Copy Markdown
Contributor

I'm not really a maintainer of the RP code... So it's not up to me in the end.

The current rust-toolchain is set to 1.92 and the latest fixed release has an msrv of 1.93, so we'd have to bump. I don't recall that being a problem before.

So I'd create a PR that does the bump to the latest version of fixed together with setting the rust-toolchain file to 1.93. Then if Dario likes it, he can just merge it. If he doesn't like it, you'll discover it then. The backup would be to update fixed to the second-last version 1.30.0 that doesn't require the toolchain bump.

@diondokter

Copy link
Copy Markdown
Contributor

Oh and set the nightly version (separate file) to Jan. 22, 2026 which is the release date of 1.93

@CBJamo

CBJamo commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

I'm not really a maintainer of the RP code... So it's not up to me in the end.

Does the RP crate have a dedicated maintainer?

fixed is (currently) only used in the pio code, and looking at fixed RELEASES.md we shouldn't have any problem updating it.

@diondokter diondokter added the e-rp Issues for the RP family of chips label Jun 17, 2026
@diondokter

Copy link
Copy Markdown
Contributor

Does the RP crate have a dedicated maintainer?

I don't think so, so that means the Dario and Ulf are

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

e-rp Issues for the RP family of chips

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants