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

pkg/bme680 driver: add support for I2C/SPI BME680 driver #10502

Closed
wants to merge 2 commits into from

Conversation

dylad
Copy link
Member

@dylad dylad commented Nov 28, 2018

Contribution description

This PR adds support for I2C/SPI BME680 sensor as package.
In a previous PR, @aabadie proposes to support this driver as a package because its quite complicate and Bosch already provides a working driver on their repo [1].

This driver is labelled as WIP because I'm not fully happy and I need some advises/reviews to enhance it. Furthermore, I only tested the I2C part on a custom board. I'll need some help to test the SPI part before merging this PR.

Testing procedure

run the dedicated test application using
make -C tests/pkg_bme680_driver
You may need to modify some params to match your hardware. (Comments are very welcome here)

Issues/PRs references

#9909

[1] https://github.com/BoschSensortec/BME680_driver

@dylad dylad added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Area: pkg Area: External package ports Area: drivers Area: Device drivers labels Nov 28, 2018
@gschorcht
Copy link
Contributor

@dylad Thanks for your contribution.

Before we start adding drivers as packages, we should discuss and find an agreement for some questions that came up with my PR #10363.

  • How can we identify a package as a driver within the package directory which is inflating if more drivers are realized as packages. Should we use driver_ as prefix as I did, or should we use _driver as suffix as you did. To have something like categories, I would prefer prefixes. Or should we start to sort packages in categories in different sub-directories? In any case, we should have a common naming scheme?

  • You have added the package as "Bare Metal". This gives the application full access to the functionality of the sensor, but makes it difficult to use it when the application just to follow the RIOT approach for sensor drivers, where a sensor is configured with a parameter set and is ready to use as soon as it is initialized with this parameter set. I think there should be at least a small wrapper module in drivers which defines the typesbme680_t and bme680_params_t and provides the functions bme680_init and bme680_read. You might take a look to my PR # 10363 to get an idea how I solved it.
    In addition, sensor drivers should provide SAUL capabilities. This would also be possible with the small wrapper driver module.

  • And last but not least, how should the documentation looks like? Would the documentation of the vendor be enough?

@dylad
Copy link
Member Author

dylad commented Nov 29, 2018

How can we identify a package as a driver within the package directory which is inflating if more drivers are realized as packages. Should we use driver_ as prefix as I did, or should we use _driver as suffix as you did. To have something like categories, I would prefer prefixes. Or should we start to sort packages in categories in different sub-directories? In any case, we should have a common naming scheme?

I have no strong opinion on it, maybe a dedicated sub-directory could be better ? But we will need to modify a bit the build system I guess. In any case, I propose to open an issue to regroup all the discussions in a specific place.

You have added the package as "Bare Metal". This gives the application full access to the functionality of the sensor, but makes it difficult to use it when the application just to follow the RIOT approach for sensor drivers, where a sensor is configured with a parameter set and is ready to use as soon as it is initialized with this parameter set. I think there should be at least a small wrapper module in drivers which defines the typesbme680_t and bme680_params_t and provides the functions bme680_init and bme680_read. You might take a look to my PR # 10363 to get an idea how I solved it.
In addition, sensor drivers should provide SAUL capabilities. This would also be possible with the small wrapper driver module.

I think it's a good approach to unify drivers and package drivers, it will require some extra works but I think it's worthwhile. We must define how it will look like before.

And last but not least, how should the documentation looks like? Would the documentation of the vendor be enough?

For this point, my suggestion is to keep the API documentation from the original repo but we also must document any patches or adaptation level to RIOT in order to explain how it works.
In addition, we can provide a wiki page for each package driver with more details and examples if needed.

I'll create an issue to centralize discussions about package drivers.

@miri64 miri64 added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Dec 4, 2018
@@ -0,0 +1,44 @@
# U8g2 Package Test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Readme copied from U8g2 without updating contents to bme680

#define BME680_SPI_SPEED (SPI_CLK_1MHZ)
#endif /* BME680_SPI_SPEED */

#ifndef BME6800_SPI_MODE
Copy link

@LloydKM LloydKM Jun 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be:

#ifndef BME680_SPI_MODE

@dylad
Copy link
Member Author

dylad commented Jul 9, 2019

Sorry for the delayed answer,
I plan to put more work on it after the RIOT summit in September.
But thanks for the reviews :)

@roberthartung
Copy link
Member

roberthartung commented Nov 6, 2019

@dylad Are you still working on this? Some of my colleagues need a driver within RIOT.

@dylad
Copy link
Member Author

dylad commented Nov 6, 2019

@roberthartung
This is on my todolist but I have an issue with my custom hardware. I'm waiting for my order so I can fix my board and work on it again. (Hopefully soon ?)

@dylad
Copy link
Member Author

dylad commented Nov 15, 2019

Close in favor of #12717

@dylad dylad closed this Nov 15, 2019
@dylad dylad deleted the pr/pkg_bme680_driver branch January 22, 2021 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: pkg Area: External package ports State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants