Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
dht20: add I2C driver for DHT20 temperature and humidity sensor #693
base: dev
Are you sure you want to change the base?
dht20: add I2C driver for DHT20 temperature and humidity sensor #693
Changes from 1 commit
315dbef
19c9fe6
3f4a9d8
3c1aaa9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the returned error ignored here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the good suggestion. I have made the correction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see related change. Error returned from
d.bus.Tx
is still ignored on several places inUpdate
andConfigure
methods.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I misread the suggestion.
The comment was regarding functions that return errors.
So, I have made the correction.
I verified it using
kisielk/errcheck
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the measurement is only triggered here that means the data that was read before is from previous measurement. If the user cod is calling
Update
once a minute (or on even longer interval) it would be always getting stale data. Would it be better to just wait 80ms until update is finished and then read the values? Other option would be to change the API, for example to have separate methods for triggering update and reading the data.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of Update() is aligned with the interface in #345. I feel that waiting for 80ms within Update() is problematic. As you mentioned, the current code results in obtaining stale data. We would like to discuss how to proceed in a separate PR or other discussion.
For now, I have added a note in the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is marked as "outdated" by github. Is the issue you all were discussing still present?
My only gripe with the code is that Update does not actually update the sensor and returns no indication of such. We could have a sentinel error for this case or choose to block until update finishes. Both are not without their pros and cons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, now come to think of it the sentinel error might be the most idiomatic way to solve this without requiring users to start using goroutines to manage several sensors which have a cooldown period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@soypat the issue I noted is that
Update
first reads the data from the sensor and stores it intoDevice
object, and then triggers next measurement, so when user code reads the temperature it is the value that was not measured after lastUpdate
call, but after second to lastUpdate
call and hance stail. MaybeUpdate
should just trigger the measurement andTemperature
andHumidity
methods should check if enough time has passed (or read the status byte) and if so, do the reading from the sensor and store the value inDevice
, all subsequent calls could just return stored values. Those methods should also return error in this case. I also agree that Update should return sentinel error if it is called before enough time has passed.Another thing I just noticed is that
which
argument ofUpdate
is ignored and both Temperature and Humidity are always updated. What ifUpdate
is called withdrivers.Humidity
? Should the Temperature also be updated?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, that sounds like it could cause several headaches. I'd document the Sensor interface such that the function returns a nil error only after it starts and ends a new sensor measurment succesfully updating all requested measurements in
which
.Temperature and Humidity need not check anything, those methods always return stale data (to some extent). If Staleness functionality should be desired on the Humidity/Temperature methods it probably be implemented in a type that contains a Sensor type and a callback to the Temperature/Humidity/Pressure/Distance method, that way it is readily reusable among different sensors.
which
should not be ignored. The simplest implementation should perform I/O only if one or more of the corresponding measurements are set. It is OK for it to update both Temperature and Humidity if only Humidity or Temperature is passed. But if neither Temp nor Humidity are set Update should return immediately with a nil error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added the check for which.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! I'm just noticing this now- you are using floats! This would present a problem on certain devices with no FPU, causing lots of CPU cycles to be spent on processing the data.
It was not explicit in the Sensor PR, but I'd expect these methods to return an integer lest the sensor send back a binary floating point representation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using integer arithmetic and representing the humidity and temperature as fixed point integers the code could take the following form (have not tested, did some basic arithmetic to get the numbers):
If users need floating point representations (which they do) we could create a
sensor
package that contains something of this sort of logic:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #332 for a previous attempt