h2zero / NimBLE-Arduino

A fork of the NimBLE library structured for compilation with Arduino, for use with ESP32, nRF5x.
https://h2zero.github.io/NimBLE-Arduino/
Apache License 2.0
672 stars 138 forks source link

Advertising.removeServiceUUID or Server.removeService() doesn't #541

Closed sdetweil closed 1 year ago

sdetweil commented 1 year ago

i call to delete a service from the advertising list, but it doesn't get removed. pServer->stopAdvertising(); to remove pAdvertising->removeServiceUUID(activeService[!index]->getUUID()); pServer->removeService(activeService[!index], true); (or false, doesn't matter)

I see in the code

/**
 * @brief Add a service uuid to exposed list of services.
 * @param [in] serviceUUID The UUID of the service to expose.
 */
void NimBLEAdvertising::removeServiceUUID(const NimBLEUUID &serviceUUID) {
    for(auto it = m_serviceUUIDs.begin(); it != m_serviceUUIDs.end(); ++it) {
        if((*it) == serviceUUID) {
            m_serviceUUIDs.erase(it);
            break;
        }
    }
    m_advDataSet = false;
} // addServiceUUID

but the Arduino IDE vector doesn't have an erase method. I see remove you don't ship a vector file.

I think I am advertising one service, but both are sent

07:06:57.566 -> starting advertising
07:06:57.566 -> D NimBLEAdvertising: >> Advertising start: customAdvData: 0, customScanResponseData: 0
07:06:57.566 -> primary service
07:06:57.566 ->            uuid 0x1800
07:06:57.566 ->          handle 1
07:06:57.601 ->      end_handle 5
07:06:57.601 -> characteristic
07:06:57.601 ->            uuid 0x2a00
07:06:57.601 ->      def_handle 2
07:06:57.601 ->      val_handle 3
07:06:57.601 ->    min_key_size 0
07:06:57.601 ->           flags [READ]
07:06:57.601 -> characteristic
07:06:57.601 ->            uuid 0x2a01
07:06:57.601 ->      def_handle 4
07:06:57.601 ->      val_handle 5
07:06:57.601 ->    min_key_size 0
07:06:57.601 ->           flags [READ]
07:06:57.601 -> primary service
07:06:57.601 ->            uuid 0x1801
07:06:57.601 ->          handle 6
07:06:57.601 ->      end_handle 9
07:06:57.601 -> characteristic
07:06:57.601 ->            uuid 0x2a05
07:06:57.639 ->      def_handle 7
07:06:57.639 ->      val_handle 8
07:06:57.639 ->    min_key_size 0
07:06:57.639 ->           flags [INDICATE]
07:06:57.639 -> ccc descriptor
07:06:57.639 ->            uuid 0x2902
07:06:57.639 ->          handle 9
07:06:57.639 ->    min_key_size 0
07:06:57.639 ->           flags [READ|WRITE]
07:06:57.639 -> primary service
07:06:57.639 ->            uuid 22020011-27b9-42f0-82aa-2e951747bbf9
07:06:57.639 ->          handle 10
07:06:57.639 ->      end_handle 21
07:06:57.639 -> characteristic
07:06:57.639 ->            uuid 0x9a61
07:06:57.639 ->      def_handle 11
07:06:57.639 ->      val_handle 12
07:06:57.639 ->    min_key_size 0
07:06:57.639 ->           flags [WRITE]
07:06:57.639 -> characteristic
07:06:57.672 ->            uuid 0x9a62
07:06:57.672 ->      def_handle 13
07:06:57.672 ->      val_handle 14
07:06:57.672 ->    min_key_size 0
07:06:57.672 ->           flags [WRITE]
07:06:57.672 -> characteristic
07:06:57.672 ->            uuid 0x9aff
07:06:57.672 ->      def_handle 15
07:06:57.672 ->      val_handle 16
07:06:57.672 ->    min_key_size 0
07:06:57.672 ->           flags [READ|WRITE]
07:06:57.672 -> characteristic
07:06:57.672 ->            uuid 0x2a05
07:06:57.672 ->      def_handle 17
07:06:57.672 ->      val_handle 18
07:06:57.672 ->    min_key_size 0
07:06:57.672 ->           flags [READ|NOTIFY]
07:06:57.672 -> ccc descriptor
07:06:57.672 ->            uuid 0x2902
07:06:57.707 ->          handle 19
07:06:57.707 ->    min_key_size 0
07:06:57.707 ->           flags [READ|WRITE]
07:06:57.707 -> characteristic
07:06:57.707 ->            uuid 0x9a63
07:06:57.707 ->      def_handle 20
07:06:57.707 ->      val_handle 21
07:06:57.707 ->    min_key_size 0
07:06:57.707 ->           flags [READ]
07:06:57.707 -> primary service
07:06:57.707 ->            uuid 22020001-27b9-42f0-82aa-2e951747bbf9
07:06:57.707 ->          handle 22
07:06:57.707 ->      end_handle 33
07:06:57.707 -> characteristic
07:06:57.707 ->            uuid 0x9a61
07:06:57.707 ->      def_handle 23
07:06:57.707 ->      val_handle 24
07:06:57.740 ->    min_key_size 0
07:06:57.740 ->           flags [WRITE]
07:06:57.740 -> characteristic
07:06:57.740 ->            uuid 0x9a62
07:06:57.740 ->      def_handle 25
07:06:57.740 ->      val_handle 26
07:06:57.740 ->    min_key_size 0
07:06:57.740 ->           flags [WRITE]
07:06:57.740 -> characteristic
07:06:57.740 ->            uuid 0x9aff
07:06:57.740 ->      def_handle 27
07:06:57.740 ->      val_handle 28
07:06:57.740 ->    min_key_size 0
07:06:57.740 ->           flags [READ|WRITE]
07:06:57.740 -> characteristic
07:06:57.740 ->            uuid 0x2a05
07:06:57.740 ->      def_handle 29
07:06:57.740 ->      val_handle 30
07:06:57.740 ->    min_key_size 0
07:06:57.777 ->           flags [READ|NOTIFY]
07:06:57.777 -> ccc descriptor
07:06:57.777 ->            uuid 0x2902
07:06:57.777 ->          handle 31
07:06:57.777 ->    min_key_size 0
07:06:57.777 ->           flags [READ|WRITE]
07:06:57.777 -> characteristic
07:06:57.777 ->            uuid 0x9a63
07:06:57.777 ->      def_handle 32
07:06:57.777 ->      val_handle 33
07:06:57.777 ->    min_key_size 0
07:06:57.777 ->           flags [READ]
07:06:57.777 -> D NimBLEAdvertising: << Advertising start
07:06:57.777 -> after starting advertising uuid=22020001-27b9-42f0-82aa-2e951747bbf9 <-... my call to advertise

my code does

#define NUM_SERVICES 2
BLEService* activeService[NUM_SERVICES] = {null, null};
  for(int index=0; index<NUM_SERVICES; index++{
 String ServiceUUID = makeuuid(index);
 activeService[index]=      new BLEService(ServiceUUID->c_str());
  // create characteristics

  activeService[index]->start();    // there is no stop

  if(index==0){            
      if (pAdvertising == null) {
        pAdvertising = BLEDevice::getAdvertising();
        pAdvertising->setScanResponse(true);
      }
//  only add to the server/advertising for the 1st one at startup 
      pServer->addService(activeService[index]);
      pAdvertising->addServiceUUID(activeService[index]->getUUID());
  }

both services are added.. if I remove the service-> start() then I get an abort crash in NimBLEServer.start()

    NIMBLE_LOGI(LOG_TAG, "Service changed characterisic handle: %d", m_svcChgChrHdl);
*/
    // Get the assigned service handles and build a vector of characteristics
    // with Notify / Indicate capabilities for event handling
    for(auto &svc : m_svcVec) {
        if(svc->m_removed == 0) {
            rc = ble_gatts_find_svc(&svc->getUUID().getNative()->u, &svc->m_handle);
            if(rc != 0) {
                abort();  <----  here
            }
        }

stack trace

     0x40083b9d:  panic_abort  at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/esp_system/panic.c: line 408'
     0x40092c15:  esp_system_abort  at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/esp_system/esp_system.c: line 137'
     0x400985ed:  abort  at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/newlib/abort.c: line 46'
     0x400d5aa8:  NimBLEServer::start()  at /Users/sam/Documents/Arduino/libraries/NimBLE-Arduino/src/NimBLEServer.cpp: line 218'
     0x400d3bed:  NimBLEAdvertising::start(unsigned int, void (*)(NimBLEAdvertising*))  at /Users/sam/Documents/Arduino/libraries/NimBLE-Arduino/src/NimBLEAdvertising.cpp:406\n (inlined by) NimBLEAdvertising::start(unsigned int, void (*)(NimBLEAdvertising*)) at /Users/sam/Documents/Arduino/libraries/NimBLE-Arduino/src/NimBLEAdvertising.cpp: line 392'
     0x400d591c:  NimBLEServer::startAdvertising()  at /Users/sam/Documents/Arduino/libraries/NimBLE-Arduino/src/NimBLEServer.cpp: line 778'
     0x400d217a:  startAdvertising()  at /Users/sam/Documents/Arduino/second_ble_esp32.ino/second_ble_esp32.ino.ino: line 594'
     0x400d23f0:  loop()  at /Users/sam/Documents/Arduino/second_ble_esp32.ino/second_ble_esp32.ino.ino: line 1086'
     0x400e8f89:  loopTask(void*)  at /Users/sam/Library/Arduino15/packages/esp32/hardware/esp32/2.0.8/cores/esp32/main.cpp: line 50

so , it seems service->start() does something that is anti server/advertising add/removeService

in my case I can't have both services advertised concurrently, they are exclusive/toggle.

sdetweil commented 1 year ago

in Non-Nimble, i have to destroy the whole stack, all the way to server, and rebuild it, to change I need Nimble for OTA update function

sdetweil commented 1 year ago

I see the remove code sets the flag delete or hide, but I don't find anything that uses those flags..

well, if you remove it at the server, advertising doesn't know , but even if u remove it both places, its still advertised

sdetweil commented 1 year ago

any advice?

h2zero commented 1 year ago

Hello, I'm not sure where you are removing the service in your code but if you are simply activating one service at a time I think your loop could be changed to accommodate this like so:

for(int index=0; index<NUM_SERVICES; index++ ) {
    String ServiceUUID = makeuuid(index);
    activeService[index]= new BLEService(ServiceUUID.c_str());
    // create characteristics
    //activeService[index]->start();    // << Moved this to inside the if block

    if(index==0){
      if (pAdvertising == nullptr) {
        pAdvertising = BLEDevice::getAdvertising();
        pAdvertising->setScanResponse(true);
      }
      //  only add to the server/advertising for the 1st one at startup
      activeService[index]->start(); // start service moved here
      pServer->addService(activeService[index]);
      pAdvertising->addServiceUUID(activeService[index]->getUUID());
    }
  }

The abort gets called if you have added a service to the server without first starting it, I will look to handle this more gracefully in upcoming releases.

sdetweil commented 1 year ago

thanks. I am trying to advertise one service at a time. they could all be running.

advertising removeServiceUUID() doesnt seem to remove the the prevvious added..

h2zero commented 1 year ago

With #544 merged I have tested with the following code that both the GATT service and the GAP service are removed as expected.

#include <NimBLEDevice.h>

#define NUM_SERVICES 2
BLEService* activeService[NUM_SERVICES] = {nullptr, nullptr};
static NimBLEServer *pServer = nullptr;
static NimBLEAdvertising *pAdvertising = nullptr;

String makeuuid (int index) {
  if (index == 0) {
    return "1234";
  }
  return "5678";
}

void ble_init(void) {
  NimBLEDevice::init("TEST");
  if (pServer == nullptr) {
    pServer = NimBLEDevice::createServer();
  }

  for(int index=0; index<NUM_SERVICES; index++ ) {
    String ServiceUUID = makeuuid(index);
    activeService[index]= new BLEService(ServiceUUID.c_str());
    // create characteristics
    activeService[index]->start();    // there is no stop

    if(index==0){
      if (pAdvertising == nullptr) {
        pAdvertising = BLEDevice::getAdvertising();
        pAdvertising->setScanResponse(true);
      }
      //  only add to the server/advertising for the 1st one at startup
      //activeService[index]->start();
      pServer->addService(activeService[index]);
    }
    pAdvertising->addServiceUUID(activeService[index]->getUUID());
  }
  pAdvertising->start();
}

void setup(void) {
    Serial.begin(115200);
    ble_init();
}

void loop(void) {
  delay(20000);
  pAdvertising->stop();
  pServer->removeService(activeService[1]);
  pAdvertising->removeServiceUUID(activeService[1]->getUUID());
  pAdvertising->start();
  ble_gatts_show_local();
  delay(20000);
  ESP.restart();
}

If this is not working for you could you please modify the code above to provide a test case that shows the issue?

sdetweil commented 1 year ago

will test. doesnt createService() do the addService() already?

sdetweil commented 1 year ago

using platformio made sure to use my fork, which is synced with #544

write characteristic, set a flag, in loop , check flag, if on, call StartOTAService

void startOTAService()
{
#ifdef ESP32
  #ifdef USE_NIMBLE
    pAdvertising->stop();
    Sprintf("removing advertised UUID=");
    Sprintln(activeService[Occupied]->getUUID().toString().c_str());
    pAdvertising->removeServiceUUID(activeService[Occupied]->getUUID());
  #else
    pAdvertising->stop();
  #endif

  OTAService = ArduinoBleOTA.begin(pServer, InternalStorage, HW_NAME_INFO.c_str(), HW_VER, SW_NAME.c_str(), SW_VER);
  ArduinoBleOTA.setUploadCallbacks( * (new myUploadCallbacks()));
  Sprintln("restart advertising after OTA Service setup");
  Sprintln("setting OTA service as only");
  OTARunning=true;
  #if USE_NIMBLE
    pServer->addService(OTAService);
    Sprintln("added service to server");
    pAdvertising->addServiceUUID(OTAService->getUUID());
    Sprintln("added UUID to advertising, should be only");
    pAdvertising->start();
    Sprintln("restart advertising");
#else
    pAdvertising->setServiceUUID(OTAService->getUUID());
    pAdvertising->start();
  #endif
#else

#endif
}

the ArduinoBleOTA lib begin does this. creates service, adds the UID to advertising starts the service returns the service address

NimBLEService *ArduinoBleOTAClass::begin(NimBLEServer *server, OTAStorage &storage,
                               const std::string &hwName, BleOtaVersion hwVersion,
                               const std::string &swName, BleOtaVersion swVersion,
                               bool enableUpload)
{
    BLEDevice::setMTU(BLE_OTA_MTU_SIZE);

    bleOtaUploader.begin(storage);
    bleOtaUploader.setEnabling(enableUpload);
    auto* service = server->createService(BLE_OTA_SERVICE_UUID);

    auto* rxCharacteristic = service->createCharacteristic(
        BLE_OTA_CHARACTERISTIC_UUID_RX,
        NIMBLE_PROPERTY::WRITE_NR
    );
    rxCharacteristic->setCallbacks(this);

    auto* txCharacteristic = service->createCharacteristic(
        BLE_OTA_CHARACTERISTIC_UUID_TX,
        NIMBLE_PROPERTY::READ | NIMBLE_PROPERTY::NOTIFY
    );
    this->txCharacteristic = txCharacteristic;

    begin(*service, hwName, hwVersion, swName, swVersion); // this creates the characteristics

    service->start();
    return service;
}

still get a crash

OTA STart requested (in loop)
in startOTAService-----
removing advertised UUID=11050001-27b9-42f0-82aa-2e951747bbf9
restart advertising after OTA Service setup
setting OTA service as only
added service to server
added UUID to advertising, should be only
restart advertising

//some time later
assert failed: ble_svc_gap_init ble_svc_gap.c:302 (rc == 0)

Backtrace: 0x40083d39:0x3ffce730 0x40092e01:0x3ffce750 0x40098a95:0x3ffce770 0x400d92bb:0x3ffce8a0 0x400d80d9:0x3ffce8c0 0x400d837e:0x3ffce8e0 0x400db065:0x3ffce950 0x400dbc59:0x3ffce970 0x400dbc80:0x3ffce9d0 0x400dfd06:0x3ffcea30 0x400dff19:0x3ffcea50 0x400de88d:0x3ffcea70 0x400813ce:0x3ffcea90 0x400d72df:0x3ffceab0
h2zero commented 1 year ago

Thanks, any chance you can decode the backtrace? I appreciate the code, could you provide a basic example that I can flash for testing?

sdetweil commented 1 year ago

I think this is a race, switching services, and disconnecting from the device at the same time.. the client app wrote a characteristic which set a flag

Stack trace:

     0x40083d39:  panic_abort  at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/esp_system/panic.c: line 408'
     0x40092e01:  esp_system_abort  at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/esp_system/esp_system.c: line 137'
     0x40098a95:  __assert_func  at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/newlib/assert.c: line 85'
     0x400daa2b:  ble_svc_gap_init  at /Users/sam/cnbeacon/.pio/libdeps/esp32doit-devkit-v1-ota-nimble/NimBLE-Arduino/src/nimble/nimble/host/services/gap/src/ble_svc_gap.c: line 302 (discriminator 1)'
     0x400d92ca:  NimBLEServer::resetGATT()  at /Users/sam/cnbeacon/.pio/libdeps/esp32doit-devkit-v1-ota-nimble/NimBLE-Arduino/src/NimBLEServer.cpp: line 720'
     0x400d9918:  NimBLEServer::handleGapEvent(ble_gap_event*, void*)  at /Users/sam/cnbeacon/.pio/libdeps/esp32doit-devkit-v1-ota-nimble/NimBLE-Arduino/src/NimBLEServer.cpp: line 395'
     0x400dcbed:  ble_gap_call_event_cb  at /Users/sam/cnbeacon/.pio/libdeps/esp32doit-devkit-v1-ota-nimble/NimBLE-Arduino/src/nimble/nimble/host/src/ble_gap.c: line 756'
     0x400ddee6:  ble_gap_conn_broken  at /Users/sam/cnbeacon/.pio/libdeps/esp32doit-devkit-v1-ota-nimble/NimBLE-Arduino/src/nimble/nimble/host/src/ble_gap.c: line 1314'
     0x400ddf0d:  ble_gap_rx_disconn_complete  at /Users/sam/cnbeacon/.pio/libdeps/esp32doit-devkit-v1-ota-nimble/NimBLE-Arduino/src/nimble/nimble/host/src/ble_gap.c: line 1342 (discriminator 4)'
     0x400e25ee:  ble_hs_hci_evt_disconn_complete  at /Users/sam/cnbeacon/.pio/libdeps/esp32doit-devkit-v1-ota-nimble/NimBLE-Arduino/src/nimble/nimble/host/src/ble_hs_hci_evt.c: line 268'
     0x400e298c:  ble_hs_hci_evt_process  at /Users/sam/cnbeacon/.pio/libdeps/esp32doit-devkit-v1-ota-nimble/NimBLE-Arduino/src/nimble/nimble/host/src/ble_hs_hci_evt.c: line 985'
     0x400e10a9:  ble_hs_event_rx_hci_ev  at /Users/sam/cnbeacon/.pio/libdeps/esp32doit-devkit-v1-ota-nimble/NimBLE-Arduino/src/nimble/nimble/host/src/ble_hs.c: line 548'
     0x400813ce:  ble_npl_event_run  at /Users/sam/cnbeacon/.pio/libdeps/esp32doit-devkit-v1-ota-nimble/NimBLE-Arduino/src/nimble/porting/npl/freertos/include/nimble/nimble_npl_os.h:526\n (inlined by) nimble_port_run at /Users/sam/cnbeacon/.pio/libdeps/esp32doit-devkit-v1-ota-nimble/NimBLE-Arduino/src/nimble/porting/nimble/src/nimble_port.c: line 319'
     0x400d8487:  NimBLEDevice::host_task(void*)  at /Users/sam/cnbeacon/.pio/libdeps/esp32doit-devkit-v1-ota-nimble/NimBLE-Arduino/src/NimBLEDevice.cpp: line 836'

numbers are different as I had to enable debug to get the backtrace decode

when it doesn't crash, I end up with TWO new services with no characteristics.. still trying to debug that. same flow works in non NimBLE.

still working on example

sdetweil commented 1 year ago

same crash if I wait and disconnect later (30 seconds)

sdetweil commented 1 year ago

I have a question, I am calling create service and characteristics from loop. does this change the memory used to hold their data?

the library author does this at setup time.

I am seeing empty service, which makes me wonder about memory allocation pool location/persistence

edit: this was caused by calling Advertising->addServiceUUID() twice with the same UUID. but only having one service instance. removing the duplicate add and the remaining service shows all the characteristics.. should there be a check for duplicate ?

sdetweil commented 1 year ago

here is the testcase https://github.com/sdetweil/testapp see the readme using platformio

oops, you will see the duplicate addServiceUUID() problem comment out line 465 , as the library has already done this, i use a modified version of the library pAdvertising->addServiceUUID(OTAService->getUUID());

h2zero commented 1 year ago

Thanks for the test code.

The cause is due to the OTA library creating the service and starting it in the begin call which calls into the stack to update services and the same happens when the client disconnects due to the service update.

The fix is quite simple though, just disconnect the clients before starting the OTA service. I added this at line 460 and it resolved the issue:

  for (auto it: pServer->getPeerDevices()) {
    pServer->disconnect(it);
  }

  while (pServer->getConnectedCount() > 0) {
    yield();
  }
sdetweil commented 1 year ago

fabulous! will try asap..high school graduation this evening

sdetweil commented 1 year ago

well, it gets rid of the crash.. but if you connect to the new 15xxxxxx service, there are two listed, both empty

I added debug to the addServiceUUID() method() to see the count of entries in the m_serviceUUIDs list, and get 1

void NimBLEAdvertising::addServiceUUID(const NimBLEUUID &serviceUUID) {
    m_serviceUUIDs.push_back(serviceUUID);
    m_advDataSet = false;
    Serial.print("adding uuid ");
    Serial.println(m_serviceUUIDs.size());
} // addServiceUUID

screenshot

h2zero commented 1 year ago

You have only removed the old services from the advertising. If you don't want to see them while connected you need to also remove them from the server. You should do that before the OTA service begin call.

sdetweil commented 1 year ago

I don't care about that. the otaservice is duplicated and has no characteristics (see the updated pic above) i this occurs in the sample and in my full firmware

its different built on my mac vs the intel laptop I used thursday/friday

but its using the same platformio.ini