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

First iteration of air_quality API based on the temperature API #16

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

RaresCon
Copy link

@RaresCon RaresCon commented Mar 27, 2023

I've written the base of air_quality API. I will add documentation as I progress with this PR. I would be glad to receive feedback, mainly on read_data_sync.

I've written the base of air_quality API, next I will add a fake sensor for
testing the API, including unit testing for the fake sensor
@alexandruradovici
Copy link

Looks good, a few comments:

  • rebase your branch to the updated master
  • export the air_quality module in lib.rs

apis/air_quality/src/lib.rs Outdated Show resolved Hide resolved
apis/air_quality/src/lib.rs Outdated Show resolved Hide resolved
I've removed the `AirQualityListener` and used a `Cell<Option<(u32,)>>`
instead. I've added a private Enum for choosing what kind of data reading
to execute, a private method that uses this Enum and two other public
functions as wrappers.
I've added a fake `AirQuality` driver and some integration tests for it.
Every test is passed and should reflect the behaviour of the real driver.
I've added unittests for each function of the AirQuality API, all pass
as expected. Documentation for the API will be part of the next commit.
@RaresCon RaresCon marked this pull request as ready for review April 4, 2023 13:42
Copy link

@alexandruradovici alexandruradovici left a comment

Choose a reason for hiding this comment

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

Please rebase.

apis/air_quality/src/lib.rs Outdated Show resolved Hide resolved
apis/air_quality/src/lib.rs Outdated Show resolved Hide resolved
apis/air_quality/src/lib.rs Outdated Show resolved Hide resolved
apis/air_quality/src/lib.rs Outdated Show resolved Hide resolved
apis/air_quality/src/tests.rs Show resolved Hide resolved
apis/air_quality/src/tests.rs Show resolved Hide resolved
@alexandruradovici alexandruradovici added the needs-rebase This pull request needs a rebase label Apr 9, 2023
apis/air_quality/src/lib.rs Outdated Show resolved Hide resolved
Added the suggested changes, including the `read_sync` function that
return a tuple of both CO2 and TVOC values. Improved the unittests for the
fake driver and added a unittest for the new function.
Copy link

@alexandruradovici alexandruradovici left a comment

Choose a reason for hiding this comment

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

Please send this to upstream and post a link here.

@RaresCon
Copy link
Author

The PR is on upstream: tock#464

@alexandruradovici alexandruradovici added upstream This pull request was opened in the upstream reposiotry and removed needs-rebase This pull request needs a rebase labels Apr 24, 2023
RaresCon and others added 4 commits May 11, 2023 22:11
I've added back the dedicated listener for this API
and rewritten the returning errors of some functions.
I've added some documentation for the functions of the API
and fixed an inclusion bug of the API in `tests.rs` that
made the CI fail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream This pull request was opened in the upstream reposiotry
Projects
Status: Sumitted to upstream
Development

Successfully merging this pull request may close these issues.

2 participants