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

BLE-MIDI Client: Problems when reconnecting to a MIDI server #40

Closed
MicroMidi opened this issue Dec 13, 2021 · 22 comments
Closed

BLE-MIDI Client: Problems when reconnecting to a MIDI server #40

MicroMidi opened this issue Dec 13, 2021 · 22 comments

Comments

@MicroMidi
Copy link

MicroMidi commented Dec 13, 2021

First of all I'd like to thank so much for your brilliant library - it works (almost) flawlessly as a base for a MIDI footswitch for my guitar effect devices.
The “Nux Mighty Plug” works without any problems with your library – it uses standard MIDI-CC messages for effect switching. But a strange "phenomena" occurs with my VOX Adio amp. This amp only works with MIDI SysEx messages - and needs a delay of 400ms after the Bluetooth connection is established and before the first MIDI message could be sent, probably for internal initialization purposes. I implemented this delay in the “OnConnect”-callback routine. Everything works fine when the amp is already switched on and the ESP32 client with your library gets started. When I switch off the amp the BLE-connection gets lost and the client starts scanning for other BLE-MIDI devices. When I turn on the amp again the client reconnects, but the amp doesn't respond to MIDI messages. Everything works fine again when I reset the ESP32-client and the connection gets re-established from scratch.
I recognized that your BLE-Client code displays a "Connected to"-message in combination with the name and MAC-address of the MIDI-Server in an intitial connection scenario. In a reconnect-scenario only the "Connected to"-message appears in serial monitor. Is a re-connect scenario treated different from an initial connection in your library and is the something special to consider? I’m quite sure that the amp in general is a very sensitive MIDI server and your library is not the cause for the re- connection problems … but probably you have a hint for me to solve my problem - thanks in advance!

@lathoub
Copy link
Owner

lathoub commented Dec 15, 2021

Hi @MicroMidi thanks for the question.
Quick question before deep diving into the issue: do you get a disconnect event?

@MicroMidi
Copy link
Author

Hi @lathoub, thanks for taking the time and effort to help me out on my issue!
My code ist mainly based on your BLE-MIDI client example. I uploaded it to my GitHub project page - if you want to have a look at it.

This is an extract of the log-messages on the serial port:
**__Advertised Device found: Name: ES3-249F, Address: e4:b3:89:86:ab:f8, serviceUUID: 6acc5540-e631-4069-944d-b8ca7598ad50
Advertised Device found: Name: Adio Air GT MIDI, Address: 44:91:60:00:8c:35, serviceUUID: 03b80e5a-ede8-4b33-a751-6ce34ec4c700
Found MIDI Service
---------CONNECTED---------
Connected to: Adio Air GT MIDI / 44:91:60:00:8c:35
First run
millis(): 4006
t0: 3605
SYSEX: (10 bytes) F0 42 30 0 1 41 42 0 0 F7
MIDI message received!
SYSEX: (8 bytes) F0 42 30 0 1 41 23 F7
MIDI message received!
---------NOT CONNECTED---------
Scanning...
Advertised Device found: Name: , Address: 5f:6a:6b:af:fb:13, manufacturer data: 4c000c0e00f5d4b1e0fa48b072658c99724f1006461e2c8deee1
Advertised Device found: Name: Mi Smart Band 4, Address: e7:ec:4c:58:4e:a3, manufacturer data: 570102ffffffffffffffffffffffffffffffff01e7ec4c584ea3, serviceUUID: 0xfee0
Advertised Device found: Name: F8B3C549AA0F33A3E9, Address: e7:e7:f6:16:5a:e0, appearance: 0, manufacturer data: a705051001000000000000021f00ca, serviceUUID: 0xfe07, txPower:
Advertised Device found: Name: , Address: 5f:6a:6b:af:fb:13, manufacturer data: 4c000c0e00f6d4f236b6b1f52585778d64931006461e2c8deee1
Advertised Device found: Name: Adio Air GT MIDI, Address: 44:91:60:00:8c:35, serviceUUID: 03b80e5a-ede8-4b33-a751-6ce34ec4c700
Found MIDI Service
---------CONNECTED---------
First run
millis(): 117406
t0: 117005

The first connection shows that the Adio amp is connected and it replys with SysEx-messages to MIDI requests when the loop() is entered for the first time after the bluetooth connection is established. When the amp is switched off and on the second connection doesn't log the device that is connected with the client. To me it looks that some client information is cached. If I switch external MIDI-devices then the BLE-MIDI client shows device information also in a re-connect scenario.

I hope that these information snippets are helpful for your investigations - thanks in advance for your great support!

Bernd

@lathoub
Copy link
Owner

lathoub commented Dec 17, 2021

I'm going to bring in @RobertoHE who wrote the client part of the library, maybe he sees what is happening

@lathoub
Copy link
Owner

lathoub commented Dec 17, 2021

Can you alter:

void onDisconnect(BLEClient *pClient)
{
//Serial.print(pClient->getPeerAddress().toString().c_str());
//Serial.println(" Disconnected - Starting scan");
if (_bluetoothEsp32)
{
_bluetoothEsp32->disconnected();
}
//Try reconnection or search a new one
NimBLEDevice::getScan()->start(1, scanEndedCB);
}

with

    void onDisconnect(BLEClient *pClient)
    {
        if (_bluetoothEsp32)
        {
            _bluetoothEsp32->disconnected();
            // Try reconnection or search a new one
            _bluetoothEsp32->scan();
        }
    }

I can't test it on my side, but worth trying

@MicroMidi
Copy link
Author

MicroMidi commented Dec 19, 2021

Thanks again for your support @lathoub! I tried your suggestion - but I didn't solve my problem. Nevertheless the onDisconnect() method was a good hint to try out some other approaches myself ... and what finally did the trick for me was to delete and create the NimBLEDevice-client. Now it looks like the client is re-instantiated from scratch after a re-connect and everything works perfectly now.

This is the current code I use with the added "Renew Client" code snippet:

void onDisconnect(BLEClient *pClient)
{
if (_bluetoothEsp32)
{
_bluetoothEsp32->disconnected();
// Try reconnection or search a new one
_bluetoothEsp32->scan();
}

// Renew Client
NimBLEDevice::deleteClient(pClient);
NimBLEDevice::createClient();

//Try reconnection or search a new one
NimBLEDevice::getScan()->start(1, scanEndedCB);
}

I guess that this is not a very smart approach because other MIDI devices may not need this kind of "hard reset" while reconneting. But for me it works without any problems.

Thanks again for your great support and kind regards
Bernd

@lathoub
Copy link
Owner

lathoub commented Dec 20, 2021

Great - happy you were to able to fix it.
Please create a PR, so i can review the changes and add it to the code-base

@MicroMidi
Copy link
Author

I'm quite new to GitHub - so I had to google for the term "PR" - it seems to be a "Pull Request" where I fork your code into my own repository and adopt my changes in that environment, right? I will try to make this happen, cross your fingers for me;-)

Thanks and greets, Bernd

@MicroMidi
Copy link
Author

... I created a PR with the modified code. Can you please have a look if everything was prepared as expected?

@lathoub
Copy link
Owner

lathoub commented Dec 20, 2021

I'm quite new to GitHub - so I had to google for the term "PR" - it seems to be a "Pull Request" where I fork your code into my own repository and adopt my changes in that environment, right? I will try to make this happen, cross your fingers for me;-)

:-) learn something new every day 👍

@lathoub
Copy link
Owner

lathoub commented Dec 20, 2021

The PR has not yet shown up in this repo. I did see it in your repo, now bring it over to this repo

@RobertoHE
Copy link
Contributor

Hi @MicroMidi and @lathoub

Be carefull with this change (it may be a correct solution, of course):

This is the current code I use with the added "Renew Client" code snippet:

