jperkin / node-rpio

Raspberry Pi GPIO library for node.js
857 stars 123 forks source link

Feature/windows mock #111

Closed trankin closed 4 years ago

trankin commented 4 years ago

I've made some changes that allow me to use your library, but to be able to use Windows for logic testing and other such outside of GPIO. These changes simply mock all of the bcm2835.c functions so that it will force the system into mock mode when running on Windows.

I've tested this on Windows and confirmed mock mode and I've tested on my Raspberry Pi 4 to confirm that the existing GPIO features I was using still work as expected.

jperkin commented 4 years ago

Thanks for the PR! How about this instead? Should be a lot cleaner and still achieve what you need:

diff --git a/binding.gyp b/binding.gyp
index 2eeb9b0..dc07da7 100644
--- a/binding.gyp
+++ b/binding.gyp
@@ -6,6 +6,13 @@
       "sources": [
         "src/bcm2835.c",
         "src/rpio.cc"
+      ],
+      "conditions": [
+        ['OS != "linux"', {
+          "sources!": [
+            "src/bcm2835.c"
+     ]
+   }]
       ]
     }
   ]
diff --git a/src/rpio.cc b/src/rpio.cc
index 3bf6f23..02c615d 100644
--- a/src/rpio.cc
+++ b/src/rpio.cc
@@ -15,7 +15,9 @@
  */

 #include <nan.h>
+#if !defined(_WIN32)
 #include <unistd.h>    /* usleep() */
+#endif
 #include "bcm2835.h"

 #define RPIO_EVENT_LOW 0x1
@@ -417,7 +419,14 @@ NAN_METHOD(rpio_usleep)

    uint32_t microseconds(Nan::To<uint32_t>(info[0]).FromJust());

+   /*
+    * Windows can only run in mock mode anyway, so just avoid sleeping
+    * rather than trying to find a Windows function that supports
+    * microseconds.
+    */
+#if !defined(_WIN32)
    usleep(microseconds);
+#endif
 }

 NAN_MODULE_INIT(setup)
jperkin commented 4 years ago

A cleaner patch for binding.gyp:

diff --git a/binding.gyp b/binding.gyp
index 2eeb9b0..b0f2b31 100644
--- a/binding.gyp
+++ b/binding.gyp
@@ -4,8 +4,14 @@
       "target_name": "rpio",
       "include_dirs": [ "<!(node -e \"require('nan')\")" ],
       "sources": [
-        "src/bcm2835.c",
         "src/rpio.cc"
+      ],
+      "conditions": [
+        ["OS == 'linux'", {
+          "sources": [
+            "src/bcm2835.c"
+         ]
+       }]
       ]
     }
   ]
trankin commented 4 years ago

Very nice, yes this is much cleaner, hah shows that this stack really isn't my go-to. I knew enough to solve my problem. One of the reasons that I would not have thought of this pattern is that I assumed that the externals still wouldn't be resolved?? Since we won't be building the bcm2835.c file? Not that it's necessary, but if you don't mind.. why does this work?

Anyway, I'll reset my branch and pull your suggestion in and test it.. then submit a new pull request if you want these changes in.

Thanks for the great library btw. knock on wood.. this is perfect and much better than the other offerings on nodejs. Very solid, very fast.. and works as expected in all of my use cases so far, even including power loss, initialization etc..

Thomas

On Fri, Jan 10, 2020 at 8:37 AM Jonathan Perkin notifications@github.com wrote:

A cleaner patch for binding.gyp:

diff --git a/binding.gyp b/binding.gyp index 2eeb9b0..b0f2b31 100644--- a/binding.gyp+++ b/binding.gyp@@ -4,8 +4,14 @@ "target_name": "rpio", "include_dirs": [ "<!(node -e \"require('nan')\")" ], "sources": [- "src/bcm2835.c", "src/rpio.cc"+ ],+ "conditions": [+ ["OS == 'linux'", {+ "sources": [+ "src/bcm2835.c"+ ]+ }] ] } ]

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jperkin/node-rpio/pull/111?email_source=notifications&email_token=AABH5SBCDBIY7MGB4DELKPTQ5CB3PA5CNFSM4KE6UJGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIUDIQI#issuecomment-573060161, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABH5SCTPLM2D3O4PGLBSJDQ5CB3PANCNFSM4KE6UJGA .

jperkin commented 4 years ago

If you were building it as an executable then yes it would fail to link, but as it's just a shared library then it's not a problem. The unresolved symbols are never called.

jperkin commented 4 years ago

I've published v1.5.0 to include this patch which should make testing easier.

trankin commented 4 years ago

