write binding tests and cleanup (#808)

- Fix bug where write callback was being called multiple times when write operations blocked
 - [windows] refactored write code to be less complex
 - [unix] refactored write code to be less complex
 - added arduino required integration tests

Author: reconbot

README.md

@@ -364,7 +364,7 @@ The `buffer` parameter accepts a [`Buffer` ](http://nodejs.org/api/buffer.html)
 
 **_callback (optional)_**
 
-Called once the write operation returns. The callback should be a function that looks like: `function (error, bytesWritten) { ... }`
+Called once the write operation returns. The callback should be a function that looks like: `function (error) { ... }`
 
 **Note:** The write operation is non-blocking. When it returns, data may still have not actually been written to the serial port. See `drain()`.
 

lib/serialport.js

@@ -262,13 +262,13 @@ SerialPort.prototype.write = function(buffer, callback) {
   if (!Buffer.isBuffer(buffer)) {
     buffer = new Buffer(buffer);
   }
-  debug('write data: ' + JSON.stringify(buffer));
-  SerialPortBinding.write(this.fd, buffer, function(err, result) {
+  debug('write ' + buffer.length + ' bytes of data');
+  SerialPortBinding.write(this.fd, buffer, function(err) {
     if (err) {
       debug('SerialPortBinding.write had an error', err);
       return this._error(err, callback);
     }
-    if (callback) { callback(null, result) }
+    if (callback) { callback(null) }
   }.bind(this));
 };
 

package.json

@@ -1,6 +1,6 @@
 {
   "name": "serialport",
-  "version": "3.1.2",
+  "version": "3.2.0-beta1",
   "description": "Node.js package to access serial ports. Welcome your robotic javascript overlords. Better yet, program them!",
   "author": {
     "name": "Chris Williams",
@@ -10,7 +10,7 @@
   "binary": {
     "module_name": "serialport",
     "module_path": "build/{configuration}/",
-    "host": "https://github.com/voodootikigod/node-serialport/releases/download/3.1.2"
+    "host": "https://github.com/voodootikigod/node-serialport/releases/download/3.2.0-beta1"
   },
   "main": "./lib/serialport",
   "repository": {
@@ -92,6 +92,7 @@
     "rebuild-all": "npm rebuild && node-gyp rebuild",
     "gyp-rebuild": "node-gyp rebuild",
     "stress": "mocha --no-timeouts test/arduinoTest/stress.js",
+    "integration": "mocha test/arduinoTest/integration.js test/integration-lite.js",
     "grunt": "grunt",
     "test": "grunt --verbose",
     "valgrind": "valgrind --leak-check=full --show-possibly-lost=no node --expose-gc --trace-gc node_modules/.bin/grunt test"

src/serialport.cpp

@@ -289,15 +289,12 @@ void EIO_AfterWrite(uv_work_t* req) {
   QueuedWrite* queuedWrite = static_cast<QueuedWrite*>(req->data);
   WriteBaton* data = static_cast<WriteBaton*>(queuedWrite->baton);
 
-  v8::Local<v8::Value> argv[2];
+  v8::Local<v8::Value> argv[1];
   if (data->errorString[0]) {
     argv[0] = v8::Exception::Error(Nan::New<v8::String>(data->errorString).ToLocalChecked());
-    argv[1] = Nan::Undefined();
   } else {
-    argv[0] = Nan::Undefined();
-    argv[1] = Nan::New<v8::Int32>(data->result);
+    argv[0] = Nan::Null();
   }
-  data->callback->Call(2, argv);
 
   if (data->offset < data->bufferLength && !data->errorString[0]) {
     // We're not done with this baton, so throw it right back onto the queue.
@@ -308,6 +305,7 @@ void EIO_AfterWrite(uv_work_t* req) {
     return;
   }
 
+  // throwing errors instead of returning them at this point is rude
   int fd = data->fd;
   _WriteQueue *q = qForFD(fd);
   if (!q) {
@@ -321,6 +319,8 @@ void EIO_AfterWrite(uv_work_t* req) {
   // remove this one from the list
   queuedWrite->remove();
 
+  data->callback->Call(1, argv);
+
   // If there are any left, start a new thread to write the next one.
   if (!write_queue.empty()) {
     // Always pull the next work item from the head of the queue

src/serialport_unix.cpp

@@ -333,31 +333,33 @@ int setup(int fd, OpenBaton *data) {
 void EIO_Write(uv_work_t* req) {
   QueuedWrite* queuedWrite = static_cast<QueuedWrite*>(req->data);
   WriteBaton* data = static_cast<WriteBaton*>(queuedWrite->baton);
+  int bytesWritten = 0;
 
-  data->result = 0;
-  errno = 0;
-
-  // We carefully *DON'T* break out of this loop.
   do {
-    if ((data->result = write(data->fd, data->bufferData + data->offset, data->bufferLength - data->offset)) == -1) {
-      if (errno == EAGAIN || errno == EWOULDBLOCK)
-        return;
-
-      // The write call might be interrupted, if it is we just try again immediately.
-      if (errno != EINTR) {
-        snprintf(data->errorString, sizeof(data->errorString), "Error: %s, calling write", strerror(errno));
-        return;
-      }
+    errno = 0; // probably don't need this
+    bytesWritten = write(data->fd, data->bufferData + data->offset, data->bufferLength - data->offset);
+    if (-1 != bytesWritten) {
+      // there wasn't an error, do the math on what we actually wrote and keep writing until finished
+      data->offset += bytesWritten;
+      continue;
+    }
 
-      // try again...
+    // The write call was interrupted before anything was written, try again immediately.
+    if (errno == EINTR) {
+      // why try again right away instead of in another event loop?
       continue;
-    } else {
-      // there wasn't an error, do the math on what we actually wrote...
-      data->offset += data->result;
     }
 
-    // if we get there, we really don't want to loop
-    // break;
+    // Try again in another event loop
+    if (errno == EAGAIN || errno == EWOULDBLOCK){
+      return;
+    }
+
+    // EBAD would mean we're "disconnected"
+
+    // a real error so lets bail
+    snprintf(data->errorString, sizeof(data->errorString), "Error: %s, calling write", strerror(errno));
+    return;
   } while (data->bufferLength > data->offset);
 }
 

src/serialport_win.cpp

@@ -57,11 +57,12 @@ void EIO_Open(uv_work_t* req) {
   HANDLE file = CreateFile(
     data->path,
     GENERIC_READ | GENERIC_WRITE,
-    0,
+    0, // dwShareMode 0 Prevents other processes from opening if they request delete, read, or write access
     NULL,
     OPEN_EXISTING,
-    FILE_FLAG_OVERLAPPED,
-    NULL);
+    FILE_FLAG_OVERLAPPED, // allows for reading and writing at the same time and sets the handle for asynchronous I/O
+    NULL
+  );
   if (file == INVALID_HANDLE_VALUE) {
     DWORD errorCode = GetLastError();
     char temp[100];
@@ -368,35 +369,31 @@ void EIO_Write(uv_work_t* req) {
     ov.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
 
     // Start write operation - synchrounous or asynchronous
-    DWORD bytesWrittenSync = 0;
-    if (!WriteFile((HANDLE)data->fd, data->bufferData, static_cast<DWORD>(data->bufferLength), &bytesWrittenSync, &ov)) {
+    DWORD bytesWritten = 0;
+    if (!WriteFile((HANDLE)data->fd, data->bufferData, static_cast<DWORD>(data->bufferLength), &bytesWritten, &ov)) {
       DWORD lastError = GetLastError();
       if (lastError != ERROR_IO_PENDING) {
         // Write operation error
         ErrorCodeToString("Writing to COM port (WriteFile)", lastError, data->errorString);
+        CloseHandle(ov.hEvent);
         return;
-      } else {
-        // Write operation is asynchronous and is pending
-        // We MUST wait for operation completion before deallocation of OVERLAPPED struct
-        // or write data buffer
-
-        // Wait for async write operation completion or timeout
-        DWORD bytesWrittenAsync = 0;
-        if (!GetOverlappedResult((HANDLE)data->fd, &ov, &bytesWrittenAsync, TRUE)) {
-          // Write operation error
-          DWORD lastError = GetLastError();
-          ErrorCodeToString("Writing to COM port (GetOverlappedResult)", lastError, data->errorString);
-          return;
-        } else {
-          // Write operation completed asynchronously
-          data->result = bytesWrittenAsync;
-        }
       }
-    } else {
-      // Write operation completed synchronously
-      data->result = bytesWrittenSync;
-    }
+      // Write operation is completing asynchronously
+      // We MUST wait for the operation completion before deallocation of OVERLAPPED struct
+      // or write data buffer
 
+      // block for async write operation completion
+      bytesWritten = 0;
+      if (!GetOverlappedResult((HANDLE)data->fd, &ov, &bytesWritten, TRUE)) {
+        // Write operation error
+        DWORD lastError = GetLastError();
+        ErrorCodeToString("Writing to COM port (GetOverlappedResult)", lastError, data->errorString);
+        CloseHandle(ov.hEvent);
+        return;
+      }
+    }
+    // Write operation completed synchronously
+    data->result = bytesWritten;
     data->offset += data->result;
     CloseHandle(ov.hEvent);
   } while (data->bufferLength > data->offset);

test/arduinoTest/arduinoEcho/arduinoEcho.ino

@@ -1,3 +1,6 @@
+#define SET_BAUD_57600 130
+#define SET_BAUD_9600 131
+
 void setup() {
   pinMode(LED_BUILTIN, OUTPUT);
   Serial.begin(9600);
@@ -7,14 +10,19 @@ void setup() {
 void loop() {
   while (Serial.available()) {
     int byte = Serial.read();
-    if (byte == 130) {
-      Serial.begin(57600);
-      Serial.write("set to 57600");
-    } else if (byte == 131) {
-      Serial.begin(9600);
-      Serial.write("set to 9600");
-    } else {
-      Serial.write(byte);
+    switch (byte) {
+      case SET_BAUD_57600:
+        Serial.begin(57600);
+        Serial.write("set to 57600");
+        break;
+      case SET_BAUD_9600:
+        Serial.begin(9600);
+        Serial.write("set to 9600");
+        break;
+      default:
+        Serial.write(byte);
+        break;
     }
   }
 }
+

test/arduinoTest/integration.js

@@ -0,0 +1,81 @@
+'use strict';
+var crypto = require('crypto');
+var assert = require('chai').assert;
+var serialPort = require('../../');
+var SerialPort = serialPort.SerialPort;
+
+var platform;
+switch (process.platform) {
+  case 'win32':
+    platform = 'win32';
+    break;
+  case 'darwin':
+    platform = 'darwin';
+    break;
+  default:
+    platform = 'unix';
+}
+
+var testPort = process.env.TEST_PORT;
+
+if (!testPort) {
+  throw 'These test require an arduino loaded with the arduinoEcho program on a serialport set to the TEST_PORT env var';
+}
+
+describe('SerialPort Integration tests', function() {
+  it('.list', function(done) {
+    serialPort.list(function(err, ports) {
+      var foundPort = false;
+      ports.forEach(function(port) {
+        if (port.comName === testPort){
+          foundPort = true;
+        }
+      });
+      assert.isTrue(foundPort);
+      done();
+    });
+  });
+
+  // Be careful to close the ports when you're done with them
+  // Ports are exclusively locked in windows and maybe other platforms eventually
+
+  describe('#update', function() {
+    if (platform === 'win32') {
+      return it("Isn't supported on windows yet");
+    }
+
+    it('allows changing the baud rate of an open port', function(done) {
+      var port = new SerialPort(testPort, function() {
+        port.update({baudRate: 57600}, function(err) {
+          assert.isNull(err);
+          port.close(done);
+        });
+      });
+    });
+    it('can still read and write after a baud change');
+  });
+
+  describe('#read and #write', function() {
+    it('writes 5k and reads it back', function(done) {
+      this.timeout(20000);
+      // 5k of random ascii
+      var output = new Buffer(crypto.randomBytes(5000).toString('ascii'));
+      var expectedInput = Buffer.concat([new Buffer('READY'), output]);
+      var port = new SerialPort(testPort);
+
+      // this will trigger from the "READY" the arduino sends when it's... ready
+      port.once('data', function(){
+        port.write(output);
+      });
+
+      var input = new Buffer(0);
+      port.on('data', function(data) {
+        input = Buffer.concat([input, data]);
+        if (input.length === expectedInput.length){
+          assert.deepEqual(expectedInput, input);
+          port.close(done);
+        }
+      });
+    });
+  });
+});

test/bindings.js

@@ -178,4 +178,42 @@ describe('SerialPortBinding', function() {
       });
     });
   });
+  describe('#write', function() {
+    if (!testPort) {
+      it('Cannot be tested as we have no test ports on ' + platform);
+      return;
+    }
+
+    beforeEach(function(done) {
+      SerialPortBinding.open(testPort, defaultPortOpenOptions, function(err, fd) {
+        assert.isNull(err);
+        assert.isNumber(fd);
+        this.fd = fd;
+        done();
+      }.bind(this));
+    });
+
+    afterEach(function(done) {
+      var fd = this.fd;
+      this.fd = null;
+      SerialPortBinding.close(fd, done);
+    });
+
+    it('calls the write callback once after a small write', function(done){
+      var data = new Buffer('simple write of 24 bytes');
+      SerialPortBinding.write(this.fd, data, function(err){
+        assert.isNull(err);
+        done();
+      });
+    });
+
+    it('calls the write callback once after a 5k write', function(done){
+      this.timeout(20000);
+      var data = new Buffer(1024 * 5);
+      SerialPortBinding.write(this.fd, data, function(err){
+        assert.isNull(err);
+        done();
+      });
+    });
+  });
 });