Remove panicking behavior

[barafael]

Hi,

thanks for taking the time to make a Rust library for your I2C sensors!

I think this no_std library should not panic when a wrong buffer size is passed, or if a crc is wrong. Instead, the methods already return Result, so I extended the Error type. Tests are adapted too.

I split the Error type in crc8::Error and crate::Error (the crc8 module has it's own Error type) because I didn't want crc8 to require embedded_hal::i2c etc. Please let me know if you know a better way.

[rnestler]

I think this no_std library should not panic when a wrong buffer size is passed, or if a crc is wrong. Instead, the methods already return Result, so I extended the Error type. Tests are adapted too.

To be clear: The library doesn't currently panic if the CRC is wrong, only if a wrong buffer size is passed.

IMO panics can and should be used if the error is a clear developer error which should never happen if the calling code is correct. In this example providing buffers which have the wrong size.

But maybe it would be better to use the type system to encode the constrains? So a buffer type that assures that it contains a reference to a slice which has a multiple of 3 bytes?

cc @dbrgn for further inspiration about the topic :slightly_smiling_face:

[dbrgn]

Is there a use case for a driver that creates dynamically sized buffers, which are then passed directly to sensirion-i2c?

If not, I'd keep the panics, since they simply ensure that invariants are honored.

(In the Sensirion device drivers that I wrote, the size is always known in advance.)

[barafael]

To be clear: The library doesn't currently panic if the CRC is wrong, only if a wrong buffer size is passed.

Fair point!

The way I see it, with result types, the user can at least still decide to panic with unwrap or try to handle the error somehow.

(In the Sensirion device drivers that I wrote, the size is always known in advance.)

I think that's the case for mine too? https://github.com/barafael/sdp8xx/blob/0e2ff653d0309866412b37a413261adeeac977f7/src/lib.rs#L148 Don't know if I want to entrust the compiler to see that though.

Please ignore my bone-headedness, I can still just use my fork, or probably embrace the panic.

(BTW, the sdp800 sensor is really fantastic)

[dbrgn]

Well, if this invariant (buffer size must be a multiple of 3) is broken, then that's a hard programming error, and no type of error handling will get you out of that.

However, it could be argued that especially on embedded, you might want to catch even unexpected programming errors in a seldomly used branch of the code and do some error handling that wouldn't be easily possible with panics.

I guess since an assert is used, and not a debug assert, there's no runtime difference between the two variants.

[barafael]

Would you be open to discuss improving the error handling of this crate, though? Unless I am doing something wrong, it's cumbersome.

Say I have a lib.rs and a product_info.rs. In the product_info module, I want to call crc8::validate. That means I have to know the Result which comes back. If I want to use '?' as in crc8::validate(...)?, I need a From<sensirion_i2c::i2c::Error<...>> for MyError, which forces me to put the I2C and so on in modules that should not have to care about it.

[rnestler]

We could do a simple newtype style wrapper though (dumping code from brain without compiling / testing :wink:):

struct I2CBuffer<'a>(&'a[u8])
impl<'a> TryFrom<&'a[u8]> {
    fn from(...) -> Result<> {
    }
}

That way one could have a guarantee that the buffer is a multiple of three bytes and users could just unwrap the try from. How does this sound?

[rnestler]

(BTW, the sdp800 sensor is really fantastic)

Thanks :heart:

[rnestler]

Would you be open to discuss improving the error handling of this crate, though? Unless I am doing something wrong, it's cumbersome.

Sure.

Say I have a lib.rs and a product_info.rs. In the product_info module, I want to call crc8::validate. That means I have to know the Result which comes back. If I want to use '?' as in crc8::validate(...)?, I need a From<sensirion_i2c::i2c::Error<...>> for MyError, which forces me to put the I2C and so on in modules that should not have to care about it.

I'm not sure I follow. crc8::validate looks like this:

 pub fn validate(buf: &[u8]) -> Result<(), ()> { 

If I recall correctly my line of thinking was, that it can just either succeed or fail, so we just return a Result<(), ()> since there aren't different error cases. But I'm not sure if you can or should implement From<()> for your MyError since it could mean close to anything.

I'd propose that we add an separate enum enum Error { CrcError } in the crc8 module and return that instead of an empty tuple.

[barafael]

I'd propose that we add an separate enum enum Error { CrcError } in the crc8 module and return that instead of an empty tuple.

Sounds good to me... Although I have to say I have no idea how to do cross-crate error handling well. It is interesting that there is not a type Status = Result<(), ()> or somesuch.

Regarding the impl<'a> TryFrom<&'a[u8]> for I2CBuffer { I like the idea!

[rnestler]

I started https://github.com/Sensirion/sensirion-i2c-rs/pull/18 to add the CrcError. Target is to allow users of the library to add a From<crc8::Error> implementation for their own error types.

[barafael]

I have tried implementing your suggestion for the I2CBuffer struct. Had trouble with the generic arguments I2cWrite and I2cRead, so had to make another enum and conversion...: https://github.com/barafael/sensirion-i2c-rs/commit/d7ad2750aedfe858111bbd0cbe656f345e05780e

[rnestler]

I have tried implementing your suggestion for the I2CBuffer struct.

I'd suggest to use the I2CBuffer then directly in the read_words_with_crc function, to eliminate that error case, such that the conversion from ConversionError to the i2c::Error is not necessary. This means the ConversionError needs to be pub of course.

[barafael]

I have tried following your suggestion but ran into this problem:

I2cBuffer is defined as a tuple struct with a shared slice of bytes as member 0. So when I try to access data.0, I cannot get a mutable reference! Removing the reference and lifetime gets me nowhere, as TryFrom requires Sized which [u8] does not fulfil.

Defining a type I2cBuffer = &mut 'a [u8]; gets me nowhere either, as I run into the orphan rule...

[rnestler]

I2cBuffer is defined as a tuple struct with a shared slice of bytes as member 0. So when I try to access data.0, I cannot get a mutable reference! Removing the reference and lifetime gets me nowhere, as TryFrom requires Sized which [u8] does not fulfil.

Uh good point. I think we'd actually need two wrapper buffers then (Similar to Fn and FnMut for example): struct I2CBufferMut<'a>(&'a mut [u8]); and struct I2CBuffer<'a>(&'a [u8]);

[rnestler]

Do you want to open a new pull request with your experiments regarding I2cBuffer? Then we could continue the discussion there and close this one.