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

getting duplicate service #547

Closed sdetweil closed 2 months ago

sdetweil commented 1 year ago

followup on previous add/removeServiceUUID issue

service created only once, is the only advertised service in nrfConnect.

connect to see services available, but this recently created service is shown twice, both empty( no characteristics)

connecting app says characteristics missing

239750299-a6432a30-cba3-49a1-b0b3-2d2d8b32b8fe

fails on the test app sample I provided last week.

I had seen this when I did addServiceUUID() multiple times, but I have verified there is only one add call done now

while in loop()

stop advertising (prior service UUID) remove prior advertising UUID ( now none) call library which creates service on server lib creates characteristics (6) lib starts service lib returns service address add serviceuuid (added debug in nimble method to report count of UUIDs, after add call, only 1) start advertising

mobile scan(nrfConnect) correct UUID advertised connect services as shown in screen shot

if I do NOT addServiceUUID after lib return, no service is advertised, but on connect the service is still shown twice, as above

h2zero commented 1 year ago

To resolve this I had to move this:

  // disconnect the(any) connected clients
  for (auto it: pServer->getPeerDevices()) {
    pServer->disconnect(it);
  }
  while (pServer->getConnectedCount() > 0) {
    yield();
  }

to line 466 and add a delay(100); after this.

Also removed this line in the OTA lib https://github.com/sdetweil/ArduinoBleOTA/blob/6cf3edbd73d72b94848ef9c62ae5592160ee2400/src/ArduinoBleOtaClassNimBle.cpp#L74

And finally added:

pServer->advertiseOnDisconnect(false);

here https://github.com/sdetweil/testapp/blob/ce7302b5a1942b65d1dd7824e1cf8e6e66e8b607/src/testapp.cpp#L521

There were simply too many things triggering service start at once.

sdetweil commented 1 year ago

thanks

  1. i had moved up the client disconnect, as there is a disconnect handler that wants to turn advertising back on. 1.5 will add the delay
  2. I had removed the service->start(), but it didn't affect the results.
  3. pServer->advertiseOnDisconnect(false); I had assumed that was the default.. cause I manually restart on disconnect (see 1!)..

  4. scanner... i have changed it to cycle less often, 2 seconds every 10. to save power... we shall see
sdetweil commented 1 year ago

ok, not there yet

now I have one service, but empty

also turned off the scanner when we are starting the OTA service onward.

in loop check for OTAService requested first, (start service and set Running then check if OTA Running (yes), skip scan

i don't restart advertising in onDisconnect handler if OTARunning

void startOTAService()
{
  OTARunning = 2;
#ifdef ESP32
  advertising = false;
  #ifdef USE_NIMBLE
    Sprintln("stop advertising");
    pAdvertising->stop();
    Sprintf("removing advertised UUID=");
    Sprintln(activeService[status]->getUUID().toString().c_str());
    pAdvertising->removeServiceUUID(activeService[status]->getUUID());
  #else
    pAdvertising->stop();
  #endif
  // disconnect the(any) connected clients
  disconnectClient();  // the code you shared + the delay(100)
  // start the Over The Air update service
  OTAService = ArduinoBleOTA.begin(pServer, InternalStorage, HW_NAME_INFO.c_str(), HW_VER, SW_NAME.c_str(), SW_VER);
  OTARunning = 1;
  ArduinoBleOTA.setUploadCallbacks(*(new myUploadCallbacks()));
  Sprintln("restart advertising after OTA Service setup");
  Sprintln("setting OTA service as only");
  #if USE_NIMBLE
     pAdvertising->addServiceUUID(OTAService->getUUID());
     Sprint("added UUID to advertising, should be only=");
     Sprintln(OTAService->getUUID().toString().c_str());
     pAdvertising->start();
     Sprintln("restart advertising");
  #else
    pAdvertising->setServiceUUID(OTAService->getUUID());
    pAdvertising->start();
  #endif
    advertising = true;
#else

#endif

}

//if characteristic write handler write characteristic=9A64 OTA Start requested

// later ...1 second called from loop

//start OtaService stop advertising removing advertised UUID=00000001-27b9-42f0-82aa-2e951747bbf9 disconnected (force disconnect) restart advertising after OTA Service setup setting OTA service as only added UUID to advertising, should be only=15c155ca-36c5-11ed-adc0-9741d6a72f04 restart advertising connected // nrfConnect on disconnect start advertising= 15c155ca-36c5-11ed-adc0-9741d6a72f04 //restart advet on ota service disconnected // nrfConnect end of onDisconnect() handler

sdetweil commented 1 year ago

I updated the testapp sample, with all these changes, and included more to stop scanning while engaging in startup and running of the ota service.

https://github.com/sdetweil/testapp

still getting duplicate empty services

h2zero commented 1 year ago

I left comments on the latest commit

sdetweil commented 1 year ago

thank you. left comments back, really on disconnect

sdetweil commented 1 year ago

is there a usecase that service->start() is appropriate?

I don't own the ota service library

h2zero commented 1 year ago

service->start() is only necessary for the initial startup, if the services are changed while running the library will take care of starting them.

sdetweil commented 1 year ago

service->start().. thanks.. is this a wasted call, if already running? or a damaging call?

again , I don't own the library, don't "want" to own my own fork...

h2zero commented 1 year ago

It's problematic to call it the way it's being done here. if you don't want to use a fork then I would suggest adding the service at the startup and just remove it until needed.

sdetweil commented 1 year ago

create. remove it's started. but not accessible

when needed addService() then addServiceUUID

sdetweil commented 1 year ago

so, I tried this, but see the OTA service when connected... altho removed

in setup

  // create the two default services
  for(int i=0; i<NUM_SERVICES; i++){
    activeService[i]=setupService(i);
  }
#ifdef ESP32
  Sprint("setting advertised service=");
  Sprintln(activeService[status]->getUUID().toString().c_str());
  // pAdvertising set in setupService()
  pAdvertising->setScanResponse(true);
  #ifdef USE_NIMBLE
    #ifdef USE_OTA
    // create the service
    OTAService = ArduinoBleOTA.begin(pServer, InternalStorage, HW_NAME_INFO.c_str(), HW_VER, SW_NAME.c_str(), SW_VER);
    // but remove it til needed... Nimble library requirement
    // can only do this for Nimble as ESPBLE has removeService(), but no addService()
    pServer->removeService(OTAService, false);
    // don't know if this is needed, doesn't make a difference
    pAdvertising->removeServiceUUID(OTAService->getUUID());
    #endif
  #endif 
#endif

in loop

void startOTAService()
{
  OTARunning = OTA_Pending;
#ifdef ESP32
  advertising = false;
  pAdvertising->stop();
  Sprintf("removing advertised UUID=");
  Sprintln(activeService[status]->getUUID().toString().c_str());
  pAdvertising->removeServiceUUID(activeService[status]->getUUID());
  delay(50);
#ifdef USE_NIMBLE
    Sprintln("nimble adding OTA service");
    pServer->addService(OTAService);
#else
    // start the Over The Air update service
    Sprintln("NOT nimble creating OTA service");
    OTAService = ArduinoBleOTA.begin(pServer, InternalStorage, HW_NAME_INFO.c_str(), HW_VER, SW_NAME.c_str(), SW_VER);
#endif
  // disconnect the(any) connected clients
  disconnectClient();
  OTARunning = OTA_Running;
  ArduinoBleOTA.setUploadCallbacks(*(new myUploadCallbacks()));
  Sprintln("restart advertising after OTA Service setup");
  Sprintln("setting OTA service as only");
  Sprint("added UUID to advertising, should be only=");
  Sprintln(OTAService->getUUID().toString().c_str());
#if USE_NIMBLE
    // there is none set
    pAdvertising->addServiceUUID(OTAService->getUUID());
  #else
    // clear al uuids and set one // local fix
    pAdvertising->setServiceUUID(OTAService->getUUID());
  #endif
    Sprintln("restart advertising");
    pAdvertising->start();
    advertising = true;
#else
   // non esp32 
#endif;
}
sdetweil commented 1 year ago

I updated the test app and the library to remove the pService->start()

but see the service in nrfconnect after connecting to the device.. shouldn't be there if I removed it..

h2zero commented 11 months ago

This works as intended by adding pServer->start(); after ArduinoBleOTA.begin