Introduce the PanicWriter trait, a standard mechanism for implementing synchronous writers for panic messages

[bradjc]

Pull Request Overview

For a long time, Tock's kernel-provided panic utilities required some type that implements IoWrite to display panic messages. How a board created such a type was up to each board, and the io.rs files we have in boards have used a variety of methods to do this. This has had the effect of creating three issues:

1. These types (that implement IoWrite) are often instantiated as static muts, which we are trying to get rid of.
2. These types often make assumptions about the state of the underlying hardware when the panic happens, and assume the kernel initialized the hardware correctly and that the object used by the normal kernel operation can still be used during a panic.
3. Implementations of the types are duplicated across many boards, sometimes identically and sometimes with minor changes.

To try to fix these issues, this PR (from the static mut task force) proposes a new trait PanicWriter that chips would implement to provide a synchronous writer suitable for panic messages. Boards then only need to select the implementation of PanicWriter they want to use for panics, and the chip-provided implementation is used by debug.rs to correctly display the panic message.

The trait looks like this:

pub trait PanicWriter {
    /// The configuration data the mechanism needs to configure the writer for panic output.
    type Config;

    /// Create a new synchronous writer capable of sending panic messages.
    unsafe fn create_panic_writer(config: Self::Config) -> impl IoWrite;
}

The config type permits the board to provide whatever settings are necessary for the implementation to correctly configure the hardware for panic output.

This addresses the three concerns by:

1. The create_panic_writer() function must instantiate the object on the stack, so it is not declared as a static mut.
2. The chip-provided implementations must create a new hardware object instantiation, eliminating sharing between the main kernel and the panic handler. Also, the config provides a method for communicating exactly the settings needed by the writer.
3. The implementations are provided by chips and not each board, eliminating redundancy.

Testing Strategy

todo

TODO or Help Wanted

Thoughts?

Need to port every chip.

Documentation Updated

• Updated the relevant files in /docs, or no updates are required.

Formatting

• Ran make prepush.

[lschuermann]

In general I agree with this approach. Given the implementation of PanicWriter will be chip-specific, and should carefully reason about whether an implementation of PanicWriter is actually safe for each UART driver, I think it might make sense to split these up over multiple PRs.

[lschuermann]

We can't really enforce that the return value will be stored on the stack, and I don't think it actually matters.

What does matter are the other guarantees we ought to require callers of this unsafe function to uphold:

• the function must only be called once: that ensures that we will never have two instances of the resulting impl IoWrite around that may be trampling over each other, and
• this function must only be called in the panic handler. In particular, after this function is called, no other code will access the hardware backing this writer. In a sense, this function will forcefully "take over" the underlying hardware, whichever state it will be in. A full system reset is required before any other code is allowed to access the underlying hardware again.

These promises by the caller are, I think, required to build reliable and sound panic writer implementations.

[bradjc]

The other two guarantees make sense and I will add.

Re: where the writer is stored, aren't the three options statically, on the heap, or on the stack? We don't have a heap. Is there a way for the implementation to declare it statically that we would find acceptable?

[alevy]

@bradjc I think you're right, but I'm not even sure what the reader is supposed to do with this:

   /// The constructed writer must be created on the stack. Because panic
   /// will never return this is effectively a static allocation.

I don't necessarily even think it's the case that it must have be static. If this drops, isn't it probably fine?

[bradjc]

I think it's to help people like me, who are used to tock drivers being statically allocated, and being confused about how to to do that within this interface. Also, I'm worried about implementors wanting to use static_init in the implementation.

[lschuermann]

I think what bugs me about this comment specifically is the word "must", which implies that I have to do something special to make this work (which is not the case). When rephrasing this into more of an explanation, as opposed to a thing that users of this function "must" do, it is less confusing. Something like:

Because the panic handler will never return, and the returned value is only used for synchronously writing a panic message once, it is fine for the return value of this function to be stored on the panic handler's stack. No static_init! or similar is necessary.

[lschuermann]

Is this safe regardless of which state the UART is in? Do we need to cancel any (potentially) in-progress DMA operations?

[alevy]

The first thing initialize does (via initialize_inner) is stop outstanding operations.

[alexandruradovici]

Is there any reason why we restrict implementations to impl core::fmt::Write types?

[bradjc]

Ok based on last week's meeting I've:

1. Removed the apollo3 changes we can't easily test.
2. Added back the old panic routines as panic_old().
3. Fixed import errors so CI passes.

[bradjc]

I updated the nrf52840dk, including adding an implementation for segger rtt. Please take a look and see if you think it is reasonable. It doesn't try to create a brand new SeggerRttMemory, but instead uses what is already setup.

[brghena]

Alright, I think this is an improvement on the current state of the world and we should merge it.

[bradjc]

I just added support for the esp32 and tested it.

Thoughts on moving forward with this?