SerialPort Streams2 Implementation #431

joshperry · 2014-12-31T00:27:38Z

These are my initial bullet points for what I'd like this PR to eventually consist of. This will change and, most likely, grow. I'm pushing this up early because I would absolutely love input/ideas on this. It's not quite ready to use yet, still working on tests and kind of hashing out plans on where and how to do things.

There are a couple issues that makes this lib mesh poorly with contemporary node.js code:

reconbot · 2014-12-31T01:12:30Z

I'll look when I get home but <3 <3 <3
On Dec 30, 2014 7:27 PM, "Joshua Perry" [email protected] wrote:

These are my initial bullet points for what I'd like this PR to eventually
consist of. This will change and, most likely, grow. I'm pushing this up
early because I would absolutely love input/ideas on this. It's not quite
ready to use yet, still working on tests and kind of hashing out plans on
where and how to do things.

There are a couple issues that makes this lib mesh poorly with
contemporary node.js code:

There are 3 ways to handle errors (global emitter, instance emitter,
callbacks).

It implements most of the streaming logic itself which makes it much

more complex than it need be.

Functions and events for reading and writing are inconsistent with

node Streams2

Implement a Duplex Streams2 stream

Make precondition errors more consistent

Error events are always emitted whether there is a callback or not

Remove callbacks from most functions and rely on stream events

Goodbye global event emitter

Goodbye opening port immediately from constructor, including
callback parameter

Add openPort() module export for simultaneously creating and
opening a port, similar to the net.createConnection for net.Socket.

Callback to open() is the same as .once(‘open’, …)

Goodbye parsers, prefer transform streams

You can merge this Pull Request by running

git pull https://github.com/prodatakey/node-serialport streams2

Or view, comment on, or merge it at:

#431
Commit Summary

Initial commit

File Changes

M .jshintrc
https://github.com/voodootikigod/node-serialport/pull/431/files#diff-0
(4)

M Gruntfile.js
https://github.com/voodootikigod/node-serialport/pull/431/files#diff-1
(25)

A arduinoTest/.jshintrc
https://github.com/voodootikigod/node-serialport/pull/431/files#diff-2
(13)

M arduinoTest/requiresComPort.js
https://github.com/voodootikigod/node-serialport/pull/431/files#diff-3
(10)

M arduinoTest/stress.js
https://github.com/voodootikigod/node-serialport/pull/431/files#diff-4
(8)

M package.json
https://github.com/voodootikigod/node-serialport/pull/431/files#diff-5
(35)

M parsers.js
https://github.com/voodootikigod/node-serialport/pull/431/files#diff-6
(11)

M serialport.js
https://github.com/voodootikigod/node-serialport/pull/431/files#diff-7
(961)

A test/.jshintrc
https://github.com/voodootikigod/node-serialport/pull/431/files#diff-8
(13)

M test/parsers.js
https://github.com/voodootikigod/node-serialport/pull/431/files#diff-9
(38)

M test/serialport-basic.js
https://github.com/voodootikigod/node-serialport/pull/431/files#diff-10
(115)

M test/serialport-c.js
https://github.com/voodootikigod/node-serialport/pull/431/files#diff-11
(14)

M test_mocks/linux-hardware.js
https://github.com/voodootikigod/node-serialport/pull/431/files#diff-12
(27)

Patch Links:

https://github.com/voodootikigod/node-serialport/pull/431.patch

https://github.com/voodootikigod/node-serialport/pull/431.diff

—
Reply to this email directly or view it on GitHub
#431.

rwaldron · 2014-12-31T04:59:55Z

arduinoTest/requiresComPort.js

These changes are irrelevant.

Irrelevant unless you want the lint step in the build process to pass. It complains when there are mixed quotes, among other things.

rwaldron · 2014-12-31T05:09:07Z

I'll test this with as many projects as I can tomorrow.

joshperry · 2014-12-31T15:35:26Z

I pushed this pretty early, just wanted to start getting feedback. It's not quite ready to be used, and may actually not even build yet. Hoping to make some rapid progress in the next couple days.

voodootikigod · 2014-12-31T15:37:46Z

@joshperry great work thus far, are you building on mac, linux, or windows (we will want to retain support for all three)?

joshperry · 2014-12-31T15:39:29Z

@voodootikigod I'm currently working on a Mac. Though, I'm not planning to make any changes to the native code so the existing state of cross-platform compatibility shouldn't be changed by this PR.

voodootikigod · 2014-12-31T15:46:15Z

Awesome!

Chris Williams

@voodootikigod http://twitter.com/voodootikigod | GitHub
http://github.com/voodootikigod

The things I make that you should check out:
SaferAging http://www.saferaging.com/ | JSConf http://jsconf.com/ |
RobotsConf http://robotsconf.com/ | RobotsWeekly
http://robotsweekly.com/

Help me end the negativity on the internet, share this
http://jsconf.eu/2011/an_end_to_negativity.html.

On Wed, Dec 31, 2014 at 10:39 AM, Joshua Perry [email protected]
wrote:

I'm currently working on a Mac. Though, I'm not planning to make any
changes to the native code so the existing state of cross-platform
compatibility shouldn't be changed by this PR.

—
Reply to this email directly or view it on GitHub
#431 (comment)
.

joshperry · 2014-12-31T15:47:55Z

I had initially planned to make this backwards compatible while still implementing the new features. As I've kind of hacked through some changes to get familiar with the code, I just don't think that's going to be the best route. I think it would be best to branch the current v1 code and merge this to be released as a major version change; 2.0 probably.

I plan to add migration instructions in the Readme. The changes shouldn't be too complex, mainly cleaning up error handling, and the read/write functions; parsers can be easily ported using through2.

No global emitter No open in constructor `list()` returns promise

Got testing set up a little more. Wrote tests and implemented the constructor. The constructor now only takes stream options and inits the object. Wrote tests and implemented the the `open` instance function. Wrote tests and implementd the `open` and `openPort` module functions. This is the recommended way to get an open SerialPort instance by passing options and an optional callback. Opening a port now gets `comName` from the options object. Opening callback is now subscribed to the open event.

joshperry · 2015-01-06T00:43:30Z

Well, constructing, opening, and reading seem to be working well now. I need tests for the reading, I was just hacking on it to figure out how the native stuff worked and kind of neglected the tests.

Going to wrap up writing next, and then the shutdown, and flush; gracefully handling errors should finish things up.

I began a rewrite/factor of the native stuff to make the platforms more homogenous with each other and with the other Node streams (tcp, udp, files), but I'm going to leave those changes out of this branch for now. Maybe when I get a little more time and gumption I'll get to that. Instead, I just made some shims (UnixSource and WindowsSource) that wrangle the native interface into a consistent shape for the rest of the code.

joshperry · 2015-01-06T01:46:14Z

I converted the terminal app to use streams and piping instead of dealing directly with data events. This should be a good example of how nice streams are to work with, well, streams of data.

joshperry · 2015-01-06T20:33:49Z

Getting close to done. Opening, reading, and writing all seem to work well when used with the data event, as a stream, and/or with the standard read/write functions. Just need to go over error conditions and gracefully closing the stream/port when there's an issue. Then, of course, documentation.

It should be useable if any of you want to pull the code and play with it against a branch of a project you have set up.

joshperry · 2015-01-14T17:15:27Z

Blarg. Doing some work on error and ending flows and realizing that the native Windows code is not in a good place.

First, it's push-only, so there's no way to control buffer backpressure. If you happen to have a rapid stream of data, at a high baud rate, you're going to blow some buffers, not that you'll know; ignorance is bliss as they say.

Secondly, the code goes catatonic in error conditions. Since we don't read from the native code, there is no path for communicating read errors. If you pull the USB device after opening the port, write just blocks indefinitely and doesn't return, throw, or callback with an error, ever.

Unfortunately this leaves us in a state where there's no way to write commercial-grade code for Windows on this foundation. Though, perhaps that's not the sort of thing that anyone is using this library for.

I guess, for now, that I'm just going to punt on errors in Windows until I attack the native codebase, but users should really be aware of this functional parity mismatch.

reconbot · 2015-01-18T15:40:38Z

One of the things that has irked me for a while is that we let the platform differences sneak up into the app logic. I'd love to see an abstraction in place that helps smooth this out.

joshperry · 2015-01-18T15:45:15Z

@reconbot Absolutely, and I've tried to do that here. The UnixSource and WindowsSource are meant to be that abstraction (at least for data flow) until I get the time to go into the native code. The biggest problem with that is that the Windows native code takes a reference to options.dataCallback when you call open() so that callback has to be set before that call. Because of this I have to create dataCallback outside of WindowsSource even though it's only needed on Windows.

reconbot · 2015-01-18T15:45:45Z

Oh never mind you're completly on top of that. I should have said.

It's amazing how you've put a shim between the different hardware abstractions. It's been irking me for ages!

reconbot · 2015-01-18T21:06:28Z

So here's the other side of things, browser serialport is doing a lot of work getting compatibility with node-serialport. I think a smaller api like you've exposed here would be all they'd have to do in this new design.

voodootikigod · 2015-01-18T22:04:49Z

Bringing in @phated and @jacobrosenthal to this convo.

Chris Williams

@voodootikigod http://twitter.com/voodootikigod | GitHub
http://github.com/voodootikigod

