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

Mojave support #804

Closed
wants to merge 1 commit into from
Closed

Mojave support #804

wants to merge 1 commit into from

Conversation

drandreas
Copy link

@drandreas drandreas commented Jun 10, 2018

Inspired by @jacobrosenthal's approach in #727. I've started to fix Mojave support. Device, service and characteristic discovery works - also read, write and notify seam to be fine.

While playing with frida I noticed that handleMsg has only one argument. I've included a fixed trace-routine below:

CBXpcConnection__handleMsg.js

  onEnter: function (log, args, state) {
    log("-[CBXpcConnection _handleMsg:" + args[2] + "]");
    
    if(args[2]) {
          var obj = ObjC.Object(args[2]);
          log(obj.toString())
    }
  },

@drandreas drandreas force-pushed the master branch 3 times, most recently from 0689abf to 926b11e Compare June 24, 2018 21:37
@drandreas drandreas force-pushed the master branch 3 times, most recently from 354723a to cb15926 Compare July 15, 2018 19:47
@mane87
Copy link

mane87 commented Jul 20, 2018

@drandreas Any update on this one?
I would love to support you. Is there anything i can help you with?

As the Mojave release is getting closer i would love to get this merged asap.

@drandreas
Copy link
Author

drandreas commented Jul 20, 2018

I've tested my implementation on developer beta 4 and all features except those work fine:

  • Reade value directly from handle
  • Write value directly to handle
  • Discover Included Services

I haven't found any native application that incorporates those. Hence, I can't reverse the kCBMsgIds. Moreover, the kCBMsgId changed from developer beta 2 to 3. Even though this version works currently I've no idea what the Ids of the final release will be.

@drandreas
Copy link
Author

drandreas commented Aug 6, 2018

I've just tested it with MacOS Mojave Beta 5. All commands still work.

@drandreas
Copy link
Author

drandreas commented Aug 7, 2018

My changes work on MacOS Mojave Beta 6 too. There is a pretty good chance that the current IDs are final.

@reidmweber
Copy link

reidmweber commented Aug 15, 2018

We just brought these changes into our app and I tested it on Mojave 18A365a. Device discovery works but connecting to devices doesn't work. We'll dig in more once we have a development environment set up. Are connections working for anyone else?
Looking at the comments, I'm thinking we'll need these:
Reade value directly from handle
Write value directly to handle
Discover Included Services

Is there any other way to get the kCBMsgId's?

@drandreas
Copy link
Author

I did a quick check and indeed the kCBMsgId's have shifted again. Connection request remained 48 but the response is now 67.

The kCBMsgId's used for XPC are defined at build time. They seem to be deterministic and shifting by just +/- 1. I assume if a OS developer removes or adds a function all following IDs are shifted.

Constantly adjusting the IDs is tedious. Perhaps noble should bind to /System/Library/Frameworks/IOBluetooth.framework//IOBluetooth and use the official API. (I must admit I didn't assess the required effort for such a refactoring.)

@ghost
Copy link

ghost commented Aug 20, 2018

@drandreas : using that /System/Library/Frameworks/IOBluetooth.framework//IOBluetooth seems like the way to go in the long term, but in the meantime, it might be a good idea to do something like this:

// at the top of the file
var DISCONNECT_ID = 'whatever';
var CONNECT_ID = 'whatever';

// wherever you use sendCBMsg
this.sendCBMsg(CONNECT_ID, whateverArgs);

// where it listens
nobleBindings.on('kCBMsgId' + CONNECT_ID, function() {
  // stuff
});

Although if you do with with that approach it'd probably be better to change those IDs i mentioned above to their full names like:

change
this.emit('kCBMsgId' + kCBMsgId, kCBMsgArgs);
to
this.emit(kCBMsgId, kCBMsgArgs);

and write it all like:

// top of the file
var CONNECT_ID = 'kCBMsgId22'; // not actually 22, but whatever it is.

// wherever you use sendCBMsg
this.sendCBMsg(CONNECT_ID, whateverArgs);

// where it listens
nobleBindings.on(CONNECT_ID, function() {
  // stuff
});

If the differences are minimal enough you could use the same bindings for Mojave as for High Sierra with this approach by doing:

if (highSierra) {
// High Sierra Ids
} else {
  //  Mojave Ids
}

This is a similiar approach as the legacy mac bindings use.
I've never actually used a mac, so perhaps it's not that easy though.

@drandreas
Copy link
Author

In lib/mac is a file for each major OS X release. They are except the IDs almost identical. So I assume until now the approach was to copy last years file and modify the IDs.

It would be great to create a 2D Dictionary [OS][MSG Type] => ID.
But who is going to Install several OS X versions to verify it?

@ghost
Copy link

ghost commented Aug 22, 2018

I've done a diff between some versions, and the difference (other than the ids) is almost nil even between the last version that supports bluetoothd vs blued. The only odd thing out is that highsierra for example doesn't implement broadcast while previous versions do.

At some point I'm going to submit a real PR to remove legacy and mavericks so those two will be a nonfactor.

@geovie
Copy link

geovie commented Aug 27, 2018

I've updated the ids for latest 10.14 beta 8 see Timeular@e930579
The functionality should be complete, every call was tested (except for broadcast which is not implemented)

Let's hope Apple doesn't change them again 🤞

@drandreas Feel free to incorporate the changes in this PR

@drandreas
Copy link
Author

@geovie thx for fixing the ids. I have dropped my changes in favour of your updated one.

@geovie
Copy link

geovie commented Sep 18, 2018

Hi, I rewrote the mac bindings to use native modules and the official CoreBluetooth API
If anyone is interested please check it out Timeular/noble-mac

@ghost
Copy link

ghost commented Sep 18, 2018

@geovie : AWESOME! Can you clarify (both here and the repo itself) which versions of Mac OS it supports? Does it matter if it's blued or bluetoothd?

Also, can you have it built on travis ci?

Can you also add engines to package.json to specify which versions of node is required?

@ghost
Copy link

ghost commented Sep 18, 2018

@geovie : are there any disadvantages to using it over the xpc variant? If not, then it'd probably be a good idea to remove the current mac bindings completely and then submit a PR that makes changes like this https://github.com/noble/noble/pull/646/files

@geovie
Copy link

geovie commented Sep 18, 2018

@jrobeson The CoreBluetooth API was introduced in macOS 10.7, so it should work starting from Mac OS X Lion. I tested it on Mojave for now. (I will add this to the readme)

It should not matter if it's blued or bluetoothd as we're using the official API which is abstracting it.

I will setup travis and node-pre-gyp soon. See Timeular/noble-mac#1

It uses node-addon-api which states in the readme that: Node.js newer than 6.14.2 is required (will update package.json)

I don't see any disadvantages (xcode is needed to build it, but that also needed for the xpc-connection package).

The goal is to integrate it in this repo. I opened #828 to see if there is any interest.
I would also appreciate some testing and feedback from others before submitting a PR.

@drandreas
Copy link
Author

drandreas commented Oct 1, 2018

I've tested Timeular/noble-mac thoroughly on OS X Mojave and it works like a charm. I encourage others to test the future proof approach and report back in #828.

@chrisbartley
Copy link

@geovie @drandreas It's failing for me on Mojave seemingly randomly with "Segmentation fault: 11". Maybe I'm just using it in a dumb way? Apologies for the dumb question, but what's the recommended way to use noble-mac since it's not in npmjs? I've cloned it, built with npm install and then tried just moving the whole noble-mac directory into the node_modules directory of my project, and changing require('noble') to require('noble-mac') in my code. Doing that and running my app initially failed the first few times with the segfault 11. I tried adding some debug statements to my code to see exactly where it was failing, then it magically stopped segfaulting. Then I removed the debug statements and it still isn't segfaulting. Strange. I turned off bluetooth just to see what my app would do with noble-mac, and it segfaulted once, then I tried again--changing no code--and it worked and now I can't get it to segfault again.

@geovie
Copy link

geovie commented Oct 2, 2018

@chrisbartley You can use the repository as git dependency https://docs.npmjs.com/files/package.json#git-urls-as-dependencies I guess the problems are because of your usage of the module. If the problem persists please open an issue in the noble-mac repository

@tobymarsden
Copy link

@chrisbartley @geovie @drandreas can confirm I'm seeing the same thing here on Mojave with noble-mac. Got segfault 11 the first three times I tried to scan, made no changes and then on the fourth and subsequent times it works like a charm.

@geovie
Copy link

geovie commented Oct 10, 2018

@tobymarsden Does the segfault appear with the samples from the repository? Do you handle unhandled exceptions in your code? Can you try to debug this with xcode? You'll have to create a xcode project with yarn xcode, then set the executable path to node and your script as argument see here

@tobymarsden
Copy link

@geovie I was testing it out from the REPL so no exception handling involved. The problem is after the first three segfaults I can't reproduce it at all, so debugging's going to be tricky...!

@fguchelaar
Copy link

Tested Timeular/noble-mac on a fresh macOS Mojave with Xcode 10, node v8.12.0 and npm 6.4.1.

Had no issues with it, works as advertised.

@antonj
Copy link

antonj commented Oct 25, 2018

Tested this as a subdependency for https://github.com/nathankellenicki/nuimojs works fine so far, maxOS 10.14 (18A391)

@jmunowitch
Copy link

Tested this. Works great so far.

@anatoliykot
Copy link

New update, Mojave 10.14.1 and it again doesn't work 🙁

@geovie geovie mentioned this pull request Nov 9, 2018
@geovie
Copy link

geovie commented Nov 9, 2018

FYI I opened PR #840 which integrates the changes from noble-mac

@drandreas
Copy link
Author

Since the IDs are obsolete again, I'm closing thins in favour of #840.

@drandreas drandreas closed this Nov 10, 2018
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.

None yet

10 participants