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

Indications are not supported. #14

Closed
mgranberry opened this issue Apr 18, 2016 · 12 comments
Closed

Indications are not supported. #14

mgranberry opened this issue Apr 18, 2016 · 12 comments
Assignees

Comments

@mgranberry
Copy link

Summary

There doesn't appear to be a way to set up indication callbacks without first enabling notifications and then updating the descriptor. The particular device that I am interacting with doesn't appear to handle that well, leading to sporadic timeouts. A setupIndication(UUID characteristic) method would be helpful to have in addition to setupNoticication()

See my hack.

@uKL
Copy link
Collaborator

uKL commented Apr 18, 2016

This is true, the library does not handle indications yet. You are welcome to open a pull request :)

@mgranberry
Copy link
Author

How much testing do you want to see? It looks like setupNotification is heavily-tested, but much of that seems like it would be redundant to replicate.

@uKL
Copy link
Collaborator

uKL commented Apr 18, 2016

If you'd use same code don't duplicate tests. Just test what is new. Also I'd review the pull request itself for any bugs.

@dariuszseweryn
Copy link
Owner

With Paweł we will think how to handle situations where user would set a notification after setting indication to the same characteristic. Edge case - never the less needs to be addressed somehow. With your current proposal first setter would win and the second one would end up using the first one.

@mgranberry
Copy link
Author

@dariuszseweryn that is correct, and the reason that I haven't made a PR yet.

@mzgreen
Copy link
Collaborator

mzgreen commented Apr 19, 2016

What about adding an overwrite parameter to setupNoticication and setupIndication methods?

When user would want to set a notification after setting indication to the same characteristic and overwrite is false then nothing happens, otherwise a notification is set. The same would apply in other direction (setting indication after setting notification to the same characteristic).

@cbodin
Copy link

cbodin commented Apr 20, 2016

This is exactly what i'm needing. Your hack works great @mzgreen.

Is the overwrite parameter really needed? It seems like an overwrite could always happen and leave it up to the developer to check if a notification/indication is already set to the same characteristic.

@mzgreen
Copy link
Collaborator

mzgreen commented Apr 20, 2016

@cbodin Actually my idea with overwrite was wrong. It could lead to unexpected behavior. One way would be to not allow to set notification after setting indication and vice-versa. There is no perfect solution that would make it easier for developer I think.

@dariuszseweryn
Copy link
Owner

We will go with emitting an exception when the indication is being set after notification was already setup (or vice-versa). We are discussing the best API still though. The code should be available within this week.

@dariuszseweryn dariuszseweryn self-assigned this Apr 25, 2016
@cbodin
Copy link

cbodin commented Apr 25, 2016

Emitting exception directly when subscribing or throwing an exception when the setupNotification method is called?

@dariuszseweryn
Copy link
Owner

The setupNotification would return an Observable that will emit BleCharacteristicNotificationOfOtherTypeAlreadySetException. (Long name...)

@dariuszseweryn
Copy link
Owner

Added support for the indications with commit: https://github.com/Polidea/RxAndroidBle/commit/f69ce78b18f635d23efc9b71e06c3e6e7d87d1c9.
A new release will be made after we will fix the bugs found. For the moment you can use the snapshot: compile 'com.polidea.rxandroidble:rxandroidble:1.1.0-SNAPSHOT'.

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

5 participants