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

Feedback on plan to create a J5 driver for the BME680 #1479

Closed
codefoster opened this issue Jun 19, 2018 · 13 comments
Closed

Feedback on plan to create a J5 driver for the BME680 #1479

codefoster opened this issue Jun 19, 2018 · 13 comments

Comments

@codefoster
Copy link

I'd like to take a stab at a driver for the BME680 device from Adafruit. It's a multi-sensor component with temperature, humidity, pressure, and air quality, so I'm leaning toward extending the thermometer, hygrometer, and barometer classes to include support for the BME680 and then add a new sensor category of gas or perhaps airQuality to provide the air quality sensor values. I would submit these as a PR. Let me know if there's any feedback or guidance.

@nebrius
Copy link
Contributor

nebrius commented Jun 19, 2018

For a bit more context, @codefoster is a coworker of mine and does a lot of rad IoT stuff :).

There's already support for the BME280 in J5, and it seems there are a number of similarities between the two devices, but also a few differences. One question I'm wondering: would it make sense to try and abstract support for the two instead of duplicating effort, and is there any precedence for how to do this?

@rwaldron
Copy link
Owner

rwaldron commented Jun 19, 2018

Advice: follow the BME280 implementation and you can't go wrong!

You'll want to look at:

  • lib/imu.js
  • lib/{thermometer,hygrometer,barometer}.js

And tests <3

Look forward to your work!

@codefoster
Copy link
Author

Got it. I see that BME280 shows up under the sensor categories of Thermometer, Hygrometer, and Barometer, but then it also shows up under Multi. I'll assume that's a recommended pattern, but I'm curious what the advantage of having it in both places is.

@dtex
Copy link
Collaborator

dtex commented Jun 21, 2018

I'm curious what the advantage of having it in both places is

The other classes are just convenience wrappers to multi/imu so the logic is not in two places. It's just two ways to reference an IMU. Sometimes people may just want to use a single class' feature set so making them available as a single, focused class is a nice option.

would it make sense to try and abstract support for the two instead of duplicating effort

I took a quick look at the specs. The register locations are different, but the bit size and format of the returned values appear to be consistent so there is an opportunity to reuse at least some of the code. It would be really cool if the 680 could be a clone of the 280 with a modified REGISTER and the addition of Gas, but it looks like the initialize method would need to be modified significantly so you wouldn't be able to reuse it and that is a big part of it.

I think the biggest challenge you will have here is designing the API for the Gas class which will be brand new to Johnny-Five.

It would be nice if more of the "sub-class" specific code (thermometer, barometer, hygrometer, gas) were stored in those classes instead of putting it all in imu. I believe it would create more opportunities for code re-use and certainly make the imu code less intimidating. That, however, is just one opinion so don't let it influence your planning.

@codefoster
Copy link
Author

Thanks, @dtex. Out of curiosity, why is the module that contains so much of the code called "imu"? That's a specific type of sensor, right, an inertial measurement unit?

@dtex
Copy link
Collaborator

dtex commented Jun 21, 2018

The pattern started out as a solution for IMU's which have sensors that fit multiple classes and then I guess someone had the same realization as you did so:

module.exports.Multi = module.exports.IMU;

https://github.com/rwaldron/johnny-five/blob/master/lib/johnny-five.js#L74

@codefoster
Copy link
Author

Sounds reasonable. I'm ever idealistic, though, and have to ask if there's been any consideration in a rewrite with classes and perhaps even types.

@dtex
Copy link
Collaborator

dtex commented Jun 21, 2018

That exact topic is on the agenda for the J5 collaborator summit. Are you going to JSConf US? You are welcome to join us.

#1463

@codefoster
Copy link
Author

I don't currently have plans to join. Sounds great though. Makes me feel good about going ahead and implementing this one device the way it works today. Converting the logic to whatever is determined to be the new way is going to be the same as all the other sensors. Thanks! Looking forward to the redesign.

@gillescochez
Copy link

gillescochez commented Nov 9, 2018

Interested in such driver myself. Found this JS which might be an useful resource.

https://github.com/johntalton/boschIEU/blob/master/src/chip/bme680.js

@stale
Copy link

stale bot commented Nov 4, 2019

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

@stale stale bot added the stale label Nov 4, 2019
@dtex
Copy link
Collaborator

dtex commented Nov 4, 2019

I've added the request for BME680 to the Johnny-Five requested features wiki page. Am closing this issue for now since the request is now listed there.

@dtex dtex closed this as completed Nov 4, 2019
@PhearZero
Copy link

Just ordered a BME686 and will be needing the interface. I agree with @nebrius and an abstraction may be the right way to go in the long term. Haven't dug into the implementation yet but thank you @rwaldron for the references. I won't speculate too wildly until I dive into J5, I needed the interface and found these threads/wiki. Let me know if this is a good path forward:

  • Duplicate Effort for BME280 to solve for BME680 and this issue.
  • Come together on a reasonable specification for interfaces and abstractions for similar manufacture product lines. (Possibly a separate issue, this could be granular or high level)

Thank you everyone for your work! It's been a great project and perfect teaching tool. 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants