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

Adjust BleCannotSetCharacteristicNotificationException to be more descriptive #225

Closed
kanat opened this issue Jun 19, 2017 · 11 comments
Closed
Assignees
Milestone

Comments

@kanat
Copy link

kanat commented Jun 19, 2017

Summary

  • app has connection to BLE device
  • BLE device has characteristic with indicate enabled
  • disconnection happens
  • when app tries to invoke rxBleConnection.setupIndication(...) BleCannotSetCharacteristicNotificationException comes, but I expected to see BleDisconnectedException

P.S. before disconnection I could see:
BleGattException{macAddress=ED:78:87:F9:85:31, status=8 (0x08 -> https://android.googlesource.com/platform/external/bluetooth/bluedroid/+/android-5.1.0_r1/stack/include/gatt_api.h), bleGattOperationType=BleGattOperation{description='CONNECTION_STATE'}}

Preconditions

Steps to reproduce actual result


1. get app connected to BLE device

2. get app disconnected from device by going away

Actual result

BleCannotSetCharacteristicNotificationException

Expected result

BleDisconnectedException

Logs

error.txt

@dariuszseweryn dariuszseweryn self-assigned this Jun 19, 2017
@dariuszseweryn dariuszseweryn added bug Bug that is caused by the library and removed bug Bug that is caused by the library labels Jun 19, 2017
@dariuszseweryn
Copy link
Owner

dariuszseweryn commented Jun 19, 2017

BleCannotSetCharacteristicNotificationException is quite accurate in this situation as that is actually what is happening. What can we do:

  • Emit the exception that caused the disconnection. This could fit in a patch release (it would be either BleDisconnectedException or BleGattException) as it would be in line with how the library works on different requests.
  • Wrap BleGattException in BleDisconnectedException. It would be a change in the behaviour and should wait at least till 1.4.0 release.

I can go with the first point now and the second for 1.4.0. Would that fit your idea?

P.S. You should not use Observables derived from an RxBleConnection after that connection has been disconnected. It shows that you have more than one .subscribe() call. It is not a good idea when using RxJava as it may introduce state somewhere. While using BLE the connection is stateful and that is why a single .subscribe() should be used.

@RobLewis
Copy link

Regarding @dariuszseweryn P.S.: apologize for off-topic; please let me know if I should open a separate issue or address this somewhere else:

My app writes to 2 different characteristics and receives notifications from 2 others. I don't see any realistic way of combining all these into a single .subscribe(), especially since one notification is a pretty constant data "firehose".

Not knowing any better, I have been simply storing the RxBleConnection in a variable and using it in multiple .subscribe()s. As far as I can tell, this has been working OK. I've investigated the ConnectionSharingAdapter but as I discussed here, I don't understand what benefits it offers over my simplistic approach, though I would love to know.

In general, some elaboration on how multiple .subscribe()s introduce state, and the potential pitfalls, might be helpful.

@kanat
Copy link
Author

kanat commented Jun 19, 2017

I can go with the first point now and the second for 1.4.0. Would that fit your idea?

Ok.
The Idea is to get what is the reason of BleCannotSetCharacteristicNotificationException.
Adding getCause() could help as well.

The second problem is I cannot ask somewhere what is a current state. The only way is to susbscribe to RxBleDevice.

You should not use Observables derived from an RxBleConnection

Yeah, that's what I'm trying detect in onError branch, but Exception tells me nothing about Disconnection.

My logic is following:

  1. Scan selected device
  2. Get connection
  3. Subscribe to indication
  4. Communicate until Disconnection
  5. Restart from 1 if it was a Disconnection or from 2 otherwise

As I understand I need to restart subscription if I get any exception from RxBleConnection. Am I right?

@dariuszseweryn
Copy link
Owner

@kanat Yes. If the .establishConnection() will emit an error then no observable derived from the connection should be used - they will emit an error anyway.

So far it seems that your use case also could be fulfilled with only one .subscribe(). You can open a question on www.stackoverflow.com with a tag rxandroidble and rx-java and someone will answer on how to get the connection error there.

@RobLewis Your question should go to www.stackoverflow.com as well as it is a general question about how to use rx-java

@kanat
Copy link
Author

kanat commented Jun 19, 2017

Ok.
The only diff between knowing it's a disconnection or not is where I'll start again from point 1 (scan) or from point 2 (if it wasn't disconnection then I can try to reconnect without scanning)

Should I close the issue? Or it will be opened until version 1.4.0?

@dariuszseweryn
Copy link
Owner

Do not close it. I will leave it be until I will better specify the behaviour.

@dariuszseweryn dariuszseweryn changed the title Getting BleCannotSetCharacteristicNotificationException instead of BleDisconnectedException Adjust BleCannotSetCharacteristicNotificationException to be more descriptive Jun 20, 2017
@dariuszseweryn dariuszseweryn added this to the 1.4.0 milestone Jun 20, 2017
@kenwdelong
Copy link

@RobLewis Rob, did you post this question on SO? I am very interested in the answer as this fits my scenario as well.

@RobLewis
Copy link

RobLewis commented Jun 20, 2017 via email

dariuszseweryn added a commit that referenced this issue Jun 28, 2017
#225
Notification setup `Observable<Observable<byte[]>>` is now observing disconnection events to better notify about exceptions. Added cause to `BleCannotSetCharacteristicNotificationException` where possible.
@dariuszseweryn
Copy link
Owner

@kanat Feel free to check and comment the above Pull Request wether it will be useful for your use-case.

@kanat
Copy link
Author

kanat commented Jul 3, 2017

@dariuszseweryn it's ok now, thanks

dariuszseweryn added a commit that referenced this issue Jul 3, 2017
#225
Notification setup `Observable<Observable<byte[]>>` is now observing disconnection events to better notify about exceptions. Added cause to `BleCannotSetCharacteristicNotificationException` where possible.
@dariuszseweryn
Copy link
Owner

Is available in 1.4.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

4 participants