macos: initial support for scanning/connecting/write characteristics/notifications #7

deadprogram · 2020-08-28T06:49:23Z

This PR adds the initial support for macOS with nothing more than scanning implemented.

It is currently WIP so I am making it draft.

deadprogram · 2020-08-28T10:47:06Z

Latest commit builds and can scan on macOS.

make smoketest-macos
# Test on macos.
GOOS=darwin CGO_ENABLED=1 go build -o /tmp/go-build-discard ./examples/scanner

/tmp/go-build-discard
scanning...
found device: ba3cd17f-dbbb-aaba-2f40-e0cffe748605 -94
found device: e8e09032-f1bd-5ebb-874e-11a05eb47579 -95
found device: 1a6b3f14-d497-a284-1e4d-2c6f2d0a0003 -94
found device: 01a84b85-e3eb-589f-ac48-81ed9f578e01 -91
found device: 305648ff-a112-bbab-f648-3e342adad436 -91
found device: 305648ff-a112-bbab-f648-3e342adad436 -91
found device: 44c60243-30f6-d8af-0d47-4a15280d7025 -83
found device: 01a84b85-e3eb-589f-ac48-81ed9f578e01 -91
found device: 01a84b85-e3eb-589f-ac48-81ed9f578e01 -90
found device: 01a84b85-e3eb-589f-ac48-81ed9f578e01 -90 [AV] Samsung Soundbar MS650
found device: 01a84b85-e3eb-589f-ac48-81ed9f578e01 -89
found device: 01a84b85-e3eb-589f-ac48-81ed9f578e01 -89
found device: 01a84b85-e3eb-589f-ac48-81ed9f578e01 -89 [AV] Samsung Soundbar MS650
found device: 21dbaf72-81ca-beb6-a44e-4dc128116156 -93

aykevl

A few initial comments on the draft.

Too bad CoreBluetooth doesn't expose MAC addresses, that makes the whole API less uniform. I had made the (apparently incorrect) assumption that a MAC is so basic that it'll be exposed everywhere.

README.md

Makefile

adapter_darwin.go

gap_darwin.go

deadprogram · 2020-08-29T07:37:43Z

I have now marked this PR as ready for review. I think it wold be good to get any other changes and then get it merged into dev before moving on to additional implementation.

deadprogram · 2020-08-29T09:38:35Z

I added the last functionality needed to declare this PR "done" which was to implement DiscoverServices() and DiscoverCharacteristics() both of which are now complete.

deadprogram · 2020-08-29T10:52:16Z

OK, just two more commits now with write characteristics.

aykevl · 2020-08-29T11:54:42Z

So, if I understand correctly the methods such as DidDiscoverPeripheral are necessary as a callback mechanism?
I'm not so sure about exposing those. They clutter the API, and are really an implementation detail (many functions that a user of the API should never call). Would it be possible to put them on an unexported struct, so that they don't affect the public API?

deadprogram · 2020-08-29T12:13:45Z

In order to take advantage of a bunch of the predemined functionality by using https://github.com/tinygo-org/bluetooth/blob/macos/adapter_darwin.go#L12 you need to implement those methods. I think without this there would be a lot more of them! macOS is macOS, we just do what they say.

aykevl · 2020-08-29T12:02:19Z

gap_darwin.go

+}
+
+// Connect starts a connection attempt to the given peripheral device address.
+func (a *Adapter) Connect(address Addresser, params ConnectionParams) (*Device, error) {


It may be a good idea to use some sort of check or mutex, right now it appears that it is not possible to connect to more than one device at once (because of a.connectChan for example).

The initial implementation only allows for one device, I was not planning on working on that now.

aykevl · 2020-08-29T12:06:55Z

gap_darwin.go

+		p.SetDelegate(d)
+		return d, nil
+	case <-time.NewTimer(10 * time.Second).C:
+		return nil, errors.New("timeout on Connect")


The documentation says:

After successfully establishing a local connection to a peripheral, the central manager object calls the centralManager:didConnectPeripheral: method of its delegate object. If the connection attempt fails, the central manager object calls the centralManager:didFailToConnectPeripheral:error: method of its delegate object instead. Attempts to connect to a peripheral don’t time out.

Ideally we'd use these instead of laying our own error handling on top of this.
(Of course, didFailToConnectPeripheral is secretly just a timeout but it's a timeout on a much lower level).

This was just a simple implementation, but can probably connect that easily enough.

gap_darwin.go

gap_linux.go

aykevl · 2020-08-29T12:14:54Z

gattc_darwin.go

+	case <-time.NewTimer(10 * time.Second).C:
+		return nil, errors.New("timeout on DiscoverServices")


I suspect this doesn't really time out either, it either succeeds or the entire connection is broken.

Same as above comment.

aykevl · 2020-08-29T12:16:31Z

gattc_darwin.go

+	case <-time.NewTimer(10 * time.Second).C:
+		return nil, errors.New("timeout on DiscoverCharacteristics")


Same as my previous comment.

aykevl · 2020-08-29T12:17:36Z

gattc_darwin.go

+		return errors.New("must provide a callback for EnableNotifications")
+	}
+
+	return nil


Is there a reason to leave this function unimplemented?

Because I am working on it, and left it unimplemented for now.

This has now been implemented.

aykevl · 2020-08-29T12:22:43Z

I see. However, they don't need to be on the Adapter type I think. They would be hidden from the user with something like this:

type Adapter struct {
    delegate centralManagerDelegate
}

type centralManagerDelegate struct {
    cbgo.CentralManagerDelegateBase
    a *Adapter
}


// CentralManagerDidUpdateState when central manager state updated.
func (delegate *centralManagerDelegate) CentralManagerDidUpdateState(cmgr cbgo.CentralManager) {
	// powered on?
	if cmgr.State() == cbgo.ManagerStatePoweredOn {
		close(delegate.a.poweredChan)
	}

	// TODO: handle other state changes.
}

This provides the same functionality without cluttering the API.

deadprogram · 2020-08-29T12:24:31Z

I discovered I can remove a bunch of the implementation that is not being used, coming in future commit. Also, I was trying to have a simpler implementation with less indirection, since there is already a lot of complexity in the macOS CB code itself.

deadprogram · 2020-08-29T13:32:51Z

Added some delegate types, and have otherwise handled all outstanding feedback items, I think.

deadprogram · 2020-08-29T14:17:32Z

Added characteristic notifications. No more features in this PR, I mean it! 😺

Signed-off-by: Ron Evans <ron@hybridgroup.com>

…ead of MAC as the BLE address for a peripheral Signed-off-by: Ron Evans <ron@hybridgroup.com>

Signed-off-by: Ron Evans <ron@hybridgroup.com>

…ntation Signed-off-by: Ron Evans <ron@hybridgroup.com>

Signed-off-by: Ron Evans <ron@hybridgroup.com>

Signed-off-by: deadprogram <ron@hybridgroup.com>

deadprogram · 2020-08-30T14:58:05Z

I squashed the commits in this branch down based on the functionality, and rebased against dev. Also, have tested on macOS and Linux plus Circuit Playground Bluefruit.

Basically, we can now finally ready to merge! Thanks for all your help on this @aykevl

deadprogram · 2020-08-31T12:12:07Z

@aykevl any more feedback on this PR?

aykevl · 2020-08-31T23:24:07Z

I'll try to get to this PR tomorrow. I was a bit busy with unrelated things (which is now mostly done).

aykevl

Looks good to me. Feel free to merge, unless you want to fix the nit.

adapter_darwin.go

Signed-off-by: deadprogram <ron@hybridgroup.com>

deadprogram · 2020-09-02T06:36:58Z

Now merging, thank you very much @aykevl for your help landing this PR!

deadprogram mentioned this pull request Aug 28, 2020

Add support for macOS #5

Closed

deadprogram changed the base branch from master to dev August 28, 2020 10:57

aykevl reviewed Aug 28, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

Makefile Show resolved Hide resolved

adapter_darwin.go Show resolved Hide resolved

adapter_darwin.go Outdated Show resolved Hide resolved

gap_darwin.go Outdated Show resolved Hide resolved

deadprogram force-pushed the macos branch from 29c900f to edcfb94 Compare August 29, 2020 07:06

deadprogram changed the title ~~macos: initial support for scanning~~ macos: initial support for scanning/connecting Aug 29, 2020

deadprogram marked this pull request as ready for review August 29, 2020 07:36

deadprogram changed the title ~~macos: initial support for scanning/connecting~~ macos: initial support for scanning/connecting/write characteristics Aug 29, 2020

aykevl reviewed Aug 29, 2020

View reviewed changes

deadprogram changed the title ~~macos: initial support for scanning/connecting/write characteristics~~ macos: initial support for scanning/connecting/write characteristics/notifications Aug 29, 2020

deadprogram added 5 commits August 30, 2020 16:39

macos: starting point for adding macOS support

105d50a

Signed-off-by: Ron Evans <ron@hybridgroup.com>

macos: able to retrieve some of the info when scanning

2791776

Signed-off-by: Ron Evans <ron@hybridgroup.com>

all: use Addresser interface to handle fact that macOS uses UUID inst…

7a1e1f6

…ead of MAC as the BLE address for a peripheral Signed-off-by: Ron Evans <ron@hybridgroup.com>

windows: use Addresser interface for the MAC address for a peripheral

35de6ef

Signed-off-by: Ron Evans <ron@hybridgroup.com>

docs: show that we can now scan on macOS

a5ff736

Signed-off-by: Ron Evans <ron@hybridgroup.com>

deadprogram force-pushed the macos branch from f775fba to dddd546 Compare August 30, 2020 14:41

deadprogram added 5 commits August 30, 2020 16:50

macos: completed initial implementation

7cc099d

Signed-off-by: Ron Evans <ron@hybridgroup.com>

macos: able to discover services and characteristics for a device

0a7f003

Signed-off-by: Ron Evans <ron@hybridgroup.com>

gap: switch to use MACAddress struct when possible for shared impleme…

be4168e

…ntation Signed-off-by: Ron Evans <ron@hybridgroup.com>

macos: remove unneeded functions to export internal implementaions.

b5b4c68

Signed-off-by: Ron Evans <ron@hybridgroup.com>

macos: added characteristic notifications

f789e84

Signed-off-by: Ron Evans <ron@hybridgroup.com>

gap: correct use of Address on Linux platform

c245754

Signed-off-by: deadprogram <ron@hybridgroup.com>

deadprogram force-pushed the macos branch from dddd546 to c245754 Compare August 30, 2020 14:52

aykevl approved these changes Sep 1, 2020

View reviewed changes

adapter_darwin.go Outdated Show resolved Hide resolved

docs: better explanation of peripheral UUID on macOS

b80b918

Signed-off-by: deadprogram <ron@hybridgroup.com>

deadprogram merged commit d570aa5 into dev Sep 2, 2020

deadprogram deleted the macos branch September 2, 2020 06:37

		case <-time.NewTimer(10 * time.Second).C:
		return nil, errors.New("timeout on DiscoverServices")

macos: initial support for scanning/connecting/write characteristics/notifications #7

macos: initial support for scanning/connecting/write characteristics/notifications #7

Uh oh!

Conversation

deadprogram commented Aug 28, 2020

Uh oh!

deadprogram commented Aug 28, 2020

Uh oh!

aykevl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deadprogram commented Aug 29, 2020

Uh oh!

deadprogram commented Aug 29, 2020

Uh oh!

deadprogram commented Aug 29, 2020

Uh oh!

aykevl commented Aug 29, 2020

Uh oh!

deadprogram commented Aug 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aykevl commented Aug 29, 2020

Uh oh!

deadprogram commented Aug 29, 2020

Uh oh!

deadprogram commented Aug 29, 2020

Uh oh!

deadprogram commented Aug 29, 2020

Uh oh!

deadprogram commented Aug 30, 2020

Uh oh!

deadprogram commented Aug 31, 2020

Uh oh!

aykevl commented Aug 31, 2020

Uh oh!

aykevl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

deadprogram commented Sep 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants