Skip to content

drivers/ds18: Add Maxim Integrated ds18 driver#6512

Closed
phectori wants to merge 2 commits intoRIOT-OS:masterfrom
phectori:ds18x
Closed

drivers/ds18: Add Maxim Integrated ds18 driver#6512
phectori wants to merge 2 commits intoRIOT-OS:masterfrom
phectori:ds18x

Conversation

@phectori
Copy link
Copy Markdown
Contributor

@phectori phectori commented Jan 28, 2017

This patch adds the driver for the Maxim Integrated ds1822 and ds18b20 temperature sensors. These sensors use one-wire as communication protocol and can be powered using the data-line.

Currently I have only tested the ds1822, but according to the datasheet they should be software compatible.

The driver is SAUL compatible.

@aabadie aabadie self-assigned this Jan 28, 2017
@aabadie aabadie added the Area: drivers Area: Device drivers label Jan 28, 2017
@aabadie aabadie added this to the Release 2017.04 milestone Jan 28, 2017
aabadie
aabadie requested changes Feb 3, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Changes looks good in general. I just have a few minor comments.
Will test on the sensor I have.

Thanks for proposing this PR btw :)

drivers/ds18/ds18.c Outdated

#include "log.h"
#include "periph/gpio.h"
#include "timex.h"
Copy link
Copy Markdown
Contributor

@aabadie aabadie Feb 3, 2017

Choose a reason for hiding this comment

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

include not needed

drivers/include/ds18.h Outdated
/**
* @brief export SAUL endpoint
*/
extern const saul_driver_t ds18_temperature_saul_driver;
Copy link
Copy Markdown
Contributor

@aabadie aabadie Feb 3, 2017

Choose a reason for hiding this comment

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

This is not needed

drivers/include/ds18.h Outdated
#define DS18_TEMP_H

#include "periph/gpio.h"
#include "saul.h"
Copy link
Copy Markdown
Contributor

@aabadie aabadie Feb 3, 2017

Choose a reason for hiding this comment

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

include not required

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Feb 3, 2017

Just noticed that an application for this is missing in the tests. tests/driver_ds18x will be fine.

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Feb 3, 2017

Tested ds18b20 an on arduino-zero with pin 12 (PA, 19) but the sensor returns incorrect values (163.77°C, this is not good, trust me ;) ).

On which board did you try ?

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Mar 1, 2017

ping @phectori, any chance for an update ?

@phectori
Copy link
Copy Markdown
Contributor Author

phectori commented Mar 1, 2017 •
edited
Loading

@aabadie Yes. I have tested the driver on an Arduino and got the same result as you. Haven't had the time to figure out why yet. I tested the driver on a nucleo board before.

Regarding the test; Is a SAUL test (using something like: USEMODULE=ds18x BOARD=<your board> make) not enough? A dedicated test wouldn't be more than that in the current state of the driver.

@phectori
Copy link
Copy Markdown
Contributor Author

phectori commented Mar 1, 2017

Removed the unnecessary includes/code you pointed out before and rebased.

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Mar 23, 2017

ping @phectori, any progress on your side ?

@aabadie aabadie added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Mar 23, 2017
@kaspar030 kaspar030 modified the milestone: Release 2017.04 Apr 21, 2017
@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jun 7, 2017

@phectori still no progress on your side ?

@jnohlgard jnohlgard added the Area: SAUL Area: Sensor/Actuator Uber Layer label Feb 19, 2018
@kaspar030 kaspar030 removed this from the Release 2018.04 milestone Apr 11, 2018
@tcschmidt
Copy link
Copy Markdown
Member

tcschmidt commented May 26, 2018

Closed due to inactivity of author,

@tcschmidt tcschmidt closed this May 26, 2018
@leandrolanzieri
Copy link
Copy Markdown
Contributor

leandrolanzieri commented Sep 21, 2018

Tested ds18b20 an on arduino-zero with pin 12 (PA, 19) but the sensor returns incorrect values (163.77°C, this is not good, trust me ;) ).

@aabadie I think I found the problem. Apparently the time measurement on the ds18_read_bit was not ok (could this be due to a problem with xtimer?). So by reducing the DS18_SAMPLE_TIME now I get correct measurements. I am testing this on the lobaro-lorabox board.

If it's ok I will take over this PR and write the missing test application.

@leandrolanzieri leandrolanzieri mentioned this pull request Sep 21, 2018
Merged
@phectori
Copy link
Copy Markdown
Contributor Author

phectori commented Sep 23, 2018

Tested ds18b20 an on arduino-zero with pin 12 (PA, 19) but the sensor returns incorrect values (163.77°C, this is not good, trust me ;) ).

@aabadie I think I found the problem. Apparently the time measurement on the ds18_read_bit was not ok (could this be due to a problem with xtimer?). So by reducing the DS18_SAMPLE_TIME now I get correct measurements. I am testing this on the lobaro-lorabox board.

If it's ok I will take over this PR and write the missing test application.

Yes please! Good luck with writing the test application.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aabadie aabadie aabadie requested changes

Assignees

@aabadie aabadie

Labels

Area: drivers Area: Device drivers Area: SAUL Area: Sensor/Actuator Uber Layer Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@phectori @aabadie @tcschmidt @leandrolanzieri @jnohlgard @kaspar030 @smlng