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

Support embedded-hal #1

Open
chrysn opened this issue Aug 27, 2018 · 3 comments · May be fixed by #2
Open

Support embedded-hal #1

chrysn opened this issue Aug 27, 2018 · 3 comments · May be fixed by #2

Comments

@chrysn
Copy link

chrysn commented Aug 27, 2018

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:

  • 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.
  • The i2csensors traits implemented require a std::Error; I'll open an issue up there next to allow (possibly cfg-gated) simpler error types.

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
Copy link
Owner

klemens commented Aug 27, 2018

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
Copy link
Author

chrysn commented Aug 27, 2018

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 martindeegan/i2cdev-sensors#4; I'll send a PR with my current status when I get feedback there, for much delta is currently workarounds.

@klemens
Copy link
Owner

klemens commented Aug 27, 2018

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. 🤣

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). 😅

@ryan-summers ryan-summers linked a pull request Jun 30, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants