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

Improve error reporting and handling #22

Open
matthijskooijman opened this issue Oct 31, 2020 · 3 comments
Open

Improve error reporting and handling #22

matthijskooijman opened this issue Oct 31, 2020 · 3 comments
Labels
enhancement New feature or request

Comments

@matthijskooijman
Copy link

Currently, this library returns very limited error status, only STATUS_OK or STATUS_FAIL. It would be convenient if some additional codes would be returned (i.e. STATUS_NO_ACK, or STATUS_CRC_FAIL or STATUS_SHORT_READ etc.).

I considered having a go at implementing this, but since this touches on code that comes from the non-arduino-specific embedded-sps repo, which again has code from the embedded-common repo, I'm not going to figure out how all this plays together and where to contribute each part for now.

A related suggestion, is that the I2c implementation should probably check the return value of Wire.requestFrom, which returns the number of bytes actually read. I was using this library on an STM32 platform, which also has a 32-byte buffer. Currently, this makes sensirion_i2c_read silently only return 32 meaningful bytes, with the rest filled with garbage, which will then (usually) be detected by the CRC-checks, but it would be nicer if the short read was detected by itself (and with the above suggestion of different status codes, actually returned as such).

@winkj
Copy link
Member

winkj commented Oct 31, 2020

Agreed, that would make debugging easier. As you saw a lot of that will have to implemented in a project this one integrated, but having the issue here should help keep track.

Totally agree on checking Wire.request from, which is again in a file integrated from embedded-common.

Finally, all recent SPS versions support readout modes that fit in the 32 byte buffer, so I created issue #24 to add that in the (hopefully near) future.

@abrauchli
Copy link

Hi @matthijskooijman, you're speaking from my heart.. a better error handling would improve the developer experience. We have an issue open in our internal issue tracker. As you correctly noted, this is mostly flowing ouf of embedded-common, so if you want a placeholder to post some ideas, it'd be best to open another issue there https://github.com/Sensirion/embedded-common/issues

@rnestler FYI

@rnestler
Copy link

rnestler commented Nov 2, 2020

There was actually some work in this direction in Sensirion/embedded-common#45.

@winkj winkj added the enhancement New feature or request label Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants