search

santigimeno / node-pcsclite

Bindings over pcsclite to access Smart Cards
ISC License
170 stars 125 forks source link

Fixes for Windows #31

Closed f0zi closed 9 years ago

f0zi commented 9 years ago

Please note that I moved a mutex unlock from HandleReaderStatusChange to HandlerFunction. This might need some additional testing/fixing.

santigimeno commented 9 years ago

@f0zi First of all thanks for the patch! Just to let you know the branch attached_threads should work on windows: it uses threads/mutexes/semaphores from libuv instead of using pthreads. I haven't had time to test it thoroughly so I haven't merged it yet. If you could test it on windows it would be great :).

santigimeno commented 9 years ago

See https://github.com/santigimeno/node-pcsclite/pull/24

f0zi commented 9 years ago

Is there a reason you are using a 3rd party thread library instead of the standard C++ threads? I saw that branch and I did not want to go with that library.

santigimeno commented 9 years ago

Because nodejs already uses libuv to implement cross-platform functionality (not only threading)

f0zi commented 9 years ago

But you don't link to nodejs, you build your own dll. And since C++13 std::thread and std::mutex is the way to go, all 3rd party libraries are just a legacy. I would not be surprised if nodejs will eventually switch to the standard libraries as well. Using libuv is just another unnecessary risk now.

santigimeno commented 9 years ago

I don't know, maybe you're right. I don't think nodejs/iojs is going to stop using libuv anytime soon though, nor libuv stop supporting threads. In addition, std::threads is pretty new and only supported in newer versions of compilers. For example in debian wheezy (gcc version 4.7.2) the support is experimental. So I think that for the moment sticking with libuv is not such a bad idea.

f0zi commented 9 years ago

Ok, fair enough.

santigimeno commented 9 years ago

@f0zi anyway, it would be great if you could incorporate the changes in common.h for windows in the attached_threads branch. Thanks :)

f0zi commented 9 years ago

@santigimeno Sorry, I hit a snag with git, not sure how I can extract these without doing another clone in github, and I have other changes in my copy already as I'm trying to get this working in atom-shell. That code is just one block and pretty much comes from here: https://msdn.microsoft.com/en-us/library/ms680582 If you don't mind, can you pull in that change into your branch? Thanks.

santigimeno commented 9 years ago

@f0zi I have incorporated your changes in https://github.com/santigimeno/node-pcsclite/pull/32. Thanks!