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

samples: sensor: update dht_polling to new API #72176

Merged

Conversation

laurenmurphyx64
Copy link
Contributor

@laurenmurphyx64 laurenmurphyx64 commented Apr 30, 2024

Updates dht_polling sample to new sensor API.

Depends on #71093 (merged) and #71160 (still waiting) (commits are included in this PR, so this is DNM until both PRs are merged and I can drop the commits)

@laurenmurphyx64 laurenmurphyx64 added the DNM This PR should not be merged (Do Not Merge) label May 3, 2024
@zephyrbot zephyrbot requested review from lixuzha and qianruh May 3, 2024 20:37
@ubieda ubieda self-requested a review May 6, 2024 14:05
@laurenmurphyx64
Copy link
Contributor Author

laurenmurphyx64 commented May 28, 2024

Rebased with updates to #71160 and dropping #71093 commit as it has been merged. Ready for review.

dev->name, temp.val1, temp.val2 / 10000,
hum.val1, hum.val2 / 10000);
uint32_t temp_fit = 0;
uint8_t temp_buf[64];
Copy link
Member

Choose a reason for hiding this comment

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

This seems too large for a single sample? Same question about hum_buf[64] below.

Copy link
Member

Choose a reason for hiding this comment

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

@teburd @laurenmurphyx64 Is there a scenario where temp_buf and hum_buf won't be struct sensor_q31_data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to use sensor_q31_data struct instead of a buffer, take a look

ubieda
ubieda previously approved these changes May 30, 2024
Copy link
Member

@ubieda ubieda left a comment

Choose a reason for hiding this comment

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

PR Looks good, assuming rebasing it's done.
I've tried it on my end (on a single instance DHT sensor) and it works as expected:

*** Booting Zephyr OS build v3.6.0-4878-g1c496f6b97f3 ***
       hts221@5f: temp is 24.87 °C humidity is 44.49 RH
       hts221@5f: temp is 25.12 °C humidity is 43.99 RH
       hts221@5f: temp is 25.12 °C humidity is 43.99 RH
       hts221@5f: temp is 25.24 °C humidity is 43.99 RH
       hts221@5f: temp is 25.24 °C humidity is 43.99 RH
       hts221@5f: temp is 25.24 °C humidity is 43.99 RH
       hts221@5f: temp is 25.37 °C humidity is 43.99 RH
       hts221@5f: temp is 25.37 °C humidity is 43.49 RH
       hts221@5f: temp is 25.37 °C humidity is 43.49 RH
       hts221@5f: temp is 25.49 °C humidity is 43.49 RH
       hts221@5f: temp is 25.49 °C humidity is 43.49 RH
       hts221@5f: temp is 25.49 °C humidity is 43.49 RH
       hts221@5f: temp is 25.49 °C humidity is 43.49 RH
       hts221@5f: temp is 25.62 °C humidity is 43.49 RH
       hts221@5f: temp is 25.62 °C humidity is 43.49 RH
       hts221@5f: temp is 25.74 °C humidity is 43.49 RH

samples/sensor/dht_polling/src/main.c Show resolved Hide resolved
@teburd
Copy link
Collaborator

teburd commented May 30, 2024

@laurenmurphyx64 Please rebase on main when you can, the sample changes look good. The buffer size concerns @MaureenHelm has should be addressed with when both #71235 and #73031 are addressed.

The buffer is large because bme280's compensation parameters are included in the buffer today and that's what @laurenmurphyx64 was testing with. We need to resolve #73031 in order to fix this part.

The buffer is large as well because we don't have a way of deciding, at compile time, how big the buffer should be without the metadata #71235 would presumably provide which @yperess is working on.

@laurenmurphyx64
Copy link
Contributor Author

@laurenmurphyx64 Please rebase on main when you can, the sample changes look good. The buffer size concerns @MaureenHelm has should be addressed with when both #71235 and #73031 are addressed.

Rebased, will standby.

Updates dht_polling sample to new sensor API.

Signed-off-by: Lauren Murphy <[email protected]>
Copy link
Collaborator

@teburd teburd left a comment

Choose a reason for hiding this comment

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

The remaining task of updating the read buffer size can be done later IMO once we've sorted out how to obtain the metadata needed, which we are working on in #71235

@laurenmurphyx64 laurenmurphyx64 removed the DNM This PR should not be merged (Do Not Merge) label Jun 5, 2024
@ubieda ubieda requested a review from MaureenHelm June 6, 2024 19:07
@nashif nashif merged commit 9ac6339 into zephyrproject-rtos:main Jun 11, 2024
23 of 24 checks passed
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.

6 participants