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

Disable notifications ACK not received, trying to enable notifications throws Status -3 #48

Closed
ghost opened this issue Nov 19, 2018 · 40 comments
Labels
Milestone

Comments

@ghost
Copy link

ghost commented Nov 19, 2018

One of the operations I perform often is an atomic enable/disable notifications and write characteristic

        Timber.d("$rawDataOn pre")
beginAtomicRequestQueue()
            .add(klEnableNotificationsRequest(sensorsRawDataCharacteristic, rawDataOn)) //enables/disables notifications
            .add(writeCharacteristic(sensorsStreamingControlCharacteristic, payload))
            .before {
                Timber.d("$rawDataOn before")
            }
            .done {
                Timber.d("$rawDataOn Completed")
            }
            .enqueue()

A standard execution when passing "false" logs something like this

12:13:20.565 false pre
12:13:20.565 false before
12:13:20.566 gatt.setCharacteristicNotification(01000001-55d0-4989-b640-cfb64e5c34e0, false)
12:13:20.566 setCharacteristicNotification() - uuid: 01000001-55d0-4989-b640-cfb64e5c34e0 enable: false
12:13:20.567 Disabling notifications and indications for 01000001-55d0-4989-b640-cfb64e5c34e0
12:13:20.568 gatt.writeDescriptor(00002902-0000-1000-8000-00805f9b34fb, value=0x00-00)
12:13:20.575 Data written to descr. 00002902-0000-1000-8000-00805f9b34fb, value: (0x) 00-00
12:13:20.575 Notifications and indications disabled
12:13:20.576 Writing characteristic 04000001-55d0-4989-b640-cfb64e5c34e0 (WRITE REQUEST)
12:13:20.576 gatt.writeCharacteristic(04000001-55d0-4989-b640-cfb64e5c34e0)
12:13:20.577 false Completed

Some times, if there are consecutive calls with enable set to false and then true, it tries to enable notifications before the disable is complete. Apparently, the "disable notifications" request never confirms that the notifications are disabled but still completes

12:13:19.467 false pre
12:13:19.495 true pre
12:13:20.485 false before
12:13:20.486 gatt.setCharacteristicNotification(01000001-55d0-4989-b640-cfb64e5c34e0, false)
12:13:20.486 setCharacteristicNotification() - uuid: 01000001-55d0-4989-b640-cfb64e5c34e0 enable: false
12:13:20.489 Disabling notifications and indications for 01000001-55d0-4989-b640-cfb64e5c34e0
12:13:20.490 gatt.writeDescriptor(00002902-0000-1000-8000-00805f9b34fb, value=0x00-00)
12:13:20.504 Writing characteristic 04000001-55d0-4989-b640-cfb64e5c34e0 (WRITE REQUEST)
12:13:20.505 gatt.writeCharacteristic(04000001-55d0-4989-b640-cfb64e5c34e0)
12:13:20.507 false Completed
12:13:20.508 true before
12:13:20.510 klEnableNotificationsRequest setNotificationCallback
12:13:20.511 gatt.setCharacteristicNotification(01000001-55d0-4989-b640-cfb64e5c34e0, true)
12:13:20.511 setCharacteristicNotification() - uuid: 01000001-55d0-4989-b640-cfb64e5c34e0 enable: true
12:13:20.514 Enabling notifications for 01000001-55d0-4989-b640-cfb64e5c34e0
12:13:20.514 gatt.writeDescriptor(00002902-0000-1000-8000-00805f9b34fb, value=0x01-00)
-> error -3 12:13:20.516 Failed to enabled notifications for 01000001-55d0-4989-b640-cfb64e5c34e0 (status = -3)
12:13:20.517 Writing characteristic 04000001-55d0-4989-b640-cfb64e5c34e0 (WRITE REQUEST)
12:13:20.518 gatt.writeCharacteristic(04000001-55d0-4989-b640-cfb64e5c34e0)
12:13:20.519 true Completed

If you compare it with the previous log, it's missing the "disable notifications" ACK

12:13:20.575 Data written to descr. 00002902-0000-1000-8000-00805f9b34fb, value: (0x) 00-00
12:13:20.575 Notifications and indications disabled

Does it sound like a bug in the library?

We develop our own firmware so there might as well be a bug there, but I don't have a sniffer to check the packets.

@ghost ghost changed the title Status -3 when trying to enable notifications before disable completes Disable notifications ACK not received, trying to enable notifications throws Status -3 Nov 19, 2018
@philips77
Copy link
Member

Yes, the atomic queue will complete no matter of the inner requests result. You may call queue.cancelQueue() in fail handler of each inner request to cancel further requests if you don't wish to continue after a fail.I wouldn't say it's a bug, rather a feature. I guess that could be more documented.
Reliable Write would cancel all remaining requests and fail, but the base atomic queue will continue.

As to the erro3 "Write Not Permitted", I would say it's a reply from your device?

@ghost
Copy link
Author

ghost commented Nov 19, 2018

What do you mean from "Write not permitted"? The status = -3? From my debugging, FailCallback.REASON_NULL_ATTRIBUTE is thrown when a gatt operation returns false

My guesses were
a) There's a fw bug
b) Somehow there's a thread blocked and the operation can't be completed

I'll really appreciate if you have other guess/thought. I've been fighting against this for too long now, I was hoping for a library bug that explained the behavior 😢

I'll close the ticket if you don't think this is a library issue

@philips77
Copy link
Member

Oh, -3, not 3 :)
Than for sure it's the lib bug. Let me dive into this.

@philips77 philips77 self-assigned this Nov 20, 2018
@philips77 philips77 added the bug label Nov 20, 2018
@philips77
Copy link
Member

Hi @maragues-kolibree,
So I find one issue in your logs. There are no entries starting from "Data written to..." (there is just one to be precise).
Each write request should end with that, and the next request should be started only when the callback is returned.
Here are my logs that look correctly:

