[riot-notifications] [RIOT-OS/RIOT] cpu/nrf5x: add driver for internal temperature sensor (#11139)

Marian Buschsieweke notifications at github.com
Fri Mar 8 12:55:58 CET 2019

maribu commented on this pull request.

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.

> +
+#include "cpu.h"
+#include "saul.h"
+#include "saul_reg.h"
+#include "phydat.h"
+void temperature_read(int16_t *temp)
+    /* Start temperature measurement task */
+    /* Wait for temperature measurement to be ready */
+    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;

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.)

> + *
+ */
+ * @ingroup     sys_auto_init_saul
+ * @{
+ *
+ * @file
+ * @brief       Auto initialization of internal temperature sensor directly mapped to SAUL reg
+ *
+ * @author      Alexandre Abadie <alexandre.abadie at inria.fr>
+ *
+ * @}
+ */

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.

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190308/7a63e0cf/attachment-0001.html>

More information about the notifications mailing list