kenjdavidson / react-native-bluetooth-classic

⚛ Bluetooth classic Android(Bluetooth)/IOS(ExternalAccessory) module for serial communication
https://kenjdavidson.github.io/react-native-bluetooth-classic
MIT License
249 stars 93 forks source link

Not receiving any data on devices running ios17 #278

Closed jim-at-jibba closed 11 months ago

jim-at-jibba commented 11 months ago

Mobile Device Environment Provide a list of operating systems on which this issue is relevant.

Application Environment Provide information about your development environment:

Describe the bug We make use of this package extensively in out application. We are able to connect to bonded devices and prior to iOS17 are receiving data from the onDataReceived callback. With upgrading to iOS17 all out users are now not receiving any data.

I have tried to debug the swift code, there seems to be nothing being passed to the stream and subsequent read functions. All values seem to be empty.

Any help is much appriciated.

Thanks

kenjdavidson commented 11 months ago

Thanks for the information. I'll be completely honest, in that I don't have much time to work on this/look into this in the near future, so hopefully you'll be willing to provide a little more information on what you're seeing.

  1. Can you confirm which development environment you're using? I've been approached about this issue from someone, and from looking it seems like Bluetooth apps need to be re-compiled in XCode 15 (or laster they exist?) in order to work on IOS17. This may just be bluetooth though.

There is no information on the Apple developer platform with regards to:

and with nothing changed in the actual library, I'm not even sure where I would start.

  1. Does this happen when using an IOS 17 emulator? I don't own IOS devices, so if this doesn't happen on an emulator, I just won't be able to assist even if I do get time to try it.

  2. If you're attempted to debug, and nothing is passed to the Streams, then there isn't much that I can do. If something deep in the Streams code has changed, without it being documented on the site, I'm not sure what course of action there is to take at this point. Could you provide more information on where exactly you're debugging and not seeing data come through?

Do you see the breakpoint in the read function getting hit? If so, what is the value of the content you're seeing?

https://github.com/kenjdavidson/react-native-bluetooth-classic/blob/83c418a6fac414f53af93e11c40174082615176b/ios/conn/DelimitedStringDeviceConnectionImpl.swift#L209

Can you attempt to change the configuration for where the stream code is scheduled? The lines below essentially set it to main and commonModes, these may need to be changed for IOS17?

                inStream.schedule(in: .main, forMode: .commonModes)
                outStream.schedule(in: .main, forMode: .commonModes)
  1. Have you reached out to the device manufacturer to confirm that their External Accessory / MFi settings and configuration work on IOS17? MFI is a bitch to deal with, and requires that the manufacturer validate and update security and configuration for each IOS and App version release.
kenjdavidson commented 11 months ago

Looks like they may have converted Bluetooth Classic (Mfi) from External Accessory to Core Bluetooth? Although there isn't much explaining this.

https://developer.apple.com/documentation/corebluetooth/using_core_bluetooth_classic

This page looks like it was introduced in:

Although this looks like the same video that I've watched numerous times stating that Core Blutooth cannot be used for Mfi devices.

https://developer.apple.com/videos/play/wwdc2019/901

Maybe they sneakily removed it.

jim-at-jibba commented 11 months ago

Thanks so much for the speedy reply.

I will take a look at the suggestions in your first reply. I wonder if it is something to do with the MFi code.

Would not be surprised if apple Had been sneaky. So is Core Bluetooth the recommended route or External Accessory? If find the Apple SDKs so hard to keep track of

Thanks again

kenjdavidson commented 11 months ago

Would not be surprised if apple Had been sneaky. So is Core Bluetooth the recommended route or External Accessory? If find the Apple SDKs so hard to keep track of

No clue to be honest. I've reached out to my contact at the hardware manufacturer to see if he's aware of anything. They would have had to make the same changes in Objective C for their own internal app.

jim-at-jibba commented 11 months ago

That's amazing thanks. I will let you know what I find out from this afternoons exploration

kenjdavidson commented 11 months ago

Looking at an example that seems to work in another library:

            _session?.inputStream?.delegate = self
            _session?.inputStream?.schedule(in: RunLoop.current, forMode: RunLoop.Mode.default)
            _session?.inputStream?.open()

maybe trying the different schedule configuration, this might help. I have noticed in the past, that setting this incorrectly on certain devices will stop data from being read. It may be required to modify this based on the device or app. I'm not sure.

jim-at-jibba commented 11 months ago

Ok thanks. I will give this a go tomorrow.

kenjdavidson commented 11 months ago

After a little more looking, i think the issue is using the .main runloop on IOS 17. I found an issue (unrelated to streams) but related to RunLoop.main which says it worked in IOS16 but not 17.

The mode .common may also be the issue. I have a feeling this combination is the problem.

kenjdavidson commented 11 months ago

Well this looks promising? Although it looks like this renaming has been done a lot earlier than just IOS17, maybe it was finally deprecated or removed?

https://developer.apple.com/documentation/foundation/runloop/mode https://codecrew.codewithchris.com/t/commonmodes-has-been-renamed-to-runloop-mode-common/2755

jim-at-jibba commented 11 months ago

God, it would be awesome if this was the fix. Sucks that no exceptions are thrown by Apple. Just seems to fail silently.

Thanks again for your help

kenjdavidson commented 11 months ago

ha. looking forward to the confirmation or denial tomorrow!!!

jim-at-jibba commented 11 months ago

Changing the schedule did not work 😢 but I have found a External Accessories demo that seems to so am going look into implementing that. Will fork this and see if I can rework the iOS code. Will pass it back to you to see if you think its legit and maybe want to merge back in to this package. I dont want to break anything for anyone else

kenjdavidson commented 11 months ago

Right on. If you have all the rest of the stuff bumped up too:

I'll probably take the time to cut off the 0.60.x releases and bump everything up to 0.70.x going forward. Let me know. From the working Swift example that I looked at, I didn't see much different, so hopefully something jumps out for you.

jim-at-jibba commented 11 months ago

I will be writing it against our application which is:

The demo is here - https://github.com/rvndios/EADemo/tree/master

Will be a couple of days but will let you know how I get on

kenjdavidson commented 11 months ago

Right on, I think it makes sense to finally bump up the react and android dependencies anyhow. Looking at this example, the only real differences are:

That's pretty much it, if the stream delegate function isn't getting hit, it's just not working. Technically the app could be updated to poll the stream, instead of use the StreamDelegate but that seems like a poor long term choice.

I guess another option is that the options[PROTOCOL_STRINGS] isn't getting into the connection class. But this would probably just throw an exception while attempting to connect, and not just sit there? But who knows with how Apple handles things.

jim-at-jibba commented 11 months ago

So I have finally gotten to the bottom of the issue.

Not to do with schedule like we first thought. Its because the delimiter which is a string is set to a string representation of a newline character when one is not supplied (which I imagine is the case most of the time). But when the read function converts the inBuffer Data object to a string the deliminter \n is not converted to a string and so the test for the index does not return a result and so message is empty. Not sure why the update to iOS17 has broken this.

https://github.com/kenjdavidson/react-native-bluetooth-classic/blob/486298da6926fcc0a600ae8e3c7043deb6043ebb/ios/conn/DelimitedStringDeviceConnectionImpl.swift#L183

Does that make sense?

I am gonna rewrite it to work in all instances and then will submit a PR

kenjdavidson commented 11 months ago

LMAO... good old Apple deciding that newline is not an actual character. Thanks for spending the time, looking forward to the PR.

Is a work around passing in the \n delimiter explicitly? Or it just doesn't like the \n convert the \n in the buffered string anymore?

jim-at-jibba commented 11 months ago

Its mighty strange.

It just seems that when converting to a buffered string the newline character just stays as a character not the string representation of it. Will have a look at passing a \n delimiter explicitly, might make things simpler.

Am aware I am relatively new to swift so want to make sure I have not broken it for anyone else so will fix and test over the next day or so.

Roughly speaking having to do something like this:

func read() -> String? {
        NSLog("(BluetoothDevice:readFromDevice) Reading device %@ until delimiter %@",
              self.accessory.serialNumber, self.delimiter)
        var message:String?
        let content = String(data: inBuffer, encoding: .utf8)!
        let characterSet = CharacterSet.newlines
        if let range = content.rangeOfCharacter(from: characterSet) {
            let lineIndex = content.distance(from: content.startIndex, to: range.lowerBound)
            print("New line character found at index: \(lineIndex)")
            message = content
            inBuffer = String(content[content.index(after: range.lowerBound)...])
                .data(using: self.encoding) ?? Data()
        } else {
            print("String does not contain a new line character")
            if (self.delimiter.count == 0) {
                message = content
                inBuffer.removeAll()
            } else {
                if let index = content.index(of: self.delimiter) {
                print("Index", index)
                message = String(content[..<index])
                inBuffer = String(content[content.index(after: index)...])
                        .data(using: self.encoding) ?? Data()
                }
            }
        }

        return message
    }
kenjdavidson commented 11 months ago

Looks like the appropriate way to encode/decode is

_dataAsString = String(bytes: buffer, encoding: String.Encoding.nonLossyASCII) as NSString?

Looks like nonLossyAscii may maintain the \n. Sadly there is no nonLossy version for utf8. This is like the most ridiculous thing I've ever seen, not encoding \n.

jim-at-jibba commented 11 months ago

Well that would make my solution much simpler 😆 . I will test this

jim-at-jibba commented 11 months ago

Looks like that this worked _dataAsString = String(bytes: buffer, encoding: String.Encoding.nonLossyASCII) as NSString?

What would you say the best direction is?

kenjdavidson commented 11 months ago

The issue with this is the line

let content = String(data: inBuffer, encoding: .utf8)!

should actually be

let content = String(data: inBuffer, encoding: self.encoding)!

as it comes from a parameter. Which may actually be possible to do in the client itself:

device.connect({
   charset: `.nonLossyASCII`
});

Essentially this means that you can never use utf and \n as the delimiter, which seems a little crazy to me. With that said the default is already ASCII anyhow, so maybe it doesn't matter?

        else if let value = self.properties["charset"] { self.encoding = String.Encoding.from(value as! CFStringEncoding) }
        else { self.encoding = String.Encoding.from(CFStringBuiltInEncodings.ASCII.rawValue) }
jim-at-jibba commented 11 months ago

Yeah sorry that was left in by me while I was testing various things. Was gonna put it back before submitting the PR

kenjdavidson commented 11 months ago

This is really bothersome though, I'd like to find the document explaining why it's happening. Like i get 99% of the time people will use ascii which is fine. But for the 1% of the time someone wants a different encoding, I'd like to understand what's going on.

Essentially, this change could blow up a number of projects.

jim-at-jibba commented 11 months ago

Yeah that is what I worry about. Have not found any docs to explain it, though I have not looked that hard.

kenjdavidson commented 11 months ago

https://stackoverflow.com/questions/34810236/swift-2-0-escaping-string-for-new-line-string-encoding

This may be the choice.

Use json encoding to encode the sting properly,

jim-at-jibba commented 11 months ago

Seems like a good solution. I have stopped for the day but will explore this tomorrow

Thanks for your help

kenjdavidson commented 11 months ago

Right on. I appreciate it. It's saving me a bunch of time. I already wasted most of my weekend getting the doc site back up and working.

jim-at-jibba commented 11 months ago

No worries. We heavily rely on the project so happy to put the leg work in to help maintain it

kenjdavidson commented 11 months ago

After thinking about it, let's just make the default the nonLossyAscii if that just works. It looks like it's backwards compatible, it's been in there since ios 8. If anyone updates to ios17 and they really want to continue using just ascii they can write their own connection impl. But honestly, I think there is limited mfi usage, only you and another company have reached out.

jim-at-jibba commented 11 months ago

OK that makes sense. I will just update the encoding default value in the DelimitedStringDeviceConnectionImpl init function. Will test and submit PR later

kenjdavidson commented 11 months ago

Cool. Do you have time to chat about the other stuff. Since you're actively using the library, and I'm assuming keeping up with the android store and Apple store requirements I'd love your input.

Android recently changed their min sdk version I believe. Have you needed to modify the library locally? Or just update your own xml file? Same with the react native peer dependency?

My thoughts were to maybe for this break off and update to

1.70.x Incrrase the react native peer dep Increase the android min version Anyhting on ios side that needs doing?

jim-at-jibba commented 11 months ago

Hey bud,

I am just trying to test my change across a few devices and then will submit the PR

A bump makes sense. I have just finished (not released yet) our upgrade to RN 0.71.14. We bumped our own build.gradle file to satisfy Google min SDK version of 33. Pretty sure the min version of ios for this version of RN is 13

My only issue (not your issue though) is that the changes in peer dependency will block my hotfix as our upgrade that satisfies these changes is not released yet. But maybe I can publish my own version of this library as is and update to your new cut when our upgrade branch is released.

jim-at-jibba commented 11 months ago

PR is here - https://github.com/kenjdavidson/react-native-bluetooth-classic/pull/280

kenjdavidson commented 11 months ago

The other option is make this sole change in 1.60.x and release that. If anything else major changes between .ascii and .nonLossyAscii then so be it. But from the two libraries we've seen, that haven't changed in quite a while, then I'll just accept the hate for the breaking change, not like I'm getting paid for this anyhow, haha.

Once this is in, I'll break off the releases/1.60.x and start accepting PRs to update the version to 1.70.x. If you don't mind, i'd love to pick your brain on doing this appropriately, since you're so active with it.

jim-at-jibba commented 11 months ago

OK great. That sounds like a plan.

I am more than happy to chat about updating to 1.70.x and also happy to work on the PR. I am not sure I am the most qualified to give advice about the best way to do it but Im sure between us we can figure out something.

Thanks for your help with this.

kenjdavidson commented 11 months ago

OK great. That sounds like a plan.

I am more than happy to chat about updating to 1.70.x and also happy to work on the PR. I am not sure I am the most qualified to give advice about the best way to do it but Im sure between us we can figure out something.

Thanks for your help with this.

I haven't worked with mobile development in over a year, so you're better than me ;)

kenjdavidson commented 11 months ago

Welp, there ya go https://www.npmjs.com/package/react-native-bluetooth-classic/v/1.60.0-rc.30

jim-at-jibba commented 11 months ago

That is awsome