arduino-libraries / ArduinoBLE

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

BLE.begin() -> BLE.end() -> BLE.begin() [again] #243

Closed ASabovic closed 2 years ago

ASabovic commented 2 years ago

Hi guys,

I am wondering if it is possible to turn off BLE when I do not need to send some data, and turn it on again when data is ready to be sent? I am trying to enable that part, but every time when BLE.end() function is called seems that I am not able to start the BLE again after some sleep time. In my case, I want to send the simple message from Arduino to my PC, and it works when BLE is constantly ON, but I want to turn it off in order to keep my device in a sleep state, and in that case, decrease the current consumption. Is this implementation possible with the Arduino BLE library?

P.S. In my case, the BLE service, characteristics, advertising,... are defined in the setup function. Thanks in advance. Cheers! :)

rmlearney-digicatapult commented 2 years ago

https://github.com/arduino-libraries/ArduinoBLE/issues/65 covers what you're describing.

Implementing this without a power cycle needs changes to the underlying CordioHCIHook

ASabovic commented 2 years ago

I have already checked this thread and also checked if src/utility/HCICordioTransport.cpp is properly changed in my case and it's same as in #92 but still I am only able to perform 2 cycles and the code breaks.

ASabovic commented 2 years ago

Is there something related to my board as I am using the Arduino Nano 33 BLE Sense based on the ublox chip? I literally changed everything based on this thread but still after only 2 cycles the device breaks and I am not able to continue executing my code. Please help as this is the most important part of my project.

baconcheese113 commented 2 years ago

Facing the same issue with Nano 33 IoT, a blank sketch with just BLE.begin() in setup increases power consumption from 22mA to 85mA so being able to turn it off is very important.

I'm trying to turn it on every 10 seconds to do a 1 second scan by calling BLE.begin() and BLE.end() a second later. This seems to work, however when setting back up a service BLE.addService() it crashes

ASabovic commented 2 years ago

In my case, I want to call BLE.begin() when data transfer is needed and after that part is finished call BLE.end() in order to put back my device into sleep state. I am not planning to use BLE very often so this feature is a great opportunity for me to keep BLE "inactive" when I do not need to use it. I tried the solution from these two threads #65 and #92 by changing HCICordioTransport.cpp file with parts that they suggested and confirmed that it works but in my case the code crashes after two cycles and it's not able to back up service and other configuration parts of BLE.

baconcheese113 commented 2 years ago

@ASabovic My issue is described here #246 If you just call BLE.begin() and BLE.end() on repeat without adding services, does it work for you indefinitely? Because that works for me, I just can't add event handlers or services after having called BLE.end()

ASabovic commented 2 years ago

I have the same situation, if I just call BLE.begin() and BLE.end() the code runs indefenitely, but if I add anything else after two cycles it will crash. I am able to send the message two times and put the BLE OFF, but after that it just stops and the code crashes. Maybe you can also try this solution proposed in the above mentioned threads and let me know if that method works in your case.

baconcheese113 commented 2 years ago

I'm using the latest ArduinoBLE version: 1.3.1 which does include @facchinm 's PR, however the Nano 33 IoT doesn't use HCICordioTransport (ARDUINO_ARCH_MBED is undefined) so no changes to that file would impact my issue.

When trying the BLE.end()/BLE.begin() method, my program crashes trying to add the service. Specifically on this line.

In this loop from GATT.cpp, characteristic->descriptorCount() and descriptor->_refCount return random high numbers right before the crash on the retain() line. Seems like they're trying to read from a corrupted memory address.

for (unsigned int j = 0; j < characteristic->descriptorCount(); j++) {
  BLELocalDescriptor* descriptor = characteristic->descriptor(j);

  descriptor->retain();
  _attributes.add(descriptor);
  descriptor->setHandle(attributeCount());
}
ASabovic commented 2 years ago

Here I am not sure what can cause an issue. Is it possible to change the memory address or to define the specific one where all parameters will be saved? Based on #64 in Nano 33 BLE family the BLE functionality is inside the chip, so there's no need for that extra steps. Furthermore, it doesn't use an UART based transport but Cordio's one. Now, I am bit confused as you said that in your case to change HCICordioTransport will not change anything...I am using the Arduino Nano 33 BLE Sense so maybe in my case I also do not have anything from these changes? Now, I am able to reach 3 cycles but then when my device is connected it crashes and that's the end of the execution. @facchinm @rmlearney-digicatapult any suggestions how to fix this issue?

rmlearney-digicatapult commented 2 years ago

I'm really sorry but I'm just a hobbyist when it comes to Arduino.

One of the Arduino team like @facchinm would be more appropriate.

ASabovic commented 2 years ago

But in #92 you @rmlearney-digicatapult confirmed that the code works for you when you tried to begin and end BLE in cycles. What did you change to make it able to work without breaks? Do you maybe have the code saved somewhere? I think that here you tested the same board that I am using right now (Arduino Nano 33 BLE Sense).

rmlearney-digicatapult commented 2 years ago

Ah. I only did it the once during testing and then realised that cycling the bluetooth connection wasn't a necessary feature of my application so have just left it on ever since. Sorry :(

Try the sketch in #65 on a Nano 33 BLE (not the Sense version) - it might cycle more than once?

ASabovic commented 2 years ago

Aha okay, I understand. Maybe then @facchinm can give as the final answer is this feasible on Nano 33 BLE Sense or not. I chose Sense version as I saw that the ML part is feasible on it. Does Nano 33 BLE (without Sense) support the machine learning part (specifically in my case the person detection model)?

ASabovic commented 2 years ago

I just checked this part should work without issues on the Arduino Nano 33 BLE Sense as the only difference between these two is number of sensors (https://create.arduino.cc/projecthub/thatiotguy/arduino-nano-33-ble-v-s-33-ble-sense-v-s-33-iot-07d2bf). It's weird that it does not work if it worked before in your case. Can you maybe just send me your HCI Cordio Transport file just to check if everything matches?

rmlearney-digicatapult commented 2 years ago

I've just been continuously updating my libraries from Arduino, and haven't used that demo code from #65 for about 2 years now.

I'm afraid that I've since packed away my Ardunio stuff from my desk moved onto different things. Sorry I can't help.

baconcheese113 commented 2 years ago

@ASabovic see if this works for you

ASabovic commented 2 years ago

@baconcheese113 I am trying like this but it does not work. NINA_RESETN is always out of scope.

#include <ArduinoBLE.h>
unsigned long prevNow = 0;

const char* uuidOfBatteryLevelChar = "2d2F88c4-f244-5a80-21f1-ee0224e80658";
const char* uuidOfService = "180F";
const char* nameofPeripheral = "BatteryMonitor";

bool wasConnected = false;
uint32_t genericTimer = 0;
BLEService batteryService(uuidOfService);

BLEUnsignedCharCharacteristic batteryLevelChar(uuidOfBatteryLevelChar, BLEIndicate); 

void setup() {
  Serial.begin(9600);
  while (!Serial);
  pinMode(NINA_RESETN, OUTPUT);
  initBLE();
}

void blePeripheralConnectHandler(BLEDevice central) {
  Serial.println("Connected event, central: ");
  Serial.println(central.address());
}

void blePeripheralDisconnectHandler(BLEDevice central) {
  Serial.println("Disconnected event, central: ");
  Serial.println(central.address());
}

void loop() {

  BLE.poll();
  long now = millis();
  if (now - prevNow >= 5000) {
    prevNow = now;
    if (BLE.connected()) {
      Serial.println("BLE device is connected!");
      int rssiValue = BLE.rssi();
      Serial.println(rssiValue);
      Serial.println("Connected to central");
      updateBatteryLevel();
      digitalWrite(NINA_RESETN, LOW)
      //BLE.disconnect();
      //BLE.end();
      wasConnected=true;
    }

    else {
      Serial.println("Not connected to central");
    }
  }

  if(wasConnected){
    Serial.println("Central is not connected and must be connected again!");
    //long now2 = millis();
    genericTimer = millis();
    while(millis() - genericTimer <= 10000){} // Wait 15 seconds after disconnect
    wasConnected = false;
    Serial.println("I have to initialize BLE again!");
    //initBLE();
    digitalWrite(NINA_RESETN, HIGH);
    delay(750);
  }
}

void initBLE(void){
  if (BLE.begin()){
    Serial.println("BLE will begin now!");
  }
  BLE.setLocalName(nameofPeripheral);
  BLE.setAdvertisedService(batteryService);
  batteryService.addCharacteristic(batteryLevelChar); // add the battery level characteristic
  BLE.addService(batteryService);
  BLE.setEventHandler(BLEConnected, blePeripheralConnectHandler);
  BLE.setEventHandler(BLEDisconnected, blePeripheralDisconnectHandler);
  BLE.advertise();

}

void updateBatteryLevel() {

  int prediction = 1;
  sprintf(prediction_status, "%d", prediction);

  //Serial.println("Waiting to send data!");
  batteryLevelChar.writeValue(prediction_status[0]); 

  Serial.println("Data is sent!");
}

Can you maybe suggest me where the mistake is (or how to implement it using your approach)?

baconcheese113 commented 2 years ago

You'd just call disconnect the regular way

BLE.disconnect();

And when everything is disconnected, you've stopped scanning/advertising, and you're ready to turn off your BLE module call

digitalWrite(NINA_RESETN, LOW);

And when the 15 seconds is up you'd turn back on the module

while (millis() - genericTimer <= 10000) {} // Wait 15 seconds after disconnect
digitalWrite(NINA_RESETN, HIGH);
delay(750);
HCI.setEventMask(0x3FFFFFFFFFFFFFFF);
wasConnected = false;

NINA_RESETN is defined in the the variant.h file for my Nano 33 IoT, you'll need to figure out what it's defined as for your board.

ASabovic commented 2 years ago

Hi @baconcheese113, I edited the code above. You added the complete library where NINA_RESETN is defined or just header file? I am not able to find a way how to define it for my board. Maybe you can help me?

baconcheese113 commented 2 years ago

After looking into your Nano 33 BLE board a bit more I noticed that the BLE functionality is integrated into the microcontroller inside the NINA-B306, as opposed to boards like the 33 IoT where the microcontroller is separate from the NINA module.

That might make it simpler in your case just using the LowPower.idle() or LowPower.sleep() functions since sleeping the microcontroller would also take care of the BLE for your board.

Also, after looking at your code, is your goal actually to turn off BLE to save power? Or just to stop scanning/advertising for 15 seconds? Are you sure the BLE functionality is drawing significant power?

ASabovic commented 2 years ago

Yes, in the meantime I checked, and everything related to the BLE is integrated inside the NINA B306 module. Based on what @rmlearney-digicatapult wrote above, I also looked for the Arduino Nano 33 BLE (classic board without SENSE) but based on the datasheet they are the same (SENSE and without SENSE) so maybe there is no sense to order it and test BLE begin/end implementation with HCI transport code.

Are you sure that I can use LowPowerIdle() and sleep() functions in order to control my MCU? I think that I tried that already but the Arduino IDE was not able to recognize the specific library for my device. Which library do I need to consider in case I want to use these functions?

This is just the test code, for my real project application, I will use BLE to send images and simple messages (1-byte char) to the Cloud so I will not use the BLE so often. That is the main reason why I am trying to enable this begin/end functionality in order to keep BLE off when I do not need to use it. Based on my measurements, when the behind jumper is removed (hardware changes in order to decrease the current), my device is consuming around 3mA and I want to keep it in the sleep state where I am able to reach around 300microAmpere. So, basically, I just want to use BLE when I need to send the data and then turn it off and go to a sleep state or even execute another task in the flow, which will not be related to BLE.

The code I provided above is just for testing the BLE begin/end functionality, and other ways how to enable BLE to be off during the time of my experiments when I am not planning to use it. I hope you can understand the concept that I want to reach and maybe provide some answers based on this "little story" :)

rmlearney-digicatapult commented 2 years ago

Have you considered an alternative technology to BLE for your use case such as LoRa?

ASabovic commented 2 years ago

Yes, I thought about LoRa but I do not think it’s possible to send a large amount of data such as image using this technology. So, in the end, I decided to take a try with BLE.

per1234 commented 2 years ago

Hi all. Thanks so much for submitting this issue and for the valuable information that has been shared here.

I will request that we keep this thread focused tightly on any deficiencies or defects in the ArduinoBLE library code base related to the original report:

when BLE.end() function is called seems that I am not able to start the BLE again after some sleep time.

General discussion and project assistance will be more appropriate on the Arduino Forum:

https://forum.arduino.cc/

ASabovic commented 2 years ago

@per1234 yes but I think that is related to the BLE library as it controls this part when we want to play with BLE and enable some things related to it. For the rest I understand but BLE begin/end issue is directly related to this library and this is the 2 years old issue that is never properly solved.

ASabovic commented 2 years ago

@baconcheese113 @rmlearney-digicatapult Hi guys, me again after some time, during my holidays I ordered Nano 33 BLE (without Sense part) and tested it, but again only two successful cycles are possible after which the device is stuck again. Do you maybe know if there were some changes in the Arduino BLE library that removed this feature? Should I also try with some older versions of same library? Thanks in advance :)

ASabovic commented 2 years ago

@facchinm based on your solution before 2 years I think, I also tried to test this feature, but it does not work as I expected. I am able to run 2 cycles after which my code breaks, the device connects to my computer for the third time, but after that, I am not able to do anything. The code stops, the data is not sent, and the only what I can do at that moment is reset the device. Do you maybe have the solution for this problem? I thought that your changes were not merged into this new release of the BLE library, but the guys told me that they have been already implemented. With changes I mean the thread #92 :)

Here is my code:

#include <ArduinoBLE.h>
unsigned long prevNow = 0;

const char* uuidOfBatteryLevelChar = "2d2F88c4-f244-5a80-21f1-ee0224e80658";
const char* uuidOfService = "180F";
const char* nameofPeripheral = "BatteryMonitor";

bool wasConnected = false;
uint32_t genericTimer = 0;

BLEService batteryService(uuidOfService);

BLEUnsignedCharCharacteristic batteryLevelChar(uuidOfBatteryLevelChar, BLEIndicate); 

char prediction_status[] = "";

void setup() {
  Serial.begin(9600);
  while (!Serial);
  initBLE();
}

void blePeripheralConnectHandler(BLEDevice central) {
  Serial.println("Connected event, central: ");
  Serial.println(central.address());
}

void blePeripheralDisconnectHandler(BLEDevice central) {
  Serial.println("Disconnected event, central: ");
  Serial.println(central.address());
}

void loop() {

  BLE.poll();
  long now = millis();
  if (now - prevNow >= 5000) {
    prevNow = now;
    if (BLE.connected()) {
      Serial.println("BLE device is connected!");
      int rssiValue = BLE.rssi();
      Serial.println(rssiValue);
      Serial.println("Connected to central");
      int prediction = 1;
      sprintf(prediction_status, "%d", prediction);
      batteryLevelChar.writeValue(prediction_status[0]);
      BLE.disconnect();
      BLE.end();
      wasConnected=true;
    }

    else {
      Serial.println("Not connected to central");
    }
  }

  if(wasConnected){
    Serial.println("Central is not connected and must be connected again!");
    genericTimer = millis();
    while(millis() - genericTimer <= 10000){} 
    wasConnected = false;
    Serial.println("I have to initialize BLE again!");
    initBLE();
  }
}

