rwaldron / johnny-five

JavaScript Robotics and IoT programming framework, developed at Bocoup.
http://johnny-five.io
Other
13.27k stars 1.76k forks source link

SHT31D hygrometer / thermometer: always return same values #1253

Closed balda closed 7 years ago

balda commented 7 years ago

There's a bug in SHT31D hygrometer / thermometer: it only return first measures. So, on("changed") is never called. Measures are "good" when the script is launched, but never change :/ (that's the bug).

Reading Adafruit driver (https://github.com/adafruit/Adafruit_SHT31/blob/master/Adafruit_SHT31.cpp#L69), there's 2 differences:

J5 SHT31D driver repeatedly read on i2c (io.i2cRead(address, READLENGTH, ...), without "reseting" register (https://github.com/rwaldron/johnny-five/blob/master/lib/imu.js#L117)

I made some update to make it work:

SHT31D = {
    // ...
    initialize: {
        value: function(board, opts) {
            var io = board.io;
            var address = opts.address || this.ADDRESSES[0];

            opts.address = address;
            console.log("initialize fixed imu");

            io.i2cConfig(opts);

            // Soft Reset
            io.i2cWrite(address, [
                this.REGISTER.SOFT_RESET >> 8,
                this.REGISTER.SOFT_RESET & 0xFF,
            ]);

            this.ioReadData(board, address);
        }
    },
    ioReadData: {
        value: function(board, address) {
            var READLENGTH = 6;
            var io = board.io;
            var computed = {
                temperature: null,
                humidity: null,
            };
            var self = this;
            io.i2cWrite(address, [
                this.REGISTER.MEASURE_HIGH_REPEATABILITY >> 8,
                this.REGISTER.MEASURE_HIGH_REPEATABILITY & 0xFF,
            ]);
            board.wait(15, function() {
                io.i2cReadOnce(address, READLENGTH, function(data) {
                    computed.temperature = int16(data[0], data[1]);
                    computed.humidity = int16(data[3], data[4]);
                    self.emit("data", computed);
                    self.ioReadData(board, address);
                }.bind(self));
            });
        }
    },
    // ...
};

I found a working Python library that only wait 15ms: https://github.com/ralf1070/Adafruit_Python_SHT31/blob/master/Adafruit_SHT31.py#L109 That's why i write board.wait(15, ...) instead of board.wait(500, ...).

I try my fix successfully on an Arduino Uno and an Arduino Micro.

Can i make a pull request to fix this?

fivdi commented 7 years ago

The issue that @balda discovered here and the proposed solution in backed up by information from the SHT3x-DIS datasheet. Section 4.3 on page 9 of the datasheet makes the following statement:

"During measurement the sensor generally does not respond to any I2C activity, i.e. I2C read and write headers are not acknowledged (NACK)."

In addition, table 4 in section 2.2 on page 5 of the datasheet specifies that the high repeatability measurement duration is up to 15 milliseconds. It is therefore appropriate to wait for 15 milliseconds after issuing a measurement command.

@balda Are you working on a PR for this issue? If not, do you mind if create a PR with a fix?

rwaldron commented 7 years ago

I don't want to add the ioReadData method to the controller object. All of the reading can be wrapped up in a function inside of the initialize function and called as needed with necessary delay. Similar approach seen in HTU21D

var readCycle = function() {
  io.i2cWrite(address, [
    this.REGISTER.MEASURE_HIGH_REPEATABILITY >> 8,
    this.REGISTER.MEASURE_HIGH_REPEATABILITY & 0xFF,
  ]);
  setTimeout(function() {
    io.i2cReadOnce(address, READLENGTH, function(data) {
      computed.temperature = int16(data[0], data[1]);
      computed.humidity = int16(data[3], data[4]);
      this.emit("data", computed);
      readCycle();
    }.bind(this));
  }.bind(this), 15);
}.bind(this);

readCycle();
fivdi commented 7 years ago

Yes. The calls to int16 should also be replaced with calls to unit16. The SHT31D has the same int16/uint16 issue that the HTU21D had (#1278). This additional issues can be fixed in the same PR, correct? Or is an additional PR needed?

rwaldron commented 7 years ago

@fivdi if you're up for writing a patch that fixes both, I would love that :)

fivdi commented 7 years ago

if you're up for writing a patch that fixes both, I would love that :)

@rwaldron ok, I'll write a patch.

balda commented 7 years ago

@fivdi thanks for making a PR to fix this bug! (i'm sorry: i haven't enough time these days to work on J5)

@rwaldron it's noted: by design, it's preferable to add functions in initialize method, instead of creating new controller methods (when it's possible) ;-) Thanks for your code review!

And Si7021 has the same uint bug (with false negative values): https://github.com/rwaldron/johnny-five/blob/master/lib/imu.js#L1952 Must i create a new specific issue or can it be fixed in the same time?

fivdi commented 7 years ago

@balda A separate issue for the Si7021 would be better. I got a SHT31D recently but don't have Si7021 and don't really want to write a patch/tests for a device I can't physically test with.

balda commented 7 years ago

I understand @fivdi, it's logical.

I'll post an issue about Si7021 bug. I'm reading your HTU21D pull request to understand what you have done in oneHundredPercentHumidity test to adapt it (it's the same bug, but adding the u is not the more difficult ;-) )

fivdi commented 7 years ago

1319 is a PR for fixing both of the issues mentioned above.

fivdi commented 7 years ago

Now that #1319 has been merged I think this issue can be closed.

rwaldron commented 7 years ago

@fivdi excellent!

balda commented 7 years ago

Thanks!