troystribling / BlueCap

iOS Bluetooth LE framework
MIT License
715 stars 115 forks source link

Add tvOS Support #52

Closed Jeehut closed 7 years ago

Jeehut commented 7 years ago

This attempts to solve #51.

Specifically the steps done are:

Everything should behave as before regarding iOS – the target name change shouldn't affect the framework name as I changed it not to rely on the target name (otherwise the iOS framework would have a differing name from the tvOS framework, which is usually not the expected behavior).

Jeehut commented 7 years ago

By the way, the changes seem to be many, but actually most of times I simply changed something like

func method() {
    // some code
}

to only be defined on the iOS platform (as there was no API on tvOS):

#if os(iOS)
    func method() {
        // some code
    }
#endif

So don't get overwhelmed of the "Files changed" section in this PR. ;)

troystribling commented 7 years ago

The biggest thing missing is that the test cases will not run for a tvOS build. Try to build and tun the test cases in the Tests directory.

The travis.yml files should be updated to run a tvOS build. This will make it quite a bit more complex. I will take a look at it.

troystribling commented 7 years ago

MutableService and MutableCharacteristics are not useful if they cannot be created since it is required to initialize a PeripheralManager. Would make more sense to remove them completely for tvOS by putting the os constraint around the entire class. Also, if PeripheralManager cannot advertise how do you connect?

troystribling commented 7 years ago

It would also by nice to see a CentralManager example for tvOS like the example for tvOS.

Jeehut commented 7 years ago

Hey Troy, thanks for looking through my request. Actually, we were considering BlueCap for our project but finally decided not to rewrite our existing Bluetooth logic as it wasn't as bad as I first thought it was. So, I'll probably have no time to finish this, I'm sorry for that.

But some inputs for anybody who might want to continue this: – The MutableService and MutableCharacteristics classes can be completely removed by disabling the check (instead of wrapping the entire class into the os() thing) – The Apple TV currently cannot server as a peripheral, it can only be a central as of tvOS 10.1

troystribling commented 7 years ago

Thanks, I will close the PR then.

Jeehut commented 7 years ago

Just wondering why you're closing this. Maybe I haven't mentioned that my changes made this work on tvOS using Carthage perfectly fine. So, I'd even consider merging this as is as a better option than simply closing it. As far as I can see the only missing parts were that tests were not configured for the tvOS part so you can't really ensure it'll work in the future. But isn't that secondary to somebody who wants to use BlueCap on tvOS given that it lacks support at all?

I think the only important thing before merging is to do the two small changes I listed my previous comment (one is to remove a check in the Xcode side pane for two classes, the other to add a line in the README that the Apple TV can't be used as a peripheral). Then even without tests having tvOS support that may break sometime is still better than none. That's at least my opinion ...

troystribling commented 7 years ago

From your comments it looks like the problem can be better handled by excluding files from the tvOS target so code changes are not needed. I am reluctant to merge changes that are not required.

I would also at least like for the tests to work for tvOS target. It would also be nice to have a tvOS example app. This is the procedure followed for the current build target and examples are provided for each major component that is supported by the project.

Also, I do not have a TV or Apple TV to support this change. It is really hard to support BLE without the HW since it cannot be tested in the simulator.

I would consider merging the PR if it just added a new BlueCapKitTvOS target for the framework project and a new target in the test project. The test target should be able to work by excluding files and with changes to helper files may need to broken in two.

If you do not want to make theses changes to the PR you can add a feature request but I will not be able to work on it until I have access to HW.