hybridgroup / cylon

JavaScript framework for robotics, drones, and the Internet of Things (IoT)
https://cylonjs.com
Other
4.2k stars 360 forks source link

Missing sensor causes crash (Edison, BMP180) #326

Closed derrell closed 8 years ago

derrell commented 8 years ago

When the BMP180 sensor is not installed, the following code crashes. Backtrace provided at end. The reason is that the provided buffer is "expected" to contain data, but if no sensor is installed, there is no data.

  cylon.robot( { name: "bmp180" } )
    .connection("edison", { adaptor: "intel-iot" } )
    .device("bmp180", { driver: "bmp180" } )
    .on("error", console.warn)
    .on(
      "ready",
      function(my)
      {
        privateData.timeout = setInterval(
          function()
          {
            var             message;

            my.bmp180.getAltitude(
              1,
              null,
              function(err, val)
              {
                var             message;

                if (err)
                {
                  console.log("getAltitude: " + err);
                  return;
                }

                // Let server know we received an action event from the curie
                message =
                  {
                    type    : "notification",
                    dest    : "bmp180",
                    command : "data",
                    data    :
                      {
                        temperature : val.temp,
                        pressure    : val.press,
                        altitude    : val.alt
                      }
                  };
                console.log("bmp180 sending message: " + JSON.stringify(message));
                sendMessage(message);
              });
            },
            config.bmp180.timeout || 10000);
      });

    // Begin reading the sensor
    cylon.start();

Backtrace:

2015-12-28 16:04:41.858 ERROR - RangeError: Trying to access beyond buffer length
    at checkOffset (buffer.js:582:11)
    at Buffer.readInt16BE (buffer.js:648:5)
    at /usr/share/wsgclient/app/node_modules/cylon-i2c/lib/bmp180.js:174:25
    at Adaptor.i2cRead (/usr/share/wsgclient/app/node_modules/cylon-intel-iot/lib/adaptor.js:274:3)
    at Bmp180.readCoefficients (/usr/share/wsgclient/app/node_modules/cylon-i2c/lib/bmp180.js:164:19)
    at Bmp180.start (/usr/share/wsgclient/app/node_modules/cylon-i2c/lib/bmp180.js:70:8)
    at Robot.startDevice (/usr/share/wsgclient/app/node_modules/cylon/lib/robot.js:365:16)
    at Robot.<anonymous> (/usr/share/wsgclient/app/node_modules/cylon/lib/robot.js:336:19)
    at /usr/share/wsgclient/app/node_modules/cylon/lib/utils/helpers.js:237:36
    at Array.forEach (native)

I don't know the API coordination defined by cylon, between sensors and higher-level code. Should Adaptor.prototype.i2cRead be returning an error? Or should Bmp180.prototype.readCoefficients be checking the callback value from this.connection.i2cRead and detecting the empty buffer there? Regardless, something in the call chain needs to handle the missing data. If you let me know how cylon handles this in general, I can provide a pull request to fix it.

deadprogram commented 8 years ago

Hi, @derrell thanks for the thoughtful issue report.

The adaptor should definitely return an error gracefully when no such i2c device is connected vs the current ugly behaviour. If the driver was also more robust, that would be good, but the adaptor seems like the place to catch this, and we'd accept a PR with gratitude.

derrell commented 8 years ago

Fixed with cylon-intel-iot pull request 19, now merged (6a13201). Thanks.

The updated cylon-intel-iot does not seem to have been pushed to the npm repository. Do you have a schedule for doing that, or "as requested," or ... ? (Please consider this such a request. :-) )

deadprogram commented 8 years ago

OK, done:

To git@github.com:hybridgroup/cylon-intel-iot.git
 * [new tag]         v0.8.1 -> v0.8.1
+ cylon-intel-iot@0.8.1

Thank you again for your contributions!

deadprogram commented 8 years ago

Since this has now been released, I am closing this issue. Please reopen if needed, and thanks again.