Crash instancing BLEServer

[copercini]

In BLE server for Arduino, creating a BLEServer like this BLEServer *pServer = new BLEServer(); everything works fine, but instancing BLEServer pServer; it compiles, create the server but crash in the client connection:

Guru Meditation Error of type LoadProhibited occurred on core  0. Exception was unhandled.
Register dump:
PC      : 0x400d4879  PS      : 0x00060d30  A0      : 0x800d4049  A1      : 0x3ffdccb0  
A2      : 0x3ffdb0d0  A3      : 0x0000000e  A4      : 0x00000004  A5      : 0x3ffdcd30  
A6      : 0x00000000  A7      : 0x00060023  A8      : 0x00060023  A9      : 0x3ffe1510  
A10     : 0x00000003  A11     : 0x00060d23  A12     : 0x00060d23  A13     : 0x00000000  
A14     : 0x00000000  A15     : 0x0000000c  SAR     : 0x00000018  EXCCAUSE: 0x0000001c  
EXCVADDR: 0x00000028  LBEG    : 0x4000c349  LEND    : 0x4000c36b  LCOUNT  : 0xffffffff  

Backtrace: 0x400d4879:0x3ffdccb0 0x400d4046:0x3ffdccd0 0x400d2a9d:0x3ffdcd10 0x400f203d:0x3ffdcd30 0x400edf60:0x3ffdcd70

CPU halted.
Decoding 8 results
0x400d4879: BLEServiceMap::handleGATTServerEvent(esp_gatts_cb_event_t, unsigned char, esp_ble_gatts_cb_param_t*) at C:\Users\Note\Documents\Arduino\hardware\esp32\esp32\libraries\BLE\src/BLEServiceMap.cpp line 92
0x400d4879: BLEServiceMap::handleGATTServerEvent(esp_gatts_cb_event_t, unsigned char, esp_ble_gatts_cb_param_t*) at C:\Users\Note\Documents\Arduino\hardware\esp32\esp32\libraries\BLE\src/BLEServiceMap.cpp line 92
0x400d4046: BLEServer::handleGATTServerEvent(esp_gatts_cb_event_t, unsigned char, esp_ble_gatts_cb_param_t*) at C:\Users\Note\Documents\Arduino\hardware\esp32\esp32\libraries\BLE\src/BLEServer.cpp line 115
0x400d2a9d: BLE::gattServerEventHandler(esp_gatts_cb_event_t, unsigned char, esp_ble_gatts_cb_param_t*) at C:\Users\Note\Documents\Arduino\hardware\esp32\esp32\libraries\BLE\src/BLE.cpp line 218
0x400f203d: btc_gatts_cb_to_app at /Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/bt/bluedroid/btc/profile/std/gatt/btc_gatts.c line 53
:  (inlined by) btc_gatts_cb_handler at /Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/bt/bluedroid/btc/profile/std/gatt/btc_gatts.c line 854
0x400edf60: btc_task at /Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/bt/bluedroid/btc/core/btc_task.c line 85

[nkolban]

Can you pastebin the complete failing sketch?

[copercini]

https://pastebin.com/81wX9zg4

[nkolban]

I studied the code for a while and then a big smile came over my face. If we look in your code ... it reads (loosely as follows)

setup() {
   BLEServer pServer;
   // do stuff with pServer
   // Start advertising ...
}

The way to read this is "We enter the function called setup and create an instance of the BLEServer object. We then do some other stuff and then we complete the setup function.

Now, in C or C++ when we end a function, any local variables created in that function are deleted. In this code fragment, there is a local variable called pServer and hence, when the setup() function comes to an end, C++ says "Ok, time to clean up any variables locally created in this function scope" ... which includes pServer. What that means is that all the "state" data relating to your BLEServer has been discarded. However we didn't shutdown the BLE server nor did we stop advertising .... what this means is that when the client does connect, a callback function is attempted to a callback routine that now no longer exists ... and we break.

We need to change the nature of this Github issue to:

When we have created a BLEServer instance, when we delete that BLEServer, make sure that any and all initialization that may have been done by it is un-done. This will include (but not be limited to) stopping the BLE server that this BLEServer models and stopping any advertising of this server and preventing any clients from attaching to this server ... simply put, if the object that represents the server goes out of scope or is otherwise deleted, we must also delete the BLE server within ESP-IDF.

[copercini]

Oh I didn't realize that, nice find!

One more thing: have to call ble.initServer("MyESP32"); before it, prevents of create BLEServer as a global thing

What do you think about initServer inside BLEServer creation if still not exist yet?

Or there is some situation which people can initServer but doesn't create BLEServer?

[nkolban]

@copercini
We are treading on "new territory" here. The BLE project currently has a "big hole" in it that will need addressed that hasn't been accommodated yet ... and that is the notion of hosting multiple servers and multiple clients. We understand that a BLE Server instance exposes a set of services and a set of services own a set of characteristics. All good. Same with a BLE Client. However, it appears from my loose study that an ESP32 can host multiple "concurrent" servers and clients. So what that means that we "could" have multiple BLE Servers and multiple BLE Clients. For example, imagine someone develops a library that creates a BLE Server that exposes multiple services etc etc ... and we want to register that BLE Server in addition to a BLE Server of our own. This means that there has to be a "container" for BLE Server instances and BLE Client instances. Let us call that container a "BLE Device". Currently, I have modeled that as the "BLE" class but we might rename it to be BLEDevice. A BLE device has one time initialization required to be performed before it can have BLE Servers associated with it. Since an ESP32 is a "BLE Device" there is no concept of having anything other than a "singleton" BLE Device ...

And that's about as far as I have taken it/thought it through. Since a BLE Device is a singleton, there should be nothing to prevent us from performing "initServer" when a BLEServer() is created ... but the reason I have separated it is that the name of the BLE device is a singleton and needs to be set before we create the BLEServer(). We don't want to pass the BLE device name through to BLEServer as we can have multiple servers but only one BLE device name. This will require us both to think about this a bit more.

[shcreasey]

@nkolban, If I may interject,
I think what @copercini is saying, is that wouldn't it make sense to put the server init as a server object method, rather than a method of the top level BLE object?

I think the problem may be that the code in this method currently is a mixture of device initialization and server initialization?

Forgive me if im speaking out of turn, but I had thought something similar to copercini myself.

Regards,
Steve.

[nkolban]

@shcreasey
Howdy Steve ... please, we are all friends here and all thoughts are equal in their consideration ... keep em coming.

How does the following "feel":

1. The BLE class exposes a Initialize(char *deviceName) method.
2. This call must precede any constructors of either BLEServer or BLEClient

Lets think of this notion ... and see if we can see any pros or cons with (1) and (2) here as compared to what we already have or what might else be possible. And again, all comments welcomed. If you say something that makes no sense (and I'm not expecting that), then that is an opportunity for the rest of us to explain better.

[shcreasey]

@nkolban
Hey Neil, Yes that looks good.
I think your idea to rename BLE to BLE_Device would aid clarity.
And then have the BLE_Device.Initialize() method take the device name parameter as you suggest.

---

You could (as microsoft do in .net sometimes) have your singleton BLE_Device then have .createServer() and .createClient() methods which then return those objects? Thereby circumventing the need to pass references back to the BLE_Device after server or client instantiation.

---

As for your 'new territory' comments:
I'm surprised to hear you say it's possible to create multiple servers on a device.
My understanding, and this could of course be incorrect, is that we have either a 'central' device, such as a smartphone etc. or a 'peripheral' device such as a mouse or keyboard for example.

I was under the impression that:

a 'peripheral' could only have a single server with multiple services, and connect with only one central device.

And a 'central' could connect to one or multiple different peripherals.

Although having said that, I do seem to recall that either the peripheral or the central can be client or server (or both?) so if, as I said above, the central is connected to multiple peripherals AND the central is running a server, perhaps it would need to expose a server for each connected peripheral? (or perhaps not?) i'm not sure about that. I think maybe a bit of spec. digging is going to be required for that.

For the time being though, I'd be tempted to limit it to a single server per device, unless someone requires otherwise.

---

I think advertising should be a function of a (peripheral) 'device' rather than a 'server' and perhaps we could have an advertisingData or 'advertisement' object that we could populate and pass in to the device, before calling advertise. or even pass it into the advertise method.?

Does any of that make sense?

Regards,
Steve.

[nkolban]

Howdy Steve,
Great feedback ... we'll tackle them one at a time ... first up the rename on BLE to BLEDevice() and then stock take after that. Starting the task now ... will report back when done and see where we are.

[nkolban]

Okly dokly ... 1st part done. The BLE class has been renamed to BLEDevice() and separate intialization functions called BLEDevice::initClient() and BLEDevice::initServer() have been consolidated into BLEDevice::init(std::string deviceName).

[nkolban]

Steve,
Lets assume I wanted to have the following capability:

BLEServer* BLEDevice::createServer()

The implementation of createServer could be as simple as:

BLEServer* BLEDevice::createServer() {
   return new BLEServer();
}

Do you have any thoughts on how to prevent a user from constructing an instance of BLEServer directly?

[DKlay]

@nkolban
The constructor of BLEServer should be private and in the same BLEServer class define the BLEDevice as a friend class.

[nkolban]

Thank you sir. New issue created to track and resolve.

[copercini]

I think it was solved, thx @nkolban