Closed copercini closed 7 years ago
Can you pastebin the complete failing sketch?
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.
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?
@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.
@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.
@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":
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.
@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.
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.
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).
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?
@nkolban The constructor of BLEServer should be private and in the same BLEServer class define the BLEDevice as a friend class.
Thank you sir. New issue created to track and resolve.
I think it was solved, thx @nkolban
In BLE server for Arduino, creating a BLEServer like this
BLEServer *pServer = new BLEServer();
everything works fine, but instancingBLEServer pServer;
it compiles, create the server but crash in the client connection: