dht20: add I2C driver for DHT20 temperature and humidity sensor

[sago35]

I am adding support for the DHT20 in this PR.
I tested it using Seeed's Grove - Temperature&Humidity Sensor (DHT20).

https://wiki.seeedstudio.com/Grove-Temperature-Humidity-Sensor-DH20/
https://cdn-shop.adafruit.com/product-files/5183/5193_DHT20.pdf

The challenging part of the design was the relationship between having to wait 80ms after triggering and the Update() function. Currently, I have designed it so that even if Update() is called multiple times within 80ms, there will be no I2C communication. If this becomes an issue, we will need to consider an alternative solution.

[FilipVranesevic]

One general note, should this be a separate driver or another device type of the existing DHT driver?
EDIT: I see now that devices in DHT driver are OneWire and this is I2C so it would not fit the API. Not sure what is the best way to organise these drivers now.

[FilipVranesevic]

Why is the returned error ignored here?

[sago35]

Thank you for the good suggestion. I have made the correction.

[FilipVranesevic]

I don't see related change. Error returned from d.bus.Tx is still ignored on several places in Update and Configure methods.

[sago35]

I misread the suggestion.
The comment was regarding functions that return errors.
So, I have made the correction.
I verified it using kisielk/errcheck.

$ errcheck -verbose ./dht20/...
checking tinygo.org/x/drivers/dht20

[FilipVranesevic]

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.

[sago35]

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.

[soypat]

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.

[soypat]

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.

[FilipVranesevic]

@soypat the issue I noted is that Update first reads the data from the sensor and stores it into Device object, and then triggers next measurement, so when user code reads the temperature it is the value that was not measured after last Update call, but after second to last Update call and hance stail. Maybe Update should just trigger the measurement and Temperature and Humidity 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 in Device, 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 of Update is ignored and both Temperature and Humidity are always updated. What if Update is called with drivers.Humidity? Should the Temperature also be updated?

[soypat]

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.

[sago35]

I have added the check for which.

[deadprogram]

Any further feedback @soypat and @FilipVranesevic ?

@sago35 do you wish to squash commits in this PR or would you prefer it is done when it is merged?

[soypat]

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.

[soypat]

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

rawHumidity := uint32(d.data[1])<<12 | uint32(d.data[2])<<4 | uint32(d.data[3])>>4
rawTemperature := uint32(d.data[3]&0x0F)<<16 | uint32(d.data[4])<<8 | uint32(d.data[5])

// humidity contains millipercent
d.humidity = int32(rawHumidity / 10) 
// temperature contains millicelsius
d.temperature = int32(rawTemperature/5) - 50000

If users need floating point representations (which they do) we could create a sensor package that contains something of this sort of logic:

type Thermometer interface {
    drivers.Sensor
    // Temperature returns fixed point representation of temperature in milicelsius [mC]. 
    Temperature() int32
}

func ReadKelvin32(t Thermometer) (float32, error) {
     err := t.Update(drivers.Temperature)
     if err != nil {
        return err
     }
     v := t.Temperature()
     return float32(v)/1000 + 273.15
}

// more advanced ADC configurations...
type sensorConversion struct {
    s drivers.Sensor
    gets []func() int32
    conversions []func(v int32) float32
}

[deadprogram]

See #332 for a previous attempt

[FilipVranesevic]

Any further feedback @soypat and @FilipVranesevic ?

@deadprogram I'm not sure the stale data issue I noted was addressed properly. This is how I see it (@sago35 please correct me if I'm wrong since I just glanced over the sensor datasheet):
Consider this realistic scenario, I want to log temperature each hour. At the start of the program I initialise the sensor and call Update. At 1PM I call Update and Temperature to get the reading. Since Update method first reads values from the sensor and then jus triggers next measurement I would get the measurement from the time first Update is called (at the start of the program). At 2PM I call Update and Temperature again and this time I get value from the measurement done at 1PM. I think this could be very confusing to the users of the driver even in this simple use case.

[soypat]

Why this case? The measurements are only updated when this is true... This case should be inverted and return an error if measurements are not updated as per the Sensor interface functioning.

if (d.data[0] & 0x80) != 0 {
    return errDHTBitSet
}

[soypat]

Hmm- I tend to agree with @FilipVranesevic. The sentinel error is problematic for reasons that have become apparent to me just now: Update() returns nil error only when sensor values are correctly updated- in adding a sentinel error that signals a future call to Update() will have the correctly updated values we are adding a lot of logic the sensor has to resolve:

• How stale can the last sensor measurements be before Update returns the sentinel error? I see there is an inconsistency in the current driver design with respect to this:
  • Currently the DHT can trigger a resense if the data is just 81ms old since the Update call following a succesful call will trigger an update to the sensor and return the sentinel error
  • The user can succesfully read arbitrarily old data, minutes, hours, days

I believe this should be resolved in a higher level of abstraction, not by the sensor, since this is an issue that would be present in many sensors, we don't want to solve staleness or async sensing in every single driver we implement in drivers. This is exactly why I pushed for the Sensor interface- so we can abstract different Sensor behaviour! So... I believe:

• The sensor should adhere to the Sensor interface and block the corresponding 80ms and return nil error if the measurements updated correctly in that call.
• We should have facilities for dealing with async sensing, a sensors package or something

[soypat]

Example of what an async API could look like. Do note I do not like this particular design, but it'd solve the aforementioned issue for all sensors with similar functioning to the DHT.

func main() {
	d := dht.New(p, t)
	as := NewAsyncSensor(d, 2, 100*time.Millisecond, func(dst []int32) {
		dst[0] = d.Temperature()
		dst[1] = d.Humidity()
	})
       for {
          // Values are best attempt at getting values close to `period` staleness.
          temp := as.Get(0)
          humidity := as.Get(1)
          print(time.Since(as.LastMeasurement()).String(), "ago ")
          println(temp, humidity)
       }
}

type AsyncSensor struct {
	s                Sensor
	lastUpdate       time.Time
	period           time.Duration
	get              func(dst []int32)
	lastMeasurements []int32
}

func NewAsyncSensor(s Sensor, n int, period time.Duration, getter func([]int32)) *AsyncSensor {
	as := &AsyncSensor{
		s:                s,
		period:           period,
		get:              getter,
		lastMeasurements: make([]int32, n),
	}
	go as.startMeasuring()
	return as
}

func (as *AsyncSensor) startMeasuring() {
	for {
		elapsed := time.Since(as.lastUpdate)
		if elapsed < as.period {
			time.Sleep(as.period - elapsed)
		}
		err := as.s.Update(drivers.AllMeasurements)
		if err != nil {
			time.Sleep(as.period)
			continue
		}
		as.get(as.lastMeasurements[:])
		as.lastUpdate = time.Now()
	}
}

func (as *AsyncSensor) LastMeasurement() time.Time {
	return as.lastUpdate
}

func (as *AsyncSensor) Get(i int) int32 {
	return as.lastMeasurements[i]
}

[FilipVranesevic]

Example of what an async API could look like. Do note I do not like this particular design, but it'd solve the aforementioned issue for all sensors with similar functioning to the DHT.

func main() {
	d := dht.New(p, t)
	as := NewAsyncSensor(d, 2, 100*time.Millisecond, func(dst []int32) {
		dst[0] = d.Temperature()
		dst[1] = d.Humidity()
	})
       for {
          // Values are best attempt at getting values close to `period` staleness.
          temp := as.Get(0)
          humidity := as.Get(1)
          print(time.Since(as.LastMeasurement()).String(), "ago ")
          println(temp, humidity)
       }
}

type AsyncSensor struct {
	s                Sensor
	lastUpdate       time.Time
	period           time.Duration
	get              func(dst []int32)
	lastMeasurements []int32
}

...

@soypat I would probably call this functionality you propose as SensorPoller or similar.AsyncSensor would in my opinion be more like one shot non blocking update with callback or channel to signal the completion. I also agree that this sort of functionality should be higher level abstraction (if it is decided that is needed) and not part of the driver.