I: disable before
D: gatt.setCharacteristicNotification(00001524-1212-efde-1523-785feabcd123, false)
V: Disabling notifications and indications for 00001524-1212-efde-1523-785feabcd123
D: gatt.writeDescriptor(00002902-0000-1000-8000-00805f9b34fb, value=0x00-00)
I: Connection parameters updated (interval: 45.0ms, latency: 0, timeout: 5000ms)
I: Data written to descr. 00002902-0000-1000-8000-00805f9b34fb, value: (0x) 00-00
I: Notifications and indications disabled
V: Writing characteristic 00001525-1212-efde-1523-785feabcd123 (WRITE REQUEST)
D: gatt.writeCharacteristic(00001525-1212-efde-1523-785feabcd123)
I: Data written to 00001525-1212-efde-1523-785feabcd123, value: (0x) 01
I: disable complete
I: enable before
D: gatt.setCharacteristicNotification(00001524-1212-efde-1523-785feabcd123, true)
V: Enabling notifications for 00001524-1212-efde-1523-785feabcd123
D: gatt.writeDescriptor(00002902-0000-1000-8000-00805f9b34fb, value=0x01-00)
I: Data written to descr. 00002902-0000-1000-8000-00805f9b34fb, value: (0x) 01-00
I: Notifications enabled
V: Writing characteristic 00001525-1212-efde-1523-785feabcd123 (WRITE REQUEST)
D: gatt.writeCharacteristic(00001525-1212-efde-1523-785feabcd123)
I: Data written to 00001525-1212-efde-1523-785feabcd123, value: (0x) 00
I: enable complete

It should not be possible to start any request before the previous operation is completed. If you indeed managed to do this, that's a bug in the library.
But you don't have Data written to... even in the first, correct log listing.

@ghost
Copy link
Author

ghost commented Nov 26, 2018

Here are some more logs. For reference, this is a method I call writeAndNotify

    /**
     * Blocking call that writes to a characteristic and waits until the response is emitted in a
     * characteristic notification.
     *
     * If the command doesn't receive a response in OPERATION_TIMEOUT_MS it throws a FailureReason
     * If the response to the command is not successful, it throws a CommandFailedException
     */
    @Throws(FailureReason::class)
    fun writeAndNotifyOperation(
        gattCharacteristicWrite: BluetoothGattCharacteristic?,
        payload: ByteArray
    ): ByteArray {
        if (gattCharacteristicWrite == null) throw FailureReason("Can't write on a null characteristic")

        val responseCharacteristic = mapToResponseCharacteristic(gattCharacteristicWrite)

        try {
            val responseContainer = object : ProfileReadResponse() {}

            Timber.d("writeAndNotifyOperation command id ${payload.first().toHex()}")
            waitForNotification(responseCharacteristic)
                .trigger(writeCharacteristic(gattCharacteristicWrite, payload))
                .timeout(OPERATION_TIMEOUT_MS)
                .awaitValid(responseContainer)

            val response = responseContainer.rawData.value ?: byteArrayOf()

            throwIfNonSuccessResponse(response, payload)

            return response
        } catch (exception: Exception) {
            ...
        }
    }

What usually happens is

  1. A sensor emits its new state
  2. I perform two operations
  • An atomic operation on characteristic 01000001-... that enables/disables notifications and writes a value. The one shown in the original message
  • An invocation to writeAndNotify on characteristic 02000004-...

If a sensor emits multiple states, I might call this sequence multiple times.

Logs for a successful invocation

09:14:34.188 1173-1286 D/ writeAndNotifyOperation command id 0x13 on 02000004-...
09:14:34.238 1173-1173 V/ Writing characteristic 02000004-... (WRITE REQUEST)
09:14:34.238 1173-1173 D/ gatt.writeCharacteristic(02000004-...)
09:14:34.318 1173-1173 I/ Data written to 02000004-..., value: (0x) 13
09:14:34.320 1173-1286 D/ writeAndNotifyOperation post command id 0x13

09:14:34.321 1173-1173 D/$enableNotificationsForSensorCharacteristicsAndWritePayload: before
09:14:34.322 1173-1173 D/ gatt.setCharacteristicNotification(01000001-..., true)
09:14:34.322 1173-1173 D/BluetoothGatt: setCharacteristicNotification() - uuid: 01000001-... enable: true
09:14:34.325 1173-1173 V/ Enabling notifications for 01000001-...
09:14:34.327 1173-1173 D/ gatt.writeDescriptor(00002902-..., value=0x01-00)
09:14:34.357 1173-1173 I/ Data written to descr. 00002902-..., value: (0x) 01-00
09:14:34.357 1173-1173 I/ Notifications enabled
09:14:34.358 1173-1173 D/$klEnableNotificationsRequest: Notifications enabled for 01000001-...
09:14:34.358 1173-1173 V/ Writing characteristic 04000001-... (WRITE REQUEST)
09:14:34.359 1173-1173 D/ gatt.writeCharacteristic(04000001-...)
09:14:34.390 1173-1173 I/ Data written to 04000001-..., value: (0x) 07-04-32-00
09:14:34.390 1173-1173 D/$enableNotificationsForSensorCharacteristicsAndWritePayload: done

Two logs for failure invocations. Both writeAndNotify fail to show the Data written to 02000004-... , whereas the success log does show it.

  • Any obvious flaw in my writeAndNotify implementation?
  • awaitValid isn't throwing an Exception and we seem to be getting a valid response, otherwise throwIfNonSuccessResponse would fail, but it's very suspicious that for both failure cases there was no Data written
  • Could this be a deadlock issue on my side?

It's not happening all the time, maybe 1 out of 10. Furthermore, I wasn't able to reproduce the issue when I commented out the invocation to writeAndNotify, so something's going on in there.

  1. Previous operation not finished when a new sensor state arrives
09:17:57.005 1173-1498 D/ writeAndNotifyOperation command id 0x13 on 02000004-...

//previous atomic operation
09:17:57.020 1173-1173 I/ Data written to descr. 00002902-..., value: (0x) 00-00
09:17:57.021 1173-1173 I/ Notifications and indications disabled
09:17:57.022 1173-1173 V/ Writing characteristic 04000001-... (WRITE REQUEST)
09:17:57.022 1173-1173 D/ gatt.writeCharacteristic(04000001-...)
09:17:57.068 1173-1173 I/ Data written to 04000001-..., value: (0x) 00-00-32-00
09:17:57.068 1173-1173 D/$enableNotificationsForSensorCharacteristicsAndWritePayload: done

//writeAndNotifyOperation command id 0x13
09:17:57.069 1173-1173 V/ Writing characteristic 02000004-... (WRITE REQUEST)
09:17:57.070 1173-1173 D/ gatt.writeCharacteristic(02000004-...)
09:17:58.138 1173-1498 D/ writeAndNotifyOperation post command id 0x13

//new atomic operation
09:17:58.138 1173-1173 D/$enableNotificationsForSensorCharacteristicsAndWritePayload: before
09:17:58.139 1173-1173 D/ gatt.setCharacteristicNotification(01000001-..., true)
09:17:58.139 1173-1173 D/BluetoothGatt: setCharacteristicNotification() - uuid: 01000001-... enable: true
09:17:58.143 1173-1173 V/ Enabling notifications for 01000001-...
09:17:58.143 1173-1173 D/ gatt.writeDescriptor(00002902-..., value=0x01-00)
09:17:58.145 1173-1173 D/$klEnableNotificationsRequest: Failed to enabled notifications for 01000001-... (status = -3)
09:17:58.146 1173-1173 V/ Writing characteristic 04000001-55d0-4989-b640-cfb64e5c34e0 (WRITE REQUEST)
09:17:58.147 1173-1173 D/ gatt.writeCharacteristic(04000001-55d0-4989-b640-cfb64e5c34e0)
09:17:58.148 1173-1173 D/$enableNotificationsForSensorCharacteristicsAndWritePayload: done
  1. Simple sequence execution
09:27:57.300 1173-1498 D/ writeAndNotifyOperation command id 0x13
09:27:57.314 1173-1173 V/ Writing characteristic 02000004-... (WRITE REQUEST)
09:27:57.315 1173-1173 D/ gatt.writeCharacteristic(02000004-...)
09:27:58.280 1173-1498 D/ writeAndNotifyOperation post command id 0x13

//new atomic operation
09:27:58.280 1173-1173 D/$enableNotificationsForSensorCharacteristicsAndWritePayload: before
09:27:58.281 1173-1173 D/ gatt.setCharacteristicNotification(01000001-..., true)
09:27:58.281 1173-1173 D/BluetoothGatt: setCharacteristicNotification() - uuid: 01000001-... enable: true
09:27:58.283 1173-1173 V/ Enabling notifications for 01000001-...
09:27:58.284 1173-1173 D/ gatt.writeDescriptor(00002902-0000-1000-8000-00805f9b34fb, value=0x01-00)
09:27:58.285 1173-1173 E/$klEnableNotificationsRequest: Failed to enabled notifications for 01000001-... (status = -3)
09:27:58.286 1173-1173 V/ Writing characteristic 04000001-... (WRITE REQUEST)
09:27:58.287 1173-1173 D/ gatt.writeCharacteristic(04000001-...)
09:27:58.288 1173-1173 D/$enableNotificationsForSensorCharacteristicsAndWritePayload: done

Let me know if I can provide any more details. Thanks for your time!

@philips77
Copy link
Member

Let's start with the easy: To make your responseContainer throw when the value is invalid, you have to extend the ProfileReadResponse class, validate the value and call onInvalidDataReceived (example). Otherwise how would it know what data do you expect and what's valid or not.

How do you wait fo the sensor to emit new state? What do you do then? Do you have the source on github, or somewhere I could have a look?

@ghost
Copy link
Author

ghost commented Nov 26, 2018

Ok, I've created a WriteAndNotifyResponse with this code

override fun onDataReceived(device: BluetoothDevice, data: Data) {
        super.onDataReceived(device, data)

        if (!validateActionCommandResponse()) {
            onInvalidDataReceived(device, data)
        }
    }

I don't think this changes anything on this issue tho, I just reproduced it and the .isValid() check passed just fine.

On the how do I wait for the sensor, it's quite convoluted and I'm afraid I'm not allowed to share the code. I'll try to summarize it

  1. Once BleManagerGattCallback is initialized, I enable certain characteristics
BleManagerGattCallback {
     ...
    override fun initialize() {
            super.initialize()

           klEnableNotificationsRequest(...).enqueue()
    }
}

fun klEnableNotificationsRequest(gattCharacteristic: BluetoothGattCharacteristic?): WriteRequest {
        if (gattCharacteristic == null) throw FailureReason("Can't write on a null characteristic")

        return enableNotifications(gattCharacteristic)
            .before {
                setNotificationCallback(gattCharacteristic)
                    .with { _, data ->
                        onCharacteristicDataReceived(gattCharacteristic, data)
                    }
            }
    }        

I wonder if there's a better way to do this

  1. Whenever I receive a value changed notification, I emit it all values to a KLManagerCallback
internal interface KLManagerCallbacks : BleManagerCallbacks {
    fun onNotify(uuid: String, value: ByteArray?)
}

fun onCharacteristicDataReceived(
        gattCharacteristic: BluetoothGattCharacteristic,
        data: Data
    ) {
        mCallbacks.onNotify(gattCharacteristic.uuid.toString(), data.value)
    }
  1. The KLManagerCallbacks instance processes the data and notifies the appropriate listeners
@Override
  public void onNotify(@NonNull String uuid, @Nullable byte[] value) {
    if (value == null) {
      return;
    }

    if (MY_CHAR.equals(uuid)) {       
      final PayloadReader payload = new PayloadReader(value);
    
      //detect the appropriate listener
      switch (...) { 
        case ...:
          boolean isEnabled = ...;

          //call that triggers the sequence mentioned in my previous comment
          listener.onSensorStateChanged(isEnabled);
          break;
        
        default:
          break;
      }
    } else  {
          ...
    } 

    //notify other subscribers
    bleNotificationDataRelay.accept(BleNotificationData.create(UUID.fromString(uuid), value));
  }

I know it's risky to invoke alien methods, but in my checks I've haven't seen onNotify blocked recently, but it's definitely a possibility that something's wrong there. It's legacy code (>4 years) and complex to refactor

Maybe it'd be wiser to move this onNotify code to the main thread or to a single executor

  1. The sensor listener does some calculations on its own and notifies its listeners on the main thread. In this case, it's an Activity that then processes the sensor state change and triggers the command sequence.

Thanks!

@philips77
Copy link
Member

Could you check from where is writeAndNotifyOperation post command id 0x13 called in the fail scenario?
In case f success it's called from onCharacteristicWriteSafe, as "Data written to..." is printed. Bun when it fails, it's called from somewhere else, as there is no this entry in log.

@ghost
Copy link
Author

ghost commented Nov 26, 2018

You mean which thread? It's always the invoker thread

Log for simple sequence failure, snippet of writeAndNotify

09:27:57.300 1173-1498 D/ writeAndNotifyOperation command id 0x13
09:27:57.314 1173-1173 V/ Writing characteristic 02000004-... (WRITE REQUEST)
09:27:57.315 1173-1173 D/ gatt.writeCharacteristic(02000004-...)
09:27:58.280 1173-1498 D/ writeAndNotifyOperation post command id 0x13

I guess you already know this but just for clarification, 1173 is the main thread, and the one next to the hyphen is the thread where the log line is written. In this case, pre and post logs are written on thread 1498.

@philips77
Copy link
Member

No, if you put a breakpoint therein writeAndNotifyOperation post command id... and check which method in Ble Manager called this callback. It should have been called from here: https://github.com/NordicSemiconductor/Android-BLE-Library/blob/master/ble/src/main/java/no/nordicsemi/android/ble/BleManager.java#L2650, but that's after logging Data written to..., so I guess it called from somewhere else.

@philips77
Copy link
Member

Also, the initialization could be changed to:

BleManagerGattCallback {
     ...
    override fun initialize() {
            super.initialize()
            setNotificationCallback(gattCharacteristic)
                .with { _, data ->
                    onCharacteristicDataReceived(gattCharacteristic, data)
                }
           klEnableNotificationsRequest(...).enqueue()
    }
}

fun klEnableNotificationsRequest(gattCharacteristic: BluetoothGattCharacteristic?): WriteRequest {
    if (gattCharacteristic == null) throw FailureReason("Can't write on a null characteristic")

    return enableNotifications(gattCharacteristic);
}     

No need to initialize the callback more than once, unless you need it. But that's also not important for the issue, and should work as well as you have it.

@philips77
Copy link
Member

Hmm.. the notification callback gets removed when you disable notifications, true. So your code is ok :) Sorry!

@ghost
Copy link
Author

ghost commented Nov 26, 2018

Yeah, I had to scratch my head to get that working.

I'm not getting any done/fail/invalid callback invocation on the trigger operation in writeAndNotify, I guess because of the awaitValid

waitForNotification(responseCharacteristic)
                .trigger(writeCharacteristic(gattCharacteristicWrite, payload)
                    .invalid { Timber.d("writeAndNotifyOperation invalid id ${payload.first().toHex()}") }
                    .done { Timber.d("writeAndNotifyOperation done id ${payload.first().toHex()}") }
                    .fail { _, status -> Timber.d("writeAndNotifyOperation fail id ${payload.first().toHex()} with status $status") }
                )
                .timeout(OPERATION_TIMEOUT_MS)
                .awaitValid(response)

If I add a breakpoint after awaitValid, the invoker is my code.

I'll keep on debugging and keep you posted

@ghost
Copy link
Author

ghost commented Nov 26, 2018

After reproducing the issue while debugging

The WriteRequest has notifyFail invoked with status = -3, but both failCallback and internalFailCallback are null.

It's invoked from BleManager#3391

I've normally seen this happening when BluetoothGatt#writeCharacteristic returns false

@philips77
Copy link
Member

One more thing, where was this nextRequest(..) called from?

@ghost
Copy link
Author

ghost commented Nov 27, 2018

Whenever a WriteCharacteristic doesn't log a Data written to..., I receive a fail { } in my enable/disable notifications request

fail invocation stack trace is as follows
BleManager#3391
BleManager#3192
BleManager#2850
BleManager#2026
MainThreadBluetoothGattCallback#135
Request#796

Also, I'm sometimes seeing WriteAndNotifyResponse having rawData == null after .awaitValid. Don't know if it's related, it doesn't happen 100% of the times that the status = -3

        ... 
        .awaitValid(response)

173        return response.responseThrowIfNotValid()
53 fun response(): ByteArray? = rawData.value

fun responseThrowIfNotValid(): ByteArray {
        if (!isValid) {
            throw CommandFailedException(...)
        }

77        return response()!!
    }
W/System.err: rawData must not be null
W/System.err:     at WriteAndNotifyResponse.response(WriteAndNotifyResponse.kt:53)
W/System.err:     at WriteAndNotifyResponse.responseThrowIfNotValid(WriteAndNotifyResponse.kt:77)
W/System.err:     at KLNordicBleManager.writeAndNotifyOperation(KLNordicBleManager.kt:173)

@ghost
Copy link
Author

