Improve error handling: impl TryFrom for I2cBuffer to ensure buffer size

[barafael]

As a result of previous disussion in #17, I am trying to add a type which ensures that the i2c buffer given has a correct length.

So far, I have implemented the type as a tuple struct with a TryFrom implementation, as suggested by @rnestler here

So when I try to access the shared slice of bytes here, I cannot get a mutable reference! Removing the reference and lifetime gets me nowhere, as TryFrom requires Sized which [u8] does not fulfil.
Defining the wrapper type as type I2cBuffer = &mut 'a [u8]; gets me nowhere either, as I run into the orphan rule.

I am opening this PR to see if we can find a solution together :)

[barafael]

Turns out I was just using mut in the wrong places. It works now I think :)

Not super ergonomic due to using .0 and &mut buffer[..], but maybe it's possible to improve that.

[rnestler]

Can you add some documentation that the buffer must hold a multiple of three bytes and enforces it with the TryFrom?

[rnestler]

Also I think the member shouldn't be pub, because with pub it can be constructed without the TryFrom, no?

[barafael]

I set the member to pub(crate), because the crc8 needs to be able to see it.

[rnestler]

This PR needs rebasing because #18 got merged.

[barafael]

After all this, I don't really like that the I2CBuffer type is not exactly easy to use (see usage in I2CBuffer doc comment for example).

What do you think of using a slice of u16 or type Word = [u8, 2]; everywhere and generating the CRC in read or write?

Sorry for bringing this up again :D the approach with I2CBuffer is ok too, I think.

[barafael]

Ok, to bring this to a conclusion: I think right now there does not seem to be a way that assures this invariant while being ergonomic.

I think this topic should be revisited when full const-generics land, probably later 2021.

Or what do you think?

[rnestler]

Sorry for not answering for some time.

Ok, to bring this to a conclusion: I think right now there does not seem to be a way that assures this invariant while being ergonomic.

That may indeed be true. But at the same time I'd like to add it anyways, since it could also help in generating the data one would like to send. Something like:

use sensirion_i2c::i2c::I2CBuffer;
use core::convert::TryFrom;
let mut data = [0x00; 6];
let mut i2c_buffer = I2CBuffer::try_from(&mut data[..]).unwrap();

i2c_buffer.add_u16(0x1234); // will add the two bytes and the crc
i2c_buffer.add_bytes(&[0x56, 0x78]);

// may also provide
i2c_buffer.add_u32(...);
i2c_buffer.add_float(...);
...

// it may even provide the method to write it to I2C
i2c_buffer.write(i2c);

// or a getter which returns the slice of the buffer which needs to be sent
let data_to_send = i2c_buffer.get_i2c_frame();

[barafael]

That sounds good to me. I think this could be done nicely like this:

trait Append<T> {
    fn append(&mut self, val: T) -> ...;
}
impl Append<u16> for I2cBuffer { ... }
impl Append<[u8; 2]> for I2cBuffer { ... }
...

Things like add_u16 would be easier if the I2cBuffer managed its own memory - this way, the usage would also be simpler and we could do without 'a, at the expense of some memory. Are there any sensirion sensors so far that require more than 32 bytes i2c block transfers?
With the buffer-wrapper-struct approach, the user would calculate how many bytes will be required for allocating their buffer (6 in your example), which is fine too, but meh.

[rnestler]

Are there any sensirion sensors so far that require more than 32 bytes i2c block transfers?

Yes: The SPS30 can require more than 32 Bytes.

[barafael]

In that case, we could use min_const_generics to allow the user to customise the internal buffer size. What do you think?

[rnestler]

Uh nice! I like where this is headed. Note that you still need to rebase this pull request for latest master (you may need to update the master branch on you fork or local checkout first). I'll need some more time to review this in depth.

[rnestler]

Btw. I enabled the Circle CI testing for external pull requests, that's why we see it failing now. Previously it just didn't run. We probably need to bump the minimum required Rust version so that const generics are available.

[dbrgn]

We probably need to bump the minimum required Rust version so that const generics are available.

Well, right now this still requires nightly, right? As far as I can tell, min_const_generics should be stabilized in 1.51: rust-lang/rust#79135

[barafael]

While we are at it: I noticed the sensirion-i2c-rs crate has been yanked in favor of sensirion-i2c, but this repo is still called sensirion-i2c-rs. It could be renamed (github will keep a redirect in place in case someone accesses an old link).
Re: CI, are you open to adding a github action for build+test (on nightly)?

