Release 1.4.1: Pull request #470: reconnect of BLE client fails

[MicroMidi]

Thanks for providing us with such a powerful and multi-functional library @h2zero! I'm using your library in combination with the BLE-MIDI-library of @lathoub (https://github.com/lathoub/Arduino-BLE-MIDI)
Since release 1.4.1 I noticed re-connection problems with my BLE-MIDI client code on an ESP32 platform (https://github.com/MicroMidi/VOX-Adio-Air-MIDI-Footswitch) - and I was able to break it down to the changes of pull request #470 that causes this problem. To be honest - I have no clue what the background of this change is, but the connected BLE-server that I use (a Vox Adio Air guitar amp) seems to have problems with that change.
Is there any chance to figure out what the reason for that behaviour could be? Could it be a problem of the BLE implementation of the Vox amp? Another BLE-MIDI effect device (NUX Mighty Plug) works flawlessly with that change - and uses almost the same code base.
Thanks in advance for any hint ...

[h2zero]

I suspect this line: https://github.com/lathoub/Arduino-BLE-MIDI/blob/929c2fc04962672ddba903a618601bea44ee1f3e/src/hardware/BLEMIDI_Client_ESP32.h#L527

Adding a third parameter, true, to that call should resolve your issue.

[MicroMidi]

Thanks for your turbo fast reply and the proposed solution. This does the trick - thanks so much for your impressive analysis of the problem👌.
Now I would like to contact the author of the BLEMIDI client code to incorporate this change in his code. May I ask for a brief explanation of the effect of this change - or is it obvious for someone that develops the client code based on your library?
Thanks again and kind regards from Germany ...

[h2zero]

You're welcome 😄

The explanation is that in 1.4.0, when subscribing to notifications/indications it was forcing a write with response on the descriptor, which was done because it's mandatory in the BLE specifications. This was reverted to let the app decide in 1.4.1 because in some situations (Tasmota and probably others) this caused issues with some devices. Your issue was the opposite, in that not writing with response caused a problem so this change manually forces that action.

[MicroMidi]

Thanks again for your detailed explanations - now I have a touch of an idea about the background of this issue😊. Looking forward to work with your brilliant library in my tiny experimental projects😉...

[lathoub]

Thanks for the thourough investigation @MicroMidi and response @h2zero.

I would suggest to add a parameter to the client connect

bool BLEMIDI_Client_ESP32::connect(bool response = true)

so to provide the 3rd parameter to the subscribe function:

                    if (_characteristic->subscribe(notifications, std::bind(&BLEMIDI_Client_ESP32::notifyCB, this, _1, _2, _3, _4), response ))

This would not break backward compatibility (defaulting to the 1.4.0 true default), yet provide an easy way to alter the value to false. Or is it better for response to default to false?

Alternative, provide access to both notifications and response parameters in the client connect call:

bool BLEMIDI_Client_ESP32::connect(bool notifications = true, bool response = true)

Also tagging @RobertoHE

[MicroMidi]

Is this line also relevant for adjustments?:
https://github.com/lathoub/Arduino-BLE-MIDI/blob/929c2fc04962672ddba903a618601bea44ee1f3e/src/hardware/BLEMIDI_Client_ESP32.h#L601

Obviously, my preferred default value for the response behaviour is true as this surprisingly solved all my reconnection problems with my Vox MIDI server device when I updated the NimBLE library to version 1.4.0😉. Now I know why ...👌

Thanks again @h2zero and @lathoub for supporting me in this issue🙏

[RobertoHE]

Thanks for the thourough investigation @MicroMidi and response @h2zero.

I would suggest to add a parameter to the client connect

bool BLEMIDI_Client_ESP32::connect(bool response = true)

so to provide the 3rd parameter to the subscribe function:

                    if (_characteristic->subscribe(notifications, std::bind(&BLEMIDI_Client_ESP32::notifyCB, this, _1, _2, _3, _4), response ))

This would not break backward compatibility (defaulting to the 1.4.0 true default), yet provide an easy way to alter the value to false. Or is it better for response to default to false?

Alternative, provide access to both notifications and response parameters in the client connect call:

bool BLEMIDI_Client_ESP32::connect(bool notifications = true, bool response = true)

Also tagging @RobertoHE

That may work fine and it is an easy way to do that.

Another way may be using CustomSetting Branch (when the pull request will get accepted and merged to the main branch) and adding in the configuration default struct the Response and Notification parameter (initialized to true by default) that may be overridden in each project at the definition of the BLE-Client Object.

[h2zero]

I would suggest using a default value of true, which will work for 99.9% of cases and is what the BLE spec mandates.

This option was actually planned to be removed but now reconsidered because of situational issues.

[RobertoHE]

I would suggest using a default value of true, which will work for 99.9% of cases and is what the BLE spec mandates.

This option was actually planned to be removed but now reconsidered because of situational issues.

As you like it.
Do you prefer to set it like a fixed value or maybe a configurable setting with default value = true?
Do you extend it to notification? Or do those two settings have different natures and must be set differently?

[h2zero]

Do you prefer to set it like a fixed value or maybe a configurable setting with default value = true?

Doesn't hurt to add an option if you'd like to.

Do you extend it to notification? Or do those two settings have different natures and must be set differently?

I would say that you could test if the characteristic has notifications or indications available and set this to prefer notifications when available and indications otherwise.

[RobertoHE]

CustomSetting Branch updated.
https://github.com/RobertoHE/Arduino-BLE-MIDI/blob/CustomSettings/src/hardware/BLEMIDI_Client_ESP32.h
This branch is developed in my fork before merging it with the master branch of @lathoub. Use it for testing. Any comment is welcome.

@h2zero Could you check the changes in subscribe method?
The main changes are the following:

struct Settings {
static const bool notification = true;
static const bool response = true;
}

if (_characteristic->subscribe(_Settings::notification, std::bind(&BLEMIDI_Client_ESP32::notifyCB, this, _1, _2, _3, _4), _Settings::response))

I can´t test it (unfortunately, I haven't any ESP32). @MicroMidi and @ketan, could you test it?
You can use the example of the Client setup of this branch. Default parameters are set to true, in this case, it is not necessary to modify the Settings struct, but you can override any change using a derivated struct like the code example of the branch.

Thanks for your help

[h2zero]

Looks good to me, left a comment in the commit.

[MicroMidi]

I can´t test it (unfortunately, I haven't any ESP32). @MicroMidi and @ketan, could you test it? You can use the example of the Client setup of this branch. Default parameters are set to true, in this case, it is not necessary to modify the Settings struct, but you can override any change using a derivated struct like the code example of the branch.

Thanks for your help

@RobertoHE: Thanks for all your time and effort - this looks very promising. And in fact - the tests with my ESP32 Wrover Test Kit and the very sensitive "Vox Adio Air" BLE-MIDI server went fine: Everything runs stable and reliable and the BLE reconnection works without any problems after switching the Vox device off and then on again. This was the scenario that caused problems when the response message on the established BLE-connection was missing. Great work👌!