ghost commented Nov 28, 2018

The same -3 status happened with a different set of commands, but the sequence is very similar

  1. A WriteRequest doesn't log Data written to... but apparently completes successfully
  2. The next request returns status = -3
11:03:09.565 31753-31850 D/: writeAndNotifyOperation command id 0x4
11:03:09.566 31753-31753 V/: Writing characteristic 03000005-...(WRITE REQUEST)
11:03:09.566 31753-31753 D/: gatt.writeCharacteristic(03000005-...)
11:03:09.591 31753-31753 I/: Data written to 03000005-..., value: (0x) 04-07-00
11:03:09.591 31753-31850 D/: writeAndNotifyOperation post command id 0x4

//Write not logging Data written
11:03:09.592 31753-31850 D/: writeAndNotifyOperation command id 0x4
11:03:09.593 31753-31753 V/: Writing characteristic 03000005-... (WRITE REQUEST)
11:03:09.593 31753-31753 D/: gatt.writeCharacteristic(03000005-...)
11:03:09.633 31753-31850 D/: writeAndNotifyOperation post command id 0x4

11:03:09.634 31753-31850 D/: writeAndNotifyOperation command id 0x4
11:03:09.635 31753-31753 V/: Writing characteristic 03000005-... (WRITE REQUEST)
11:03:09.635 31753-31753 D/: gatt.writeCharacteristic(03000005-...)
11:03:09.636 31753-31753 I/: Data written to 03000005-..., value: (0x) 04-09-00
11:03:09.637 31753-31850 E/: java.lang.RuntimeException: com.kolibree.android.sdk.error.FailureReason: Request failed with status -3

@ghost
Copy link
Author

ghost commented Nov 29, 2018

I think I've identified WaitForValueChangedRequest as the source of issues

  • This works perfectly fine and never returns status = -3
writeCharacteristic(deviceParametersCharacteristic, payload)
                .fail { _, status ->
                    Timber.e("writeCharacteristic sensors failed with status %s",status)
                }
                .enqueue()
  • This returns status = -3 quite often, both using enqueue() and awaitValid(response)
waitForNotification(deviceParametersCharacteristic!!)
                .trigger(writeCharacteristic(deviceParametersCharacteristic, payload)
                    .fail { _, status ->
                      Timber.e("writeCharacteristic sensors failed with status %s",status)
                    }
                )
                .timeout(OPERATION_TIMEOUT_MS)
                .enqueue()

Sorry to spam the thread, but this is biting us and we need to fix it 😢

@philips77
Copy link
Member

Are you receiving the new sensor state events on the same characteristic that you then use for waitAndNofity, or a different one?

@philips77
Copy link
Member

philips77 commented Nov 29, 2018

To me the issue is indeed related to the waitForNotification and the trigger. It may happen, when you receive the notification before the trigger was started or completed, and then 2 instances of nextRequest are called. I'm trying to get the exact order of events that may create the issue.
PS. And I got it.

@ghost
Copy link
Author

ghost commented Nov 29, 2018

Really happy to hear to you were able to reproduce the issue!

I also want to check with our FW team, because it's happening in some devices while I can't reproduce it on others which theoretically share the FW code. I'll come back to you if they discover anything valuable.

Turns out I was wrong, it's happening in all devices.

@ghost
Copy link
Author

ghost commented Nov 29, 2018

Regarding your characteristic notification question

  1. In the scenarios I'm describing, WaitForValueChangedRequest writes and listens to the same characteristic
  2. In the sequence of commands that I talked about in previous comments, writeAndNotify and the AtomicOperation act on different characteristics, as you can see in the logs
09:14:34.188 1173-1286 D/ writeAndNotifyOperation command id 0x13 on 02000004-...

09:14:34.322 1173-1173 D/ gatt.setCharacteristicNotification(01000001-..., true)

It's even a different service, if that makes any difference

philips77 added a commit that referenced this issue Nov 29, 2018
@philips77
Copy link
Member

The way I reproduced the issue was by playing with nRF Blinky. I've added:

			waitForNotification(mButtonCharacteristic)
					.before(device -> Log.i("AAAA", "Before wait for"))
					.trigger(
							writeCharacteristic(mLedCharacteristic, BlinkyLED.turnOn())
									.before(device -> Log.i("AAAA", "Before trigger"))
									.fail((device, status) -> Log.w("AAAA", "Trigger failed: " + status))
									.with((device, data) -> Log.i("AAAA", "Data sent: " + data))
									.done(device -> Log.i("AAAA", "Trigger done"))
					)
					.fail((device, status) -> {
						Log.w("AAAA", "Wait for failed: " + status);
					})
					.done(device -> {
						Log.i("AAAA", "Wait for done");
					})
					.with((device, data) -> Log.i("AAAA", "Notification received: " + data))
					.timeout(10000)
					.enqueue();

			waitForNotification(mButtonCharacteristic)
					.before(device -> Log.i("AAAA", "Before wait for 2"))
					.trigger(
							writeCharacteristic(mLedCharacteristic, BlinkyLED.turnOff())
									.before(device -> Log.i("AAAA", "Before trigger 2"))
									.fail((device, status) -> Log.w("AAAA", "Trigger 2 failed: " + status))
									.with((device, data) -> Log.i("AAAA", "Data sent 2: " + data))
									.done(device -> Log.i("AAAA", "Trigger 2 done"))
					)
					.fail((device, status) -> {
						Log.w("AAAA", "Wait for 2 failed: " + status);
					})
					.done(device -> {
						Log.i("AAAA", "Wait for 2 done");
					})
					.with((device, data) -> Log.i("AAAA", "Notification 2 received: " + data))
					.timeout(10000)
					.enqueue();

