Fix MTU not updated when changed by the peripheral. (closes#293) #294

dariuszseweryn · 2017-10-16T09:03:31Z

Previously the RxBleConnection.currentMtu was updated only when the MTU was updated by the central. This prevented Long Writes from utilising the increased Maximum Transfer Unit or the used from having a valid current MTU value—this made the utilisation of available bandwidth suboptimal.

…hanged by the peripheral. (closes#293) Previously the `RxBleConnection.currentMtu` was updated only when the MTU was updated by the central. This prevented Long Writes from utilising the increased Maximum Transfer Unit or the used from having a valid current MTU value—this made the utilisation of available bandwidth suboptimal.

uKL

I suggest some changes.

uKL · 2017-10-16T09:13:31Z

...droidble/src/main/java/com/polidea/rxandroidble/internal/connection/RxBleConnectionImpl.java

@@ -83,6 +83,21 @@ public RxBleConnectionImpl(
        this.longWriteOperationBuilderProvider = longWriteOperationBuilderProvider;
        this.callbackScheduler = callbackScheduler;
        this.illegalOperationChecker = illegalOperationChecker;
+
+        // unsubscription is not needed as all objects are on @ConnectionScope and will get GCed together
+        gattCallback.getOnMtuChanged()


At first, this is a good spot that the field is not updated properly, however, the proposed fix is a big NO-NO by design. I dislike the fact that there is some kind of logic in the constructor.

I'm thinking of a place, where we could put it and I think the ConnectorImpl layer is the best place. Its responsibility is to wire up the connection state.

uKL · 2017-10-16T09:26:36Z

...droidble/src/main/java/com/polidea/rxandroidble/internal/connection/RxBleConnectionImpl.java

+
+        // unsubscription is not needed as all objects are on @ConnectionScope and will get GCed together
+        gattCallback.getOnMtuChanged()
+                .retryWhen(new Func1<Observable<? extends Throwable>, Observable<?>>() {


Do we need retryWhen here? Isn't a retry enough?

uKL · 2017-10-16T10:18:35Z

...droidble/src/main/java/com/polidea/rxandroidble/internal/connection/RxBleConnectionImpl.java

@@ -83,6 +83,21 @@ public RxBleConnectionImpl(
        this.longWriteOperationBuilderProvider = longWriteOperationBuilderProvider;
        this.callbackScheduler = callbackScheduler;
        this.illegalOperationChecker = illegalOperationChecker;
+
+        // unsubscription is not needed as all objects are on @ConnectionScope and will get GCed together


The assumption on object lifecycle from within the object is generally not a good pattern, even if in this case it's true.

uKL

Some more thoughts to discuss.

uKL · 2017-10-17T13:32:48Z

rxandroidble/src/main/java/com/polidea/rxandroidble/internal/connection/MtuWatcher.java

+    @Inject
+    MtuWatcher(
+            final RxBleGattCallback rxBleGattCallback,
+            final AtomicInteger atomicInteger,


What is the reason of injecting an AtomicInteger rather than creating it in place? It seems to be only a container for a state. It's more like an implementation detail and should be hidden.

It is referenced in the new OnSubscribe() and it cannot be referenced there if it is a member of the MtuWatcher. The problem is that there is no easy way of extending a Completable. Maybe it is even an anti pattern as we can have independent Completable subscriptions and shared AtomicInteger

uKL · 2017-10-17T13:41:22Z

rxandroidble/src/main/java/com/polidea/rxandroidble/internal/connection/MtuWatcher.java

+@ConnectionScope
+class MtuWatcher extends Completable implements MtuProvider {
+
+    private final AtomicInteger currentMtuAtomicInteger;


Does it have to be atomic? Do we expect concurrent updates from different threads?

It has to be a reference so it could be referenced from the OnSubscribe

uKL · 2017-10-17T13:41:46Z

rxandroidble/src/main/java/com/polidea/rxandroidble/internal/connection/MtuWatcher.java

+            @Override
+            public void call(CompletableSubscriber completableSubscriber) {
+                Subscription mtuSubscription = rxBleGattCallback.getOnMtuChanged()
+                        .retry()


What is the scenario for retry() is required at all?

When the BluetoothGattCallback.onMtuChanged() would be called with status != GATT_SUCCESS

uKL · 2017-10-17T13:44:12Z

rxandroidble/src/main/java/com/polidea/rxandroidble/internal/connection/MtuWatcher.java

+                                atomicInteger.set(newMtu);
+                            }
+                        });
+                completableSubscriber.onSubscribe(mtuSubscription);


Would it be updated if no one is listening to this completable?

No. That is why it is subscribed in the ConnectorImpl

uKL · 2017-10-17T13:53:33Z

rxandroidble/src/main/java/com/polidea/rxandroidble/internal/connection/ConnectorImpl.java

+                return Observable.merge(
+                        newConnectionObservable.delaySubscription(connectedObservable),
+                        disconnectedErrorObservable,
+                        connectionComponent.mtuWatcherCompletable().<RxBleConnection>toObservable()


This is acceptable but I'm thinking about some better way. This introduces API of a Completable that doesn't do anything but stays subscribed. Is there any good way to watch for connect/disconnect events and notify MtuWatcher to subscribe/unsubscribe from the MTU changes? This would be more descriptive that merging with a magic Completable.

I just try to avoid manual state management

Why is manual state management bad? I think it might be more descriptive.

If you want to stick with this solution I would name the method some other way. It should clearly say that it is responsible for the watcher state.

uKL · 2017-10-17T13:54:28Z

rxandroidble/src/main/java/com/polidea/rxandroidble/internal/connection/MtuWatcher.java

+import rx.functions.Action1;
+
+@ConnectionScope
+class MtuWatcher extends Completable implements MtuProvider {


MtuWatcher or just MtuCache?

Currently it is watching the Mtu and providing the current value, name may be changed

uKL

Please think about method name in MtuWatcher as it could refer to its responsibility. The rest is ok.

… and `DisconnectAction`. In the future `ConnectionOperationQueue` will also use it probably.

uKL · 2017-10-24T12:05:59Z

rxandroidble/src/main/java/com/polidea/rxandroidble/internal/connection/MtuWatcher.java

+class MtuWatcher implements ConnectionSubscriptionWatcher, MtuProvider, Action1<Integer> {
+
+    private Integer currentMtu;
+


Do we need empty lines here? In other places, we don't keep them.

dariuszseweryn added the bug Bug that is caused by the library label Oct 16, 2017

dariuszseweryn added this to the 1.4.2 milestone Oct 16, 2017

dariuszseweryn requested a review from uKL October 16, 2017 09:03

uKL requested changes Oct 16, 2017

View reviewed changes

dariuszseweryn added 2 commits October 16, 2017 13:58

Code review updates.

c909589

Fixed MtuWatcher initial value.

ec4c658

uKL requested changes Oct 17, 2017

View reviewed changes

uKL approved these changes Oct 18, 2017

View reviewed changes

dariuszseweryn added 4 commits October 23, 2017 17:39

Introduced ConnectionSubscriptionWatcher interface for MtuWatcher…

803319e

… and `DisconnectAction`. In the future `ConnectionOperationQueue` will also use it probably.

Fixed function names to reflect the new interface name.

da20a72

Added changelog.

870ffe6

Added Javadoc to ConnectionSubsriptionWatcher interface.

1689f43

uKL approved these changes Oct 24, 2017

View reviewed changes

Removed empty lines.

50af93c

dariuszseweryn merged commit 55089f4 into master Oct 24, 2017

dariuszseweryn deleted the fix/issue_293_mtu_update_by_peripheral branch October 24, 2017 12:34

dariuszseweryn mentioned this pull request Oct 24, 2017

MTU is not updated if requested by the peripheral #293

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix MTU not updated when changed by the peripheral. (closes#293) #294

Fix MTU not updated when changed by the peripheral. (closes#293) #294

dariuszseweryn commented Oct 16, 2017

uKL left a comment

uKL Oct 16, 2017

uKL Oct 16, 2017

dariuszseweryn Oct 17, 2017

uKL Oct 16, 2017

dariuszseweryn Oct 17, 2017

uKL Oct 16, 2017

dariuszseweryn Oct 17, 2017

uKL left a comment

uKL Oct 17, 2017

dariuszseweryn Oct 17, 2017

uKL Oct 17, 2017

dariuszseweryn Oct 17, 2017

uKL Oct 17, 2017

dariuszseweryn Oct 17, 2017

uKL Oct 17, 2017

dariuszseweryn Oct 17, 2017

uKL Oct 17, 2017

dariuszseweryn Oct 17, 2017

uKL Oct 18, 2017

uKL Oct 17, 2017

dariuszseweryn Oct 17, 2017

uKL Oct 18, 2017

uKL left a comment

uKL Oct 24, 2017

		class MtuWatcher implements ConnectionSubscriptionWatcher, MtuProvider, Action1<Integer> {

		private Integer currentMtu;

Fix MTU not updated when changed by the peripheral. (closes#293) #294

Fix MTU not updated when changed by the peripheral. (closes#293) #294

Conversation

dariuszseweryn commented Oct 16, 2017

uKL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uKL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uKL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment