Clarifying the docs on Write::write

[steveklabnik]

Hey folks! 👋

So, I ran into a fairly big issue today, and that's that I assumed that Write::write would be getting an entire log frame at a time, whereas instead, it gets bits of a log frame each time.

Would you like a docs PR that clarifies this? I figured I'd ask before I started looking at it, and I'm not 100% sure what exactly I should say...

(This came up when trying to polyfill #108)

[jonas-schievink]

Yeah, we'd love a PR that clarifies the docs here!

I think we might also want to add a method to Write that is called before or after each log frame, because that allows adding framing in the transport (and sounds like it would help you as well). Not sure about the best design here though.

[japaric]

I think in terms of documenting the write method I would say "whether the bytes argument is an entire log frame or a fragment of it is unspecified". It seems unlikely that we'll ever be able to pass a whole log frame as bytes: that would require a buffer large enough to hold the frame and frames can be arbitrarily large but as future proofing goes I would like to not set in stone that it will always be a fragment.

In terms of attaching a header or footer to the frame I think something like adding start_of_frame and end_of_frame methods to Write as well as some sort of associated State type to it seems like it should be flexible enough. At least that seems like it would cover adding a CRC as a footer to detect transmission errors; I'm not sure if that would be enough to add COBS framing (is COBS "streaming" possible? I don't know). Knowing in what kind of ways people want to extend frames would help pick a design here.

[MathiasKoch]

In terms of attaching a header or footer to the frame I think something like adding start_of_frame and end_of_frame methods to Write as well as some sort of associated State type to it seems like it should be flexible enough. At least that seems like it would cover adding a CRC as a footer to detect transmission errors; I'm not sure if that would be enough to add COBS framing (is COBS "streaming" possible? I don't know). Knowing in what kind of ways people want to extend frames would help pick a design here.

I think that would be very helpful!

At least it would allow for something like (assuming start_of_frame would contain the total length of the frame):

static mut LOGPRODUCER: Option<LogProducer> = None;

struct LogProducer {
    producer: bbqueue::Producer<'static, LogBufferSize>,
    encoder: Option<(bbqueue::GrantW<'static, LogBufferSize>, cobs::CobsEncoder<'static>)>
}

impl LogProducer {
    pub fn new(producer: bbqueue::Producer<'static, LogBufferSize>) -> Self {
        Self {
            producer,
            encoder: None
        }
    }

    pub fn start_encoder(&mut self, len: usize) -> Result<(), ()> {
        if self.encoder.is_some() {
            return Err(())
        }
        match self.producer.grant_exact(cobs::max_encoding_length(len)) {
            Ok(mut grant) => {
                let buf = unsafe { grant.as_static_mut_buf() };
                self.encoder = Some((grant, cobs::CobsEncoder::new(buf)));
                Ok(())
            }
            Err(_) => Err(())
        }
    }

    pub fn encode(&mut self, bytes: &[u8]) -> Result<(), ()> {
        if let Some((_, ref mut encoder)) = self.encoder {
            encoder.push(bytes)
        } else {
            Err(())
        }
    }

    pub fn finalize_encoder(&mut self) -> Result<(), ()> {
        if let Some((grant, encoder)) = self.encoder.take() {
            let size = encoder.finalize()?;
            grant.commit(size);
            Ok(())
        } else {
            Err(())
        }
    }
}

#[defmt::global_logger]
struct Logger;

impl defmt::Write for Logger {
    fn start_of_frame(&mut self, len: usize) {
        handle().start_encoder(len).ok();
    }

    fn end_of_frame(&mut self) {
        handle().finalize_encoder().ok();
    }

    fn write(&mut self, bytes: &[u8]) {
        handle().encode(bytes).ok();
    }
}

#[inline]
fn handle() -> &'static mut LogProducer {
    unsafe {
        match LOGPRODUCER {
            Some(ref mut x) => x,
            // FIXME: Should not panic here!
            None => panic!(),
        }
    }
}

[steveklabnik]

I think we might also want to add a method to Write that is called before or after each log frame, because that allows adding framing in the transport (and sounds like it would help you as well). Not sure about the best design here though.

Yep! That would help, as it is effectively what I am doing.

I think in terms of documenting the write method I would say "whether the bytes argument is an entire log frame or a fragment of it is unspecified". It seems unlikely that we'll ever be able to pass a whole log frame as bytes: that would require a buffer large enough to hold the frame and frames can be arbitrarily large but as future proofing goes I would like to not set in stone that it will always be a fragment.

👍

In terms of attaching a header or footer to the frame I think something like adding start_of_frame and end_of_frame methods to Write as well as some sort of associated State type to it seems like it should be flexible enough. At least that seems like it would cover adding a CRC as a footer to detect transmission errors; I'm not sure if that would be enough to add COBS framing (is COBS "streaming" possible? I don't know). Knowing in what kind of ways people want to extend frames would help pick a design here.

So for me, the framing is purely a work around for the lack of structured logging. And also using structured logging is kind of a workaround for the fact that defmt seems to be designed around the idea of there being one main program, and hence logger, running on the entire system. Whereas I'm in a position where I'm implementing an OS, and need each program/process/task to have its own logger. This is pretty far afield of the original issue but if yinz ever want to talk about it, I'm happy to fill you in.