Thank you very much for engaging in this. We are certainly outside of my comfort zone at this point. I have checked out the 1.5.0 version and am getting build errors on Windows, regarding unresolved externals.

screenshot of error messages below. https://sharex.tomrankin.net/ShareX/2020/01/pwsh_USbWWKcoES.png

I have confirmed that I still get a clean build on the Raspberry Pi 4.

Thanks again,

Thomas

On Fri, Jan 10, 2020 at 9:30 AM Jonathan Perkin notifications@github.com wrote:

I've published v1.5.0 to include this patch which should make testing easier.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/jperkin/node-rpio/pull/111?email_source=notifications&email_token=AABH5SGAPYJYCBBI7LLPN5TQ5CICFA5CNFSM4KE6UJGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIUIRIA#issuecomment-573081760, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABH5SDVUHFFMHEWEK7L4RDQ5CICFANCNFSM4KE6UJGA .

trankin commented 4 years ago

Also, just to confirm. I do have your latest commit, and these are different error messages than I was getting when building a clean 1.4.0 version. 1.4.0 version bombed resolving mman.h and unistd.h and never got to these symbols.

On Fri, Jan 10, 2020 at 10:34 AM Thomas Rankin tom@tomrankin.net wrote:

Thank you very much for engaging in this. We are certainly outside of my comfort zone at this point. I have checked out the 1.5.0 version and am getting build errors on Windows, regarding unresolved externals.

screenshot of error messages below. https://sharex.tomrankin.net/ShareX/2020/01/pwsh_USbWWKcoES.png

I have confirmed that I still get a clean build on the Raspberry Pi 4.

Thanks again,

On Fri, Jan 10, 2020 at 9:30 AM Jonathan Perkin notifications@github.com wrote:

I've published v1.5.0 to include this patch which should make testing easier.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/jperkin/node-rpio/pull/111?email_source=notifications&email_token=AABH5SGAPYJYCBBI7LLPN5TQ5CICFA5CNFSM4KE6UJGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIUIRIA#issuecomment-573081760, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABH5SDVUHFFMHEWEK7L4RDQ5CICFANCNFSM4KE6UJGA .

jperkin commented 4 years ago

Oh ok, I guess Windows does things differently. I'll fix this.

jperkin commented 4 years ago

I just published v1.5.2, want to try that?

jperkin commented 4 years ago

FWIW I figured out how to add Windows to the Travis tests, and that looks successful: https://travis-ci.org/jperkin/node-rpio/jobs/635375386

trankin commented 4 years ago

Thank you very much for this. Will speed up my workflow significantly.

On Fri, Jan 10, 2020, 11:36 AM Jonathan Perkin notifications@github.com wrote:

FWIW I figured out how to add Windows to the Travis tests, and that looks successful: https://travis-ci.org/jperkin/node-rpio/jobs/635375386

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/jperkin/node-rpio/pull/111?email_source=notifications&email_token=AABH5SCPKA4RTPOYYMUGKJ3Q5CW3HA5CNFSM4KE6UJGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIUU6YQ#issuecomment-573132642, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABH5SGU2BU2NODS4LGNDS3Q5CW3HANCNFSM4KE6UJGA .

trankin commented 4 years ago

Sorry for the long delay. Just wanted you to know that I grabbed your 1.7.1 build today and can confirm that it works perfectly on my existing app for both Windows and my Pi 4.

Thanks much for your support...... would love to send you some cash for beer (coffee, dinner) .. do you use any of the convenient services for sharing money? paypal, venmo,

On Fri, Jan 10, 2020 at 1:42 PM Thomas Rankin tom@tomrankin.net wrote:

Thank you very much for this. Will speed up my workflow significantly.

On Fri, Jan 10, 2020, 11:36 AM Jonathan Perkin notifications@github.com wrote:

FWIW I figured out how to add Windows to the Travis tests, and that looks successful: https://travis-ci.org/jperkin/node-rpio/jobs/635375386

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/jperkin/node-rpio/pull/111?email_source=notifications&email_token=AABH5SCPKA4RTPOYYMUGKJ3Q5CW3HA5CNFSM4KE6UJGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIUU6YQ#issuecomment-573132642, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABH5SGU2BU2NODS4LGNDS3Q5CW3HANCNFSM4KE6UJGA .

jperkin commented 4 years ago

Glad to hear it's working! Thanks for the offer to contribute, that's really kind. I've set up a liberapay account at https://liberapay.com/jperkin - any donations will be gratefully received and will go directly back into continued support for node-rpio.

jperkin commented 4 years ago

Thanks so much!