Skip to content

fix: panic caused by defmt forwarding#46

Open
optlink wants to merge 1 commit intorust-embedded:masterfrom
optlink:master
Open

fix: panic caused by defmt forwarding#46
optlink wants to merge 1 commit intorust-embedded:masterfrom
optlink:master

Conversation

@optlink
Copy link

@optlink optlink commented May 20, 2025

This is a particularly nasty issue that I spent several days tracking down. See knurling-rs/defmt#723 for another example.

@optlink optlink requested a review from a team as a code owner May 20, 2025 15:46
@BartMassey
Copy link
Member

We talked about this at the REWG meeting today. First, sincere apologies for being so slow to act on this. It really slipped through the cracks.

Is this panic still reproducible or has it been fixed in defmt since? The patch certainly looks fine and we'll take it unless it's now redundant.

Thanks for looking into this!

@optlink
Copy link
Author

optlink commented Feb 25, 2026

I will be revisiting the project where I am using nb in March so I'll have a chance to reproduce it then. Though based on the comments and documentation in the linked issue it seems like this is intended behavior for defmt and so the patch is still required.

@jannic
Copy link
Member

jannic commented Feb 27, 2026

This bug is still reproducible. I struggled to trigger it at first, because this works just fine:

    struct My {}

    impl defmt::Format for My {
        fn format(&self, f: defmt::Formatter<'_>) {
            defmt::write!(f, "some logging");
        }
    }

    let s = My {};
    let e = nb::Error::Other(s);
    defmt::info!("Error: {}", e);

However, if I don't implement defmt::Format manually, but use the derive macro, I can reproduce the panic:

    #[derive(defmt::Format)]
    struct My {}

    let s = My {};
    let e = nb::Error::Other(s);
    defmt::info!("Error: {}", e);

This panics in RttEncoder::acquire because the defmt encoder is acquired recursively.

I can also confirm that this PR fixes the issue, at least for the case mentioned above.

To me, this looks like an issue that should be solved in defmt, as it is easy to accidentally use the defmt API in a way triggering this panic. However, I have no idea how to fix that in defmt or if it is even possible without a breaking change.

So even if this PR is only a workaround, it is one that doesn't hurt at all, so we should apply it.

Copy link
Member

@jannic jannic left a comment

Choose a reason for hiding this comment

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

LGTM. The issue reported by the CI pipeline is already fixed on the master branch so the PR only needs to be rebased.

@optlink
Copy link
Author

optlink commented Feb 27, 2026

Rebased

Copy link
Member

@BartMassey BartMassey left a comment

Choose a reason for hiding this comment

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

Thanks again for this.

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.

3 participants