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.

Sign up for GitHub

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

Jump to bottom

cpu/nrf5x: add driver for internal temperature sensor #11139

Merged
merged 3 commits into from
Mar 10, 2019
Merged

cpu/nrf5x: add driver for internal temperature sensor #11139

merged 3 commits into from
Mar 10, 2019

Conversation

aabadie
Copy link
Contributor

@aabadie aabadie commented Mar 8, 2019

Contribution description

The Nordic MCUs provide an internal temperature sensor. This PR is providing a basic implementation for it and wraps it in the saul API.

The driver is enabled for all nrf (51 and 52) boards.

I'm not sure of the design approach of this PR, so review comments to improve are welcome.

Testing procedure

Build and flash examples/saul on any nrf based board, and read the temperature.

Issues/PRs references

None

@aabadie aabadie added Platform: ARM Platform: This PR/issue effects ARM-based platforms 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 Area: cpu Area: CPU/MCU ports labels Mar 8, 2019
@aabadie aabadie requested a review from haukepetersen March 8, 2019 08:31
@aabadie aabadie force-pushed the pr/cpu/nrf5x_temperature branch from 0bfd51a to b2a4028 Compare March 8, 2019 09:08
maribu
maribu reviewed Mar 8, 2019
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

OK, looks good to me. I'll test later. I was first confused with the name saul_temperature and thought it should be a pseudo module to enable all present temperature senors. I'd think you should better rename that to include nrf5x in the name somewhere.

cpu/nrf5x_common/periph/temperature.c
while (!NRF_TEMP->EVENTS_DATARDY); /* takes 36us according to manual */

/* temperature is in 0.25°C step, so just divide by 4 */
*temp = (int16_t)NRF_TEMP->TEMP >> 2;
Copy link
Member

Choose a reason for hiding this comment

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

I'd guess the last one or two bits of this make a good entropy source for random generators. (<-- Totally unrelated to this PR, I'm just curious.)

sys/auto_init/saul/auto_init_temperature.c Outdated
* @}
*/

#ifdef MODULE_SAUL_TEMPERATURE
Copy link
Member

Choose a reason for hiding this comment

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

I think it is unlikely that other temperature sensors can share this code for auto init, as e.g. all external sensors need to be configured (at least the interface needs to be specified). Also, other auto init codes allows to add multiple sensors (e.g. when interfacing with an array of DHT11 sensors), which makes no sense for this driver - there will never be more than one CPU internal temp sensor on a board. I really would rename this to include nrf5x somewhere.

@aabadie
Copy link
Contributor Author

aabadie commented Mar 8, 2019

I'd think you should better rename that to include nrf5x in the name somewhere.

I agree with this proposition and will update. What about saul_nrf_temperature ?

@aabadie aabadie force-pushed the pr/cpu/nrf5x_temperature branch from b2a4028 to eee75b3 Compare March 8, 2019 17:48
@aabadie
Copy link
Contributor Author

aabadie commented Mar 8, 2019

@maribu, I renamed the saul function to include nrf. Tested again on nrf51dk/nrf52dk, still works.

@maribu maribu added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Mar 8, 2019
@maribu
Copy link
Member

maribu commented Mar 8, 2019

I'll test on Monday. Feel free to squash

@aabadie aabadie force-pushed the pr/cpu/nrf5x_temperature branch from eee75b3 to f390a8d Compare March 8, 2019 20:14
@maribu maribu added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Mar 10, 2019
maribu
maribu approved these changes Mar 10, 2019
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Works on a nrf52840dk. Let's rerun Murdock and then hit merge.

@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 10, 2019
@maribu maribu merged commit 0f587e0 into RIOT-OS:master Mar 10, 2019
@aabadie
Copy link
Contributor Author

aabadie commented Mar 11, 2019

Thanks @maribu !

@maribu
Copy link
Member

maribu commented Mar 11, 2019

No problem. Thanks for the contribution!

@danpetry danpetry added this to the Release 2019.04 milestone Mar 11, 2019
@bergzand bergzand mentioned this pull request Mar 12, 2019
Merged
@aabadie aabadie deleted the pr/cpu/nrf5x_temperature branch July 4, 2019 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu approved these changes

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

Assignees
No one assigned
Labels
Area: cpu Area: CPU/MCU ports 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 Platform: ARM Platform: This PR/issue effects ARM-based platforms Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was 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 Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines
Projects
None yet
Milestone
Release 2019.04
Development

Successfully merging this pull request may close these issues.
3 participants
@aabadie @maribu @danpetry