to initialize(). It sends LED On, then waits for a notification, then sends LED Off and waits for another notification. When I click and release the button on DK quickly, the notification is received before the trigger was sent, but after the waitForNotification was started. After receiving the notification it went into the last if statement in onCharacteristicChangedSafe and called for a next request. But the trigger task was already ongoing, so there were 2 running simultaneously.
Logs:

I: Before wait for
I: Before trigger
V: Writing characteristic 00001525-1212-efde-1523-785feabcd123 (WRITE REQUEST)
D: gatt.writeCharacteristic(00001525-1212-efde-1523-785feabcd123)
I: Notification received from 00001524-1212-efde-1523-785feabcd123, value: (0x) 01
I: Notification received: (0x) 01
I: Wait for done
I: Before wait for 2
I: Before trigger 2
V: Writing characteristic 00001525-1212-efde-1523-785feabcd123 (WRITE REQUEST)
D: gatt.writeCharacteristic(00001525-1212-efde-1523-785feabcd123)
W: Trigger 2 failed: -3
W: Wait for 2 failed: -3
I: Data written to 00001525-1212-efde-1523-785feabcd123, value: (0x) 00
I: Notification received from 00001524-1212-efde-1523-785feabcd123, value: (0x) 00
I: Notification received from 00001524-1212-efde-1523-785feabcd123, value: (0x) 01
I: Notification received from 00001524-1212-efde-1523-785feabcd123, value: (0x) 00

I pushed a change to develop branch with a fix (I hope). I'm not sure if it really fixes your issue, could you try it before I publish the version?

@ghost
Copy link
Author

ghost commented Nov 29, 2018

I'm getting a timeout

13:14:26.987 27003-27085/ D/KLNordicBleManager: writeAndNotifyOperation command id 0x33
13:14:26.996 27003-27003/ V/KLNordicBleManager: Writing characteristic 02000004-55d0-4989-b640-cfb64e5c34e0 (WRITE REQUEST)
13:14:26.997 27003-27003/ D/KLNordicBleManager: gatt.writeCharacteristic(02000004-55d0-4989-b640-cfb64e5c34e0)
13:14:27.026 27003-27003/ I/KLNordicBleManager: Data written to 02000004-55d0-4989-b640-cfb64e5c34e0, value: (0x) 33-12-0B-1D-0C-0E-1A

13:14:31.003 27003-27085/ W/System.err: Caused by: java.lang.InterruptedException
13:14:31.003 27003-27085/ W/System.err:     at no.nordicsemi.android.ble.TimeoutableRequest.await(TimeoutableRequest.java:128)
13:14:31.003 27003-27085/ W/System.err:     at no.nordicsemi.android.ble.TimeoutableValueRequest.await(TimeoutableValueRequest.java:114)
13:14:31.004 27003-27085/ W/System.err:     at no.nordicsemi.android.ble.WaitForValueChangedRequest.await(WaitForValueChangedRequest.java:182)
13:14:31.004 27003-27085/ W/System.err:     at no.nordicsemi.android.ble.WaitForValueChangedRequest.awaitValid(WaitForValueChangedRequest.java:224)
13:14:31.005 27003-27085/ W/System.err:     at com.kolibree.android.sdk.core.driver.ble.nordic.KLNordicBleManager.writeAndNotifyOperation(KLNordicBleManager.kt:174)
 waitForNotification(responseCharacteristic)
                .trigger(writeCharacteristic(gattCharacteristicWrite, payload))
                .timeout(4000)
                .awaitValid(response)

Notifications were enabled previously

3:14:22.843 27003-27003/ D/KLNordicBleManager: Notifications has been enabled for 02000004-55d0-4989-b640-cfb64e5c34e0

@ghost
Copy link
Author

ghost commented Nov 29, 2018

If you think it can help, I'm open to a pair programming session

@philips77
Copy link
Member

Are you sure you are getting the notification before the timeout after the 33-12-0B-... was sent?
There should be a log "Notification ...".

We can work on it tomorrow. I was quite busy with some other tasks today, sorry.

@ghost
Copy link
Author

ghost commented Nov 29, 2018

You are right, I was filtering out the notification log line. This is the full log

