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

updating manufacturer data in BLEAdvertisementData instance - impossible? #564

Closed mhaberler closed 1 month ago

mhaberler commented 1 year ago

I'm building a sensor which reports in the manufacturer data hence the manufacturer data changes frequently

this does work, however only on a fresh instance of BLEAdvertisementData():

  advData = new BLEAdvertisementData();
  advData->setManufacturerData(manufacturerData);

  pAdvertising->setAdvertisementData(*advData);
  pAdvertising->setAdvertisementType(BLE_GAP_CONN_MODE_NON);

  pAdvertising->start();
  delete advData;

reusing the same BLEAdvertisementData() fails - I think the payload is only appended to and there is no way to reset the payload my solution deletes and re-creates objects needlessly

Am I overlooking something?

if not: can I suggest a BLEAdvertisementData.reset() method which resets the payload?

thanks for a great and well supported library!

Michael

repo: https://github.com/mhaberler/flowsensor/blob/main/src/beacon.cpp#L45-L55

h2zero commented 1 year ago

Hi @mhaberler, yes that is a deficiency in the class and a reset method should definitely be implemented. Thanks for creating this issue.

mhaberler commented 1 year ago

would you accept a PR? if so, on which branch - master?

h2zero commented 1 year ago

Yes, feel free to submit a PR to the release/1.4 branch. I will add it to the master branch after.

mhaberler commented 1 year ago

there is something else fishy in the code I quoted:

if I call this code too frequently (for instance from a UI button pressed in rapid succession), I get a crash

it seems I try to fiddle the advertising before the previous fiddle has finished/synced to the host

is there any predicate I can test to prevent this?

  #0  0x403769ed:0x3fcf27c0 in panic_abort at /Users/mah/.platformio/packages/framework-espidf/components/esp_system/panic.c:408
  #1  0x4037fa6d:0x3fcf27e0 in esp_system_abort at /Users/mah/.platformio/packages/framework-espidf/components/esp_system/esp_system.c:137
  #2  0x403878d1:0x3fcf2800 in __assert_func at /Users/mah/.platformio/packages/framework-espidf/components/newlib/assert.c:85
  #3  0x40386a5f:0x3fcf2920 in tlsf_free at /Users/mah/.platformio/packages/framework-espidf/components/heap/heap_tlsf.c:964 (discriminator 1)
  #4  0x40387484:0x3fcf2940 in multi_heap_free_impl at /Users/mah/.platformio/packages/framework-espidf/components/heap/multi_heap.c:225
  #5  0x403778d2:0x3fcf2960 in heap_caps_free at /Users/mah/.platformio/packages/framework-espidf/components/heap/heap_caps.c:361
  #6  0x40387901:0x3fcf2980 in free at /Users/mah/.platformio/packages/framework-espidf/components/newlib/heap.c:39
  #7  0x4207afed:0x3fcf29a0 in operator delete(void*) at /builds/idf/crosstool-NG/.build/HOST-x86_64-apple-darwin12/xtensa-esp32s3-elf/src/gcc/libstdc++-v3/libsupc++/del_op.cc:49
  #8  0x42000189:0x3fcf29c0 in __gnu_cxx::new_allocator<char>::deallocate(char*, unsigned int) at /Users/mah/.platformio/packages/toolchain-xtensa-esp32s3/xtensa-esp32s3-elf/include/c++/8.4.0/ext/new_allocator.h:125
      (inlined by) std::allocator_traits<std::allocator<char> >::deallocate(std::allocator<char>&, char*, unsigned int) at /Users/mah/.platformio/packages/toolchain-xtensa-esp32s3/xtensa-esp32s3-elf/include/c++/8.4.0/bits/alloc_traits.h:462
      (inlined by) std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_destroy(unsigned int) at /Users/mah/.platformio/packages/toolchain-xtensa-esp32s3/xtensa-esp32s3-elf/include/c++/8.4.0/bits/basic_string.h:226
      (inlined by) std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_dispose() at /Users/mah/.platformio/packages/toolchain-xtensa-esp32s3/xtensa-esp32s3-elf/include/c++/8.4.0/bits/basic_string.h:221
      (inlined by) std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string() at /Users/mah/.platformio/packages/toolchain-xtensa-esp32s3/xtensa-esp32s3-elf/include/c++/8.4.0/bits/basic_string.h:657
      (inlined by) NimBLEAdvertisementData::~NimBLEAdvertisementData() at lib/esp-nimble-cpp/src/NimBLEAdvertising.h:50
      (inlined by) beacon_update_manufacturer_data(unsigned char*, unsigned int) at src/beacon.cpp:49
  #9  0x420008ac:0x3fcf2a00 in sensor_update(bool) at src/main.cpp:147
  #10 0x420009a4:0x3fcf2a20 in setup()::{lambda()#2}::_FUN() at src/main.cpp:113
      (inlined by) _FUN at src/main.cpp:114
  #11 0x420582cf:0x3fcf2a40 in timer_process_alarm at /Users/mah/.platformio/packages/framework-espidf/components/esp_timer/src/esp_timer.c:360
  #12 0x42058325:0x3fcf2a70 in timer_task at /Users/mah/.platformio/packages/framework-espidf/components/esp_timer/src/esp_timer.c:386 (discriminator 1)
  #13 0x40382fd5:0x3fcf2a90 in vPortTaskWrapper at /Users/mah/.platformio/packages/framework-espidf/components/freertos/port/xtensa/port.c:142
mhaberler commented 1 year ago

if I may tack on a question (is there a better venue like a forum?):

is the approach "stop advertising", "fiddle manufacturer data", "restart advertising" the right one, or is there some more elegant method I overlooked?

thanks in advance Michael

h2zero commented 1 year ago

The approach is correct in that the advertising should be stopped, data updated, then restarted. The issue here is that the stop process takes some time to complete and you need to wait for that before changing data.

mhaberler commented 1 year ago

for my understanding - would this fix the race?

edit: it seems not, because pAdvertising->reset() does not trigger the advCompleteCB callback

edit2: it seems this has done the trick: https://github.com/mhaberler/flowsensor/blob/main/src/beacon.cpp#L41-L52

h2zero commented 1 year ago

That should be the process, looks good to me.

mhaberler commented 1 year ago

thanks! leaving this open until I have a reset() method PR and the eventual working code can be referred to