void initBLE(void){
  if (BLE.begin()){
    Serial.println("BLE will begin now!");
  }
  BLE.setLocalName(nameofPeripheral);
  Serial.println("First step is done!");
  BLE.setAdvertisedService(batteryService);
  Serial.println("Second step is done!");
  batteryService.addCharacteristic(batteryLevelChar); // add the battery level characteristic
  Serial.println("Third step is done!");
  BLE.addService(batteryService);
  Serial.println("Fourth step is done!");
  BLE.setEventHandler(BLEConnected, blePeripheralConnectHandler);
  Serial.println("Fifth step is done!");
  BLE.setEventHandler(BLEDisconnected, blePeripheralDisconnectHandler);
  Serial.println("Sixth step is done!");
  Serial.println("Before seventh step and immediately after sixth step!");
  Serial.println("Seventh step is done!");
  BLE.advertise();
}

And this is the output:

BLE will begin now!
First step is done!
Second step is done!
Third step is done!
Fourth step is done!
Fifth step is done!
Sixth step is done!
Before seventh step and immediately after sixth step!
Seventh step is done!
Not connected to central
Connected event, central: 
3c:6a:a7:42:cc:0e
BLE device is connected!
-37
Connected to central
Central is not connected and must be connected again!
I have to initialize BLE again!
BLE will begin now!
First step is done!
Second step is done!
Third step is done!
Fourth step is done!
Fifth step is done!
Sixth step is done!
Before seventh step and immediately after sixth step!
Seventh step is done!
Not connected to central
Connected event, central: 
3c:6a:a7:42:cc:0e
BLE device is connected!
-41
Connected to central
Central is not connected and must be connected again!
I have to initialize BLE again!
BLE will begin now!
First step is done!
Second step is done!
Third step is done!
Fourth step is done!
Fifth step is done!
Sixth step is done!
Before seventh step and immediately after sixth step!
Seventh step is done!
Not connected to central
Connected event, central: 
3c:6a:a7:42:cc:0e
dominsch commented 2 years ago

Just tried your code directly on esp32. It crashed after the second connect as well and it reported a higher memory usage. Needs further investigation.

dominsch commented 2 years ago

Your code is a mess but here is a quick fix:

void initBLE(void){
  BLEService batteryService(uuidOfService); // <-- move declaration here to fix memory leak
  if (BLE.begin()){
    Serial.println("BLE will begin now!");
  }
  BLE.setLocalName(nameofPeripheral);
  BLE.addService(batteryService); // <-- add Service before advertising to fix crash
  BLE.setAdvertisedService(batteryService);
  batteryService.addCharacteristic(batteryLevelChar);
  BLE.setEventHandler(BLEConnected, blePeripheralConnectHandler);
  BLE.setEventHandler(BLEDisconnected, blePeripheralDisconnectHandler);
  BLE.advertise();
}

No issues with ArduinoBLE as far as I can tell

ASabovic commented 2 years ago

Yes, I agree, it is a complete mess, but I was just using it for testing, and many commented things should be removed. Sorry anyway! I just tried your implementation with my board, but it does not work. I mean, the board is able to execute multiple cycles, but the data never arrived at the receiver, which is the computer in my case. On the computer side, access is constantly denied if things are moved in places you did in the code above. I went through #92 and #65 again and made some changes in the BLE library (HCICordioTransport.cpp and GATT.cpp files), after which it started to work without issues. Now, I am able to execute an indefinite number of begin->end cycles with the data received on the computer side.

Have you also tested your code with the received node as well or just run it on your board to see if it will be able to execute more cycles without breaking?

facchinm commented 2 years ago

Hi @ASabovic @dominsch @baconcheese113 @rmlearney-digicatapult I just pushed https://github.com/arduino-libraries/ArduinoBLE/pull/260 to try and fix the end() -> begin() -> end() memory corruption; after testing in various scenarios it look like the leak is gone and there's no need to instantiate the BLEService inside a function (but it still works and is supported)

If you could test with your sketches and report the results it would be very useful, thanks!

dominsch commented 2 years ago

Works on esp32 and nRF connect for android. Might be a good time for another release.

ASabovic commented 2 years ago

@facchinm I confirm it works with the Arduino Nano 33 BLE and Bleak (library that enables BLE on PC) without issues.

facchinm commented 2 years ago

Super, thanks @dominsch and @ASabovic for testing, merging and releasing