Closed exsilium closed 6 years ago
First of all, thank's a lot this is much appreciated! π
I think it should be discussed a bit more first.
Duplex streams are readable/writable and both ends of the stream engage in a two-way interaction, sending back and forth messages like a telephone. An rpc exchange is a good example of a duplex stream.
- Sounds exactly how we should implement a streaming xbee API, since the command frames/AT commands are basically RPC over serial? Or am I missing something?Hi! Thanks for the feedback, this was actually my goal - to get more concrete discussions going on how to move forward. As it stands now, the StreamTransformer can be implemented quite easily independently from xbee-api, so this Pull Request also serves as awareness for others stuck with old node-serialport
due to the missing custom transformer. Feel free not to merge this π
I agree that this solution is not the most elegant nor will it solve all the problems but I'd like this module to stay relevant and to do so there must be mechanisms in place so that it can be used with the modern Node stack (read: node-serialport
).
There are always people who are tied to older versions of something and that's OK, but I think it's more fair of them to stay behind and lock themselves to an older version of xbee-api
, than to keep supporting Node versions which are End-of-Life going forward. It's almost a year since the LTS Maintenance was dropped for Node v0.x and soon v4 will be next. #cantkillprogress ;) But of course it's up to you if the requirement is there to keep supporting the older versions (and there's still demand for it) of node (as well as older versions of node-serialport
for that matter).
Code duplication is indeed a lazy way to handle this. π Again, not meant as a permanent solution to the problem but I didn't want to disrupt the code that works well and people rely on. One of the key questions here is also for how long a custom parser interface should be kept going. I think it's a good idea to merge the parsing logic going forward.
Well, the StreamTransformer extends the Transformer and you only instantiate it if you actually need one. It's especially true if the xbee-api
is to remain functional accross all the versions of node-serialport
you need to provide 2 different interfaces - this is what node-serialport
expects.
As for the Duplex vs Transform stream, what would be the usefulness of Duplex stream if we just read it? In any case, I'm not an expert myself, node-serialport
's suggestion to custom parser upgrade path was to migrate to custom transformers and I started moving in that direction.
Thanks for your response. Indeed, I don't think it would be a good idea to merge this as it is, since it will only cause more work. I will check out your PR and see if I can't make both interfaces share the same parser. As for the discussion:
Could you try and see if 01630e3b497c2012f0dafb845103718f6b4fa227 works for you?
Thanks! I'll try it this evening and get back to you!
Ok, I see that you only push the frame object that we get via the data
event and you don't emit the frame_object
when using the StreamTransformer. In any case, this works wonderfully! π Ship it! π’ :shipit: π π₯ π° π
Yeah I thought that there is no need to be "backwards compatible" (i.e. using the "frame_data" event name besides the stream standart 'data'), as the event is now coming from the newly introduced parser/transformer object anyway, and not the xbeeApi object. IF we decide on this implementation, I would probably try make the parser/transformer object have the "frame_data" event emitted back on the xbeeApi object.
However, I'm considering whether a complete streaming implementation isn't preferable, where the xbeeApi object is both a read stream that transforms from raw serial to frame_objects, as well as a write stream that transforms frame_objects to raw serial. So instead of:
var parser = xbeeAPI.newStream();
serialport.pipe(parser);
serialport.write(xbeeAPI.buildFrame(frame_obj));
parser.on('data', function(frame_obj) { ... });
We can do:
serialport.pipe(xbeeAPI).pipe(serialport);
xbeeAPI.write(frame_obj);
xbeeAPI.on('data', function(frame_obj) { ... });
I think this should even be possible next to and without breaking the non-streaming api. I'm just not quite sure if this needs two transform streams or is actually exactly one duplex stream...
Personally, I fail to see the practical value of a write stream in this case. Apart from having the same interfacing principles in both ways, what's the potential use case? Streams are preferred and used in places where all the data is not available due to some limitation (e.g big data & the amount of memory available). To build a frame and to send it - I view it as a transaction. node-serialport
.write is an operation with a string or a buffer in a finite length.
Reading back via serialport is a bit different (especially when we have radios attached). On a slow link the data comes in chunks so the Streaming interface makes more sense in such scenario. But of course you can experiment and iterate further on - I just hope you can publish the reading part while you're at it. :bowtie: π
I see your points, but having the same interfacing principles in both ways is more than reason enough for me.
What is more, if we're adding a new interface to use the api, as a user, I don't want to bother with these details. Those familiar with the stream paradigm might expect: "if data comes out like an (object) stream, data should go in like an (object) stream". So why have a half assed version.
Finally, there is no reason why xbee-api couldn't be a middle piece in some streaming application in such a non-finite data scenario you describe. Think of streaming a live video stream to an xbee-enabled LED matrix, by chunking up the video data and putting it in the payload of transmit requests (which are then piped to xbeeAPI).
Or perhaps some HTTP over XBee setting, where our application route, where a transform pipe filters out payload from data packets containing http requests and forwards them to an http pipe, transforming the response back to transmit requests with the http response in the payload.
The interface in a66f3796367bf59b4bfdfdff29aded3013d4ea67 works without the extra StreamTransform class and makes two transform stream objects inline. One "xbeeAPI.parser", one "xbeeAPI.builder".
This version should not break things, and interfaces the stream api even tighter with the xbeeAPI functions, as there is no need to copy/reference half of the xbeeAPI class into the new class. I don't think instantiating the two transformers even if not used will cause any overhead. And the state of the xbeeAPI object will not be affected by them until actually connected to a pipe.
Here the gist:
var xbeeAPI = new xbee_api.XBeeAPI();
serialport.pipe(xbeeAPI.parser);
xbeeAPI.builder.pipe(serialport);
xbeeAPI.builder.write(frame_obj); // build and send frame to xbee
xbeeAPI.parser.on('data', function(frame_obj) { ... }); // parsed frame from xbee
Really nice! Thanks! Tried this and it works well and have to admit the consistency of the API looks good as well! Both the traditional buildFrame()
as well as the stream write seems to work nicely and should make the upgrade path friction free! π π
Hi!
Serialport is already at v6, I thought to get us somewhere by providing alternative frame parsing mechanism via Transform interface. Or at least start us discussing how to provide a solution for using higher version serialport. Preserves backwards compatibility with serialport 4 and also keeps the
frame_object
event for smooth transition. Addresses #62.Drops end-of-life node versions, min node engine >=4, added new node versions to travis. devDep serialport to version 6.
Feedback welcome, To use/test the new parser, here's an example:
Cheers! π»