[rnestler]

While we are at it: I noticed the sensirion-i2c-rs crate has been yanked in favor of sensirion-i2c, but this repo is still called sensirion-i2c-rs. It could be renamed (github will keep a redirect in place in case someone accesses an old link).

I'm actually fine with the repository name indicating that it is a Rust repository. But for the crate name it is kind of redundant since it obviously is for Rust. I think it is a quite nice convention to use -rs postfix for the repository name but remove it from the crates name.

Re: CI, are you open to adding a github action for build+test (on nightly)?

I'm a bit reluctant to make the crate require nightly, but since the feature will probably hit stable in around two months (1.51.0 2021-03-25) I guess it should be fine. Switching from CircleCI to GitHub actions I'd prefer to do in a separate pull request though, since this pull-request is already quite huge (and with 30 commits a bit hard to keep an overview, needs some squashing)

[barafael]

Re: nightly, we could merge this when the const generics implementation hits stable.
Regarding rebase/squash, I am really confused, maybe because there was a fair number of changes which were discarded later. I don't know how to do this (I tried, but at some point I can't keep track of which changes to merge where during merge conflicts).

[rnestler]

Regarding rebase/squash, I am really confused, maybe because there was a fair number of changes which were discarded later. I don't know how to do this (I tried, but at some point I can't keep track of which changes to merge where during merge conflicts).

Ah sorry. So there was one merge to master that happened from #18. So the master branch from your fork of the project: https://github.com/barafael/sensirion-i2c-rs/tree/master is out of sync with the master branch here. (You pushed 4 commits on it and in the meantime #18 was merged)

I suggest you force push to the master branch of your fork like this:

# you can skip cloning, but make sure to not destroy your working branch.
$ git clone git@github.com:barafael/sensirion-i2c-rs.git
Cloning into 'sensirion-i2c-rs'...
remote: Enumerating objects: 145, done.
remote: Counting objects: 100% (145/145), done.
remote: Compressing objects: 100% (105/105), done.
$ cd sensirion-i2c-rs
$ git remote add upstream git@github.com:Sensirion/sensirion-i2c-rs.git
$ git fetch --all
Fetching origin
Fetching upstream
remote: Enumerating objects: 4, done.
remote: Counting objects: 100% (4/4), done.
remote: Compressing objects: 100% (3/3), done.
remote: Total 4 (delta 1), reused 2 (delta 1), pack-reused 0
Unpacking objects: 100% (4/4), 1.19 KiB | 611.00 KiB/s, done.
From github.com:Sensirion/sensirion-i2c-rs
 * [new branch]      master     -> upstream/master
$ git reset --hard upstream/master 
HEAD is now at 8728692 crc8: Use explicit error type (#18)
$ git push -f
Total 0 (delta 0), reused 0 (delta 0)
To github.com:barafael/sensirion-i2c-rs.git
 ! [remote rejected] master -> master (permission denied)
error: failed to push some refs to 'git@github.com:barafael/sensirion-i2c-rs.git'

The force push obviously fails for me, since I don't have permissions to do so.

Then you can rebase your tryfrom_for_i2cbuffer branch on top of upstream/master and fix the conflicts as you go

$ git checkout tryfrom_for_i2cbuffer
Branch 'tryfrom_for_i2cbuffer' set up to track remote branch 'tryfrom_for_i2cbuffer' from 'origin'.
Switched to a new branch 'tryfrom_for_i2cbuffer'
$ git rebase -i upstream/master
Auto-merging src/crc8.rs
CONFLICT (content): Merge conflict in src/crc8.rs
error: could not apply 3541c84... Remove panicking behavior from crc8 module
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 3541c84... Remove panicking behavior from crc8 module

During interactive rebase you may also squash together small fixes. I hope this helps 🙂

[barafael]

Alright, that was easier. I think that didn't erase anything. Having tests helps. Thanks for your help!
Speaking of tests, line coverage is at about 96% FWIW.

[barafael]

1.51 is out 🎉🎉🎉

(wink wink, but really there is no urgency.)

How does the MSRV thing work, should this be added? Or make I2cBuffer a feature?

[barafael]

I don't want to push too hard - but please do let me know if there is anything I can do to get this released (my sdp800 driver release kind of depends on it).