Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

COBS with footer PoC #17

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

korken89
Copy link

@korken89 korken89 commented Apr 8, 2020

I made an PoC for #16, feedback is very welcome!
The deserailization is a bit hacky right now, but it shows the concept, if no footer is requested from the new method the code is exactly as it was before without bloat.

@korken89
Copy link
Author

korken89 commented Apr 8, 2020

This should probably not be accepted, it's just a PoC. :)

@jamesmunns
Copy link
Owner

jamesmunns commented Apr 9, 2020

So, I need to take a closer look, but I think the "right" way to do this would be just with stacking flavors. You can think if it like the OSI model, every flavor is entirely wrapped by the next layer down, and you can add whatever you want to the start or end (or modify parts in the middle) for each flavor. So if you wanted to append a CRC32 to cobs encoded data (and put it into a slice), you would do:

CRC32Flavor<CobsFlavor<SliceFlavor<T>>>

If you wanted to have the CRC32 inside of the COBS encoding, you would do:

CobsFlavor<CRC32Flavor<SliceFlavor<T>>>

Let me know if I misunderstood, I'll take a closer look at this after I process the Mu PR!

@korken89
Copy link
Author

Thanks for the feedback!
I had a look at the CobsFlavor<CRC32Flavor<SliceFlavor<T>>>, as it would do what I want.
The issue is that the current CobsFlavor would not finish the CRC32Flavor, letting its bytes be written, before CobsFlavor finishes itself, see this line:

self.flav.release()

What I think is missing in the SerFlavor trait is an optional finalize method, as this flavor needs to finalize and push its bytes.

Or I might have misunderstood your idea here?

@korken89
Copy link
Author

Hi, @jamesmunns

Do you have feedback here?
I'm still looking into how to have "footers" generated with postcard, mainly for checksum generation.

What do you think about the finalize extension to the trait?

@jamesmunns
Copy link
Owner

Hey @korken89, I'll look at this after Oxidize, feel free to reping me then!

@MathiasKoch
Copy link

MathiasKoch commented Sep 10, 2020

Is this completely stale?

I am looking for something like CobsFlavor<CRC16Flavor<SliceFlavor<T>>>

PING @jamesmunns @korken89

@korken89
Copy link
Author

This was mostly a way to hack it in, so its not something that should be used.
I have added earlier what is needed from my PoV for this concept to be possible.

For now I am doing this as a workaround (part of the code):

/// Serialize a message into a buffer with a provided CRC16-CCITT hasher.
pub fn serialize_with_crc16<H: Hasher>(
    hasher: &mut H,
    msg: &Message,
    buf: &mut [u8],
) -> Result<usize> {
    let len = serialize_with_flavor(msg, Slice::new(buf))?.len();

    // Apply CRC
    hasher.write(&buf[0..len]);
    let crc = (hasher.finish() as u16).to_le_bytes();
    buf[len] = crc[0];
    buf[len + 1] = crc[1];

    // Apply COBS encoding
    let len = cobs::encode(&buf[0..len + 2], buf);
    buf[len] = 0; // Add the ending 0 (cobs crate does not handle that)

    Ok(len + 1)
}

@blaind
Copy link

blaind commented Sep 15, 2020

I did it like:

use crc32fast::Hasher;

pub struct Crc32Flavor<B> where B: SerFlavor {
    flav: B,
    hasher: Hasher,
}

impl<B> Crc32Flavor<B> where B: SerFlavor {
    pub fn new(bee: B) -> Self {
        let hasher = Hasher::new();
        Self { flav: bee, hasher }
    }
}

impl<'a, B> SerFlavor for Crc32Flavor<B> where B: SerFlavor {
    type Output = <B as SerFlavor>::Output;
  
    fn try_push(&mut self, data: u8) -> Result<(), ()> {
        self.hasher.update(&[data]);
        self.flav.try_push(data)
    }
  
    fn release(mut self) -> Result<Self::Output, ()> {
        let hash = self.hasher.finalize();
        for byte in hash.to_le_bytes().iter {
            self.flav.try_push(*byte)?;
        }
    
        self.flav.release();
    }
}

and then using Crc32Flavor as the "outermost" flavor.

@korken89
Copy link
Author

Thanks for the example @blaind !
I will have to take a look and compare with my old notes on the issues I saw, it looks like I was overthinking the issue :)

@korken89
Copy link
Author

One question @blaind , when you deserialize - how do you perform the extraction of the CRC?

@blaind
Copy link

blaind commented Sep 17, 2020

Btw, I noticed that crc32fast Hasher results to really large binary. Had to switch to another method, but also that hasher takes u32:s in instead of u8 as in above example. That made the solution a bit more complex (for now), but I guess it can be abstracted away to the hasher implementation, if needed.

Anyway, about the extraction:

  1. Decode cobs (using cobs crate)
  2. Get output slice that's basically [u8]: [data..., c4, c3, c2, c1]
  3. Convert c4-c1 slice to u32 (u32::from_le_bytes). Basically I'll just make sure that slice.len() is at least 4, and take the last 4 bytes out
  4. Calculate crc32 for data...
  5. Compare crc32 & u32(c4-c1)

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.

4 participants