klemens / si7021-rs

Rust I²C driver for the Si7021 hygrometer and thermometer
0 stars 1 forks source link

Support embedded-hal #1

Open chrysn opened 5 years ago

chrysn commented 5 years ago

This sensor is suitable for use in embedded (bare-metal or embedded OS, no_std) environments. As it is now, the crate can not be used there as it depends on i2cdev, which is explicitly linux-bound.

The embedded-hal crate provides traits for I2C access (but not device creation, that stays platform dependent) which can be implemented in a portable way, and is no_std. There exists a linux-embedded-hal crate that implements the embedded-hal blocking I2C trait in terms of i2cdev.

Would you consider rebasing si7021 onto embedded-hal to make it usable on smaller systems? Most of the changes are trivial, the big ones are:

Is this a route that you'd be, in general, willing to explore / to go?

I do have a working implementation of that which just needs some clean-up before being submitted as a PR.

klemens commented 5 years ago

This sounds good!

Creation of devices differs; the embedded-hal i2c trait wants the address on every invocation rather than i2cdev which takes a fixed address at construction, so the constructor based on embedded-hal needs an additional parameter.

This is fine. However, it seems to be implemented by reopening the device instead of just calling set_slave_address? I guess we can fix this.

The i2csensors traits implemented require a std::Error; I'll open an issue up there next to allow (possibly cfg-gated) simpler error types.

Yeah, I also don't see a reason for the std::Error bound.

chrysn commented 5 years ago

However, it seems to be implemented by reopening the device instead of just calling set_slave_address?

My gut feeling about this is that it's "It's good enough to work, so nobody ever bothered" situation; will you send an issue or PR there?

i2csensors ... std::Error

Now tracked as https://github.com/martindeegan/i2cdev-sensors/pull/4; I'll send a PR with my current status when I get feedback there, for much delta is currently workarounds.

klemens commented 5 years ago

My gut feeling about this is that it's "It's good enough to work, so nobody ever bothered" situation; will you send an issue or PR there?

Found the discussion, and set_slave_address was actually added because of the implementation in linux-embedded-hal. :rofl:

It also seems wose and me were writing si7021 drivers at the same time, although his is much more feature-complete (supports no_std, but no i2csensors traits). :sweat_smile: