Description
I have some issues with semaphore use in the BLE classes, and thought I'd open a discussion here (or a Neil might say flesh out 'the story'). I fully expect that many of the defined semaphores are required for reasons that I've not thought of or encountered, and if so, I'd like to expand my understanding.
Broadly speaking, it appears that calls to functions of the esp-idf Bluetooth stack create internal 'events' that are posted to handler queues to be serviced by internal tasks. I can see the need for semaphore use to synchronize 'app code' with the underlying BT stack tasks (for instance to ensure a 'service' has actually been created, before allowing the 'application code' to continue with adding characteristics to it, etc.).
However, using a semaphore in BLECharacteristic to 'stall' application code that calls 'indicate()' until an ESP_GATTS_CONF_EVT shows a connected client confirmed the change was successfully received (via 'm_semaphoreConfEvt.wait()' seems pointless to me, unless you plan on spawning a new thread to update attribute changes for each & every attribute (?). If the indicate was not yet acknowledged, and a subsequent indicate was called on the same attribute, calling 'm_semaphoreConfEvt.wait()' just before 'm_semaphoreConfEvt.take() would stall the calling thread just as easily, and this way would allow a single 'update thread' to update multiple changed attributes (possibly on a timer), with a simple loop:
while(1) {
if (attribute_a_changed == true) {
pAttribA->setValue( (uint8_t *)&attribA, sizeof(attribA) );
pAttribA->indicate(); // send indicate to any clients that have enabled it
pAttribA->notify(); // send notify to any clients that have enabled it
}
if (attribute_b_changed == true) {
pAttribB->setValue( (uint8_t *)&attribB, sizeof(attribB) );
pAttribB->indicate(); // send indicate to any clients that have enabled it
pAttribB->notify(); // send notify to any clients that have enabled it
}
...
vTaskDelay( pdMS_TO_TICKS(200) );
}
I'm aware that Neil raised the issue of ESP_GATTS_CONF_EVT being raised on notify() #749, and the response that "it's a feature of our system". Regardless, since BLE defines notify() as equivalent to UDP (vs TCP), I'd make the case that there is certainly NO valid reason for a semaphore against it.
Note that @chegewara is currently working on multi-device connections, which will make all of this semaphore handling much more complicated, when it IS legitimate, so taking some time to do some thoughtful cleanup now would be very helpful.