The things I make that you should check out:
SaferAging http://www.saferaging.com/ | JSConf http://jsconf.com/ |
RobotsConf http://robotsconf.com/ | RobotsWeekly
http://robotsweekly.com/

Help me end the negativity on the internet, share this
http://jsconf.eu/2011/an_end_to_negativity.html.

On Sun, Jan 18, 2015 at 4:06 PM, Francis Gulotta [email protected]
wrote:

So here's the other side of things, browser serialport is doing a lot of
work getting compatibility with node-serialport. I think a smaller api like
you've exposed here would be all they'd have to do in this new design.

—
Reply to this email directly or view it on GitHub
#431 (comment)
.

phated · 2015-01-18T22:28:01Z

As much as I like the idea of making this a stream interface, node streams are horrible. The largest glaring problem is one we see all the time in gulp: if a single error event is emitted, node destroys your entire stream pipeline. This requires a lot of hacky code to deal with and I don't know if the benefits outweigh that negative.

joshperry · 2015-01-19T00:05:26Z

@phated I agree that node streams have a few idiosyncrasies, especially in error handling, but they are unique and useful. Very similar to piping in a shell, if one of the commands between pipes fails the whole thing has to be run again.

Gulp itself is also pretty unique, as far as using node streams is concerned. I don't see many end-user projects doing a large amount of piping. Have you guys talked about a better way to do a custom stream interface for Gulp that would solve the issues that node streams have?

What is nice to have though is a standard interface for streaming data to and from something so that a socket can be swapped for a USB connection, for a serial connection, or even a for a file. We gain a lot from going this direction even if the user is only using the readable event, read(), and write(), or the data event.

phated · 2015-01-19T00:12:21Z

@joshperry actually, @chrisdickinson put a lot of work into a stream error handling solution that got completely ignored or (worse) torn apart by the node community (see nodejs/node-v0.x-archive#8306).

My concerns are that node-serialport is fairly stable currently and doesn't get slammed with tons bug reports and such. By switching to this streams interface, that likely won't be true anymore. The amount of bug reports that get opened on gulp and many gulp plugins for the errors and unpiping far exceed any other.

Like I said, I like the idea of node-serialport being a stream, but it's the little things that will bite you. P.S. - just thought of another one: if you don't end your pipeline with a writeable (I think) stream, the backpressure will cause flowing of object mode streams after 16 events. We are trying to overcome this issue in gulp by using https://github.com/chrisdickinson/stream-exhaust which has its own set of problems.

joshperry · 2015-01-19T00:30:22Z

@phated, node-serialport is already a stream. All I'm trying to do is make it a contemporary Streams2 stream. I picture most people still just gobbling up the data event like they do now.

I guess "fairly stable" is fair, but there are still all the horrible inconsistencies in the API and the deeper issues of the Windows native implementation that I'm trying to fix. To most users, if they are using serialport like the docs show, will only need to make maybe a 1LOC change to use v2, if they decide to upgrade the dep; otherwise they stay blissfully happy with their fairly stable v1 branch.

phated · 2015-01-19T00:31:57Z

Yes, I understand more than most the inconsistencies, but what about the parsers being through2 streams? That doesn't stay the same and if your pipeline explodes, "oh well"?

joshperry · 2015-01-19T00:48:31Z

Well, I thought having a parser as a through stream was an elegant solution. Having the last, and only, stream in the pipe isn't going to cause much of a headache, it would only be errors propagated down from serialport at that point. But honestly, the idea of a parser really doesn't make any sense to me, it's basically implementing something that's better done outside of the serial port.

Why is this:

var parser = function(emitter, data) {
  // parsey, parse, parse
  emitter.emit('awesomesauce', parsed);
}
port = new SerialPort('/dev/usbthingy', { parser: parser });

consumer(port);

Better than (I don't like this way either, but it's a direct analog):

function parse(data) {
  // parsey, parse, parse
  this.emit('awesomesauce', parsed);
};
port.on('data', parse);

consumer(port);

I'm cool if we prefer to document this, or something similar, as the preferred way to deprecate parsers.

jacobrosenthal · 2015-01-19T01:35:22Z

I'd love to separately talk about native code cleanup.
#443

reconbot · 2015-01-19T15:20:41Z

I think the native code cleanup is important, but I think I'd actually like to steal the WindowsSource and UnixSource and bring them into our current serial port implementation. I think defining a consistent api at a lower level of abstraction will force us to refactor the native code a bit.

voodootikigod · 2015-05-07T14:58:26Z

Status on this is....

reconbot · 2016-04-10T04:33:08Z

I'm going to close this issue due to age.

@joshperry I'm really sorry this didn't get merged at the time. The project has diverged far enough from 2014 that this PR is no longer mergable, but it still contains a ton of great ideas and goals and a reference for how it could work. I've opened #746 to discuss api changes for our next major release. I was considering an incremental change to the api to improve usability while giving us time to restructure the bindings layer, but I think that restructuring should consider what streams2 might demand of it.

Initial commit

a035a24

rwaldron reviewed Dec 31, 2014
View reviewed changes

voodootikigod mentioned this pull request Dec 31, 2014

Consider a change to the API to derive from Node Stream #246

Closed

joshperry added 10 commits December 31, 2014 10:12

Replace deprecated

dd9e638

No global emitter No open in constructor `list()` returns promise

Merge remote-tracking branch 'upstream/master' into streams2

2504d3e

Fix dependency

f5ef189

Test options, change from sandboxed to proxyquire

040abe5

Get platform from the os module

5d7649f

Goodbye parsers

87e103a

Work on opening port

addd918

Fix line endings

56c829b

Initial read logic, need tests

118b393

joshperry force-pushed the streams2 branch from c1a8c01 to 118b393 Compare January 6, 2015 00:20

joshperry added 2 commits January 5, 2015 17:26

Fix buffer math

9142964

Modify terminal bin to use new code

153467b

Convert terminal to use piped streams

5bded04

joshperry added 2 commits January 6, 2015 13:21

Initial read/write tests and impl

ee743a8

Fix case insensitive options

9bd2b65

private host v2 binaries for now

23e1c56

joshperry force-pushed the streams2 branch from 469571e to 23e1c56 Compare January 14, 2015 17:04

joshperry and others added 6 commits February 27, 2015 13:15

Begin updating readme

de6ceab

Multiple serial ports will now have their own write queue and thread.

22a33ea

Minor tweaks to close handling of write queue.

1912fee

Temporarily removing cleanup on close.

30e72df

Fixed uninitialized pointer.

db20b77

Merge multi write buf patch

ddef966

joshperry added 4 commits June 15, 2015 20:01

Add close() functionality

ca42d15

Set hardware flowcontrol when rtscts is true

20141e3

Version bump

fc1dad0

Nan v2 and Node v4 support

cd86856

reconbot closed this Apr 10, 2016

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SerialPort Streams2 Implementation #431

SerialPort Streams2 Implementation #431

joshperry commented Dec 31, 2014

reconbot commented Dec 31, 2014

more complex than it need be.

node Streams2

rwaldron Dec 31, 2014

joshperry Dec 31, 2014

rwaldron commented Dec 31, 2014

joshperry commented Dec 31, 2014

voodootikigod commented Dec 31, 2014

joshperry commented Dec 31, 2014

voodootikigod commented Dec 31, 2014

joshperry commented Dec 31, 2014

joshperry commented Jan 6, 2015

joshperry commented Jan 6, 2015

joshperry commented Jan 6, 2015

joshperry commented Jan 14, 2015

reconbot commented Jan 18, 2015

joshperry commented Jan 18, 2015

reconbot commented Jan 18, 2015

reconbot commented Jan 18, 2015

voodootikigod commented Jan 18, 2015

phated commented Jan 18, 2015

joshperry commented Jan 19, 2015

phated commented Jan 19, 2015

joshperry commented Jan 19, 2015

phated commented Jan 19, 2015

joshperry commented Jan 19, 2015

jacobrosenthal commented Jan 19, 2015

reconbot commented Jan 19, 2015

voodootikigod commented May 7, 2015

reconbot commented Apr 10, 2016

SerialPort Streams2 Implementation #431

SerialPort Streams2 Implementation #431

Conversation

joshperry commented Dec 31, 2014

reconbot commented Dec 31, 2014

more complex than it need be.

node Streams2

rwaldron Dec 31, 2014

Choose a reason for hiding this comment

joshperry Dec 31, 2014

Choose a reason for hiding this comment

rwaldron commented Dec 31, 2014

joshperry commented Dec 31, 2014

voodootikigod commented Dec 31, 2014

joshperry commented Dec 31, 2014

voodootikigod commented Dec 31, 2014

joshperry commented Dec 31, 2014

joshperry commented Jan 6, 2015

joshperry commented Jan 6, 2015

joshperry commented Jan 6, 2015

joshperry commented Jan 14, 2015

reconbot commented Jan 18, 2015

joshperry commented Jan 18, 2015

reconbot commented Jan 18, 2015

reconbot commented Jan 18, 2015

voodootikigod commented Jan 18, 2015

phated commented Jan 18, 2015

joshperry commented Jan 19, 2015

phated commented Jan 19, 2015

joshperry commented Jan 19, 2015

phated commented Jan 19, 2015

joshperry commented Jan 19, 2015

jacobrosenthal commented Jan 19, 2015

reconbot commented Jan 19, 2015

voodootikigod commented May 7, 2015

reconbot commented Apr 10, 2016