void onDisconnect(BLEClient *pClient) { if (_bluetoothEsp32) { _bluetoothEsp32->disconnected(); // Try reconnection or search a new one _bluetoothEsp32->scan(); }

// Renew Client NimBLEDevice::deleteClient(pClient); NimBLEDevice::createClient();

//Try reconnection or search a new one NimBLEDevice::getScan()->start(1, scanEndedCB); }

I guess that this is not a very smart approach because other MIDI devices may not need this kind of "hard reset" while reconneting. But for me it works without any problems.

It affects to the next part of the code:

/** Check if we have a client we should reuse first
* Special case when we already know this device
* This saves considerable time and power.
*/
if (_client)
{
if (_client == NimBLEDevice::getClientByPeerAddress(myAdvCB.advDevice.getAddress()))
{
if (_client->connect(&myAdvCB.advDevice, false))
{
if (_characteristic->canNotify())
{
if (_characteristic->subscribe(true, std::bind(&BLEMIDI_Client_ESP32::notifyCB, this, _1, _2, _3, _4)))
{
//Re-connection SUCCESS
return true;
}
}
/** Disconnect if subscribe failed */
_client->disconnect();
}
/* If any connection problem exits, delete previous client and try again in the next attemp as new client*/
NimBLEDevice::deleteClient(_client);
_client = nullptr;
return false;
}
/*If client does not match, delete previous client and create a new one*/
NimBLEDevice::deleteClient(_client);
_client = nullptr;
}

These lines of code try to reconnect the device using the previous info of the device. This method is recommended by nimBLE lib because (theoretically) saves time and power because skips send some initial config messages.

Your solution may be valid too, but it may cause some little delays after a reconnection because need a little bit more time than a reconnection. They may be inappreciable but it is a thing to consider.

If the solution that propose @MicroMidi is accepted for a merge, maybe the lines that I put before may be removed from the code because the client never can perform a reconnection because the client will removed after a disconnection,

@RobertoHE
Copy link
Contributor

And it may be recommended add _client = nullptr; too after NimBLEDevice::deleteClient(pClient); NimBLEDevice::createClient();

@MicroMidi
Copy link
Author

MicroMidi commented Dec 21, 2021

@RobertoHE: Thanks for the clarification ... now I understand another concept of your BLE MIDI Client code.

To me it seems now that my proposed code change enforces that the BLE connection will always be built up from scratch - and this makes the part of the code, that you mentioned in your previous post, obvious. I guess that 99% of all MIDI servers can deal with the reconnection approach without any problems - so we should definitely keep this functionality and discard my code change.

To provide some flexibility for the library users in terms of reconnect behaviour a possible approach could be to declare a preprocessing option (“#define BLEMIDI_CLIENT_DISABLERECONNET”) in the header file of the BLE MIDI client and let the library user decide if the functionality should be activated or not (with default active!). This will have no implications for all current library user and provides an option for users that observe problems with the reconnection functionality. Do you think that this could be a practicable solution?

Thanks Bernd

@RobertoHE
Copy link
Contributor

Hi @MicroMidi .

Let me some days for implemets the changes.

@MicroMidi
Copy link
Author

Thanks @RobertoHE , no need to hurry - please take all the time you need!

RobertoHE pushed a commit to RobertoHE/Arduino-BLE-MIDI that referenced this issue Dec 23, 2021
Add functionalities for lathoub#40 issue.
@RobertoHE
Copy link
Contributor

@MicroMidi
Check the proposed changes in this fork https://github.com/RobertoHE/Arduino-BLE-MIDI before that I will do a pull request to official repo.

@MicroMidi
Copy link
Author

@RobertoHE
Thanks for your time and effort! When I use your updated library then the bluetooth scan stops after disconnecting the bluetooth server. The scan runs exactly once after the disconnect happens and then the scan stops. I haven't found out why - but it happens identically with two different amps that I tested as bluetooth server...

@RobertoHE
Copy link
Contributor

Hi again @MicroMidi
Sorry for the delay.

I added your changes to code.

Would you check the repo again? Uncomment this line before compile https://github.com/RobertoHE/Arduino-BLE-MIDI/blob/ebe18daaea8b1cea0826bbeae893402d12b73f2c/src/hardware/BLEMIDI_Client_ESP32.h#L132

If it works, I will pull request the changes to main repo.

@MicroMidi
Copy link
Author

MicroMidi commented Jan 19, 2022 via email

@RobertoHE
Copy link
Contributor

I have proposed a pull request, @lathoub.
Would you check and merge it to the main repo?
Thanks.

@lathoub
Copy link
Owner

lathoub commented Jan 19, 2022

@RobertoHE thank you for the work, looks like it works - yeah! Checked and merged.
(thank you @MicroMidi for raising the issue and testing)

With this done, I will create a new release (and make available in the Arduino library)

@lathoub lathoub closed this as completed Jan 19, 2022
@MicroMidi
Copy link
Author

@lathoub @RobertoHE : This is pretty perfect - thanks for incorporating my request in an official release – it works really great and reliable now!

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

No branches or pull requests

3 participants