RFC: Simplify user-experience of trait Logger

[Urhengulas]

In the current state, a user who implements our trait Logger needs to care about "How to correctly use the encoder?".

static ENCODER: defmt::Encoder = Encoder::new();

impl defmt::Logger for Logger {
    fn acquire() {
        // acquire lock...

        unsafe { ENCODER.start_frame(do_write) }
    }
    unsafe fn flush() { /* ... */ }
    unsafe fn release() {
        ENCODER.end_frame(do_write);

        // release lock...
    }
    unsafe fn write(bytes: &[u8]) {
        ENCODER.write(bytes, do_write);
    }
}

fn do_write(bytes: &[u8]) {
    // write to the host...
}

This is sub-optimal, since the user should only need to care about their transport-specific question and not also how the encoding specific questions, mainly "When to start and end a frame?".
While it is relatively trivial to infer this from the existing implementations and is "just" a bit of boilerplate code, I'd consider it better to not have the need for this boilerplate at all.

To simplify this, the proposal is to add "wrapper methods" with default implementation for .acquire, .release and .write, as well as a method .ENCODER to give access to the encoder:

pub unsafe trait Logger {
    fn acquire();
    fn outer_acquire() {
        Self::acquire();
        unsafe { Self::ENCODER().start_frame(Self::write) }
    }

    unsafe fn flush();

    unsafe fn release();
    unsafe fn outer_release() {
        Self::ENCODER().end_frame(Self::write);
        Self::release();
    }

    fn write(bytes: &[u8]);
    unsafe fn outer_write(bytes: &[u8]) {
        Self::ENCODER().write(bytes, Self::write);
    }

    fn ENCODER() -> &'static mut defmt::Encoder;
}

Note that a user could still implement .outer_* themselves, if they'd like to handle framing differently.

The adapted implementation would look like:

static ENCODER: defmt::Encoder = Encoder::new();

impl defmt::Logger for Logger {
    fn acquire() {
        // acquire lock...
    }
    unsafe fn flush() { /* ... */ }
    unsafe fn release() {
        // release lock...
    }
    fn write(bytes: &[u8]) {
        // write to the host...
    }
}

Pro / Con

• Advantage
  • Easier to implement for the user
• Disadvantage
  • It makes the trait a bit more ugly and complicated.

(cc @Dirbaio: What do you think, since the current design is from you)

[Urhengulas]

Another though I just had, since we currently hand fn do_write to every method call of struct Encoder: Couldn't we not just hand it to the constructor of the encoder and remove it from the call-sites? This could obviously be mixed with the proposal above.

static ENCODER: defmt::Encoder = Encoder::new(|bytes: &[u8]| {
    // write to the host...
});

impl defmt::Logger for Logger {
    fn acquire() {
        // acquire lock...

        unsafe { ENCODER.start_frame() }
    }
    unsafe fn flush() { /* ... */ }
    unsafe fn release() {
        ENCODER.end_frame();

        // release lock...
    }
    unsafe fn write(bytes: &[u8]) {
        ENCODER.write(bytes);
    }
}

[japaric]

let me preamble this with: any change to the Logger trait is v0.4.0 material. I don't think we should do any change to it during the v0.3.x series.

I think this proposal is a step in the right direction: the implementer shouldn't need to care about the Encoder.
I wouldn't add more methods but rather tweak the contract (this is a breaking change, hence v0.4.0 material) of the existing ones so that write already deals with encoded bytes.
This way the implementer of Logger does not need to call the Encoder methods -- in fact, those methods should be made private (another breaking change).
The catch is that the Encoder still needs to be allocated somewhere.
The framework cannot do that because the transport may actually need several encoders -- this would be the case of a lock-free design that uses multiple in-memory ring buffers ("RTT" buffers), one buffer per priority level.

To accommodate that I think the trait would end like this:

(As I have stated before I'm not a fan of the current Self-less design of the trait but have been told Self is bad for binary size so I'll take that as a design constraint and see what I can make out of it)

pub unsafe trait Logger {
    // CONTRACT CHANGED: does not need to call the encoder
    fn acquire();
    unsafe fn release();

    // contract unchanged
    unsafe fn flush();

    // CONTRACT CHANGED: receives post-Encoder bytes
    unsafe fn write(bytes: &[u8]);

    // GUARANTEE: called after succesful `acquire` and before `release`
    // CONTRACT: returns a pointer to an allocated Encoder
    unsafe fn encoder() -> NonNull<Encoder>; // (+)
}

(+) why not references? fn() -> &'static mut T (note: not unsafe) is too error prone; call it twice and you have Undefined Behavior.

Then for a disable-all-interrupts logger you would have:

struct SomeLogger;

// the constructor `new` is the only public method Encoder has
static mut ENCODER: Encoder = Encoder::new();

unsafe impl Logger for SomeLogger {
    fn acquire() {
        // disable interrupts
        // guard aganst nested-acquire
    }

    unsafe fn release() {
        // re-enable interrupts
    }

    unsafe fn flush() {
        // do flush
    }

    unsafe fn write(bytes: &[u8]) {
        // write bytes VERBATIM
    }

    unsafe fn encoder() -> NonNull<Encoder> {
        NonNull::from(&ENCODER)
    }

}

For a lock-free logger you would have many encoders, stored in static variables.
acquire would need to guard against nested / overlapping invocations.
encoder would need to branch to hand out the right encoder for the current priority level.
write would need to branch to write bytes into the right in-memory buffer.

Then on the framework side you would have something like this:

SomeLogger::acquire();
let encoder = SomeLogger::encoder();
encoder.as_mut().start_frame(SomeLogger::write);

// maybe called several times
encoder.as_mut().write(bytes, SomeLogger::write);

encoder.as_mut().end_frame(SomeLogger::write);
SomeLogger::release();

[Dirbaio]

I think getting the pointer to encoder once at the beginning and passing it around will cause bloat issues similar to the old Formatter. Perhaps less because it's just 1 word, so it'd probably stay in registers and never hit the stack, but still something to keep in mind.

Generated code could get the pointer every time instead, but this will cause bloat too:

SomeLogger::acquire();
SomeLogger::encoder().as_mut().start_frame(SomeLogger::write);

// maybe called several times
SomeLogger::encoder().as_mut().write(bytes, SomeLogger::write);

SomeLogger::encoder().as_mut().end_frame(SomeLogger::write);
SomeLogger::release();

To combat the bloat, wrapper funcs could be made. This'll make it slower though, as SomeLogger funcs are noinline (#257, the compiler was inlining them everywhere causing giant bloat.)

fn do_start_frame() { SomeLogger::encoder().as_mut().start_frame(SomeLogger::write); }
fn do_end_frame() { SomeLogger::encoder().as_mut().end_frame(SomeLogger::write); }
fn do_write(data: &[u8]) { SomeLogger::encoder().as_mut().write(data, SomeLogger::write); }

SomeLogger::acquire();
do_start_frame();
// maybe called several times
do_write(bytes);
do_end_frame()
SomeLogger::release();

I'm pretty sure the current trait generates the smallest+fastest code...

Also, the Logger trait already has a pretty complicated safety contract. It's already quite hard to implement, adding the fn encoder() doesn't remove any "real" complexity. The complicated aquire/release safety contract still applies, and the user still has to allocate the encoder. The only improvement is doing the "get encoder" in one place (encoder) instead of three (acquire, release, write), which is very minor IMO, and not worth sacrificing code size/speed, and not worth a 0.3 -> 0.4 major bump.

[Urhengulas]

Thank you @japaric and @Dirbaio for your input. I will close this for now, since it seems it won't be relevant in the short or medium term.