drivers/ds18: Add Maxim Integrated 1-Wire temperature sensor driver #10011

leandrolanzieri · 2018-09-21T12:39:01Z

Contribution description

This PR is based on #6512. It adds the driver for the 1-Wire temperature sensors DS18 and SAUL interface.

What this PR adds:

Fixes the temperature reading problem reported in drivers/ds18: Add Maxim Integrated ds18 driver #6512.
Fixes some type errors on the API.
Adds a timeout to the ds18_read_bit function.
Test application for the driver.

Testing procedure

Connect the sensor to a free GPIO on the board.
Define DS18_PARAM_PIN and DS18_PARAM_PULL accordingly to your board.
Run the test application with something like: BOARD=sensebox_samd21 make flash term -C tests/driver_ds18.
You should see an output similar to:

# DS18B20 test application
# 
# +------------Initializing------------+
# 
# +--------Starting Measurements--------+
# Temperature [ºC]: 25.87
# +-------------------------------------+

Issues/PRs references

Based on #6512.

EDIT
Some measurements were performed on different platforms. Due to the need of timing accuracy for the implementation of the 1-wire protocol a whitelist of boards has been added to the test application. These boards have been tested and are known to work with the current implementation of the driver.

Measurements and captures can be found here: time_measurements_2.zip

@danpetry Reported the following time measurements (he took 3 samples of '1' and 3 of '0'):

Sensebox
- read 1: 3.685 us x3
- read 0: 29.187 us x3
Samr21-xpro
- read 1: 3.312 us x 2, 3.25 us x 1
- read 0: 28.5 us x 3
Nucleo L152RE
- read 1: 5.875 us x 2, 5.9375 us x 1
- read 0: 28.5 us x 3

leandrolanzieri · 2018-09-24T20:11:04Z

@aabadie Do you still have access to the hardware to test this?

aabadie · 2018-09-25T07:55:27Z

Do you still have access to the hardware to test this?

Normally yes but I have to find it in my mess :)

aabadie · 2018-09-25T09:11:33Z

@leandrolanzieri, I found it but I still can't make the sensor to give a valid temperature (tested with different DS18_PARAM_PULL values PU/PD) but it always display the following (with debug enabled):

2018-09-25 11:08:08,323 - INFO # [DS18] Reset and read scratchpad
2018-09-25 11:08:08,325 - INFO # [DS18] Received byte: 0xff
2018-09-25 11:08:08,326 - INFO # [DS18] Received byte: 0xff
2018-09-25 11:08:08,326 - INFO # Temperature [ºC]: 163.77

I tested with an Arduino Zero and a Nucleo L073. I setup the pin correctly for both boards. Maybe my sensor is broken ? I only have this one so I cannot test with another one to check this.

leandrolanzieri · 2018-09-25T09:27:20Z

@aabadie thanks a lot for testing!
On the lobaro_lorabox board I found that the problem was that DS18_SAMPLE_TIME was too big and the bits were always read as '1', so I reduced that and solved it. I don't know if this is caused by a not accurate time measurement.
I will try to get a sensor so I can perform further tests.

danpetry · 2018-09-27T13:49:39Z

Partial ACK: on the fundamentals.
PR reasoning and testing instructions are clearly laid out and I can't see any reason this breaks from current concepts. Rights of current PR authors are respected in commit history.

haukepetersen

Nice one, have been waiting for this driver a long time :-). Found some issues that I see, I think all of them should be quite easy to fix.

The reason I had some concrete code snippets at hand is, that I also implemented this driver some time (~2 years) ago, but never got to quite finish it. But maybe there is more useful stuff for you in my branch: https://github.com/haukepetersen/RIOT/tree/add_driver_ds18b20

But for now, I'd say lets go with the driver as is (only the small fixes left), and once its merged we could think about things like factoring out the 1wire driver etc...

haukepetersen · 2018-09-27T16:10:19Z

drivers/ds18/Makefile

@@ -0,0 +1,2 @@
+MODULE = ds18


