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

Modify behaviour of .observeConnectionStateChanges() #50

Closed
farmazon3000 opened this issue Aug 5, 2016 · 12 comments
Closed

Modify behaviour of .observeConnectionStateChanges() #50

farmazon3000 opened this issue Aug 5, 2016 · 12 comments
Assignees
Milestone

Comments

@farmazon3000
Copy link

I'd like to react in my app when RxBle#Radio: FINISHED happened - because for example I'd like to inform my UI when device actually finshed disconnecting.

Now, I believe, I can only unsubscribe from subscription on RxBleDevice#establishConnection and I don't know what happened later.

@uKL
Copy link
Collaborator

uKL commented Aug 5, 2016

Hey,

you can use RxBleDevice#getConnectionState() to get detailed information about the lifecycle.

@uKL uKL added help wanted question / library Issue containing question / discussion about library workings labels Aug 5, 2016
@uKL uKL self-assigned this Aug 5, 2016
@farmazon3000
Copy link
Author

farmazon3000 commented Aug 5, 2016

I was using RxBleDevice#getConnectionState but I was unsubscribing from subscription on RxBleDevice#getConnectionState as soon as I disconnect (i.e. unsubscribing from subscription on RxBleDevice#establishConnection).

This is why I was not informed by RxBleDevice#getConnectionState().

@farmazon3000
Copy link
Author

farmazon3000 commented Aug 5, 2016

Anyway, when I removed unsubscribing from subscription on RxBleDevice#getConnectionState, on logcat I see:

08-05 11:03:57.330 1736-1736/com.test.app D/BluetoothGatt: setCharacteristicNotification() - uuid: 00002a19-0000-1000-8000-00805f9b34fb enable: false
08-05 11:03:57.332 1736-1736/com.test.app D/RxBle#Radio:   QUEUED RxBleRadioOperationDescriptorWrite(177380646)
08-05 11:03:57.333 1736-1736/com.test.app V/XXXBleXX:XX:XX:XX:XX:XX: establishConnection doOnUnsubscribeXX:XX:XX:XX:XX:XX
08-05 11:03:57.335 1736-1795/com.test.app D/RxBle#Radio:  STARTED RxBleRadioOperationDescriptorWrite(177380646)
08-05 11:03:57.336 1736-1736/com.test.app V/XXXConnStateSubscriber: onNext: RxBleConnectionState{DISCONNECTED} thread Thread[main,5,main]
08-05 11:03:57.337 1736-1736/com.test.app D/XXXBleEngine: disconnected on thread Thread[main,5,main]
08-05 11:03:57.338 1736-1736/com.test.app V/XXXBleXX:XX:XX:XX:XX:XX: observeConnectionStateChanges doOnUnsubscribeXX:XX:XX:XX:XX:XX
08-05 11:03:57.339 1736-1736/com.test.app D/RxBle#Radio:   QUEUED RxBleRadioOperationDisconnect(177432340)
08-05 11:03:57.339 1736-1736/com.test.app D/XXXBleEngine: syncWithStorage, onCompleted
08-05 11:03:59.563 1736-1748/com.test.app D/RxBle#BluetoothGatt: onDescriptorWrite descriptor=00002902-0000-1000-8000-00805f9b34fb status=0
08-05 11:03:59.572 1736-1795/com.test.app D/RxBle#Radio: FINISHED RxBleRadioOperationDescriptorWrite(177380646)
08-05 11:03:59.572 1736-1795/com.test.app D/RxBle#Radio:  STARTED RxBleRadioOperationDisconnect(177432340)
08-05 11:03:59.574 1736-1736/com.test.app D/BluetoothManager: getConnectionState()
08-05 11:03:59.574 1736-1736/com.test.app D/BluetoothManager: getConnectedDevices
08-05 11:03:59.580 1736-1736/com.test.app D/BluetoothGatt: cancelOpen() - device: XX:XX:XX:XX:XX:XX
08-05 11:03:59.605 1736-1872/com.test.app D/BluetoothGatt: onClientConnectionState() - status=0 clientIf=5 device=XX:XX:XX:XX:XX:XX
08-05 11:03:59.605 1736-1872/com.test.app D/RxBle#BluetoothGatt: onConnectionStateChange newState=0 status=0
08-05 11:03:59.610 1736-1736/com.test.app D/BluetoothGatt: close()
08-05 11:03:59.610 1736-1736/com.test.app D/BluetoothGatt: unregisterApp() - mClientIf=5
08-05 11:03:59.612 1736-1795/com.test.app D/RxBle#Radio: FINISHED RxBleRadioOperationDisconnect(177432340)

The setCharacteristicNotification() - uuid: 00002a19-0000-1000-8000-00805f9b34fb enable: false goes just after I called disconnect (i.e. unsubscribing from subscription on RxBleDevice#establishConnection) and I had notifications enabled on Battery Level characteristic.

So onNext: RxBleConnectionState{DISCONNECTED} happens more than 2s before RxBle#Radio: FINISHED.

@dariuszseweryn
Copy link
Owner

@farmazon3000 I am revisiting this issue and I have an idea.
I do not think it will be possible to react on RxBle#Radio: FINISHED but it should be possible to modify the moment when .observeConnectionStateChanges() will emit RxBleConnectionState.DISCONNECTED (when the RxBleRadioOperationDisconnect would finish).

This change would cause a need for further changes. Things to consider:

  • At what point the CONNECTING state should be emitted?
    I think that it should be emitted right after the subscription to .establishConnection() regardless of the execution of connect operation as this may be a distant operation in the queue and the user would like to know if the library acknowledged the intent.
  • In the above case what should happen if user would like to establish a new connection after he unsubscribed from the previous .establishConnection() but the disconnect operation did not yet finished?
    My opinion is that in this situation emitted sequence would look like: CONNECTING -> CONNECTED -> DISCONNECTING -> CONNECTING.

This is an invite for a discussion on how the feature should look like.
FYI @uKL @Cierpliwy

@dariuszseweryn dariuszseweryn changed the title Add possiblity to react on RxBle#Radio: FINISHED Modify behaviour of .observeConnectionStateChanges() Jun 6, 2017
@dariuszseweryn dariuszseweryn added enhancement and removed help wanted question / library Issue containing question / discussion about library workings labels Jun 6, 2017
@dariuszseweryn dariuszseweryn assigned dariuszseweryn and unassigned uKL Jun 6, 2017
@uKL
Copy link
Collaborator

uKL commented Jun 7, 2017

A few thoughts from me. I think that status should reflect the library state of readiness when the DISCONNECTED is received it should be safe to establish the connection right away.

I think the current implementation is a little bit too optimistic in terms when it is emitted.

The correct logic is implemented within theRxBleGattCallback which reacts on the status from the onConnectionStateChange. It may be a good idea to rework status reporting at all to have one consitent source.

@dariuszseweryn
Copy link
Owner

Currently when the RxBleDevice emits DISCONNECTED it is safe to establish a new connection as that is the exact moment when the old RxBleConnection is freed — in terms that it is not yet closed but it will be closed before the next .establishConnection() will be able to start.

There is a difference between what RxBleGattCallback.getOnConnectionStateChange() is returning and RxBleDevice.observeConnectionStateChanges(). The former is reporting changes for a particular bluetooth connection as it is implemented in the Android BLE stack whereas the latter is emitting library state of connection — when the user will subscribe to .establishConnection() but there are other queued operations the library changes state for a particular RxBleDevice to CONNECTING even before the actual function BluetoothDevice.connectGatt() will be called. RxBleGattCallback and RxBleDevice states are actually in sync when the emission of CONNECTED state is happening.

@Cierpliwy
Copy link

when the user will subscribe to .establishConnection() but there are other queued operations the library changes state for a particular RxBleDevice to CONNECTING even before the actual function BluetoothDevice.connectGatt() will be called.

I don't like this behaviour at all. It is inconsistent with what is actually happening and it is a reason of a lot of edge cases which need to be handled. Every subscription is asynchronous by nature and I don't see any need to immediately apply CONNECTING state when .establishConnection() is called. Does it help with anything? User could do that by himself if he needs it?

@dariuszseweryn
Copy link
Owner

Then we could have an opposite situation to:

So onNext: RxBleConnectionState{DISCONNECTED} happens more than 2s before RxBle#Radio: FINISHED.

The onNext: RxBleConnectionState{CONNECTING} could happen long time after the .establishConnection() is subscribed due to RxBleRadio being busy. Maybe it is acceptable and understandable by the user?

@Cierpliwy
Copy link

For me it would be acceptable, but I would like to hear feedback from other developers.

@kenwdelong
Copy link

I think CONNECTING would best be when bleDevice.connectGatt() is called. If I want to know when the library got the request, I can use doOnSubscribe(), can't I?

For DISCONNECTED, I'd like to get that after the disconnect radio operation finishes. As noted, after RxBleRadioOperationDisconnect would be fine.

@dariuszseweryn
Copy link
Owner

That is true. Though in some situations the time distance between subscribing to .establishConnection() and emission of CONNECTING may be significant. But I get your point. Maybe someone else would also share their point of view.

dariuszseweryn added a commit that referenced this issue Jun 29, 2017
…tate changes of the actual `BluetoothGatt`.

#50
Previously `RxBleDevice.observeConnectionStateChanges()` was emitting immediately the initial state. The state was derived from user’s actions (i.e. `CONNECTING` was emitted right after user has subscribed to `RxBleDevice.establishConnection()`) rather than state in which the `BluetoothGatt` was. This behaviour has been changed to best reflect the state of `BluetoothGatt` at any given time.
@dariuszseweryn dariuszseweryn added this to the 1.4.0 milestone Jun 29, 2017
dariuszseweryn added a commit that referenced this issue Jul 4, 2017
`RxBleDevice.observeConnectionStateChanges()` emits only connection state changes of the actual `BluetoothGatt`.
#50
Previously `RxBleDevice.observeConnectionStateChanges()` was emitting immediately the initial state. The state was derived from user’s actions (i.e. `CONNECTING` was emitted right after user has subscribed to `RxBleDevice.establishConnection()`) rather than state in which the `BluetoothGatt` was. This behaviour has been changed to best reflect the state of `BluetoothGatt` at any given time.
@dariuszseweryn
Copy link
Owner

Should be available in 1.4.0-SNAPSHOT shortly.

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