crc8 buffer size validation

[dbrgn]

The docs for crc8::validate and i2c::read_words_with_crc state:

Note: This method will consider every third byte a checksum byte. If the buffer size is not a multiple of 3, then not all data will be validated.

This is the implementation:

    debug_assert!(buf.len() % 3 == 0, "Buffer must be a multiple of 3");
    for chunk in buf.chunks(3) {
        if chunk.len() == 3 && calculate(&[chunk[0], chunk[1]]) != chunk[2] {
            return Err(());
        }
    }
    Ok(())

This implementation was taken from the sgp30 driver where we can ensure that buffer sizes are always multiples of 3. However, as a standalone library with a public API I wonder if this constraint should be enforced, since it's quite easy to get things wrong otherwise without noticing...

Maybe two variants could be provided, validate_unchecked (potentially unsafe) with the same behavior as now and validate that panics or returns an error if the size is not a multiple of 3? It's not a memory safety issue, but a correctness issue.

Alternatively, always validate (I don't think the size check will be much overhead).

(A static assertion would be nice of course, but I don't think that's possible.)

[dbrgn]

Ah, since we already check the chunk size anyways, we can do it like this, without additional overhead:

if chunk.len() == 3 {
    if calculate(&[chunk[0], chunk[1]]) != chunk[2] {
        return Err(());
    }
} else {
    return Err(...); // Or panic
}

Maybe a proper error enum would be good though.

[rnestler]

Alternatively, always validate (I don't think the size check will be much overhead).

I guess since the buffer size is clearly a programmer error we can just replace the debug_assert! with assert! and call it a day?

[rnestler]

Maybe a proper error enum would be good though.

This would need a separate Error enum for the crc8 module, since I don't rally want to leak the I2C errors into the crc crate.

[dbrgn]

I guess since the buffer size is clearly a programmer error we can just replace the debug_assert! with assert! and call it a day?

Yeah, that would work (but the panic would need to be documented). I don't think the overhead of a single length check is a problem.