ADT7310 SPI temperature sensor driver #2980
Conversation
jnohlgard
added
Type: new feature
jnohlgard
force-pushed
the
pr/adt7310-initial
branch
from
ec725df
to
e28c508
Compare
|
In case you don't want to implement interrupt mode etc. as part of this PR you could document this missing feature in the API.. Compare http://riot-os.org/api/group__driver__isl29125.html#details |
|
@LudwigOrtmann Thank you for your suggestion. I have updated the PR with a little more documentation for the main page. |
jnohlgard
added
the
CI: needs squashing
|
I fixed the cppcheck warning and removed the extra whitespaces that showed up in Travis. Waiting for a new Travis run. |
| int adt7310_init(adt7310_t *dev, spi_t spi, gpio_t cs); | ||
|
|
||
| /** | ||
| * @brief Reset the TMP006 sensor. After that, the sensor should be reinitialized. |
There was a problem hiding this comment.
TMP006 -> ADT7310 I guess.
|
@LudwigOrtmann Addressed comments |
| * | ||
| * @param[out] dev device descriptor of sensor to initialize | ||
| * @param[in] spi SPI bus the sensor is connected to | ||
| * @param[in] cs GPIO pin which the chip select signal is connected to |
There was a problem hiding this comment.
is "which" correct here?
|
Looks valid to me at first sight. |
| * | ||
| * @return floating point representation of temperature in degrees Celsius | ||
| */ | ||
| float adt7310_convert(int16_t raw); |
There was a problem hiding this comment.
just a suggestion: how about changing the interface to
int16_t adt7310_read_raw(dev); // return raw value
int16_t adt7310_read(dev); // return value in m°C
and
float adt7310_read_float(dev); // return value in °CThe error conditions you could either omit (just return 0 or something) or return INT16_MIN or similar. Opinions on this?
There was a problem hiding this comment.
good idea, however it will have to be int32_t adt7310_read(dev); in order to represent temperatures above 32.767 C
|
I updated the code but I think I might have broke my sensor or some wire, every SPI transfer comes back with 0xff.. I'll post back when I have tested this. |
|
Very nice. Code looks good to me. The only thing I am not sure if I like it, is the external initialization of the SPI port (I know this in general is subject to ongoing discussion). IMHO this (i) breaks the consistency with all other existing drivers and (ii) does make it harder to use a device. Actually what I like to prevent is the situation, that you have to look into the initialization code/doc everytime before you use a driver to find out which of the used peripherals you have to initialize beforehand... Also in the end, the driver knows best, which parameters to use (e.g. speed, etc.). |
|
+1 for moving spi initialization into the driver. |
|
@gebart: how about moving the initialization into the driver for know and we give the general discussion a push to decide how we handle this situation in general for the future? |
|
@haukepetersen I can live with that for the time being until a consensus is reached in #2528 |
|
I can if you document it properly for the ACK when this is done. |
|
@gebart: I don't really understand my own last comment anymore... Where were we at with this PR? If you could either add documentation to the init function, that the SPI bus needs to be pre-initialized or if you could move the SPI init into the driver init, you can squash and we can merge this. |
jnohlgard
force-pushed
the
pr/adt7310-initial
branch
from
6f0a417
to
1caed0b
Compare
jnohlgard
force-pushed
the
pr/adt7310-initial
branch
2 times, most recently
from
c7c4849
to
2c5e673
Compare
jnohlgard
added
CI: ready for build
Tested on real hardware on my side. Waiting for Travis. |
jnohlgard
force-pushed
the
pr/adt7310-initial
branch
from
4daaf27
to
17f9dcc
Compare
This is a driver for Analog Devices ADT7310 +/-0.5°C Accurate, 16-Bit Digital SPI Temperature Sensor. Only basic reading is supported. Interrupt and compare are not implemented yet. See: http://www.analog.com/en/products/analog-to-digital-converters/integrated-special-purpose-converters/integrated-temperature-sensors/adt7310.html
jnohlgard
force-pushed
the
pr/adt7310-initial
branch
from
17f9dcc
to
fb90e67
Compare
|
pre-ACKed, and go =) |
ADT7310 SPI temperature sensor driver
drivers/adt7310: Add ADT7310 temperature sensor driver
This is a driver for Analog Devices ADT7310 +/-0.5°C Accurate, 16-Bit Digital SPI Temperature Sensor.
Only basic reading is supported. Interrupt and compare are not implemented yet.
See: http://www.analog.com/en/products/analog-to-digital-converters/integrated-special-purpose-converters/integrated-temperature-sensors/adt7310.html
Tested on mulle