[Report] Potential issue with Battery Service module

[Hieu]

Not really a question, but I found a potential issue with the Battery Service module in ble_bas_battery_level_update(). This is in both SDK v10 and SDK v12.

The problem lies in the following block of code

if (battery_level != p_bas->battery_level_last)
{
    // Initialize value struct.
    memset(&gatts_value, 0, sizeof(gatts_value));

    gatts_value.len     = sizeof(uint8_t);
    gatts_value.offset  = 0;
    gatts_value.p_value = &battery_level;

    // Update database.
    err_code = sd_ble_gatts_value_set(p_bas->conn_handle,
                                      p_bas->battery_level_handles.value_handle,
                                      &gatts_value);
    if (err_code == NRF_SUCCESS)
    {
        // Save new battery value.
        p_bas->battery_level_last = battery_level;
    }
    else
    {
        return err_code;
    }

    // Send value if connected and notifying.
    if ((p_bas->conn_handle != BLE_CONN_HANDLE_INVALID) && p_bas->is_notification_supported)
    {
        ble_gatts_hvx_params_t hvx_params;

        memset(&hvx_params, 0, sizeof(hvx_params));

        hvx_params.handle = p_bas->battery_level_handles.value_handle;
        hvx_params.type   = BLE_GATT_HVX_NOTIFICATION;
        hvx_params.offset = gatts_value.offset;
        hvx_params.p_len  = &gatts_value.len;
        hvx_params.p_data = gatts_value.p_value;

        err_code = sd_ble_gatts_hvx(p_bas->conn_handle, &hvx_params);
    }
    else
    {
        err_code = NRF_ERROR_INVALID_STATE;
    }
}

return err_code;

If sd_ble_gatts_value_set() succeeded while later steps failed, then battery_level_last would have already been updated with a new value. The next time the system reattempt to send the failed notification with that same battery level value, the notification attempt will be disregarded by the Battery Service module due to the check if (battery_level != p_bas->battery_level_last).

The peer device would never learn of the new battery level if they rely solely on notification.

As such, I believe battery_level_last should only be updated after sd_ble_gatts_hvx() returns NRF_SUCCESS.

A work around that I have been issue is immediately call ble_bas_battery_level_update() again with 0xFF as the battery level. This way when I get to reattempt the battery level update, I would not run into the described issue, and the notification would be made properly.

The way Battery Service module is currently designed, the peer device would also never be notified of the exact battery level at the time of connection establishment. Of course, the peer device could always simply read the Battery Level Characteristic instead.

[Ole Bauck]

The bas module is made to also be used without notifications, which is the reason sd_ble_gatts_value_set(..) is used. If you are only doing notifications you can use only sd_ble_gatts_hvx(..) as this will update the value in addition to sending it if the value is non-NULL.

The problem you are referring to is if sd_ble_gatts_value_set(..) succeeds, but sd_ble_gatts_hvx(..) fails. Then you will have problems, but it is unlikely that this will happen.

On connection establishment the central can read the value of the characteristic.

I will discuss with the developers if this module can be done better. Thanks for the feedback.