16:30:54.242 7431-7840/ D/KLNordicBleManager: writeAndNotifyOperation command id 0x44
16:30:54.245 7431-7431/ V/KLNordicBleManager: Writing characteristic 02000004-55d0-4989-b640-cfb64e5c34e0 (WRITE REQUEST)
16:30:54.246 7431-7431/ D/KLNordicBleManager: gatt.writeCharacteristic(02000004-55d0-4989-b640-cfb64e5c34e0)
16:30:54.269 7431-7431/ I/KLNordicBleManager: Notification received from 02000004-55d0-4989-b640-cfb64e5c34e0, value: (0x) 44-01-01-00-07-00-5A-12-10
16:30:54.270 7431-7431/ I/KLNordicBleManager: Data written to 02000004-55d0-4989-b640-cfb64e5c34e0, value: (0x) 44-01-01-00-07-00-5A-12-10
16:30:58.246 7431-7840/ E/KLNordicBleManager: Error attempting to get device parameter 0x44
16:30:58.247 7431-7840/ W/System.err: com.kolibree.android.sdk.error.FailureReason: Command id 0x44 on 02000004-55d0-4989-b640-cfb64e5c34e0 Original message was (null)
16:30:58.248 7431-7840/ W/System.err:     at core.driver.ble.nordic.KLNordicBleManager.writeAndNotifyOperation(KLNordicBleManager.kt:185)
16:30:58.249 7431-7840/ W/System.err:     at core.driver.ble.nordic.KLNordicBleManager.getDeviceParameter(KLNordicBleManager.kt:274)
16:30:58.249 7431-7840/ W/System.err:     at core.driver.ble.KolibreeBleDriver.loadGruDataInfo(KolibreeBleDriver.java:547)
16:30:58.250 7431-7840/ W/System.err:     at core.driver.ble.KolibreeBleDriver.onFirstConnectionEstablished(KolibreeBleDriver.java:192)
16:30:58.250 7431-7840/ W/System.err:     at core.driver.ble.-$$Lambda$AL9n0ahcOq1C5PrhRUIo07cQXzQ.run(Unknown Source:2)
16:30:58.251 7431-7840/ W/System.err:     at io.reactivex.internal.operators.completable.CompletableFromAction.subscribeActual(CompletableFromAction.java:34)
16:30:58.251 7431-7840/ W/System.err:     at io.reactivex.Completable.subscribe(Completable.java:2185)
16:30:58.251 7431-7840/ W/System.err:     at io.reactivex.internal.operators.completable.CompletableSubscribeOn$SubscribeOnObserver.run(CompletableSubscribeOn.java:64)
16:30:58.252 7431-7840/ W/System.err:     at io.reactivex.internal.schedulers.ScheduledDirectTask.call(ScheduledDirectTask.java:38)
16:30:58.252 7431-7840/ W/System.err:     at io.reactivex.internal.schedulers.ScheduledDirectTask.call(ScheduledDirectTask.java:26)
16:30:58.253 7431-7840/ W/System.err:     at java.util.concurrent.FutureTask.run(FutureTask.java:266)
16:30:58.255 7431-7840/ W/System.err:     at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1162)
16:30:58.255 7431-7840/ W/System.err:     at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:636)
16:30:58.255 7431-7840/ W/System.err:     at java.lang.Thread.run(Thread.java:764)
16:30:58.258 7431-7840/ W/System.err: Caused by: java.lang.InterruptedException
16:30:58.258 7431-7840/ W/System.err:     at no.nordicsemi.android.ble.TimeoutableRequest.await(TimeoutableRequest.java:128)
16:30:58.259 7431-7840/ W/System.err:     at no.nordicsemi.android.ble.TimeoutableValueRequest.await(TimeoutableValueRequest.java:114)
16:30:58.259 7431-7840/ W/System.err:     at no.nordicsemi.android.ble.WaitForValueChangedRequest.await(WaitForValueChangedRequest.java:182)
16:30:58.260 7431-7840/ W/System.err:     at no.nordicsemi.android.ble.WaitForValueChangedRequest.awaitValid(WaitForValueChangedRequest.java:224)
16:30:58.260 7431-7840/ W/System.err:     at com.kolibree.android.sdk.core.driver.ble.nordic.KLNordicBleManager.writeAndNotifyOperation(KLNordicBleManager.kt:174)
16:30:58.261 7431-7840/ W/System.err: 	... 13 more

More important, this is only happening in one of our devices, the other works fine, so maybe we unveiled a firmware bug.

@ghost
Copy link
Author

ghost commented Nov 29, 2018

The notification on characteristic 02000004 does arrive, but before the Data written to 02000004. On the device that works fine, the order is as I'd expect, first the write, then the notification change

Failure writeAndNotify

17:19:18.697 10463-10664/ D/KLNordicBleManager: writeAndNotifyOperation command id 0x44
17:19:18.701 10463-10463/ V/KLNordicBleManager: Writing characteristic 02000004-... (WRITE REQUEST)
17:19:18.702 10463-10463/ D/KLNordicBleManager: gatt.writeCharacteristic(02000004-...)
17:19:18.729 10463-10463/ I/KLNordicBleManager: Notification received from 02000004-..., value: ...
17:19:18.732 10463-10463/ I/KLNordicBleManager: Data written to 02000004-..., value: ...
17:19:22.702 10463-10664/ E/KLNordicBleManager: Error attempting to get device parameter 0x44
17:19:22.715 10463-10664/ W/System.err: Caused by: java.lang.InterruptedException
17:19:22.716 10463-10664/ W/System.err:     at no.nordicsemi.android.ble.TimeoutableRequest.await(TimeoutableRequest.java:128)
17:19:22.716 10463-10664/ W/System.err:     at no.nordicsemi.android.ble.TimeoutableValueRequest.await(TimeoutableValueRequest.java:114)
17:19:22.717 10463-10664/ W/System.err:     at no.nordicsemi.android.ble.WaitForValueChangedRequest.await(WaitForValueChangedRequest.java:182)
17:19:22.717 10463-10664/ W/System.err:     at no.nordicsemi.android.ble.WaitForValueChangedRequest.awaitValid(WaitForValueChangedRequest.java:224)
17:19:22.718 10463-10664/ W/System.err:     at com.kolibree.android.sdk.core.driver.ble.nordic.KLNordicBleManager.writeAndNotifyOperation(KLNordicBleManager.kt:174)

Successful writeAndNotify

17:21:29.870 10463-10690/ D/KLNordicBleManager: writeAndNotifyOperation command id 0x38
17:21:29.872 10463-10463/ V/KLNordicBleManager: Writing characteristic 02000004- (WRITE REQUEST)
17:21:29.872 10463-10463/ D/KLNordicBleManager: gatt.writeCharacteristic(02000004-)
17:21:31.568 10463-10463/ I/KLNordicBleManager: Data written to 02000004-, value: ...
17:21:31.571 10463-10463/ I/KLNordicBleManager: Notification received from 02000004-, value: ...
17:21:31.574 10463-10690/ D/KLNordicBleManager: writeAndNotifyOperation post command id 0x38

PS. Nevermind my previous comment

@philips77
Copy link
Member

Yep. Order is wrong, but that may happen (if Android handles it incorrectly). I'll add a fix for this tomorrow. A notification received after the trigger was executed (but not completed) will work, but the lib will still wait for the write callback before going forward.

@philips77
Copy link
Member

Btw, if your device if flooding with notifications, than you shouldn't use the trigger. My intention was to use this for command-responses use cases, where it is the command that triggers notification that would not be sent otherwise. In your case it looks like the notification was received before the trigger was sent, so in fact the lib should ignore it and wait for a next one.

