Adding WindowsBinding reading #1018

munyirik · 2016-11-17T03:36:07Z

@reconbot I took a stab at #994 today.
I've only done minimal testing but I wanted to open the PR early and get your feedback.

reconbot

Since we loop until we have some data we don't need to reque any more work.

reconbot · 2016-11-17T14:15:22Z

src/serialport.cpp

@@ -681,6 +731,7 @@ extern "C" {
    Nan::SetMethod(target, "open", Open);
    Nan::SetMethod(target, "update", Update);
    Nan::SetMethod(target, "write", Write);
+    Nan::SetMethod(target, "read", Read);


Should we move all of this to serialport_win.cpp

I'd like to leave it since serialport_uwp will also use it. I can use ifdef WIN32 since the macro is defined when UWP is used.

or maybe I could keep it in a 'common' cpp file. I could avoid using an ifdef if there was something like a SetExtraMethods() in the init function where native bindings could add extra methods (in this case read) or just do nothing. What do you think?

lets just ifdef win32 but make sure to put the after read in this file too

reconbot · 2016-11-17T14:15:43Z

src/serialport.h

@@ -26,6 +26,10 @@ NAN_METHOD(Write);
 void EIO_Write(uv_work_t* req);
 void EIO_AfterWrite(uv_work_t* req);

+NAN_METHOD(Read);
+void EIO_Read(uv_work_t* req);


And this stuff inside an ifdef

reconbot · 2016-11-17T14:21:38Z

src/serialport_win.cpp

+  if (data->errorString[0]) {
+    argv[0] = Nan::Error(data->errorString);
+	argv[1] = argv[2] = Nan::Undefined();
+	Sleep(100); // prevent the errors from occurring too fast


Is this left over from when we used to loop endlessly?

Yes. Is the sleep still needed? I didn't want to take it out in case it was there for a good reason.

Previously we'd emit c++ buffers as fast as we could, now we take a js buffer write to it and return. I can't imagine why we'd want to sleep. You have my blessing to rip out anything you don't understand.

reconbot · 2016-11-17T14:23:17Z

src/serialport_win.cpp

+  } else {
+    argv[0] = Nan::Null();
+	argv[1] = Nan::New<v8::Integer>((int)data->bytesRead);
+	argv[2] = Nan::NewBuffer(data->bufferData, data->bufferLength, FinalizerCallback, req).ToLocalChecked();


Yeah, we have a buffer from js land, this path seems not needed

I was following the readcallback signature. Should I just leave it undefined or is there a way to assign the buffer pointer to argv[2]? ...not sure how to do that.

You're right. That path isn't needed. I hadn't initially realized that the callback is actually the onRead function in push-methods :-|

reconbot · 2016-11-17T14:27:01Z

👏👌

reconbot · 2016-11-19T16:50:09Z

src/serialport.h

@@ -141,6 +145,17 @@ struct QueuedWrite {
  }
 };

+struct ReadBaton {
+  int fd;
+  char* bufferData;


So I think the crux of the change will be to make this a pointer to a v8::Local<v8::Object> buffer object, that way we can pass it all the way through. Write to it and then pass it to the callback. I don't know how to write to a buffer as if it were a char[] maybe @indutny can throw us some pointers.

I'd say Persistent rather than Local. Local is for something on-stack.

- Fix CI failure to build - Tests (including read) now pass locally - Changes based on review feedback

munyirik · 2016-12-09T18:46:52Z

@reconbot could you take a look at the latest updates when you get a chance.

reconbot

Looking pretty good!

@Zensey brought up CreateIoCompletionPorts which should be on our radar for future PRs.

I'd like to land this soon =)

reconbot · 2016-12-09T21:09:05Z

lib/bindings-win32.js

-WindowsBinding.prototype.read = require('./read-unix');
+WindowsBinding.prototype.read = function(buffer, offset, length, cb) {
+  if (!Buffer.isBuffer(buffer)) {
+    throw new TypeError('"buffer" is not a Buffer');


I'm pretty sure your c++ throws would bubble up just fine for type checking

Yeah seems like overkill

reconbot · 2016-12-09T21:13:14Z

src/serialport.cpp

+    argv[1] = Nan::Undefined();
+  } else {
+    argv[0] = Nan::Null();
+    argv[1] = Nan::New<v8::Integer>((int)data->bytesRead);


We should be calling the read callback with the buffer as the 3rd arg. The decision was to stay in party with fs.read.

Ok. I had left it out since the push-methods _read wasn't using that parameter - https://github.com/EmergingTechnologyAdvisors/node-serialport/blob/master/lib/push-methods.js#L31

reconbot · 2016-12-09T21:16:08Z

src/serialport_win.cpp

+    DWORD bytesReadSync = 0;
+    if (!ReadFile((HANDLE)data->fd, offsetPtr, data->bytesToRead, &bytesReadSync, &ov)) {
+      errorCode = GetLastError();
+      if (errorCode != ERROR_IO_PENDING) {


might be worth looking at #1020

Thanks. I'll keep my eye on it. Looks like someone's still able to repro with the proposed fix.

reconbot · 2016-12-10T18:15:50Z

I realized the comments I made aren't blocking, so lets land this. =)

Adding WindowsBinding reading

Adding WindowsBinding reading

975b270

reconbot reviewed Nov 17, 2016

View reviewed changes

reconbot reviewed Nov 19, 2016

View reviewed changes

Munyiri Kamau added 2 commits November 21, 2016 16:30

Fix build and test issues + updates based on review feedback

7276817

- Fix CI failure to build - Tests (including read) now pass locally - Changes based on review feedback

Remove space before function parentheses

91d7f4a

reconbot requested changes Dec 9, 2016

View reviewed changes

reconbot approved these changes Dec 10, 2016

View reviewed changes

reconbot merged commit b80025e into serialport:master Dec 10, 2016

reconbot added a commit that referenced this pull request Jul 24, 2018

Merge pull request #1018 from munyirik/win32read

53f1f8b

Adding WindowsBinding reading

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

Adding WindowsBinding reading #1018

Adding WindowsBinding reading #1018

munyirik commented Nov 17, 2016

reconbot left a comment

reconbot Nov 17, 2016

munyirik Nov 17, 2016

munyirik Nov 17, 2016

reconbot Nov 17, 2016

reconbot Nov 17, 2016

reconbot Nov 17, 2016

munyirik Nov 17, 2016

reconbot Nov 17, 2016

reconbot Nov 17, 2016

munyirik Nov 18, 2016

munyirik Nov 22, 2016

reconbot commented Nov 17, 2016

reconbot Nov 19, 2016

indutny Nov 19, 2016

munyirik commented Dec 9, 2016

reconbot left a comment

reconbot Dec 9, 2016

munyirik Dec 9, 2016

reconbot Dec 9, 2016

munyirik Dec 9, 2016

reconbot Dec 9, 2016

munyirik Dec 9, 2016

reconbot commented Dec 10, 2016

Adding WindowsBinding reading #1018

Adding WindowsBinding reading #1018

Conversation

munyirik commented Nov 17, 2016

reconbot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reconbot commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

munyirik commented Dec 9, 2016

reconbot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reconbot commented Dec 10, 2016