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 driver for SDS011 active laser dust sensor #10458

Merged
merged 5 commits into from
Jan 31, 2019

Conversation

MichelRottleuthner
Copy link
Contributor

Contribution description

Adds a driver and SAUL integration for the SDS011 dust sensor for measuring particulate matter concentration.

Testing procedure

Below example is only true for pba-d-01-kw2x, for other boards adapt as needed.
For SAUL use the default test application:

cd tests/saul
USEMODULE=sds011 CFLAGS="-DSDS011_PARAM_UART_DEV=UART_DEV\(1\) -DSDS011_PARAM_PWR_PIN=GPIO_PIN\(3,5\)" make BOARD=pba-d-01-kw2x all flash term

With the provided test:

cd tests/driver_sds011
CFLAGS="-DSDS011_PARAM_UART_DEV=UART_DEV\(1\) -DSDS011_PARAM_PWR_PIN=GPIO_PIN\(3,5\)" make BOARD=pba-d-01-kw2x all flash term

Issues/PRs references

@MichelRottleuthner MichelRottleuthner added the Area: drivers Area: Device drivers label Nov 22, 2018
Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

first sweep, looks nice already - only minor coding style issues, check position of braces!

drivers/include/sds011.h Outdated Show resolved Hide resolved
drivers/include/sds011.h Outdated Show resolved Hide resolved
drivers/include/sds011.h Outdated Show resolved Hide resolved
drivers/include/sds011.h Outdated Show resolved Hide resolved
drivers/sds011/sds011.c Show resolved Hide resolved
drivers/sds011/sds011.c Show resolved Hide resolved
drivers/sds011/sds011.c Outdated Show resolved Hide resolved
drivers/sds011/sds011.c Outdated Show resolved Hide resolved
drivers/sds011/sds011.c Outdated Show resolved Hide resolved
drivers/sds011/sds011.c Outdated Show resolved Hide resolved
@RIOT-OS RIOT-OS deleted a comment Nov 23, 2018
Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

Looks really nice. Just a few comments, most of them typos.

drivers/include/sds011.h Outdated Show resolved Hide resolved
drivers/include/sds011.h Outdated Show resolved Hide resolved
drivers/include/sds011.h Outdated Show resolved Hide resolved
drivers/include/sds011.h Outdated Show resolved Hide resolved
drivers/include/sds011.h Outdated Show resolved Hide resolved
drivers/sds011/sds011.c Outdated Show resolved Hide resolved
drivers/sds011/sds011.c Outdated Show resolved Hide resolved
drivers/sds011/sds011.c Show resolved Hide resolved
tests/driver_sds011/main.c Outdated Show resolved Hide resolved
tests/driver_sds011/main.c Outdated Show resolved Hide resolved
@leandrolanzieri leandrolanzieri added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Nov 24, 2018
@MichelRottleuthner
Copy link
Contributor Author

@smlng @leandrolanzieri thank you for reviewing! I think all requests should be addressed now

@leandrolanzieri leandrolanzieri added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Dec 18, 2018
Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

All my comments have been addressed. I ran the test and works as expected. Looks good to me.

@leandrolanzieri
Copy link
Contributor

@smlng ping?

tests/driver_sds011/main.c Outdated Show resolved Hide resolved
drivers/sds011/sds011.c Outdated Show resolved Hide resolved
drivers/sds011/sds011.c Show resolved Hide resolved
drivers/sds011/sds011.c Show resolved Hide resolved
drivers/sds011/sds011.c Outdated Show resolved Hide resolved
drivers/sds011/sds011.c Show resolved Hide resolved
drivers/sds011/sds011.c Outdated Show resolved Hide resolved
drivers/sds011/sds011_saul.c Outdated Show resolved Hide resolved
@MichelRottleuthner
Copy link
Contributor Author

@smlng ping

@MichelRottleuthner
Copy link
Contributor Author

rebased again @smlng ping

Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

in the documentation many functions have a preconditions like <param> != NULL, for these there should be a matching assert in every implementation to verify this during development. This is missing, e.g. in sds011_get_fw_version and others.

drivers/include/sds011.h Outdated Show resolved Hide resolved
drivers/sds011/include/sds011_params.h Outdated Show resolved Hide resolved
drivers/sds011/sds011.c Outdated Show resolved Hide resolved
drivers/sds011/sds011.c Outdated Show resolved Hide resolved
drivers/include/sds011.h Outdated Show resolved Hide resolved
drivers/include/sds011.h Outdated Show resolved Hide resolved
@MichelRottleuthner
Copy link
Contributor Author

@smlng everything addressed now? and may I squash?

Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

all good except one small inconsistency 😬 Please amend directly and squash previous changes as needed, too.

drivers/sds011/include/sds011_params.h Outdated Show resolved Hide resolved
@MichelRottleuthner MichelRottleuthner force-pushed the pr_driver_sds011 branch 2 times, most recently from fdc15bd to 8a5f110 Compare January 29, 2019 20:20
@MichelRottleuthner MichelRottleuthner added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 29, 2019
@MichelRottleuthner
Copy link
Contributor Author

@smlng I fixed the remaining problems reported by murdock and removed the separate thread in the test to make it fit on the small arduinos. So, finally all spaces where they should be?^^

@smlng smlng merged commit 805efb6 into RIOT-OS:master Jan 31, 2019
@danpetry danpetry added this to the Release 2019.04 milestone Mar 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines 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.

4 participants