Skip to content

Jira 793 Support Characteristic UUID that is not subset of Service, g… #451

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 1, 2017
Merged

Jira 793 Support Characteristic UUID that is not subset of Service, g… #451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions libraries/CurieBLE/src/internal/BLECallbacks.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,23 @@ uint8_t profile_service_read_rsp_process(bt_conn_t *conn,
return ret;
}

uint8_t profile_characteristic_read_rsp_process(bt_conn_t *conn,
int err,
bt_gatt_read_params_t *params,
const void *data,
uint16_t length)
{
BLEDevice bleDevice(bt_conn_get_dst(conn));
BLEServiceImp* service_imp = BLEProfileManager::instance()->getServiceBySubHandle(bleDevice, params->single.handle);

uint8_t ret = service_imp->characteristicReadRspProc(conn,
err,
params,
data,
length);
pr_debug(LOG_MODULE_BLE, "%s-%d:ret-%d", __FUNCTION__, __LINE__, ret);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pr_debug? should this be here? not sure if these prints are disabled or not for release builds

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only for debug. For the release, the log will never output to the terminal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. I tested it out also and saw no serial output.

return ret;
}


void bleConnectEventHandler(bt_conn_t *conn,
Expand Down
5 changes: 5 additions & 0 deletions libraries/CurieBLE/src/internal/BLECallbacks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,11 @@ uint8_t profile_service_read_rsp_process(bt_conn_t *conn,

void ble_on_write_no_rsp_complete(struct bt_conn *conn, uint8_t err,
const void *data);
uint8_t profile_characteristic_read_rsp_process(bt_conn_t *conn,
int err,
bt_gatt_read_params_t *params,
const void *data,
uint16_t length);

#endif

34 changes: 34 additions & 0 deletions libraries/CurieBLE/src/internal/BLEProfileManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,40 @@ BLEServiceImp* BLEProfileManager::service(const BLEDevice &bledevice, int index)
return serviceImp;
}

BLEServiceImp* BLEProfileManager::getServiceBySubHandle(const BLEDevice &bledevice, uint16_t handle) const
{
BLEServiceImp* serviceImp = NULL;
uint16_t start_handle;
uint16_t end_handle;

const BLEServiceLinkNodeHeader* serviceHeader = getServiceHeader(bledevice);
if (NULL == serviceHeader)
{
// Doesn't find the service
return NULL;
}
BLEServiceNodePtr node = serviceHeader->next;

while (node != NULL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lot of extra (slightly confusing) code in lines 524 - 540. It took me a few reads to understand what was actually happening here. This while loop could be greatly simplified, and thus easier on the eyes.....

while (node != NULL) {
    serviceImp = node->value;
    end_handle = serviceImp->endHandle();
    if (handle >= start_handle && handle <= end_handle)
    {
        return serviceImp;
    }
    node = node->next;
}

return NULL;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like use break instead of return. Because we may extend the code in the feature, we don't need add the code in the loop if we use the break. Just add the code at the end of the function. But if you persist in your style, I can update the code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it's OK. I understand your explanation, just looking for the reasoning :)

{
serviceImp = node->value;
start_handle = serviceImp->startHandle();
end_handle = serviceImp->endHandle();
if (handle >= start_handle && handle <= end_handle)
{
break;
}
node = node->next;
}

if (NULL == node)
{
serviceImp = NULL;
}

return serviceImp;
}

void BLEProfileManager::handleConnectedEvent(const bt_addr_le_t* deviceAddr)
{
int index = getUnusedIndex();
Expand Down
1 change: 1 addition & 0 deletions libraries/CurieBLE/src/internal/BLEProfileManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ class BLEProfileManager{
BLEServiceImp* service(const BLEDevice &bledevice, const char * uuid) const;
BLEServiceImp* service(const BLEDevice &bledevice, int index) const;
BLEServiceImp* service(const BLEDevice &bledevice, const bt_uuid_t* uuid) const;
BLEServiceImp* getServiceBySubHandle(const BLEDevice &bledevice, uint16_t handle) const;
int serviceCount(const BLEDevice &bledevice) const;
int characteristicCount(const BLEDevice &bledevice) const;

Expand Down
200 changes: 161 additions & 39 deletions libraries/CurieBLE/src/internal/BLEServiceImp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "BLECharacteristicImp.h"

bt_uuid_16_t BLEServiceImp::_gatt_primary_uuid = {BT_UUID_TYPE_16, BT_UUID_GATT_PRIMARY_VAL};
bt_gatt_read_params_t BLEServiceImp::_read_params;

bt_uuid_t *BLEServiceImp::getPrimayUuid(void)
{
Expand All @@ -36,6 +37,7 @@ BLEServiceImp::BLEServiceImp(BLEService& service):
BLEAttribute(service.uuid(), BLETypeService),
_start_handle(0),
_end_handle(0xFFFF),
_reading(false),
_cur_discover_chrc(NULL)
{
memset(&_characteristics_header, 0, sizeof(_characteristics_header));
Expand All @@ -46,6 +48,7 @@ BLEServiceImp::BLEServiceImp(const bt_uuid_t* uuid):
BLEAttribute(uuid, BLETypeService),
_start_handle(0),
_end_handle(0xFFFF),
_reading(false),
_cur_discover_chrc(NULL)
{
memset(&_characteristics_header, 0, sizeof(_characteristics_header));
Expand Down Expand Up @@ -242,10 +245,17 @@ BLECharacteristicImp* BLEServiceImp::characteristic(const char* uuid)

bool BLEServiceImp::discovering()
{
return (_cur_discover_chrc != NULL);
return (_cur_discover_chrc != NULL || _reading);
}

bool BLEServiceImp::discoverAttributes(BLEDevice* device)
{
return discoverAttributes(device, _start_handle, _end_handle);
}

bool BLEServiceImp::discoverAttributes(BLEDevice* device,
uint16_t start_handle,
uint16_t end_handle)
{
pr_debug(LOG_MODULE_BLE, "%s-%d", __FUNCTION__, __LINE__);
int err;
Expand Down Expand Up @@ -273,8 +283,8 @@ bool BLEServiceImp::discoverAttributes(BLEDevice* device)
return false;
}
temp = &_discover_params;
temp->start_handle = _start_handle;
temp->end_handle = _end_handle;
temp->start_handle = start_handle;
temp->end_handle = end_handle;
temp->uuid = NULL;
temp->type = BT_GATT_DISCOVER_CHARACTERISTIC;
temp->func = profile_discover_process;
Expand Down Expand Up @@ -309,21 +319,34 @@ uint8_t BLEServiceImp::discoverResponseProc(bt_conn_t *conn,
//const bt_uuid_t* chrc_uuid = attr->uuid;
uint16_t chrc_handle = attr->handle + 1;
struct bt_gatt_chrc* psttemp = (struct bt_gatt_chrc*)attr->user_data;
int retval = (int)addCharacteristic(device,
psttemp->uuid,
chrc_handle,
psttemp->properties);
const bt_uuid_t* chrc_uuid = psttemp->uuid;

//pr_debug(LOG_MODULE_BLE, "%s-%d:handle-%d:%d", __FUNCTION__, __LINE__,attr->handle, chrc_handle);
if (BLE_STATUS_SUCCESS != retval)
uint16_t le16;
memcpy(&le16, &BT_UUID_16(chrc_uuid)->val, sizeof(le16));
if (chrc_uuid->type == BT_UUID_TYPE_16 &&
le16 == 0)
{
pr_error(LOG_MODULE_BLE, "%s-%d: Error-%d",
__FUNCTION__, __LINE__, retval);
errno = ENOMEM;
// Read the UUID
readCharacteristic(device, chrc_handle);
retVal = BT_GATT_ITER_CONTINUE;
}
else
{
retVal = BT_GATT_ITER_CONTINUE;
int retval = (int)addCharacteristic(device,
psttemp->uuid,
chrc_handle,
psttemp->properties);

if (BLE_STATUS_SUCCESS != retval)
{
pr_error(LOG_MODULE_BLE, "%s-%d: Error-%d",
__FUNCTION__, __LINE__, retval);
errno = ENOMEM;
}
else
{
retVal = BT_GATT_ITER_CONTINUE;
}
}
}
break;
Expand All @@ -335,8 +358,8 @@ uint8_t BLEServiceImp::discoverResponseProc(bt_conn_t *conn,
if (NULL != _cur_discover_chrc)
{
retVal = _cur_discover_chrc->discoverResponseProc(conn,
attr,
params);
attr,
params);
}
break;
}
Expand All @@ -347,44 +370,143 @@ uint8_t BLEServiceImp::discoverResponseProc(bt_conn_t *conn,
}
}

pr_debug(LOG_MODULE_BLE, "%s-%d:ret-%d",__FUNCTION__, __LINE__, retVal);
//pr_debug(LOG_MODULE_BLE, "%s-%d:ret-%d",__FUNCTION__, __LINE__, retVal);
if (retVal == BT_GATT_ITER_STOP)
{
if (errno == ENOMEM)
{
_cur_discover_chrc = NULL;
return retVal;
}
const BLECharacteristicLinkNodeHeader* chrcHeader = &_characteristics_header;
BLECharacteristicImp* chrcCurImp = NULL;
BLECharacteristicNodePtr node = chrcHeader->next;

pr_debug(LOG_MODULE_BLE, "%s-%d: node-%p",__FUNCTION__, __LINE__, node);
// Discover next service
while (node != NULL)
if (false == _reading)
{
chrcCurImp = node->value;

if (NULL == _cur_discover_chrc)
discoverNextCharacteristic(device);
}
}
return retVal;
}

void BLEServiceImp::discoverNextCharacteristic(BLEDevice &bledevice)
{
const BLECharacteristicLinkNodeHeader* chrcHeader = &_characteristics_header;
BLECharacteristicImp* chrcCurImp = NULL;
BLECharacteristicNodePtr node = chrcHeader->next;

//pr_debug(LOG_MODULE_BLE, "%s-%d: node-%p",__FUNCTION__, __LINE__, node);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented pr_debug? I forgot what our policy is on this. Personally I'd take it out.

// Discover next service
while (node != NULL)
{
chrcCurImp = node->value;

if (NULL == _cur_discover_chrc)
{
bool result = chrcCurImp->discoverAttributes(&bledevice);
pr_debug(LOG_MODULE_BLE, "%s-%d",__FUNCTION__, __LINE__);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pr_debug? should this line be removed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted.

if (result == true)
{
bool result = chrcCurImp->discoverAttributes(&device);
pr_debug(LOG_MODULE_BLE, "%s-%d",__FUNCTION__, __LINE__);
if (result == true)
{
// Record the current discovering service
_cur_discover_chrc = chrcCurImp;
break;
}
// Record the current discovering service
_cur_discover_chrc = chrcCurImp;
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use return here instead of break. The intention is clearer, and it is easier to follow the loop logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as before.

}
else if (_cur_discover_chrc == chrcCurImp)
}
else if (_cur_discover_chrc == chrcCurImp)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to me like perhaps just else would be sufficient here. @sgbihu could you confirm/deny please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not correct. The else-if is necessary, because the code need find the previous characteristic, and find next one by previous characteristic. So this is try to reset it to NULL.

{
// Find next discoverable service
_cur_discover_chrc = NULL;
}
node = node->next;
}
}

bool BLEServiceImp::readCharacteristic(const BLEDevice &bledevice, uint16_t handle)
{
int retval = 0;
bt_conn_t* conn = NULL;

if (true == BLEUtils::isLocalBLE(bledevice))
{
// GATT server can't write
return false;
}

if (_reading)
{
return false;
}

_read_params.func = profile_characteristic_read_rsp_process;
_read_params.handle_count = 1;
_read_params.single.handle = handle - 1;
_read_params.single.offset = 0;

if (0 == _read_params.single.handle)
{
// Discover not complete
return false;
}

conn = bt_conn_lookup_addr_le(bledevice.bt_le_address());
if (NULL == conn)
{
return false;
}
// Send read request
retval = bt_gatt_read(conn, &_read_params);
bt_conn_unref(conn);
if (0 == retval)
{
_reading = true;
}
pr_debug(LOG_MODULE_BLE, "%s-%d", __FUNCTION__, __LINE__);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe all these calls to pr_debug will incur some serial I/O. If this is the case, please remove them..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we disable the debug, the serial I/O will not be called.

return _reading;
}

uint8_t BLEServiceImp::characteristicReadRspProc(bt_conn_t *conn,
int err,
bt_gatt_read_params_t *params,
const void *data,
uint16_t length)
{
_reading = false;
if (NULL == data)
{
return BT_GATT_ITER_STOP;
}
BLEDevice bleDevice(bt_conn_get_dst(conn));

pr_debug(LOG_MODULE_BLE, "%s-%d:length-%d", __FUNCTION__, __LINE__, length);
if (length == UUID_SIZE_128 + 3)
{
const uint8_t* rspdata = (const uint8_t*) data;
bt_uuid_128_t uuid_tmp;
uint16_t chrc_handle = rspdata[1] | (rspdata[2] << 8);
uuid_tmp.uuid.type = BT_UUID_TYPE_128;
memcpy(uuid_tmp.val, &rspdata[3], UUID_SIZE_128);
int retval = (int)addCharacteristic(bleDevice,
(const bt_uuid_t*)&uuid_tmp,
chrc_handle,
rspdata[0]);

if (BLE_STATUS_SUCCESS != retval)
{
pr_error(LOG_MODULE_BLE, "%s-%d: Error-%d",
__FUNCTION__, __LINE__, retval);
errno = ENOMEM;
}
else
{
if (false == discovering())
{
// Find next discoverable service
_cur_discover_chrc = NULL;
if (false == discoverAttributes(&bleDevice, chrc_handle + 1, _end_handle))
{
discoverNextCharacteristic(bleDevice);
}
}
node = node->next;
}
}
return retVal;
pr_debug(LOG_MODULE_BLE, "%s-%d", __FUNCTION__, __LINE__);

return BT_GATT_ITER_STOP;
}


16 changes: 16 additions & 0 deletions libraries/CurieBLE/src/internal/BLEServiceImp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,12 @@ class BLEServiceImp: public BLEAttribute{
uint8_t discoverResponseProc(bt_conn_t *conn,
const bt_gatt_attr_t *attr,
bt_gatt_discover_params_t *params);

uint8_t characteristicReadRspProc(bt_conn_t *conn,
int err,
bt_gatt_read_params_t *params,
const void *data,
uint16_t length);
bool discovering();

static bt_uuid_t *getPrimayUuid(void);
Expand All @@ -81,6 +87,13 @@ class BLEServiceImp: public BLEAttribute{


int updateProfile(bt_gatt_attr_t *attr_start, int& index);

private:
void discoverNextCharacteristic(BLEDevice &bledevice);
bool readCharacteristic(const BLEDevice &bledevice, uint16_t handle);
bool discoverAttributes(BLEDevice* device,
uint16_t start_handle,
uint16_t end_handle);
private:
typedef LinkNode<BLECharacteristicImp *> BLECharacteristicLinkNodeHeader;
typedef LinkNode<BLECharacteristicImp *>* BLECharacteristicNodePtr;
Expand All @@ -89,6 +102,9 @@ class BLEServiceImp: public BLEAttribute{
uint16_t _start_handle;
uint16_t _end_handle;

static bt_gatt_read_params_t _read_params;
bool _reading;

void releaseCharacteristic();
BLECharacteristicImp *_cur_discover_chrc;

Expand Down