no need for this line

haukepetersen · 2018-09-27T16:11:30Z

drivers/ds18/ds18.c

+
+static void ds18_release(ds18_t *dev)
+{
+    /* Init pin as input and enable pull-up */


comment doesnt fit to code anymore, 'pull-up' is not always ture, depending on dev->in_mode...

haukepetersen · 2018-09-27T16:24:22Z

drivers/ds18/ds18_saul.c

+    ds18_t *d = (ds18_t *)dev;
+
+    ds18_get_temperature(d, &temperature);
+    res->val[0] = temperature;


how about:

ds18_get_temperature((ds18_t *)dev, &res->val[0]);

would reduce the code above into a single line.

But aside from that: it would be good to also check the return value of read_temperature and return a -ECANCELED in case of an error.

You are right, much better.

haukepetersen · 2018-09-27T16:27:10Z

drivers/ds18/ds18.c

+
+    /* Measure time low of device pin, timeout after slot time*/
+    start = xtimer_now_usec();
+    while (!gpio_read(dev->pin) && measurement < DS18_DELAY_SLOT) {


this seems to be wasting resources by by reading the pin in a loop. As the pulse widths are specified, it would be more efficient to sample the pin state once in the middle of the corresponding pulse. The code I wrote for this (a long time ago) did something like this:

gpio_clear(owi->pin); xtimer_usleep(T_RW_PULSE); gpio_init(owi->pin, owi->imode); xtimer_usleep(T_R_SAMPLE); in = gpio_read(owi->pin); xtimer_usleep(T_R_RECOVER); gpio_init(owi->pin, owi->omode); gpio_set(owi->pin);

with

#define T_RW_PULSE (3) #define T_W_HOLD (57) #define T_W_RECOVER (10) #define T_R_SAMPLE (7) #define T_R_RECOVER (55)

also this way, all this timeout stuff is not needed anymore...

I am having trouble with this one. There seems to be a problem with the xtimer on the board I am using (which has the sensor). I also did some tests on some nucleo boards and raised an issue here #10073.
I am not getting the needed time accuracy so I did not change the reading function just yet, as I am not able to test it right now. I already ordered a sensor so I can test it with different boards.
Any ideas to approach this problem?

True, more-or-less precise timings are quite mandatory for using my proposed code block...

So until we get the timer imprecision sorted out, how about we go with a dual approach? We could include both code blocks in the code, #if - #elseing them based on a sub(pseudo)module, making my variant optional. This way people can use the more efficient code once they made sure their platform is handle the needed timings...

Of course we would need to document this in the module description :-)

haukepetersen · 2018-09-27T16:42:46Z

drivers/ds18/ds18.c

+    }
+
+    DEBUG("[DS18] Convert T\n");
+    ds18_write_byte(dev, DS18_CMD_SKIPROM);


maybe put a comment in that this command triggers a conversion on all devices connected to the bus...

haukepetersen · 2018-09-27T16:47:00Z

drivers/ds18/ds18.c

+
+    /* Fixed point value to uint16_t */
+    uint16_t degrees  = (b2 << 4) + (b1 >> 4);
+    uint16_t mdegrees = (b1 & 0xF) * 6.25;


Seeing a floating point number (6.25) in the code makes my head hurt :-) In the worst case, this pulls in ~5K of ROM or soft floating point library into the code - I guess this is not wanted here, right?

My proposal for the conversion:

int32_t res = ((int32_t)(b2 << 8 | b1) * 625); *temperature = (int16_t)(res / 10);

The trick is to use a larger type for intermediate conversion by basically shifting the position of the decimal point temporarily.

Thanks, changed this.

haukepetersen · 2018-09-27T16:49:05Z

drivers/ds18/include/ds18_internal.h

@@ -0,0 +1,64 @@
+/*


I would move this file to drivers/ds18/ds18_internal.h. This way it is only accessible from this driver and pulled out of the global include path...

haukepetersen · 2018-09-27T16:52:22Z

drivers/include/ds18.h

+ */
+typedef struct {
+    gpio_t pin;             /**< Pin the sensor is connected to */
+    gpio_mode_t in_mode;    /**< Pin output mode */


What about the output mode?! I think it makes much sense to specify it as well. Or maybe even only specify the output mode in the configuration, and deduct the input mode from it:

// in the _init() function: dev->in_mode = (params->out_mode == GPIO_OD_PU) ? GPIO_IN_PU : GPIO_IN;

rationale behind this is, that CPUs that allow for GPIO_OD_PU have pull-up resistors implemented. These are then also usable for input mode...

Makes sense, changed the param struct to have out_mode

haukepetersen · 2018-09-27T16:55:01Z

drivers/include/ds18.h

+ * @return                   0 on success
+ * @return                  -1 on error
+ */
+int ds18_get_temperature(ds18_t *dev, int16_t *temperature);


I would prefer to split this function into 3 separate ones, something like:

int ds18_trigger(dev); // trigger a new conversion int ds18_read(dev, temp); // read the results of the last conversion int ds18_get_temp(dev, temp); // convenience function for trigger->wait->read

Splitted the function

haukepetersen · 2018-09-27T17:03:22Z

drivers/include/ds18.h

+ * @ingroup     drivers_saul
+ * @brief       Driver interface for the DS18 temperature sensors
+ *
+ * This driver provides @ref drivers_saul capabilities.


Would you please also add some more documentation about the supported (or not supported) features and limitations of this driver?!

E.g.

does not allow for addressing devices -> only supports a single device on the bus

hardcodes the 1wire bus functionality into the driver

does not allow for configuration of the sampling width

leandrolanzieri · 2018-10-15T09:10:15Z

I added a submodule ds18_optimized to change de read function. Also rebased.

haukepetersen

few more findings, once fixed I am happy :-)

haukepetersen · 2018-10-17T09:02:40Z

drivers/ds18/ds18.c

+{
+    int res;
+
+    res = ds18_reset(dev);


please remove this here, it is done already in ds18_trigger()

haukepetersen · 2018-10-17T09:06:35Z

drivers/ds18/ds18.c

+
+#if defined(MODULE_DS18_OPTIMIZED)
+    DEBUG("[DS18] Using optimized read function");
+#endif


please remove this block, I think it is rather for internal debugging...

haukepetersen · 2018-10-17T09:06:54Z

drivers/ds18/ds18_internal.h

+#define DS18_DELAY_R_RECOVER        (DS18_DELAY_SLOT - DS18_SAMPLE_TIME)
+/** @} */
+
+


remove newline

haukepetersen · 2018-10-17T09:09:42Z

drivers/include/ds18.h

+/**
+ * @brief   convenience fuction for triggering a conversion and reading the
+ * value
+ *


here should also be a note or description that this function will block for an amount of time (don't remember the value...).

I added a note on that.

haukepetersen · 2018-10-17T09:10:53Z

sys/auto_init/auto_init.c

@@ -437,6 +437,42 @@ void auto_init(void)
    extern void auto_init_veml6070(void);
    auto_init_veml6070();
 #endif
+#ifdef MODULE_DS18


seems like something went wrong when you rebased, most of these entries are unrelated to this PR, right?!

Oops, I screwed up yep :(

haukepetersen

ACK from my side for concept, code, and documentation.

I did however NOT test the code nor did I check for coding style... So @danpetry would you mind?!

danpetry · 2018-10-18T08:51:00Z

Yes I'll do that tomorrow.

danpetry

A few small nits, just putting them down now in case I forget on monday :)

danpetry · 2018-10-19T14:52:00Z

drivers/ds18/ds18.c

+
+int ds18_get_temperature(ds18_t *dev, int16_t *temperature)
+{
+    int res;


unused variable. Weren't you getting compile errors?

danpetry · 2018-10-19T15:25:00Z

drivers/ds18/ds18_internal.h

@@ -0,0 +1,64 @@
+/*


This file should go in the /internal folder in the driver

I haven't seen an internal folder in the common drivers structure

Sorry, /include

Here #10011 (comment) @haukepetersen asked to move it out of the folder so it is not in the global path.

danpetry · 2018-10-19T16:23:36Z

drivers/ds18/ds18.c

+    start = xtimer_now_usec();
+    while (!gpio_read(dev->pin) && measurement < DS18_DELAY_SLOT) {
+        measurement = xtimer_now_usec() - start;
+    };


please remove semicolon

leandrolanzieri · 2018-10-23T13:30:47Z

After @danpetry reported failures in readings with sensebox_samd21 board I performed some measurements, on both the sensebox_samd21 and samr21_xpro boards. Here are the captures together with a summary of the measurements: time_measurements.zip

I found that the delayed introduced by the call to xtimer_usleep at the beginning of the read and write slots was causing some trouble, so I decided to remove it. The delay in the gpio toggle is enough to start a slot.

EDIT:
Also tested on nucleo-l152re and works.
Updated measurements (i.e. including nucleo-l152re) can be found in PR description

tests/driver_ds18: Add test application for DS18B20 sensor. tests/driver_ds18: Add whitelist of boards

danpetry · 2018-10-25T15:24:41Z

Tested on all three whitelisted boards

danpetry self-requested a review September 27, 2018 13:33

danpetry added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers labels Sep 27, 2018

haukepetersen requested changes Sep 27, 2018

View reviewed changes

leandrolanzieri mentioned this pull request Sep 28, 2018

xtimer_usleep wrong delay time #10073

Closed

leandrolanzieri force-pushed the pr/drivers/ds18 branch from 0ac1887 to f8f3b1a Compare October 4, 2018 08:21

leandrolanzieri force-pushed the pr/drivers/ds18 branch from 17bac9b to a612d6d Compare October 15, 2018 09:09

haukepetersen requested changes Oct 17, 2018

View reviewed changes

leandrolanzieri force-pushed the pr/drivers/ds18 branch 2 times, most recently from 91840bd to f185003 Compare October 17, 2018 10:08

haukepetersen approved these changes Oct 18, 2018

View reviewed changes

danpetry suggested changes Oct 19, 2018

View reviewed changes

danpetry added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 24, 2018

danpetry mentioned this pull request Oct 24, 2018

1-wire driver #10236

Closed

drivers/ds18: Add Maxim Integrated ds18 driver

8b8790c

tests/driver_ds18: Add test application for DS18B20 sensor. tests/driver_ds18: Add whitelist of boards

leandrolanzieri force-pushed the pr/drivers/ds18 branch from 4608e54 to 8b8790c Compare October 24, 2018 14:09

danpetry added 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 labels Oct 25, 2018

danpetry approved these changes Oct 25, 2018

View reviewed changes

danpetry merged commit 99ae495 into RIOT-OS:master Oct 25, 2018

danpetry mentioned this pull request Apr 4, 2019

tests/drivers_ds18: add tested boards to the board whitelist #11338

Merged

aabadie mentioned this pull request Dec 11, 2019

drivers: fix alphabetical order in Makefile.dep/include files #12925

Merged

		#define DS18_DELAY_R_RECOVER (DS18_DELAY_SLOT - DS18_SAMPLE_TIME)
		/** @} */

drivers/ds18: Add Maxim Integrated 1-Wire temperature sensor driver #10011

drivers/ds18: Add Maxim Integrated 1-Wire temperature sensor driver #10011

Uh oh!

Conversation

leandrolanzieri commented Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

leandrolanzieri commented Sep 24, 2018

Uh oh!

aabadie commented Sep 25, 2018

Uh oh!

aabadie commented Sep 25, 2018

Uh oh!

leandrolanzieri commented Sep 25, 2018

Uh oh!

danpetry commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haukepetersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri commented Oct 15, 2018

Uh oh!

haukepetersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

leandrolanzieri commented Sep 21, 2018 •

edited

Loading

danpetry commented Sep 27, 2018 •

edited

Loading