drivers/tmp117: TMP117 driver#22310
Conversation
tests/drivers/tmp117: test for the TMP117 driver This driver add the support for the TMP117 i2c temperature sensor.
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
| tmp117_params_t params; | ||
| uint8_t is_initialized; | ||
| } tmp117_t; | ||
|
|
There was a problem hiding this comment.
Too many blank lines.
Static checks will highlight a few more.
There was a problem hiding this comment.
still more blank than needed
|
Thanks @leocordier. Could you also check if SAUL support would be easy to add? See https://docs.riot-os.org/advanced_tutorials/device_drivers/#saul |
|
I gave it another quick pass. It's getting there :-) |
| #pragma once | ||
|
|
||
| /** | ||
| * @defgroup driver_tmp117 |
There was a problem hiding this comment.
| * @defgroup driver_tmp117 | |
| * @defgroup drivers_tmp117 |
Other sensors use drivers_XXX. This needs to be adapted in multiple files.
| int tmp117_set_averaging(tmp117_t *dev, tmp117_avg_t avg) | ||
| { | ||
| int res = TMP117_NOI2C; | ||
| uint16_t value; |
There was a problem hiding this comment.
| uint16_t value; | |
| uint16_t conf_reg; |
| @@ -0,0 +1 @@ | |||
| FEATURES_REQUIRED += periph_i2c No newline at end of file | |||
| @@ -0,0 +1,2 @@ | |||
| USEMODULE_INCLUDES_tmp117 := $(LAST_MAKEFILEDIR)/include | |||
| USEMODULE_INCLUDES += $(USEMODULE_INCLUDES_tmp117) No newline at end of file | |||
| USEMODULE += ztimer | ||
| USEMODULE += ztimer_msec | ||
|
|
||
| include $(RIOTBASE)/Makefile.include No newline at end of file |
| int tmp117_set_conversion_cycle(tmp117_t *dev, tmp117_conv_cycle_t cycle) | ||
| { | ||
| int res = TMP117_NOI2C; | ||
| uint16_t value; |
There was a problem hiding this comment.
| uint16_t value; | |
| uint16_t conf_reg; |
| int tmp117_set_conversion_mode(tmp117_t *dev, tmp117_conv_mode_t mode) | ||
| { | ||
| int res = TMP117_NOI2C; | ||
| uint16_t value; |
There was a problem hiding this comment.
| uint16_t value; | |
| uint16_t conf_reg; |
|
|
||
| status = _read_16_reg(dev, TMP117_REG_DEVICE_ID, ®); | ||
|
|
||
| //checking if sensor is a tmp117 |
There was a problem hiding this comment.
@basilfx is right we do not like sane c++ single line comments we always want /* */
|
If you think your code is working as intended please test with your hardware and post:
|
| }; | ||
|
|
||
| typedef enum { | ||
| TMP117_CONV_CC = 0, /**< continous conversion mode */ |
There was a problem hiding this comment.
somehow tabs made it into this file please use 4 space for indention only
(makefiles aside)
| #define DEFAULT_CONFIG_VALUE (dev->params.conv_mode << 10 | dev->params.conv_cycle << \ | ||
| 7 | dev->params.avg << 5) |
There was a problem hiding this comment.
use your defines
#define TMP117_DATA_READY_SHIFT 13 /**< data ready bit shift in configuration register */
#define TMP117_CONV_MODE_SHIFT 10 /**< conversion mode bits shift in configuration register */
#define TMP117_CONV_CYCLE_SHIFT 7 /**< conversion cycle bits shift in configuration register */
#define TMP117_AVG_SHIFT
| #define DEFAULT_CONFIG_VALUE (dev->params.conv_mode << 10 | dev->params.conv_cycle << \ | |
| 7 | dev->params.avg << 5) | |
| #define DEFAULT_CONFIG_VALUE ( dev->params.conv_mode << TMP117_CONV_MODE_SHIFT \ | |
| | dev->params.conv_cycle << TMP117_CONV_CYCLE_SHIFT \ | |
| | dev->params.avg << TMP117_AVG_SHIFT) |
Here is the setup i use to test the TMP117 driver:
Result |
I see your test PCB has multiple of these sensors is that supported by the driver What is the i2c address translation IC?' is that LTC4316? |
kfessel
left a comment
There was a problem hiding this comment.
sorry for all the white space things but there will be a time the static test will complain again
| /** | ||
| * @brief Define the number of configured sensors | ||
| */ | ||
| #define TMP117_NUM (unsigned)(sizeof((tmp117_params)) / sizeof((tmp117_params)[0])) |
There was a problem hiding this comment.
did you include container.h
I assume it is this run
https://ci.riot-os.org/details/786c4cfa69eb4a8089a198ac2141682f
| /** | ||
| * @brief Define the number of saul info | ||
| */ | ||
| #define TMP117_INFO_NUM (unsigned)(sizeof((tmp117_saul_info)) / sizeof((tmp117_saul_info)[0])) |
| * @{ | ||
| */ | ||
| #ifndef TMP117_PARAM_I2C | ||
| # define TMP117_PARAM_I2C (I2C_DEV(0)) |
There was a problem hiding this comment.
| # define TMP117_PARAM_I2C (I2C_DEV(0)) | |
| # define TMP117_PARAM_I2C (I2C_DEV(0)) |
| # define TMP117_PARAM_I2C (I2C_DEV(0)) | ||
| #endif | ||
| #ifndef TMP117_PARAM_ADDR | ||
| # define TMP117_PARAM_ADDR (0x48) |
There was a problem hiding this comment.
| # define TMP117_PARAM_ADDR (0x48) | |
| # define TMP117_PARAM_ADDR (0x48) |
| #ifndef TMP117_PARAMS | ||
| # define TMP117_PARAMS { .i2c = TMP117_PARAM_I2C, \ | ||
| .addr = TMP117_PARAM_ADDR, \ | ||
| .conv_mode = TMP117_PARAM_CONV_MODE, \ | ||
| .conv_cycle = TMP117_PARAM_CONV_CYCLE, \ | ||
| .avg = TMP117_PARAM_AVG } | ||
| #endif |
There was a problem hiding this comment.
| #ifndef TMP117_PARAMS | |
| # define TMP117_PARAMS { .i2c = TMP117_PARAM_I2C, \ | |
| .addr = TMP117_PARAM_ADDR, \ | |
| .conv_mode = TMP117_PARAM_CONV_MODE, \ | |
| .conv_cycle = TMP117_PARAM_CONV_CYCLE, \ | |
| .avg = TMP117_PARAM_AVG } | |
| #endif | |
| #ifndef TMP117_PARAMS | |
| # define TMP117_PARAMS { .i2c = TMP117_PARAM_I2C, \ | |
| .addr = TMP117_PARAM_ADDR, \ | |
| .conv_mode = TMP117_PARAM_CONV_MODE, \ | |
| .conv_cycle = TMP117_PARAM_CONV_CYCLE, \ | |
| .avg = TMP117_PARAM_AVG } | |
| #endif |
|
|
||
| conf_reg &= ~TMP117_AVG_MASK; | ||
| conf_reg |= avg << TMP117_AVG_SHIFT; | ||
|
|
| DEBUG("[tmp117] set averaging - error: unable to read conf reg \n"); | ||
| goto release; | ||
| } | ||
|
|
|
|
||
|
|
|
|
||
| #include "tmp117.h" | ||
| #include "tmp117_params.h" | ||
|
|
|
|
||
| ztimer_sleep(ZTIMER_MSEC, 1500); | ||
| } | ||
|
|
Hello, And yes the i2c address shifting IC is a LTCLTC4316. |
Is there a tool to automatically delete those blank lines and apply the appropriate spacing? |
I usually have my editor configured to remove trailing whitespaces. For example VS Code can do this. I also have a plugin to highlight them. |
I just removed them by hand, it should be good now |
crasbe
left a comment
There was a problem hiding this comment.
Also a lot of other review comments are still open.
| * conversion cycle : 1s | ||
| * averaging : 8 samples | ||
|
|
||
| After the initialization, the temperature value is displayed every 1s5. |
There was a problem hiding this comment.
| After the initialization, the temperature value is displayed every 1s5. | |
| After the initialization, the temperature value is displayed every 1.5 seconds. |
| * conversion mode : continuous | ||
| * conversion cycle : 1s | ||
| * averaging : 8 samples |
There was a problem hiding this comment.
| * conversion mode : continuous | |
| * conversion cycle : 1s | |
| * averaging : 8 samples | |
| * conversion mode: continuous | |
| * conversion cycle: 1s | |
| * averaging: 8 samples |
| puts("Initializing tmp117 sensor ..."); | ||
|
|
||
| if (tmp117_init(&tmp117, tmp117_params) != TMP117_OK) { | ||
| return -1; |
There was a problem hiding this comment.
| return -1; | |
| puts(" failed.\n"); | |
| return -1; |
|
|
||
| while (1) { | ||
| if (tmp117_read_temperature(&tmp117, &raw_temp) == TMP117_OK) { | ||
| printf("temperature : %02d.%02d°C\n", raw_temp / 100, raw_temp % 100); |
There was a problem hiding this comment.
| printf("temperature : %02d.%02d°C\n", raw_temp / 100, raw_temp % 100); | |
| printf("Temperature: %02d.%02d°C\n", raw_temp / 100, raw_temp % 100); |
| printf("temperature : %02d.%02d°C\n", raw_temp / 100, raw_temp % 100); | ||
| } | ||
| else { | ||
| puts("[tmp117 test] error : unable to read temperature\n"); |
There was a problem hiding this comment.
| puts("[tmp117 test] error : unable to read temperature\n"); | |
| puts("ERROR: unable to read temperature\n"); |
I think in this case it's clear which test is running.
| * @param[in, out] dev device descriptor | ||
| * @param[out] value read value in centi-celsius | ||
| * | ||
| * @return TMP117_OK on success | ||
| * @return TMP117_I2C if other error occurs |
There was a problem hiding this comment.
| * @param[in, out] dev device descriptor | |
| * @param[out] value read value in centi-celsius | |
| * | |
| * @return TMP117_OK on success | |
| * @return TMP117_I2C if other error occurs | |
| * @param[in,out] dev device descriptor | |
| * @param[out] value read value in centi-celsius | |
| * | |
| * @retval TMP117_OK on success | |
| * @retval TMP117_I2C if other error occurs |
| tmp117_conv_mode_t conv_mode; /**< conversion mode */ | ||
| tmp117_conv_cycle_t conv_cycle; /**< conversion cycle time */ | ||
| tmp117_avg_t avg; /**< data averaging*/ | ||
| }tmp117_params_t; |
There was a problem hiding this comment.
| }tmp117_params_t; | |
| } tmp117_params_t; |
| assert(TMP117_NUM == TMP117_INFO_NUM); | ||
|
|
||
| for (unsigned i = 0; i < TMP117_NUM; i++) { | ||
| LOG_DEBUG("[auto_init_saul] initializing tmp117 #%u\n", i); |
There was a problem hiding this comment.
| LOG_DEBUG("[auto_init_saul] initializing tmp117 #%u\n", i); | |
| LOG_DEBUG("[auto_init_saul] initializing TMP117 #%u\n", i); |
| LOG_DEBUG("[auto_init_saul] initializing tmp117 #%u\n", i); | ||
|
|
||
| if (tmp117_init(&tmp117_devs[i], &tmp117_params[i]) != TMP117_OK) { | ||
| LOG_ERROR("[auto_init_saul] error initializing tmp117 #%u\n", i); |
There was a problem hiding this comment.
| LOG_ERROR("[auto_init_saul] error initializing tmp117 #%u\n", i); | |
| LOG_ERROR("[auto_init_saul] error initializing TMP117 #%u\n", i); |



Contribution description
This pull request add a basic driver for the TMP117 sensor with the following capabilities:
This pull request add a driver in the
driversfolder and a test in thetest/driversfolderTesting procedure
The basic test that i wrote, initialize the sensor with default paramaters and i2c address.
After the initialization, it display the measured temperature every 1.5 seconds.
If the initialization or the temperature reading fail, it displays anb error.
Issues/PRs references
issue #22308
Declaration of AI-Tools / LLMs usage:
AI-Tools / LLMs that were used are: