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

Add support for BH1750FVI ambient light sensors #2261

Closed
wants to merge 2 commits into from

Conversation

pawel-sw
Copy link

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/en/*.

@devsaurus
Copy link
Member

Please add I2C id as parameter to the Lua functions, also see #2249 (comment).

@TerryE
Copy link
Collaborator

TerryE commented Mar 12, 2018

Surely, the i2c_idparameter is a setup /configuration parameter. It make no sense passing it on other functions such as read.

@pawel-sw
Copy link
Author

@TerryE it is a stateless component. If you store i2c_id during setup, you won't be able to use 2 sensors on 2 separate I2C buses.

@TerryE
Copy link
Collaborator

TerryE commented Mar 12, 2018

@TerryE it is a stateless component. If you store i2c_id during setup, you won't be able to use 2 sensors on 2 separate I2C buses.

So what happens when you setup bus 0 and attempt to read bus 15243523 or even bus 1? This is an ambient light sensor.. If the manufacturers really thought that the same bus master was going to need to read two sensors then the'd have exposed a couple of address pins as pull up / down pins on the IC.

@mickley
Copy link

mickley commented Mar 16, 2018

I'm currently using a setup with two BH1750FVI sensors and my own lua module, and could forsee needing to use more than two. The BH1750 actually does have an address pin too, so two are supported on the same bus, which is what I am doing. This module doesn't have an option to use I2C address 0x5C (@s-pw).

Moreover, I think needlessly restricting useful features and forcing hack-style programming for stuff like multiple sensors is bad form. I've had the same problem with BME280, which doesn't support multiple sensors on the same bus (#2241 ). These are the sorts of basic things that should be supported for every sensor!

@TerryE
Copy link
Collaborator

TerryE commented Mar 16, 2018

I think needlessly restricting useful features and forcing hack-style programming for stuff like multiple sensors is bad form

In which case the setup should return a userdata object and all further references should use use Lua class features. I didn't suggest that you should hack this. Do single instance or multi-instance properly.

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 16, 2022
@stale stale bot closed this Apr 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants