Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 2f22e8fe authored by Jakub Tyszkowski's avatar Jakub Tyszkowski
Browse files

VolumeControl: Fix multi-read request 

Handle properly case where MTU size is to small to hold the entire
multi-read request response.

Bug: 382118485
Test: atest bluetooth_vc_test
Flag: EXEMPT; regression covered with unit test
Change-Id: Icaa97e40cf6de76d3e4d79c68554ce66260a03c8
parent 3b15d68f

system/bta/Android.bp

+1 −0
Original line number Diff line number Diff line
@@ -634,6 +634,7 @@ cc_test { 
    srcs: [

        ":TestCommonMockFunctions",

        ":TestMockStackBtmInterface",

        ":TestMockStackGatt",

        "gatt/database.cc",

        "gatt/database_builder.cc",

        "test/common/bta_gatt_api_mock.cc",

system/bta/vc/device.cc

+52 −32
Original line number Diff line number Diff line
@@ -36,6 +36,7 @@ 
#include "gatt/database.h"

#include "gattdefs.h"

#include "stack/btm/btm_sec.h"

#include "stack/gatt/gatt_int.h"

#include "stack/include/bt_types.h"

#include "stack/include/gatt_api.h"

#include "types/bluetooth/uuid.h"
@@ -421,49 +422,68 @@ void VolumeControlDevice::EnqueueRemainingRequests(tGATT_IF /*gatt_if*/, 
                                                   GATT_READ_OP_CB chrc_read_cb,

                                                   GATT_READ_MULTI_OP_CB chrc_multi_read_cb,

                                                   GATT_WRITE_OP_CB /*cccd_write_cb*/) {

  std::vector<uint16_t> handles_to_read;

  const auto is_eatt_supported = gatt_profile_get_eatt_support_by_conn_id(connection_id);



  for (auto const& input : audio_inputs.volume_audio_inputs) {

    handles_to_read.push_back(input.state_handle);

    handles_to_read.push_back(input.gain_setting_handle);

    handles_to_read.push_back(input.type_handle);

    handles_to_read.push_back(input.status_handle);

    handles_to_read.push_back(input.description_handle);

  }

  /* List of handles to the attributes having known and fixed-size values to read using the

   * ATT_READ_MULTIPLE_REQ. The `.second` component contains 1 octet for the length + the actual

   * attribute value length, exactly as in the received HCI packet for ATT_READ_MULTIPLE_RSP.

   * We use this to make sure the request response will fit the current MTU size.

   */

  std::list<std::pair<uint16_t, size_t>> handles_to_read;



  /* Variable-length attributes - always read using the regular read requests to automatically

   * handle truncation in the  GATT layer if MTU is to small to fit even a single complete value.

   */

  std::vector<uint16_t> handles_to_read_variable_length;



  for (auto const& offset : audio_offsets.volume_offsets) {

    handles_to_read.push_back(offset.state_handle);

    handles_to_read.push_back(offset.audio_location_handle);

    handles_to_read.push_back(offset.audio_descr_handle);

    handles_to_read.push_back(std::make_pair(offset.state_handle, 4));

    handles_to_read.push_back(std::make_pair(offset.audio_location_handle, 5));

    handles_to_read_variable_length.push_back(offset.audio_descr_handle);

  }



  log::debug("{}, number of handles={}", address, handles_to_read.size());



  if (!com::android::bluetooth::flags::le_ase_read_multiple_variable()) {

    for (auto const& handle : handles_to_read) {

      BtaGattQueue::ReadCharacteristic(connection_id, handle, chrc_read_cb, nullptr);

    }

    return;

  for (auto const& input : audio_inputs.volume_audio_inputs) {

    handles_to_read.push_back(std::make_pair(input.state_handle, 5));

    handles_to_read.push_back(std::make_pair(input.gain_setting_handle, 4));

    handles_to_read.push_back(std::make_pair(input.type_handle, 2));

    handles_to_read.push_back(std::make_pair(input.status_handle, 2));

    handles_to_read_variable_length.push_back(input.description_handle);

  }



  size_t sent_cnt = 0;

  log::debug("{}, number of fixed-size attribute handles={}", address, handles_to_read.size());

  log::debug("{}, number of variable-size attribute handles={}", address,

             handles_to_read_variable_length.size());



  while (sent_cnt < handles_to_read.size()) {

    tBTA_GATTC_MULTI multi_read{};

    size_t remain_cnt = (handles_to_read.size() - sent_cnt);

  if (com::android::bluetooth::flags::le_ase_read_multiple_variable() && is_eatt_supported) {

    const size_t payload_limit = this->mtu_ - 1;



    multi_read.num_attr =

            remain_cnt > GATT_MAX_READ_MULTI_HANDLES ? GATT_MAX_READ_MULTI_HANDLES : remain_cnt;

    auto pair_it = handles_to_read.begin();

    while (pair_it != handles_to_read.end()) {

      tBTA_GATTC_MULTI multi_read{.num_attr = 0};

      size_t size_limit = 0;



    auto handles_begin = handles_to_read.begin() + sent_cnt;

    std::copy(handles_begin, handles_begin + multi_read.num_attr, multi_read.handles);



    sent_cnt += multi_read.num_attr;

    log::debug{"{}, calling multi with {} attributes, sent_cnt {} ", address, multi_read.num_attr,

               sent_cnt};

      // Send at once just enough attributes to stay below the MTU size limit for the response

      while ((pair_it != handles_to_read.end()) && (size_limit + pair_it->second < payload_limit) &&

             (multi_read.num_attr < GATT_MAX_READ_MULTI_HANDLES)) {

        multi_read.handles[multi_read.num_attr] = pair_it->first;

        size_limit += pair_it->second;

        ++multi_read.num_attr;

        ++pair_it;

      }



      log::debug{"{}, calling multi-read with {} attributes, {} left", address, multi_read.num_attr,

                 std::distance(pair_it, handles_to_read.end())};

      BtaGattQueue::ReadMultiCharacteristic(connection_id, multi_read, chrc_multi_read_cb, nullptr);

    }

  } else {

    for (auto const& [handle, _] : handles_to_read) {

      BtaGattQueue::ReadCharacteristic(connection_id, handle, chrc_read_cb, nullptr);

    }

  }



  for (auto const& handle : handles_to_read_variable_length) {

    BtaGattQueue::ReadCharacteristic(connection_id, handle, chrc_read_cb, nullptr);

  }

}



bool VolumeControlDevice::VerifyReady(uint16_t handle) {

system/bta/vc/devices.h

+1 −0
Original line number Diff line number Diff line
@@ -48,6 +48,7 @@ public: 
  uint8_t flags;



  tCONN_ID connection_id;

  uint16_t mtu_ = GATT_DEF_BLE_MTU_SIZE;



  /* Volume Control Service */

  uint16_t volume_state_handle;

system/bta/vc/devices_test.cc

+40 −10
Original line number Diff line number Diff line
@@ -692,25 +692,39 @@ TEST_F(VolumeControlDeviceTest, test_enqueue_remaining_requests_multiread) { 
  tGATT_IF gatt_if = 0x0001;

  std::vector<uint8_t> register_for_notification_data({0x01, 0x00});



  // The amount of attributes read at once is limited by the MTU size - 1 (here 22)

  tBTA_GATTC_MULTI expected_to_read_part_1 = {

          .num_attr = 10,

          .num_attr = 4,

          .handles = {0x0022 /* audio input state 1 */, 0x0025 /* gain setting properties 1 */,

                      0x0027 /* audio input type 1 */, 0x0029 /* audio input status 1 */,

                      0x002e /* audio input description 1 */, 0x0042 /* audio input state 2 */,

                      0x0045 /* gain setting properties 2 */, 0x0047 /* audio input type 2 */,

                      0x0049 /* audio input status 2 */, 0x004e /* audio input description 2 */},

                      0x0027 /* audio input type 1 */, 0x0029 /* audio input status 1 */},

  };



  tBTA_GATTC_MULTI expected_to_read_part_2 = {

          .num_attr = 6,

          .handles = {0x0062 /* audio output state 1 */, 0x0065 /* audio output location 1 */,

                      0x0069 /* audio output description 1 */, 0x0082 /* audio output state 1 */,

                      0x0085 /* audio output location 1 */,

                      0x008a /* audio output description 1 */},

          .num_attr = 5,

          .handles = {0x0042 /* audio input state 2 */, 0x0045 /* gain setting properties 2 */,

                      0x0047 /* audio input type 2 */, 0x0049 /* audio input status 2 */,

                      0x0062 /* audio output state 1 */},

  };



  tBTA_GATTC_MULTI expected_to_read_part_3 = {

          .num_attr = 3,

          .handles = {0x0065 /* audio output location 1 */, 0x0082 /* audio output state 1 */,

                      0x0085 /* audio output location 1 */},

  };



  uint16_t expected_audio_input_description_1 = 0x002e;

  uint16_t expected_audio_input_description_2 = 0x004e;

  uint16_t expected_audio_output_description_1 = 0x0069;

  uint16_t expected_audio_output_description_2 = 0x008a;



  tBTA_GATTC_MULTI received_to_read_part_1{};

  tBTA_GATTC_MULTI received_to_read_part_2{};

  tBTA_GATTC_MULTI received_to_read_part_3{};



  uint16_t audio_input_description_1 = 0;

  uint16_t audio_input_description_2 = 0;

  uint16_t audio_output_description_1 = 0;

  uint16_t audio_output_description_2 = 0;



  {

    testing::InSequence s;
@@ -719,6 +733,16 @@ TEST_F(VolumeControlDeviceTest, test_enqueue_remaining_requests_multiread) { 
            .WillOnce(SaveArg<1>(&received_to_read_part_1));

    EXPECT_CALL(gatt_queue, ReadMultiCharacteristic(_, _, _, _))

            .WillOnce(SaveArg<1>(&received_to_read_part_2));

    EXPECT_CALL(gatt_queue, ReadMultiCharacteristic(_, _, _, _))

            .WillOnce(SaveArg<1>(&received_to_read_part_3));

    EXPECT_CALL(gatt_queue, ReadCharacteristic(_, _, _, _))

            .WillOnce(SaveArg<1>(&audio_output_description_1));

    EXPECT_CALL(gatt_queue, ReadCharacteristic(_, _, _, _))

            .WillOnce(SaveArg<1>(&audio_output_description_2));

    EXPECT_CALL(gatt_queue, ReadCharacteristic(_, _, _, _))

            .WillOnce(SaveArg<1>(&audio_input_description_1));

    EXPECT_CALL(gatt_queue, ReadCharacteristic(_, _, _, _))

            .WillOnce(SaveArg<1>(&audio_input_description_2));

  }

  EXPECT_CALL(gatt_queue, WriteDescriptor(_, _, _, GATT_WRITE, _, _)).Times(0);

  EXPECT_CALL(gatt_interface, RegisterForNotifications(_, _, _)).Times(0);
@@ -738,6 +762,12 @@ TEST_F(VolumeControlDeviceTest, test_enqueue_remaining_requests_multiread) { 


  ASSERT_EQ(expected_to_read_part_1.num_attr, received_to_read_part_1.num_attr);

  ASSERT_EQ(expected_to_read_part_2.num_attr, received_to_read_part_2.num_attr);

  ASSERT_EQ(expected_to_read_part_3.num_attr, received_to_read_part_3.num_attr);



  EXPECT_EQ(expected_audio_input_description_1, audio_input_description_1);

  EXPECT_EQ(expected_audio_input_description_2, audio_input_description_2);

  EXPECT_EQ(expected_audio_output_description_1, audio_output_description_1);

  EXPECT_EQ(expected_audio_output_description_2, audio_output_description_2);

}



TEST_F(VolumeControlDeviceTest, test_check_link_encrypted) {

system/bta/vc/vc.cc

+18 −4
Original line number Diff line number Diff line
@@ -176,7 +176,7 @@ public: 
  }



  void OnGattConnected(tGATT_STATUS status, tCONN_ID connection_id, tGATT_IF /*client_if*/,

                       RawAddress address, tBT_TRANSPORT transport, uint16_t /*mtu*/) {

                       RawAddress address, tBT_TRANSPORT transport, uint16_t mtu) {

    bluetooth::log::info("{}, conn_id=0x{:04x}, transport={}, status={}(0x{:02x})", address,

                         connection_id, bt_transport_text(transport), gatt_status_text(status),

                         status);
@@ -202,6 +202,7 @@ public: 
    }



    device->connection_id = connection_id;

    device->mtu_ = mtu;



    /* Make sure to remove device from background connect.

     * It will be added back if needed, when device got disconnected
@@ -275,6 +276,15 @@ public: 
    ClearDeviceInformationAndStartSearch(device);

  }



  void OnMtuChanged(tCONN_ID conn_id, uint16_t mtu) {

    VolumeControlDevice* device = volume_control_devices_.FindByConnId(conn_id);

    if (!device) {

      bluetooth::log::error("Skipping unknown device conn_id: {}", conn_id);

      return;

    }

    device->mtu_ = mtu;

  }



  void OnServiceDiscDoneEvent(const RawAddress& address) {

    VolumeControlDevice* device = volume_control_devices_.FindByAddress(address);

    if (!device) {
@@ -1461,6 +1471,7 @@ private: 
    device->Disconnect(gatt_if_);



    RemoveDeviceFromOperationList(device->address);

    device->mtu_ = GATT_DEF_BLE_MTU_SIZE;



    if (notify) {

      callbacks_->OnConnectionState(ConnectionState::DISCONNECTED, device->address);
@@ -1545,6 +1556,10 @@ private: 
        OnServiceChangeEvent(p_data->service_changed.remote_bda);

        break;



      case BTA_GATTC_CFG_MTU_EVT:

        OnMtuChanged(p_data->cfg_mtu.conn_id, p_data->cfg_mtu.mtu);

        break;



      case BTA_GATTC_SRVC_DISC_DONE_EVT:

        OnServiceDiscDoneEvent(p_data->service_discovery_done.remote_bda);

        break;
@@ -1599,14 +1614,13 @@ private: 
      instance->OnCharacteristicValueChanged(conn_id, status, hdl, len, ptr,

                                             ((index == (handles.num_attr - 1)) ? data : nullptr),

                                             false);



      position += len + 2; /* skip the length of data */

      index++;

    }



    if (handles.num_attr - 1 != index) {

    if (handles.num_attr != index) {

      bluetooth::log::warn("Attempted to read {} handles, but received just {} values",

                           +handles.num_attr, index + 1);

                           +handles.num_attr, index);

    }

  }

};