Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers/dht: initial import #2990

Merged
merged 2 commits into from May 29, 2015
Merged

Conversation

LudwigKnuepfer
Copy link
Member

Unified driver for DHT11 and DHT22.
Test application included.

@LudwigKnuepfer LudwigKnuepfer added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers labels May 15, 2015
@LudwigKnuepfer
Copy link
Member Author

There are some cleanup activities left.. Will tend to this PR again tomorrow.

Anyway:
Both sensors were tested with sub-zero temperatures, the DHT11 was also tested for > 60 degrees C which resulted in a slightly burnt/molten case. All readings made sense.

/* read 1, set bit */
les_bits |= 0x0000000000000001;
/* wait for low */
while (gpio_read(dev)) ;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line is a leftover of the previous approach with using sleep.

Copy link
Member Author

Choose a reason for hiding this comment

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

right!

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label May 16, 2015
@LudwigKnuepfer
Copy link
Member Author

Can't test anymore, my sensor died.

@jnohlgard
Copy link
Member

@LudwigOrtmann

Can't test anymore, my sensor died.

Did it die because of this PR? 😜

@LudwigKnuepfer
Copy link
Member Author

@gebart Due to the cause chain: definitely ;)
But really, I'm not sure, it went hot and stopped responding didn't respond when I wanted to test the patches.
I did not test yesterdays state again today. Could be a wiring mishap or the result of yesterdays heating accident.

@LudwigKnuepfer
Copy link
Member Author

dht11-molten

Doesn't even look too bad and did still work yesterday...

@miri64
Copy link
Member

miri64 commented May 16, 2015

RIP

@LudwigKnuepfer
Copy link
Member Author

Tested patches with an intact DHT22, worked as expected.


void dht_parse_22(dht_data_t *data, float *outhum, float *outtemp)
{
*outhum = data->humidity / 10;
Copy link
Member

Choose a reason for hiding this comment

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

I didn't find this in the datasheet immediately but I guess this has to do with the higher resoultion of the dht22?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, also it's what the datasheets say the conversion is

@PeterKietzmann
Copy link
Member

@LudwigOrtmann the driver looks good to me besides some minor comments. I don't have the hardware to test it. I never "researched" on it but I thought about a common 1-wire interface for the future. After implementing this rudimentary protocol, do you think it makes sense so create a common 1-wire API?

@jnohlgard
Copy link
Member

@PeterKietzmann I don't think the DHT22 is really 1-wire (I'm assuming you were referring to the 1-wire brand name by Maxim Integrated). From the data sheet the DHT22 seems similar but the length of each bit depends on if it is a zero or a one, which it does not in the (Maxim) 1-wire protocol.

@LudwigKnuepfer
Copy link
Member Author

I superficially looked into this as well and can confirm that the DHT family is not speaking "1-wire". While it has some similarities, I wouldn't base a unified communication module on it.

@PeterKietzmann
Copy link
Member

Thanks for your answers. Well, if it is proprietary, I agree there is no reason for an interface...

@PeterKietzmann
Copy link
Member

Does anyone have the hardware to test this driver?

* @param[out] hum pointer to variable humidity is stored to
* @param[out] temp pointer to variable temperature is stored to
*/
void dht_parse(dht_t *dev, dht_data_t *data, float *hum, float *temp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as in #2980: I would prefer to have three functions here:

void dht_read_raw(); // return raw values
void dht_read(dev, int32_t *tmp, int16_t? *hum); // return milidegree C and mg/m^3
void dht_read(dev, float *tmp, float *hum); // return degree and g/m^3

Copy link
Member Author

Choose a reason for hiding this comment

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

void dht_read_raw(); // return raw values

As a side-effect?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, probably not .. anyways - I think the parse function allows for higher flexibility regarding reading and transforming of the values. I can add convenience functions that call both in one go if you insist.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I do. In the end, in 99% of the cases, users are not interested in the raw value, but in the actual physical ones. So forcing them to call parse after any reading reduces IMHO the usability of the driver...

Could you elaborate on how an explicit parse function increases the flexibility?

Copy link
Member Author

Choose a reason for hiding this comment

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

You can read values at one and convert them at a later point in time. I imagine this might be useful in a time constrained situation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not going to implement an optimized integer reading version any time soon, knock yourself out in a follow-up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the timings for the sensor and the time it takes for the conversion I say the conversion time is insignificantly small... But if you will not fix it anyway, fine with me.

@LudwigKnuepfer
Copy link
Member Author

@haukepetersen @PeterKietzmann can I squash?

@PeterKietzmann
Copy link
Member

@LudwigOrtmann the code looks good to me. Can anyone test the driver? Maybe @mehlis has a device? Let's wait with squashing until tested.

@OlegHahm
Copy link
Member

Why do we need with squashing for a test?

@PeterKietzmann
Copy link
Member

Honestly I don't really know :-) ! Fine, please squash. ACK when tests went positive

@LudwigKnuepfer
Copy link
Member Author

Why do we need with squashing for a test?

I don't understand the question.

@haukepetersen
Copy link
Contributor

ACK when suqashed and Travis happy.

@OlegHahm OlegHahm modified the milestone: Release 2015.06 May 28, 2015
@LudwigKnuepfer LudwigKnuepfer removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label May 29, 2015
@PeterKietzmann PeterKietzmann added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 29, 2015
@PeterKietzmann
Copy link
Member

And go

PeterKietzmann added a commit that referenced this pull request May 29, 2015
@PeterKietzmann PeterKietzmann merged commit b771e91 into RIOT-OS:master May 29, 2015
@LudwigKnuepfer LudwigKnuepfer deleted the pr/dht branch May 29, 2015 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants