nkolban / esp32-snippets

Sample ESP32 snippets and code fragments
https://leanpub.com/kolban-ESP32
Apache License 2.0
2.35k stars 710 forks source link

Need to check that disconnected BLEClient doesn't try and use connection #228

Open nkolban opened 6 years ago

nkolban commented 6 years ago

Imagine the ESP32 is running as a BLE Client. It connects to the target BLE Server and reads and writes characteristics. All is well. Now imagine that the client is "disconnected" from the BLE Server. We will be notified that we have been disconnected. All good. However, what if the programmer then attempts to continue to read/write to the BLE Server now the connection is gone? We don't handle that at all.

Today, we interact with a BLE Server by getting a BLERemoteCharacteristic object and then calling its readValue() and writeValue() methods. Since we can't get a BLERemoteCharacteristic object without first connecting to the server, we don't have to worry about attempting to interact with the server without first having formed a connection. What we failed to appreciate is that having a BLERemoteCharacteristic NOW doesn't mean that this object will be good into the future. Specifically, it is no longer good once we have lost the connection. We need to maintain state in the client such that if the connection is lost, an attempt (mistakenly) by the programmer to use the connection is rejected.

nkolban commented 6 years ago

One immediate workaround without code changes in the library appears to be to invoke the:

BLEClient::isConnected()

and only if we are connected, then perform the read or write.

We also need to add an onDisconnect() method to the BLEClientCallbacks class.

skyng22003 commented 6 years ago

I see, thank you very much for your help!

nkolban commented 6 years ago

Studying the traces when a server disappears from a connected ESP32 BLE Client, I seem to see two events. These are ESP_GATTC_DISCONNECT_EVT and ESP_GATTC_CLOSE_EVT. Now I need to study to determine the distinction between these.

nkolban commented 6 years ago

Some non trivial changes have now been made.

reaper7 commented 6 years ago

I tried to use this callback on esp32 arduino but I got:

C:\PROGRAMY\arduino\hardware\espressif\esp32\libraries\BLE\src/BLEExceptions.h:13:2: error: #error "C++ exception handling must be enabled within make menuconfig. See Compiler Options > Enable C++ Exceptions."

 #error "C++ exception handling must be enabled within make menuconfig. See Compiler Options > Enable C++ Exceptions."
chegewara commented 6 years ago

You have to edit sdkconfig.h file and set this option: CXX_EXCEPTIONS=y

reaper7 commented 6 years ago

ok :) thank you for the tips. This change will be introduced in arduino-esp? Is anyone working on it? I use daily sync with master repo and I do not want to change files locally.

chegewara commented 6 years ago

This is option you have to change locally, sorry but we cant change it

nkolban commented 6 years ago

Action: Work with @me-no-dev to switch on Exception handling in Arduino-ESP32 libs ... or determine why that is a bad idea.

me-no-dev commented 6 years ago

Let me have a look... I think there was a reason why I did not enable them.

me-no-dev commented 6 years ago

ok :) will have it enabled in the next Arduino update ;)

nkolban commented 6 years ago

@reaper7 based on mr @me-no-dev response (thank you sir), we now have a dependency chain on an update to the Arduino-ESP32 before a new version of the BLE C++ classes can be formally packaged with the Arduino-ESP32 libs. However, as Mr @chegewara says, in the interim, we can manually switch on that flag in the Arduino sdkconfig.h file. I'm hoping that will be "sufficient" to enable exception handling in Arduino-ESP32 land ... I can't think of any obvious reason that would be a problem.

In your previous post, you said:

I use daily sync with master repo and I do not want to change files locally.

Can you clarify, which projects are you syncing? I see possibilities including:

me-no-dev commented 6 years ago

@nkolban the only way to get that working with Arduino before I release the next build is to actually use it as IDF component. Changing that in the prebuilt version will not change anything.

nkolban commented 6 years ago

@me-no-dev .. as you know I miss obvious things and apologies for that. Can you help me understand?

My thinking is that in current ESP-IDF with current toolchains, C++ exception handling was enabled. However, there is a modest RAM cost for enabling exception handling and since that is only applicable to C++ programs, Espressif decided to make the "availability" of C++ exception handling optional and off by default. To "switch it on", one would normally run "make menuconfig" and enable the setting. The "low-level" result of that the sdkconfig.h file is modified to create a new definition which appears to be:

CXX_EXCEPTIONS=1

In the latest BLE C++ classes, we added usage of exceptions and added a guard to validate that the exception support was enabled by checking that the value was set and, if not, aborting compilation with a human readable message asking the user to switch it on.

My understanding is that Arduino-ESP32 also leverages the ESP-IDF and toolchain. However, make menuconfig isn't present/available/desirable but the sdkconfig.h file still exists ... and as set by the Arduino-ESP32 project in development. To enable C++ exception handling in Arduino-ESP32 before a formal fix/change, would it not be sufficient for a user to edit the Arduino-ESP32 supplied sdkconfig.h file and add the entry that would enable exception processing?

chegewara commented 6 years ago

I didnt check that particular option/flag how it will behave if changed in sdkconfig.h, i just assumed it will works if changing options about stack size in WiFi stack and mbedtls or bluetooth makes visible changes in compiled application.

me-no-dev commented 6 years ago

Well here is the thing :) All IDF components are precompiled in Arduino with the sdkconfig that is attached. Changing values to sdkconfig could only influence code that is not compiled. All code that is already compiled and in binary form will not change. Definitions work only at compile, not at link time :)

nkolban commented 6 years ago

@me-no-dev Aha ... gotcha sir. The light bulb went on for me (only 25W in my case though). That makes perfect sense. We will wait for your nod that in a future release of the Arduino-ESP32 libs that flag has been turned on at compilation time when you build the ESP-IDF for distribution. However, I noted your excellent suggestion about using the Arduino-ESP32 as an ESP-IDF component. My assumption there is that in that pattern, we don't leverage the pre-supplied ESP-IDF compiled libraries but instead leverage the ESP-IDF that is in the environment of the project for the user?

me-no-dev commented 6 years ago

Yup exactly :) it's like the tools/sdk folder of Arduino does not even exist and you are compiling the whole IDF based on the project's sdkconfig

chegewara commented 6 years ago

I know it is possible but without tutorial i wouldnt know how to do that and this is something i would like to know. There is few libraries its available only on arduino ide and its pain to refactor it to idf

nkolban commented 6 years ago

@chegewara Contact me on evenings/weekends via IRC and I'll walk you through it. Study the notes in my book ... I should have written it up and, if not, that would be an ideal topic.

reaper7 commented 6 years ago

@nkolban

Can you clarify, which projects are you syncing? I see possibilities including:

I sync (pull) almost all stared by me (on github) libs for arduino (esp32, esp8266, others) and of course cores for esp32 and esp8266 and Your cpp_utils (ble part) too. but no ESP-IDF :)

chegewara commented 6 years ago

Do we have some progress here? Is c++ exceptions compiled yet? @nkolban @me-no-dev

me-no-dev commented 6 years ago

since idf 3.1 pre was tagged over the weekend, I can whip out a release in the next couple of days

newkit commented 6 years ago

Whats the status? I think c++ exceptions are not in yet, correct?

nkolban commented 6 years ago

To the best of my knowledge, that is correct. We still await the nod from mr @me-no-dev . If we look at the files found here:

https://github.com/espressif/arduino-esp32/tree/master/tools/sdk/lib

We see the precompiled ESP-IDF that is supplied with the Arduino-ESP32 libs (as best as I understand it). At the time of writing, these appear to be over a month old and (to me) indicate that we haven't yet achieved a release with C++ exceptions compiled in.

newkit commented 6 years ago

Does there a tutorial exist how to enable CXX_EXCEPTIONS=1 for arduino IDE?

chegewara commented 6 years ago

No, there is no tutorial because you would have to build whole development environment and compile esp-idf libraries to arduino-ide. As for now you can use this library (i will update it today) where i commented out all exceptions usage, but its only for testing purpose to test new ble features not to build production app. https://github.com/nkolban/esp32-snippets/pull/315