@ghost
Copy link
Author

ghost commented Nov 30, 2018

The notification flood was on a different characteristic. Does it still affect the WaitForValueChangedRequest?

It can happen that the characteristic emits a notification change that's not a direct answer to our command. For example, if the user presses a button and the device starts vibrating, the "is vibrating" sensor will emit a notification changed on the same characteristic we are trying to write on. Will that cause issues?

In our case, the response's first byte matches the command we sent. I don't know if it'd be possible to pass some kind of filter to WaitForValueChangedRequest, but I guess that belongs in a separate feature request.

Failure log, where we can see that the notification received was indeed a response to the command we sent

07:37:45.703 15611-15803/ D/KLNordicBleManager: writeAndNotifyOperation command id 0x44
07:37:45.733 15611-15611/ I/: Notification received from 02000004-..., value: (0x) 44-...
07:37:45.736 15611-15611/ I/: Data written to 02000004-..., value: (0x) 44-...

@philips77
Copy link
Member

The notification flood was on a different characteristic. Does it still affect the WaitForValueChangedRequest?

No, other characteristics are safe.

It can happen that the characteristic emits a notification change that's not a direct answer to our command. For example, if the user presses a button and the device starts vibrating, the "is vibrating" sensor will emit a notification changed on the same characteristic we are trying to write on. Will that cause issues?

The notification given as a parameter to waitForNotification(...) matters. It should get a notification after the request is complete, not before/during. But also the lib should not crash, like what you experience.

In our case, the response's first byte matches the command we sent. I don't know if it'd be possible to pass some kind of filter to WaitForValueChangedRequest, but I guess that belongs in a separate feature request.

Yes, let's leave it for the future.

If your notification was indeed a response for the request, that means that it may come even before the onCharacteristicWrite callback, so I have to fix it in the code. Expect a commit soon.

philips77 added a commit that referenced this issue Nov 30, 2018
@philips77
Copy link
Member

Hi, could you try with the latest update? The manager will now wait until a notification is received after the trigger request was at least started. When it comes before the trigger was finished, it will also wait until the trigger's request is received.

@ghost
Copy link
Author

ghost commented Nov 30, 2018

That seems to fix the issues, thank you so much!!

@philips77
Copy link
Member

I'll release 2.0.1 soon.

@philips77
Copy link
Member

Released in 2.0.1.

@philips77 philips77 removed their assignment Nov 30, 2018
@vicobz
Copy link

vicobz commented Dec 11, 2018

Hi @philips77,

Thanks for this great library!

I'm wondering if I'm facing an issue similar to what is described above. I use the library to connect to a (Nordic) device and subscribe to notifications from one specific characteristic.

To communicate in both ways, I (the Android app) receive notifications from the characteristic and when I need to send a message, I write to that characteristic and the device replies with the same exact data.

Multiple issues were fixed from 2.0.1 & 2.0.3 releases.

However, I'm still facing something weird. I sometimes receive a notification from the Nordic device and then, I can see in logs that the exact same data is automatically written to the device, without no link with my own code, meaning I'm not sending any write request for that specific message.

Unfortunately, this issue breaks the communication protocol between our Nordic device and the Android app as we rely on a specific sequence order.

Do you have any idea why this could happen?

I will try to add logs asap when facing the issue again as it is not systematically reproducible.

Thanks

@vicobz
Copy link

vicobz commented Dec 11, 2018

Here are the logs:

D:  BleManager Log - Message: Writing characteristic 00000001-XXXX-XXXX-XXXX-000000000000 (WRITE COMMAND)
D:  BleManager Log - Message: gatt.writeCharacteristic(00000001-XXXX-XXXX-XXXX-000000000000)
D:  BleManager Log - Message: Data written to 00000001-XXXX-XXXX-XXXX-000000000000, value: (0x) YY-YY-YY-YY-YY-YY-YY-YY-YY-YY-YY-YY
I:  Data sent to device CD:XX:XX:XX:XX:XX (0x) YY-YY-YY-YY-YY-YY-YY-YY-YY-YY-YY-YY
I:  onWriteDone
D:  BleManager Log - Message: Writing characteristic 00000001-XXXX-XXXX-XXXX-000000000000 (WRITE COMMAND)
D:  BleManager Log - Message: gatt.writeCharacteristic(00000001-XXXX-XXXX-XXXX-000000000000)
D:  BleManager Log - Message: Notification received from 00000001-XXXX-XXXX-XXXX-000000000000, value: (0x) AA-AA-AA-AA-AA-AA-AA-AA-AA-AA
I:  Data received from device CD:XX:XX:XX:XX:XX (0x) AA-AA-AA-AA-AA-AA-AA-AA-AA-AA
I:  Receive (0x) AA-AA-AA-AA-AA-AA-AA-AA-AA-AA
D:  BleManager Log - Message: Data written to 00000001-XXXX-XXXX-XXXX-000000000000, value: (0x) AA-AA-AA-AA-AA-AA-AA-AA-AA-AA
I:  Data sent to device CD:XX:XX:XX:XX:XX (0x) BB-BB-BB-BB-BB-BB-BB-BB-BB-BB-BB-BB-BB-BB-BB
I:  onWriteDone

All values have been changed , but the issue is visible with the AA-AA-AA-AA-AA-AA-AA-AA-AA-AA message.

I receive a notification with this value on line 8, and that same frame is automatically sent back to the device, visible on line 11, but I didn't chose to send this message from my code.

@philips77
Copy link
Member

Hi, could you please create a new issue with both your last posts? This seams to be related, but in fact, I'm afraid, isn't, and actually can't be fixed easily. I'll explain why in the other issue. Thank you!

@vicobz
Copy link

vicobz commented Dec 12, 2018

Hi @philips77, thanks for your quick reply.

Here is the issue: #60

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

No branches or pull requests

2 participants