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

High Sierra support #727

Merged
merged 4 commits into from Jan 31, 2018

Conversation

Projects
None yet
7 participants
@jacobrosenthal
Member

jacobrosenthal commented Dec 26, 2017

The adafruit scanner seems to work on high sierra where LightBlue doesnt appear to be able to read characteristics.

You can play along at home if I missed your favorite xpcid. I used frida instead of swizzling, because I couldnt get swizzling to work. I think maybe the CBXpcConnection are now private and maybe they used to be public? but I had never done it before so it could be my failure.

For frida

$ frida-trace -m "-[CBXpcConnection sendMsg:args:]:"  -m "-[CBXpcConnection _handleMsg:]" *Adafruit*
Instrumenting functions...                                              
-[CBXpcConnection sendMsg:args:]: Loaded handler at "/Users/jacobrosenthal/__handlers__/__CBXpcConnection_sendMsg_args__.js"
-[CBXpcConnection _handleMsg:]: Loaded handler at "/Users/jacobrosenthal/__handlers__/__CBXpcConnection__handleMsg__.js"

You can see it autogenerated some javascript files we can use to inspect and debug
CBXpcConnection_sendMsg_args.js

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

CBXpcConnection__handleMsg.js

{
  onEnter: function (log, args, state) {
    log("-[CBManager sendMsg:" + args[2] + " args:" + args[3] + "]");

    if(args[3]){
          var obj = ObjC.Object(args[3]);
          log(obj.toString())
    }
  }
}

and just poked around the Adafruit app then later painstakingly decoded the logs

I grabbed all the xpcids I could find from #689, but I wanted to keep a little better git hygiene so started fresh but credit due there.

Please test

closes #679

Show outdated Hide outdated lib/mac/highsierra.js Outdated
Show outdated Hide outdated lib/mac/highsierra.js Outdated
@sublimator

This comment has been minimized.

Show comment
Hide comment
@sublimator

sublimator Dec 28, 2017

Thanks for this! Will report back test results

sublimator commented Dec 28, 2017

Thanks for this! Will report back test results

@elafargue

This comment has been minimized.

Show comment
Hide comment
@elafargue

elafargue Jan 7, 2018

Contributor

Seems to be working fine on my Mac with High Sierra 10.13.2, thanks for this PR!

Any chance those can get merged at some point? I noticed there hasn't been a release on this package for a while...

Contributor

elafargue commented Jan 7, 2018

Seems to be working fine on my Mac with High Sierra 10.13.2, thanks for this PR!

Any chance those can get merged at some point? I noticed there hasn't been a release on this package for a while...

@elafargue

This comment has been minimized.

Show comment
Hide comment
@elafargue

elafargue Jan 14, 2018

Contributor

Is this project still actively maintained?

Contributor

elafargue commented Jan 14, 2018

Is this project still actively maintained?

@sublimator

This comment has been minimized.

Show comment
Hide comment
@sublimator

sublimator Jan 14, 2018

sublimator commented Jan 14, 2018

@jacobrosenthal

This comment has been minimized.

Show comment
Hide comment
@jacobrosenthal

jacobrosenthal Jan 14, 2018

Member

Broadcast code is still sort of in high sierra, but pretty deprecated. No one spoke up so I deleted it and that path has a throw in it to say the functionality is removed. I think this PR is as complete as our current level of user reporting will allow.

Member

jacobrosenthal commented Jan 14, 2018

Broadcast code is still sort of in high sierra, but pretty deprecated. No one spoke up so I deleted it and that path has a throw in it to say the functionality is removed. I think this PR is as complete as our current level of user reporting will allow.

@pvanallen

This comment has been minimized.

Show comment
Hide comment
@pvanallen

pvanallen Jan 25, 2018

Thanks for this! It works for me on High Sierra 10.13.2 communicating with an Arduino 101. Does it this PR work with earlier versions of macOS?

pvanallen commented Jan 25, 2018

Thanks for this! It works for me on High Sierra 10.13.2 communicating with an Arduino 101. Does it this PR work with earlier versions of macOS?

@jakerockland

This comment has been minimized.

Show comment
Hide comment
@jakerockland

jakerockland Jan 25, 2018

This is working for us on High Sierra 10.13.1. Is there a plan for this to be merged soon?

jakerockland commented Jan 25, 2018

This is working for us on High Sierra 10.13.1. Is there a plan for this to be merged soon?

@elafargue

This comment has been minimized.

Show comment
Hide comment
@elafargue

elafargue Jan 25, 2018

Contributor
Contributor

elafargue commented Jan 25, 2018

@vvo

This comment has been minimized.

Show comment
Hide comment
@vvo

vvo Jan 26, 2018

This is working for me too

vvo commented Jan 26, 2018

This is working for me too

@jacobrosenthal

This comment has been minimized.

Show comment
Hide comment
@jacobrosenthal

jacobrosenthal Jan 28, 2018

Member

Were seeing reports that people who use the isNotification flag are broken. It turns out Apple seems to have removed it and so the fix would seem to be downstream users rewriting their code not to depend on that flag.

yosemite-bindings xpcEvent: {
"kCBMsgId": 71,
"kCBMsgArgs": {
"kCBMsgArgCharacteristicHandle": 106,
"kCBMsgArgDeviceUUID": {
"type": "Buffer",
"data": [
6,
238,
186,
37,
182,
218,
65,
134,
157,
122,
163,
193,
146,
60,
106,
16
]
},
"kCBMsgArgData": {
"type": "Buffer",
"data": [
2
]
},
"kCBMsgArgIsNotification": 1
}
} +2s
highsierra-bindings xpcEvent: {
"kCBMsgId": 83,
"kCBMsgArgs": {
"kCBMsgArgCharacteristicHandle": 106,
"kCBMsgArgDeviceUUID": {
  "type": "Buffer",
  "data": [
    61,
    31,
    196,
    97,
    216,
    81,
    69,
    123,
    130,
    183,
    61,
    137,
    246,
    194,
    222,
    67
  ]
},
"kCBMsgArgData": {
  "type": "Buffer",
  "data": [
    2
  ]
}
}

https://developer.apple.com/library/content/releasenotes/MacOSX/WhatsNewInOSX/Articles/macOS_10_13_0.html#//apple_ref/doc/uid/TP40017638-SW1
"Updated the APIs in the Core Bluetooth framework to match across iOS, tvOS, watchOS, and macOS, and marked the platform availability of each API."

and indeed isnotification looks to be gone from the mac apis
https://developer.apple.com/documentation/corebluetooth/cbperipheraldelegate/1518708-peripheral

These guys have the runtime headers dumped for 10.12 and 10.13
https://github.com/onmyway133/Runtime-Headers/blob/master/macOS/

clearly quite a few changes on cbcentralmanager anyway
https://github.com/onmyway133/Runtime-Headers/blob/master/macOS/10.12/CoreBluetooth.framework/CBCentralManager.h
https://github.com/onmyway133/Runtime-Headers/blob/master/macOS/10.13/CoreBluetooth.framework/CBCentralManager.h

but we dont seem to have cbperipheraldelegate headers? so cant see the diff there, though we clearly know it used to be there and is not now

Hey neat:
http://codeworkshop.net/objc-diff/sdkdiffs/macos/10.13/CoreBluetooth.html

i dont know that isnotifying was ever exposes on the offical api?

Maybe theres more little changes we havent seen yet (including new goodies).. It would be valuable look through this thoroughly

related, webbluetooth struggled with this for a while
WebBluetoothCG/web-bluetooth#274

Technically this is now a api breaking change and would mean a 2.0 release but I dont know how committed we are to semver for such a small break. From searching github this we think this 'functionality' is actually rarely used. anyway

Member

jacobrosenthal commented Jan 28, 2018

Were seeing reports that people who use the isNotification flag are broken. It turns out Apple seems to have removed it and so the fix would seem to be downstream users rewriting their code not to depend on that flag.

yosemite-bindings xpcEvent: {
"kCBMsgId": 71,
"kCBMsgArgs": {
"kCBMsgArgCharacteristicHandle": 106,
"kCBMsgArgDeviceUUID": {
"type": "Buffer",
"data": [
6,
238,
186,
37,
182,
218,
65,
134,
157,
122,
163,
193,
146,
60,
106,
16
]
},
"kCBMsgArgData": {
"type": "Buffer",
"data": [
2
]
},
"kCBMsgArgIsNotification": 1
}
} +2s
highsierra-bindings xpcEvent: {
"kCBMsgId": 83,
"kCBMsgArgs": {
"kCBMsgArgCharacteristicHandle": 106,
"kCBMsgArgDeviceUUID": {
  "type": "Buffer",
  "data": [
    61,
    31,
    196,
    97,
    216,
    81,
    69,
    123,
    130,
    183,
    61,
    137,
    246,
    194,
    222,
    67
  ]
},
"kCBMsgArgData": {
  "type": "Buffer",
  "data": [
    2
  ]
}
}

https://developer.apple.com/library/content/releasenotes/MacOSX/WhatsNewInOSX/Articles/macOS_10_13_0.html#//apple_ref/doc/uid/TP40017638-SW1
"Updated the APIs in the Core Bluetooth framework to match across iOS, tvOS, watchOS, and macOS, and marked the platform availability of each API."

and indeed isnotification looks to be gone from the mac apis
https://developer.apple.com/documentation/corebluetooth/cbperipheraldelegate/1518708-peripheral

These guys have the runtime headers dumped for 10.12 and 10.13
https://github.com/onmyway133/Runtime-Headers/blob/master/macOS/

clearly quite a few changes on cbcentralmanager anyway
https://github.com/onmyway133/Runtime-Headers/blob/master/macOS/10.12/CoreBluetooth.framework/CBCentralManager.h
https://github.com/onmyway133/Runtime-Headers/blob/master/macOS/10.13/CoreBluetooth.framework/CBCentralManager.h

but we dont seem to have cbperipheraldelegate headers? so cant see the diff there, though we clearly know it used to be there and is not now

Hey neat:
http://codeworkshop.net/objc-diff/sdkdiffs/macos/10.13/CoreBluetooth.html

i dont know that isnotifying was ever exposes on the offical api?

Maybe theres more little changes we havent seen yet (including new goodies).. It would be valuable look through this thoroughly

related, webbluetooth struggled with this for a while
WebBluetoothCG/web-bluetooth#274

Technically this is now a api breaking change and would mean a 2.0 release but I dont know how committed we are to semver for such a small break. From searching github this we think this 'functionality' is actually rarely used. anyway

@noble noble deleted a comment from andrewjaykeller Jan 30, 2018

@sandeepmistry sandeepmistry changed the title from (complete?) High Sierra support to High Sierra support Jan 31, 2018

Add note on isNotification parameter for characteristic data event po…
…ssibly being undefined

It is also deprecated after release v1.8.1
@jacobrosenthal

This comment has been minimized.

Show comment
Hide comment
@jacobrosenthal

jacobrosenthal Jan 31, 2018

Member

I guess users and are probably pegged to ^1 and mac users are used to mac breaking (and indeed are broken right now) I dont feel bad about just shipping the breaking change of isNotification as undefined as a minor release. In the docs well say its deprecated (and indeed gone) as of high sierra

Member

jacobrosenthal commented Jan 31, 2018

I guess users and are probably pegged to ^1 and mac users are used to mac breaking (and indeed are broken right now) I dont feel bad about just shipping the breaking change of isNotification as undefined as a minor release. In the docs well say its deprecated (and indeed gone) as of high sierra

@sandeepmistry sandeepmistry merged commit 38dd42e into noble:master Jan 31, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@vvo

This comment has been minimized.

Show comment
Hide comment
@vvo

vvo Feb 2, 2018

Awesome!

vvo commented Feb 2, 2018

Awesome!

@vvo

This comment has been minimized.

Show comment
Hide comment
@vvo

vvo Feb 2, 2018

I guess users and are probably pegged to ^1 and mac users are used to mac breaking (and indeed are broken right now) I dont feel bad about just shipping the breaking change of isNotification as undefined as a minor release. In the docs well say its deprecated (and indeed gone) as of high sierra

@jacobrosenthal Do you mean there's no way to get notifications like the issue you chipped in at voodootikigod/node-rolling-spider#103 (comment)?

Thanks!

vvo commented Feb 2, 2018

I guess users and are probably pegged to ^1 and mac users are used to mac breaking (and indeed are broken right now) I dont feel bad about just shipping the breaking change of isNotification as undefined as a minor release. In the docs well say its deprecated (and indeed gone) as of high sierra

@jacobrosenthal Do you mean there's no way to get notifications like the issue you chipped in at voodootikigod/node-rolling-spider#103 (comment)?

Thanks!

@jacobrosenthal

This comment has been minimized.

Show comment
Hide comment
@jacobrosenthal

jacobrosenthal Feb 2, 2018

Member

Theres no longer any way to know if its a notification or a response to a manual read request

Member

jacobrosenthal commented Feb 2, 2018

Theres no longer any way to know if its a notification or a response to a manual read request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment