Skip to content

Breaking change to ESP-IDF Bluetooth Support in pre 3.0 ESP-IDF master #82

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

Closed
nkolban opened this issue Sep 23, 2017 · 14 comments
Closed
Assignees

Comments

@nkolban
Copy link
Owner

nkolban commented Sep 23, 2017

As of today (2017-09-23) it has been found that the ESP-IDF master for pre-release ESP-IDF 3.0 has dramatically changed the API interface to BLE APIs. This has resulted in the BLE C++ classes no longer working on this release. The ESP-IDF v2.1 release continues to work but BLE C++ APIs will now no-longer work with the pre-3.0 ESP-IDF master.

See the following commit log for the changes made in ESP-IDF:

espressif/esp-idf@1759a47

This is a dramatic change to the APIs and will now cause us to pause to consider how to progress. If we change the BLE C++ implementation to match the pre-release 3.0, then it will not work with the generally available 2.1. If we leave the BLE C++ code base alone, it will not work with current master and will eventually need to be changed in the future.

See also this forum thread:

https://esp32.com/viewtopic.php?f=13&t=3116

@ciotlosm
Copy link

What is the best approach for someone trying to build a BLE app using Arduino IDE to overcome the compile error?

@vijfhoek
Copy link

You could consider leaving in both versions, and using preprocessor conditionals to choose between the two.

@nkolban
Copy link
Owner Author

nkolban commented Sep 23, 2017

Update: I have read through the new APIs provided by the latest ESP-IDF. They have some dramatic changes. My approach to this puzzle is as follows:

  1. Read the docs on the new APIs.
  2. Write up the new stories in my book of notes on the ESP32.
  3. Re-work the BLE C++ APIs to leverage the new ESP-IDF BLE APIs.
  4. Release for testing to all once I get clean compiles and have re-run the battery of tests I have at my disposal.

We have come to a cross-roads ... the code will still work with ESP-IDF v2.1 but the Arduino-ESP32 package today distributes the latest ESP-IDF (based on Master). So I have the following choices:

  1. Maintain two code branches ... one for ESP-IDF v2.1 and one for ESP-IDF Git master
  2. Do nothing and wait for ESP-IDF v3.0. Code will only work on ESP-IDF V2.1 and will no longer work with current Arduino.
  3. Rework the BLE C++ classes so that they leverage the ESP-IDF Git master. The BLE code will no longer work with ESP-IDF v2.1.

I have opted to go with option (3) but this will take time to get right.

I'll keep everyone appraised of status. I have a "hope" that we should be working again by mid next week.

@ciotlosm
Copy link

@nkolban Nice going. I recommend you make a tag and add that in your master branch documentation for people still searching something that still works with ESP-IDF V2.1.

@nkolban
Copy link
Owner Author

nkolban commented Sep 23, 2017

Progress continues and I have my mind around the new design and am getting clean compiles. Unfortunately, we have hit a new snag. Apparently the commits to master were bigger than just changes to BLE ... but we also saw the arrival of a whole new toolchain for compilation plus the support of pthreads and C++ exception handling (yeah!!!). Unfortunately, the combination of all these code merges has resulted in a new problem that I have written up here:

espressif/esp-idf#1032

It now appears that when we try and call some inconspicuous C++ classes that have nothing to do with BLE the call's just hang. Needless to say the BLE C++ classes make good use of C++ classes and if the underlying C++ class handling isn't working as hoped/desired/expected ... we can't make much progress. Hopefully the issue referenced in this post on ESP-IDF will be resolved in a timely fashion. It makes no sense at all to try and find code circumventions to using the standard C++ classes that are currently blocking.

nkolban added a commit that referenced this issue Sep 25, 2017
@nkolban
Copy link
Owner Author

nkolban commented Sep 25, 2017

Status report: End of day: 2017-09-24
New source files have been committed to the project that now allow for a clean build. Sniff testing seems to show that we are now back where we were before the ESP-IDF changes. I have not yet tested with Arduino and there is not yet an Arduino BLE C++ ZIP distribution. However compiling and building with ESP-IDF BLE C++ applications seems to work.

The changes only affected building ESP32 BLE C++ clients and not ESP32 BLE C++ servers so if you had been building servers, you should be good to go now. For clients, everything is in place with the exception of registering for notifications which I need to test further. In other good news, since the BLE C++ classes provided an abstraction over the lower level ESP-IDF and it was the ESP-IDF that changed its APIs and signatures, the rework has resulted in no changes to the exposed BLE C++ APIs. This means that any apps you had written using the BLE C++ API should just re-compile and run just fine.

The ESP-IDF compilation of C++ applications with the new toolchain still remains broken (see last post) however it seems that if we use the older toolchain (the one just before this weeks refresh) C++ still works just fine. As such, consider NOT upgrading to the latest toolchain just yet until we hear more about the problems being encountered there.

This issue is far from closed ... a ton more testing on my part still needs to be done ... but the good news is that we should be in a position to compile cleanly and I'll be paying close attention to problems reported that say that something that should work isn't.

Running with ESP-IDF 2.1 is now no longer a possibility with the latest Git master of the this code. In order to run this code, you currently must use the latest Git master of ESP-IDF.

@FulminatingGhost
Copy link
Contributor

Everything seems to work again for me! Thanks, you're great!

@AmrutaCh
Copy link

@nkolban Thanks for fixing the issues. I just wanted to ask if you have tested it with Arduino. I tried BLE_server.ino. It's compiling but I am not getting the expected output. It seems it is stuck at void BLEServer::registerApp() probably waiting for the mutex.

@nkolban
Copy link
Owner Author

nkolban commented Sep 25, 2017

@AmrutaCh Sorry sir ... haven't yet tested with Arduino. Fortunately (?) I have a job and must work 9-5 Monday to Friday ... so work will be performed in the evenings and weekends ... so it will be slower going now. My immediate work plan is:

  • BLERemoteDescriptor implementation and testing (that was omitted at first and needs fixed now)
  • BLEClient inbound notifications
  • Arduino

However I do hear you on BLEServer::registerApp() and that will be first up this evening to see where we are and if I can reproduce.

@nkolban
Copy link
Owner Author

nkolban commented Sep 26, 2017

Well phooey ... progress continues on the reworked BLE C++ classes to accommodate the ESP-IDF changes. We also made the discovery that the latest "toolchain" is failing with functions like std::stringstream (standard C++ libraries). Fortunately I found that the previous to current toolchain could still be used with impunity. Tonight I tested with Arduino and, unfortunately, it too has switched over to the latest toolchain.

To see the current toolchain, run

xtensa-esp32-elf-gcc -v

The toolchain just shipped has the id:

crosstool-NG crosstool-ng-1.22.0-73-ge28a011

while the older one (that continues to work) reports as:

crosstool-NG crosstool-ng-1.22.0-61-gab8375a

There were some large changes made in the toolchain including additions of multi-threading and exception handling and it is possible/likely that one of those has caught us. Unfortunately, there isn't much I can offer just now other than trying to come up with a recipe to "back-out" the Arduino toolchain to a back-level ... but by the time we have gone to those extremes, the benefits of "low-maintenance" Arduino are lost and you might as well be working in ESP-IDF. We have a defect open against ESP-IDF for them to examine the toolchain ... so let's keep our eyes on that and see what happens there.

@nkolban
Copy link
Owner Author

nkolban commented Sep 26, 2017

@AmrutaCh ... Unfortunately the "stall" issue that you are experiencing is due to the latest toolchain that is supplied by ESP-IDF (and is now also the one used in Arduino-ESP32 distributions) apparently having a problem ... see the links above for links to the ESP-IDF issue.

@coder543
Copy link

coder543 commented Oct 3, 2017

using the old toolchain the latest ESP-IDF master, I'm able to get Sample1 working... when the log level is Debug or Verbose. When the log level is Info, the custom characteristic doesn't actually appear through the nRF Connect app, but the device is there and named correctly.

For obvious reasons, I'm having trouble debugging this, since the issue appears to be timing dependent, and the log statements slow the code down just enough to avoid the issue.

Do you have any thoughts? Or should I open a new issue?

@nkolban
Copy link
Owner Author

nkolban commented Oct 3, 2017

Howdy @coder543 If you can, lets go ahead and create a new issue to deal with that problem exclusively. That way I can keep the story/issue contained to its own specific concept. If it turns out to be the same problems as described here, we can logically merge the two, however, the problem with the latest ESP-IDF master doesn't exhibit as timing issues.

You might want to look at this issue:

espressif/esp-idf#857

It might be close to your story ... but we won't know until we go deeper.

@nkolban
Copy link
Owner Author

nkolban commented Oct 14, 2017

The great news is that the Arduino ESP32 project has been updated. Please re-build your base Arduino ESP32 environment. The even better news is that the owner of that project has decided to include the BLE C++ classes as part of the distribution. So it is now baked in.

@nkolban nkolban closed this as completed Oct 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants