Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use upstream implementation of native bindings #44

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

eelcocramer
Copy link

I notice that a lot of your current problems have already been fixed in the upstream module, which I maintain, and where you copied the sources from.

This PR makes you module depend on the upstream module so you are always using the latest sources and allow you to focus on maintaining your own javascript bindings.

@eelcocramer
Copy link
Author

Will most likely fix #40 and #45

@mdsaddam
Copy link

mdsaddam commented Nov 6, 2019

@eelcocramer any time lines on fixing issue #45 ?

@eelcocramer
Copy link
Author

May also fix #48.

@frankjoke
Copy link

@eelcocramer , can you fork this repo to your account and do the changes there so it can be used?
I work on different linux systems (Example Debian buster10 or Ubuntu) for ioBroker and used the library in the past, but now I need to test apps also on node V12 and it does not work anymore there.

@eelcocramer
Copy link
Author

eelcocramer commented May 12, 2020

@frankjoke the forked repo is already there. This PR originates from there. You can find the link below the title where it says eelcocramer:master but I've also added it here for your convenience ;-)

@frankjoke
Copy link

frankjoke commented May 13, 2020

Ok, I tried this and could not load it, got
` [email protected] install /home/pi/ioBroker.radar2/node_modules/node-bluetooth

node node_modules/copyfiles/copyfiles -f node_modules/bluetooth-serial-port/build/Release/BluetoothSerialPort.node Release
internal/modules/cjs/loader.js:960
throw err;
^
Error: Cannot find module '/home/pi/ioBroker.radar2/node_modules/node-bluetooth/node_modules/copyfiles/copyfiles'
at Function.Module._resolveFilename (internal/modules/cjs/loader.js:957:15)
at Function.Module._load (internal/modules/cjs/loader.js:840:27)
at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:74:12)
at internal/main/run_main_module.js:18:47 {
code: 'MODULE_NOT_FOUND',
requireStack: []
}
if I link it directly. I believe that you use a module copyfiles which is not loaded automatically, instead integrating directly your bindings into the code.
Did you create an npm package from it which could be included?
npm init -scope=@eelcocamer npm publish --access public
In this way we could include it as @eelcocramer/node-bluetooth in our package.json with normal npm handling.
You would need to change the installation instruction in that way as well.

p.s.: I am not in in writing/managing nodejs code with bindings and C++ therefore I doi not kinow exactly what is needed there. I am mainly writin web apps or home automation adapters and tools.

@eelcocramer
Copy link
Author

Well, I'm not going to publish it. But you can fork it and publish it yourself if you need it.

@frankjoke
Copy link

I tried this and managed after a view changes to your install script to get a module which could be installed on multiple systems. Youi can find it here https://github.com/frankjoke/node-bluetooth and it's npm is @frankjoke/node-bluetooth.
but: It does not work! On one syste I get a crash of node if I start .scan() and on the other one scan is never returning and never finding anything. It seems that the the bindings were maybe not tested on many systems.

@lsongdev lsongdev self-requested a review May 14, 2020 02:05
@rzr rzr mentioned this pull request Jun 12, 2021
@rzr
Copy link

rzr commented Aug 12, 2021

Feedback also welcome at

#55

@eelcocramer
Copy link
Author

For the convenience of folks that use the javascript patterns used in @song940's module I've updated my PR to use the experimental C sources from my fix branch. So you can now use my fork of his repo to verify if these fixes work.

I don't think the inquiry functions work but for some folks reading and writing may be fine.

Feedback is appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants