search

arduino-libraries / ArduinoBLE

ArduinoBLE library for Arduino
GNU Lesser General Public License v2.1
311 stars 205 forks source link

ArduinoBLE crashes when BLE.begin BLE.end is called multiple times (possible solution included) #192

Closed Klaus-KK closed 2 years ago

Klaus-KK commented 3 years ago

The issue was reported by a user in the Arduino forum, who wants to switch between BLE and WiFi on a regular basis.

https://forum.arduino.cc/t/repeated-ble-begin-end-crashes-board/885009

I wrote the following sketch for testing and it crashes after 50 loops.

#include <ArduinoBLE.h>

void setup()
{
  Serial.begin( 9600 );
  while ( !Serial );
  Serial.println( "ArduinoBLE memory leak test" );
}

void loop()
{
  static uint32_t counter = 0;

  BLE.begin();
  counter++;
  Serial.print( "C: " );
  Serial.println( counter );
  BLE.end();
}

With my limited C++ knowledge I found in void GATTClass::begin() a couple of objects are created with new. They are not deleted in void GATTClass::end().

Adding the 5 deletes to GATTClass::end seems to fix the issue. Can you confirm this is the right way to fix this and implement this in the next version of the library?

void GATTClass::end()
{
  delete( _genericAccessService );
  delete( _deviceNameCharacteristic );
  delete( _appearanceCharacteristic );
  delete( _genericAttributeService );
  delete( _servicesChangedCharacteristic );
 _attributes.clear();
}
sneha-dsp commented 3 years ago

I am getting same issue as @Klaus-KK my Arduino Nano 33 BLE gets stuck in either BLE.begin() or BLE.discoverattributes() and never recovers from it unless a hard reset.

mcxiv commented 2 years ago

Hi,

Same issue for me with MKR 1010. It crashes at loop n°45. Your workaround @Klaus-KK works for me, thanks. 😊

Quentin

ricozinn commented 2 years ago

Same crash for me with the Artemis Apollo3 with MbedOS (or seems like it), but the proposed solution doesn't fix it. It crashes for me at the 26 or 27 count each time even after applying the fix to GATT.cpp. Does my crash info look like what you were seeing Klaus? Thanks!

13:23:01.306 -> C: 25
13:23:04.216 -> C: 26
13:23:05.966 -> 
13:23:05.966 -> ++ MbedOS Fault Handler ++
13:23:05.966 -> 
13:23:05.966 -> FaultType: HardFault
13:23:05.966 -> 
13:23:05.966 -> Context:
13:23:05.966 -> R0: 0
13:23:05.966 -> R1: 1644D
13:23:05.966 -> R2: 10004257
13:23:05.966 -> R3: 0
13:23:05.966 -> R4: 10005E38
13:23:05.966 -> R5: 1644D
13:23:05.966 -> R6: 45
13:23:05.966 -> R7: 10005E98
13:23:05.966 -> R8: 3E8
13:23:05.966 -> R9: 10005EA4
13:23:05.966 -> R10: 0
13:23:05.966 -> R11: 0
13:23:05.966 -> R12: 0
13:23:05.966 -> SP   : 10008360
13:23:05.966 -> LR   : 25A45
13:23:05.966 -> PC   : 0
13:23:05.966 -> xPSR : 40000000
13:23:05.966 -> PSP  : 10008340
13:23:05.966 -> MSP  : 1005FF70
13:23:05.966 -> CPUID: 410FC241
13:23:05.966 -> HFSR : 40000000
13:23:05.966 -> MMFSR: 0
13:23:05.966 -> BFSR : 0
13:23:05.966 -> UFSR : 2
13:23:05.966 -> DFSR : 0
13:23:05.966 -> AFSR : 0
13:23:05.966 -> Mode : Thread
13:23:05.966 -> Priv : Privileged
13:23:05.966 -> Stack: PSP
13:23:05.999 -> 
13:23:05.999 -> -- MbedOS Fault Handler --
13:23:05.999 -> 
13:23:05.999 -> 
13:23:05.999 -> 
13:23:05.999 -> ++ MbedOS Error Info ++
13:23:05.999 -> Error Status: 0x80FF013D Code: 317 Module: 255
13:23:05.999 -> Error Message: Fault exception
13:23:05.999 -> Location: 0x0
13:23:05.999 -> Error Value: 0x10005EA8
13:23:05.999 -> Current Thread: application_unnamed_thread Id: 0x10008458 Entry: 0x29425 StackSize: 0x1000 StackMem: 0x100073D8 SP: 0x10008360 
13:23:05.999 -> For more info, visit: https://mbed.com/s/error?error=0x80FF013D&tgt=SFE_ARTEMIS_ATP
13:23:05.999 -> -- MbedOS Error Info --
alranel commented 2 years ago

I created a pull request containing this fix: https://github.com/arduino-libraries/ArduinoBLE/pull/237

buffcode commented 2 years ago

No leak in 1.1.3, but starting with 1.2.0 most likely due to https://github.com/arduino-libraries/ArduinoBLE/commit/de392e0b96ff2210a2bf76a657ca54aca045123f

ricozinn commented 2 years ago

No leak in 1.1.3, but starting with 1.2.0 most likely due to de392e0

I just tried undoing those changes you referenced and on the Artemis chip it still crashes at the same place (after calling BLE.begin() followed by BLE.end() 26 times.

buffcode commented 2 years ago

No leak in 1.1.3, but starting with 1.2.0 most likely due to de392e0

I just tried undoing those changes you referenced and on the Artemis chip it still crashes at the same place (after calling BLE.begin() followed by BLE.end() 26 times.

@ricozinn Hm okay, did you also try to use 1.1.3 completely? I have no memory leak with that version, but maybe the referenced commit is not (fully) responsible.

ricozinn commented 2 years ago

@alranel I tried your changed in this PR: https://github.com/arduino-libraries/ArduinoBLE/pull/237 with Artemis chip and it fails (same MBedOS hard fault) after only one cycle. I think the clearAttributes() tries to release memory already released in the lines above it.

I've tried several variations of your suggested changes and for Artemis they crash after 26 cycles of BLE.begin() followed by BLE.end().

Meaning, your proposed changes have the same net effect of the hard fault in MBedOS as leaving the ::end as this:

void GATTClass::end()
{ 
  _attributes.clear();
}

I can't use that MemoryUsage library though to see what's happening to memory at each step because it isn't compatible with Artemis.

Thanks for proposing solutions for this, let me know if you have any other ideas? I'm stumped on a project trying to use ArduinoBLE currently that needs to be able to turn BLE on and off until I can either think of an alternative or find a way to get this working.

ricozinn commented 2 years ago

@buffcode, unfortunately since 1.1.3 release they added support for the Apollo3 core (support for Artemis) that I am using, so I can't go all the way back to that one.

On Fri, May 20, 2022 at 9:18 AM buffcode @.***> wrote:

No leak in 1.1.3, but starting with 1.2.0 most likely due to de392e0 https://github.com/arduino-libraries/ArduinoBLE/commit/de392e0b96ff2210a2bf76a657ca54aca045123f

I just tried undoing those changes you referenced and on the Artemis chip it still crashes at the same place (after calling BLE.begin() followed by BLE.end() 26 times.

@ricozinn https://github.com/ricozinn Hm okay, did you also try to use 1.1.3 completely? I have no memory leak with that version, but maybe the referenced commit is not (fully) responsible.

— Reply to this email directly, view it on GitHub https://github.com/arduino-libraries/ArduinoBLE/issues/192#issuecomment-1133090100, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANI7NP24B2ZXR76BYPKSFLVK63MTANCNFSM5AKW5KKQ . You are receiving this because you were mentioned.Message ID: @.***>

buffcode commented 2 years ago

@alranel I tried your changed in this PR: #237 with Artemis chip and it fails (same MBedOS hard fault) after only one cycle. I think the clearAttributes() tries to release memory already released in the lines above it.

You are right, the services are part of the attributes and as such are already released. So only the characteristics and attributes need to be released explicitly, otherwise the board will crash (in my case).

The memory leak is somewhat reduced but still there and kills the board after 20-ish runs.

ricozinn commented 2 years ago

Ok, glad you are seeing it too. Let me konw if you discover a fix. Thanks! Are you also using the Apollo3 core? Or some other board?

On Fri, May 20, 2022 at 9:33 AM buffcode @.***> wrote:

@alranel https://github.com/alranel I tried your changed in this PR:

237 https://github.com/arduino-libraries/ArduinoBLE/pull/237 with

Artemis chip and it fails (same MBedOS hard fault) after only one cycle. I think the clearAttributes() tries to release memory already released in the lines above it.

You are right, the services are part of the attributes and as such are already released. So only the characteristics and attributes need to be released explicitly, otherwise the board will crash (in my case).

The memory leak is somewhat reduced but still there and kills the board after 20-ish runs.

— Reply to this email directly, view it on GitHub https://github.com/arduino-libraries/ArduinoBLE/issues/192#issuecomment-1133102905, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANI7NMAPMKCWHX5CTOAIE3VK65GDANCNFSM5AKW5KKQ . You are receiving this because you were mentioned.Message ID: @.***>

mcxiv commented 2 years ago

@alranel I tried your changed in this PR: https://github.com/arduino-libraries/ArduinoBLE/pull/237 with Artemis chip and it fails (same MBedOS hard fault) after only one cycle. I think the clearAttributes() tries to release memory already released in the lines above it.

I've tried several variations of your suggested changes and for Artemis they crash after 26 cycles of BLE.begin() followed by BLE.end().

Meaning, your proposed changes have the same net effect of the hard fault in MBedOS as leaving the ::end as this:


void GATTClass::end()

{ 

  _attributes.clear();

}

I can't use that MemoryUsage library though to see what's happening to memory at each step because it isn't compatible with Artemis.

Thanks for proposing solutions for this, let me know if you have any other ideas? I'm stumped on a project trying to use ArduinoBLE currently that needs to be able to turn BLE on and off until I can either think of an alternative or find a way to get this working.

Had the same problem, had to use a watchdog to reset the board when this problem occurred.