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

RxBleRadio operation off main thread #136

Closed
maoueh opened this issue Feb 16, 2017 · 8 comments
Closed

RxBleRadio operation off main thread #136

maoueh opened this issue Feb 16, 2017 · 8 comments
Assignees

Comments

@maoueh
Copy link
Contributor

maoueh commented Feb 16, 2017

Summary

I would like to have a way to schedule bluetooth operation off the main thread.

Ultimately, it would be nice if the library was doing this on it's own so that everybody can benefit from it without further interventions. But I understand this might mean keeping a list of devices that needs to execute on the main thread.

We have such fix in the wild right now (not in mass production but tested on lots of various devices), and never heard about problems with it. In our code, if phone is samsung and at API level 18, we do stuff on main thread, on all other cases, it's being done on a dedicated single thread scheduler

@maoueh
Copy link
Contributor Author

maoueh commented Feb 16, 2017

@dariuszseweryn Copying your answer from a different thread (see here):

I am a bit afraid that there may be more devices that are more stable while using the main thread than we know of. In this situation we would need to keep a list of devices that need a specific solution - which could be a burden.

@maoueh
Copy link
Contributor Author

maoueh commented Feb 16, 2017

I agree with that statement and I don't want to put to much burden on you guys about maintaining this list, mainly also because it means that we get aware of such problems when people report them.

From our current tests on multiple different devices (nexus 4, nexus 5, LG g5, motorola, a Chinese phone, Samsung s3 mini, HTC tablet, another tablet I don't recall the model), we never had any issue running off the main thread yet. Note that we never tested on API 18 however, our lowest target was a Samsung S3 mini running API level 19.

An in-between could be to keep main thread only for API level 18 (or even more restricted).

But I understand it might difficult for you guys to maintain. As a first thing, you would guys allow a way for us, users of the library, to change the default scheduler?

Not sure how it would look like, but if you agree, I would be happy to check some ways about doing this.

@dariuszseweryn
Copy link
Owner

Hello,
I am thinking of advantages that may come from this change. I do not think that it is good to give this flexibility to the users. The purpose (and responsibility) of this library is to allow BLE interactions in a way that is most stable - with this change we could expect people trying to use a non main thread for all devices (because of the better performance) even with Samsung S3 API 18 and related bugs.
I would imagine that it may be a good idea to restrict BLE operations to be run on the main thread only to Samsung and API 18. I wonder if on >API 18 devices this change would impact with a lower number of successful connections - this scenario would be hard to spot without A/B testing on a large number of devices.
Could you give information what is the performance gain of changing the operation execution thread? Maybe with a custom operation support ( #137 ) this change is not that relevant?

@maoueh
Copy link
Contributor Author

maoueh commented Feb 20, 2017

@dariuszseweryn When I first implemented the flashing, I did it in a debug screen that is doing little to no UI at all. Then, when I ported the code to the actual real UI screen, the performance was dramatically being impacted by I would say a 1.5x to 2x factor (going from 70s to around 130s depending on the run and device).

It was to such extend that we suggested to beta testers to actually close the phone screen when flashing to improve speed (no UI was giving the BLE operations almost full control of main thread).

But, indeed, a custom operation could resolve both of my issue. First, I would be able to schedule the operation off the main thread (doing it at my own risk) and also access lower level components for speed improvements.

I will try to come up with some numbers once I integrated the changes we did last week into the project. At this point, I will test the impact of running the operation on & off the main thread (and only this change) and come up with some numbers for you.

Like I say, the impact it not really important when we do minor operations (read & write a few characteristics). It was more painful when transferring lots of data and UI displaying some intensive graphics (progress, chart, etc.)

@RobLewis
Copy link

RobLewis commented Mar 7, 2017

This may become relevant to an app I'm developing to support a BLE peripheral data acquisition device. It sends blocks of data (up to 4KB) as fast as possible. The data needs to be graphed on the Android screen as it arrives, so there may be little or no benefit to receiving it on a different thread. I had been thinking to receive on an io thread, perhaps analyze and format the data on a compute thread, and hand off to the UI thread just to draw the graph. Any advice or comments appreciated—and apologies in advance if I am misunderstanding the issue.

@dariuszseweryn
Copy link
Owner

@RobLewis You say that the data needs to be displayed on the screen as it arrives - so I assume that the central (phone) is receiving the data via notification mechanism - in this situation there is no real benefit of this issue as it only touches operations that are performed from the central side (explicit characteristic read, characteristic write, setup notifications, etc.). Getting the notifications is a totally different story as it is executed on a different thread all along.

@maoueh Have you been able to check if this issue is still valid for you by any chance?

@maoueh
Copy link
Contributor Author

maoueh commented Mar 7, 2017

@dariuszseweryn On my side, since the addition of custom operations, I can do it on my own if needed. So for me, it's not relevant anymore.

@dariuszseweryn
Copy link
Owner

@maoueh Perhaps you have checked the difference between Write Long Op vs Custom Op? This info could be informative for people interested in #41

Anyway - closing this issue for now. If anyone would need this - feel free to ping me here.

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

No branches or pull requests

3 participants