gapp_server example: obscure code/logic

[rappscallion]

Hello,

I am wondering about a few lines in the gatts_server example:
https://github.com/espressif/esp-idf/bl ... tts_demo.c

Code: Select all

static uint8_t adv_config_done = 0;
#define adv_config_flag      (1 << 0)
#define scan_rsp_config_flag (1 << 1)

In the gatts_profile_a_event_handler the adv_config_done is set with:

Code: Select all

        esp_err_t raw_adv_ret = esp_ble_gap_config_adv_data_raw(raw_adv_data, sizeof(raw_adv_data));
        […] // error handling
        adv_config_done |= adv_config_flag;
        esp_err_t raw_scan_ret = esp_ble_gap_config_scan_rsp_data_raw(raw_scan_rsp_data, sizeof(raw_scan_rsp_data));
        […] // error handling
        adv_config_done |= scan_rsp_config_flag;

and used in the gap_event_handler as follows:

Code: Select all

    case ESP_GAP_BLE_ADV_DATA_RAW_SET_COMPLETE_EVT:
        adv_config_done &= (~adv_config_flag);
        if (adv_config_done==0){
            esp_ble_gap_start_advertising(&adv_params);
        }
        break;
    case ESP_GAP_BLE_SCAN_RSP_DATA_RAW_SET_COMPLETE_EVT:
        adv_config_done &= (~scan_rsp_config_flag);
        if (adv_config_done==0){
            esp_ble_gap_start_advertising(&adv_params);
        }
        break;

It took me some while to figure out what is done exactly and I think the logic is quite obscure.
What happens:
- from underlying ble: the switch cases get only executed if respective config function was called
- if only one config was done the case gets executed.
- if both configs are done the first case statement's if statement returns false but deletes its own flag. Then the next if statement is true so esp_ble_gap_start_advertising is called only once.

Can we replace the entire logic with one variable is_advertising?

Code: Select all

// global variable
static uint8_t is_advertising = 0;

// in gap_event_handler(…) function:
case ESP_GAP_BLE_ADV_DATA_RAW_SET_COMPLETE_EVT:
        if (is_advertising==0){
            esp_ble_gap_start_advertising(&adv_params);
            is_advertising = 1;
        }
    break;
case ESP_GAP_BLE_SCAN_RSP_DATA_RAW_SET_COMPLETE_EVT:
        if (is_advertising==0){
            esp_ble_gap_start_advertising(&adv_params);
            is_advertising = 1;
        }
    break;

In my opinion this would be way clearer and easier to understand.
Furthermore the logic is also dependent on ble generating the right events. If you take the logic to somewhere else you might get some surprises:
The logic returns always true except that both flags are set simultaneously. Even it no flag is set at all it evaluates as true (does not happen here as mentioned above because ble only raises proper events).
I attach a c file where I tested the logic

But I am no expert on ble so I am asking if the changes would have unintended side effects I did not see?
(like with multiple gatt profiles, or whatever else).
Otherwise I would like to change the example.


I am looking forward to feedback so I can stop thinking about this :-)

Another quick question: would a simple ble scanner example be worth an own example?
Just for scanning available devices you don't need a gatt layer and on my first attempt it took me a bit to strip the gatt example down to a simple scanner.
However I think that that is a common think to do. Should I suggest it as an example?

Best regards!

[chegewara]

Hello,

I am wondering about a few lines in the gatts_server example:
https://github.com/espressif/esp-idf/bl ... tts_demo.c

Code: Select all

static uint8_t adv_config_done = 0;
#define adv_config_flag      (1 << 0)
#define scan_rsp_config_flag (1 << 1)

In the gatts_profile_a_event_handler the adv_config_done is set with:

Code: Select all

        esp_err_t raw_adv_ret = esp_ble_gap_config_adv_data_raw(raw_adv_data, sizeof(raw_adv_data));
        […] // error handling
        adv_config_done |= adv_config_flag;
        esp_err_t raw_scan_ret = esp_ble_gap_config_scan_rsp_data_raw(raw_scan_rsp_data, sizeof(raw_scan_rsp_data));
        […] // error handling
        adv_config_done |= scan_rsp_config_flag;

and used in the gap_event_handler as follows:

Code: Select all

    case ESP_GAP_BLE_ADV_DATA_RAW_SET_COMPLETE_EVT:
        adv_config_done &= (~adv_config_flag);
        if (adv_config_done==0){
            esp_ble_gap_start_advertising(&adv_params);
        }
        break;
    case ESP_GAP_BLE_SCAN_RSP_DATA_RAW_SET_COMPLETE_EVT:
        adv_config_done &= (~scan_rsp_config_flag);
        if (adv_config_done==0){
            esp_ble_gap_start_advertising(&adv_params);
        }
        break;

In ble gap you have 2 independent packets which can be set with separate adv_params value (in this example it is the same value). Now first part of this quoted code is only setting accordingly value for each packet(advertising packet and scan response packet), when there is no error in adv_packet structure then function returns ESP_OK and flag is set. Next you have event for both function that will clear bit flag in adv_config_done and when now you have few options:
- you want to setup and advertise only one packet, then only one bit flag is set and then is clear on event,
- you want to setup and advertise both packets, so both events need to clear bit flag for own configuration
when both flags are cleared then adv_config_done == 0 and you can start advertising.

Now in your code:

It took me some while to figure out what is done exactly and I think the logic is quite obscure.
What happens:
- from underlying ble: the switch cases get only executed if respective config function was called
- if only one config was done the case gets executed.
- if both configs are done the first case statement's if statement returns false but deletes its own flag. Then the next if statement is true so esp_ble_gap_start_advertising is called only once.

Can we replace the entire logic with one variable is_advertising?

Code: Select all

// global variable
static uint8_t is_advertising = 0;

// in gap_event_handler(…) function:
case ESP_GAP_BLE_ADV_DATA_RAW_SET_COMPLETE_EVT:
        if (is_advertising==0){
            esp_ble_gap_start_advertising(&adv_params);
            is_advertising = 1;
        }
    break;
case ESP_GAP_BLE_SCAN_RSP_DATA_RAW_SET_COMPLETE_EVT:
        if (is_advertising==0){
            esp_ble_gap_start_advertising(&adv_params);
            is_advertising = 1;
        }
    break;

Best regards!

You will start advertising always when one of advertising packets is set, which means:
- advertising or response packet may be not set and wont be advertised at all,
- you will try to start advertising 2 times, each time when event occurs.