Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

driver/tmp00x: make tmp006 more generic #12022

Merged
merged 1 commit into from
Aug 23, 2019

Conversation

JannesVolkens
Copy link
Contributor

Contribution description

The driver for the TMP006 Infrared Thermopile Sensor has been modified to work for the TMP006 and TMP007. The driver was tested with the CC2650-launchpad

Testing procedure

The test under tests/driver_tmp00x can be performed normally for the TMP006 sensor. The following parameter must be set for the TMP007 sensor: DRIVER=tmp007

@PeterKietzmann PeterKietzmann added Area: drivers Area: Device drivers Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Aug 19, 2019
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Generally looks pretty good. There are a few things to improve. I think we can also work on splitting the commits up a little bit better.

Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Much better. Just the #define location (from .c to .h) then I will move on to testing!

@JannesVolkens JannesVolkens force-pushed the tmp00x branch 2 times, most recently from 160d64e to 39bcdae Compare August 21, 2019 09:31
@MrKevinWeiss MrKevinWeiss added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 21, 2019
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Last things

Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

I was having a bit of trouble yesterday even getting this thing up and running (on master too) and ran out of time. I will have to pick the testing up a bit later.

@@ -88,30 +89,67 @@ extern "C"
{
#endif

#define TMP00X_CONFIG_RST (1 << 15) /**< Reset register */
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, a lot of this should be in the tmp00x_regs.h not here. This is only to be exposed to the user of the driver. They probably shouldn't have to care about the shift register and masks.

Rename TMP006 to TMP00x
Add TMP007 sensor support to TMP00X
Change uint8_t reg to uint16_t
Add to doxygen documentation group
Expose compile time configurations
Move defines from .c to .h
Change double to float, because double is not needed
Add TMP007 register information
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Witnessed testing. It seems fine, seems like a improvement in maintainability as well.
ACK

@MrKevinWeiss MrKevinWeiss merged commit 1b1c902 into RIOT-OS:master Aug 23, 2019
@MrKevinWeiss
Copy link
Contributor

Congrats on your first PR!

# for free to join this conversation on GitHub. Already have an account? # 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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants