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

Feature DiscoverService with clearCache #453

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

GuillaumeBourge
Copy link
Contributor

Allow to refresh entierely the discover cache, to perform a new Gatt discover and be able to see a new service appeared during the connection

@ghost
Copy link

ghost commented Jul 26, 2018

waiting for this to be merged impatiently

Copy link
Owner

@dariuszseweryn dariuszseweryn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally got a moment to comment on this PR. I have one doubt about it:

The thing is that some Observables returned by RxBleConnection use the RxBleConnection.discoverServices() internally. If services will get changed during a connection all those previously created Observables may be out of sync. If a handle for a BluetoothGattCharacteristic with a given UUID will get changed then the user would need to create a new one using i.e. RxBleConnection.readCharacteristic(UUID) as the previous one may no longer use the correct handle which may lead to hard to debug errors...

I was thinking about this issue but haven't yet implemented an appropriate architecture for that. It is a part of new connection API proposition which should not allow to use BluetoothGattCharacteristic/descriptor/UUID directly

@GuillaumeBourge
Copy link
Contributor Author

That's good comment, i understand your wish to carry the user/dev to avoid any possible mistake or oversight and i am totally ok with it !
Maybe we should add in the javadoc a sentence about this case ?
I imagined this feature for developers who need more while controlling the subject (nobody will make a total refresh of his services without have this specific need i think).
Maybe you should integrate an "expert mode like" to no bridle the library mostly if you go further on the follow of the connection ? i mean by "expert mode" just mark functions by a tag in the doc or something ?
Your queue function for custom operation over the gatt is already a part of this in some way.

@dariuszseweryn
Copy link
Owner

I am not a fan of adding unsecure APIs. Users tend not to read the documentation frequently enough :(

The expert mode is the CustomOperation interface. One could actually create a CustomOperation that would make service discovery again and then start to use the part of the API that uses i.e. BluetoothGattCharacteristic instead of UUID

@GuillaumeBourge
Copy link
Contributor Author

i do not see how today we can make this feature with customOperation...
There is cache on differents levels, we have to manipulate the ServiceDiscoveryManager.java to clears theses caches :/

"Users tend not to read the documentation frequently enough" i agree with you. Here the little difference is to access to the refresh mode the user has to use the most complex version of the function. The basic user will use the simplier method first i think

@dariuszseweryn
Copy link
Owner

One can make an operation with RxBleCustomOperation<List<BluetoothGattService>> signature. In RxBleCustomOperation the user has access to both BluetoothGatt and RxBleGattCallback and can perform a service discovery that bypasses all library caching levels.

The only downside is that the user cannot use the API that takes UUID as the parameter as the lower levels will have no knowledge of what the custom operation performed

@GuillaumeBourge
Copy link
Contributor Author

Yes we can perform a discover from CustomOperation but in "ServiceDiscoveryManager.java" there are also 2 level of cache, the observable saved and the rx ".cache()" on the other observable...
So the services will be refresh a the Gatt level but the lib will not follow the new state of Gatt. That is more dangerous than refresh properly from the lib no?

@dariuszseweryn
Copy link
Owner

dariuszseweryn commented Aug 7, 2018

Usually the Observable is stateless without operators like .cache(). The deviceServicesObservable first checks BluetoothGatt if it has cached services. If there are none then it schedules a service discovery operation. If any of those will return only then the .cache() operator starts to work so next usages of the "UUID API" will not trigger this part of the code again.

That is more dangerous than refresh properly from the lib no?

Yes — most probably. I plan to add this feature at some point but not yet. My arguments:

  1. BluetoothGatt.refresh() seems to be already quite well tested — this is not a problem. The issue is that it should change the behaviour of other parts of the implementation like ServiceDiscoveryManager / RxBleConnection.discoverServices() (which is used by all UUID API under the hood)
  2. Managing of the ServiceDiscoveryManager state is not easy to get right. Ideally all RxBleConnection observables should work correctly even after performing BluetoothGatt.refresh(). With the current implementation it is not possible to guarantee that. It will even need a significant refactoring inside the library just to provide meaningful errors so the user would know what they have done wrong — this will also have an impact on the library performance
  3. Since the intended behaviour is possible to get via RxBleCustomOperation usage I do not see this feature to be needed just yet. It would need a lot of patching and introduce some bugs / performance degradation for a fairly large user base to help a fairly small amount of people. I feel that it is better to concentrate now on a new API and implementation that would fully cover the topic with the least possible negative impact

@GuillaumeBourge
Copy link
Contributor Author

For me yes this may introduce miss understanding behaviour for basic use (where this feature don't aim). But at the opposite, not having it, may introduce the same for user who needs real discovers feature.

  • the user thinks to do a discover and in the reality nop
  • the user has his ble device hiding service or characteristic at the run time -> failure with the wrong error message cause the stack believe that char is already here but in reality anymore.
  • the user has his ble device showing service or characteristic at the run time -> he is forced to disconnect/reconnect with a refresh to perform read/write...

I won't see how its possible through the RxBleCustomOperation.. May the solution for "expert mode" is to supply some feature like this with rxBleCustomOperation class ?
Apparently i am not the only one who has a need with this feature : @zcelaloglu , i am sure we can find other people :)

@seasonyuu
Copy link

Why still not merged?

@GuillaumeBourge
Copy link
Contributor Author

Any news about integrating this feature in this shape or another @dariuszseweryn ?

@dariuszseweryn
Copy link
Owner

Unfortunately I didn't had time to properly handle this project in the recent months. My previous doubts still stand

@GuillaumeBourge
Copy link
Contributor Author

GuillaumeBourge commented Jan 30, 2019

We worked on the service changed BLE feature (0x2A05) cause iOS needs it to be able to refresh services, it works fine with it. On Android we can see in HCI log the incoming command but the GATT does not refresh the services list and there is no event fired by the GATT to handle...
So the only possibility is to refresh the service list manually that is trivial in gatt direct access but locked here...

@dariuszseweryn
Copy link
Owner

To relieve your pain in awaiting a proper fix I am posting implementation on how one can refresh gatt on their own and then perform service discovery bypassing all caches

First the BluetoothGatt.refresh() custom operation:

RxBleCustomOperation<Void> bluetoothGattRefreshCustomOp = (bluetoothGatt, rxBleGattCallback, scheduler) -> {
    try {
        Method bluetoothGattRefreshFunction = bluetoothGatt.getClass().getMethod("refresh");
        boolean success = (Boolean) bluetoothGattRefreshFunction.invoke(bluetoothGatt);
        if (!success) return Observable.error(new RuntimeException("BluetoothGatt.refresh() returned false"));
        return Observable.<Void>empty().delay(500, TimeUnit.MILLISECONDS);
    } catch (NoSuchMethodException e) {
        return Observable.error(e);
    } catch (IllegalAccessException e) {
        return Observable.error(e);
    } catch (InvocationTargetException e) {
        return Observable.error(e);
    }
};

Then a BluetoothGatt.discoverServices() custom operation:

RxBleCustomOperation<List<BluetoothGattService>> discoverServicesCustomOp = (bluetoothGatt, rxBleGattCallback, scheduler) -> {
    boolean success = bluetoothGatt.discoverServices();
    if (!success) return Observable.error(new RuntimeException("BluetoothGatt.discoverServices() returned false"));
    return rxBleGattCallback.getOnServicesDiscovered()
            .take(1) // so this RxBleCustomOperation will complete after the first result from BluetoothGattCallback.onServicesDiscovered()
            .map(RxBleDeviceServices::getBluetoothGattServices);
};

And usage example:

connection.queue(bluetoothGattRefreshCustomOp).ignoreElements()
        .andThen(connection.queue(discoverServicesCustomOp))
        .flatMapSingle(bluetoothGattServices -> {
            BluetoothGattCharacteristic characteristic = null; // get your characteristic from bluetoothGattServices
            return connection.readCharacteristic(characteristic);
        });

Hope this help you for now

@CLAassistant
Copy link

CLAassistant commented Aug 12, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ francoisadam
❌ Guillaume Bourge


Guillaume Bourge seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

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

Successfully merging this pull request may close these issues.

5 participants