Skip to content

BLE: Fix GattServer callbacks not being copied to GattServer instance of the service #14084

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.

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 2 commits into from
Jan 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 7 additions & 3 deletions connectivity/FEATURE_BLE/include/ble/GattServer.h
Original file line number Diff line number Diff line change
Expand Up @@ -316,14 +316,18 @@ class GattServer {
*
* The process assigns a unique attribute handle to all the elements added
* into the attribute table. This handle is an ID that must be used for
* subsequent interractions with the elements.
* subsequent interactions with the elements.
*
* @note There is no mirror function that removes a single service.
* Application code can remove all the registered services by calling
* reset().
*
* @attention Service, characteristics and descriptors objects registered
* within the GattServer must remain reachable until reset() is called.
* @attention GattServer allocates its own memory for all the attributes.
* The GattServer will set the handles on the service passed in and the
* characteristics it contains. You may record the handles you want to
* interact with in the future. After that the service and characteristics
* you passed in as the parameter may be freed. To write to the GattServer
* instances of the characteristics you have to use the saved handles.
*
* @param[in] service The service to be added; attribute handle of services,
* characteristic and characteristic descriptors are updated by the
Expand Down
77 changes: 52 additions & 25 deletions connectivity/FEATURE_BLE/source/cordio/source/GattServerImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -416,11 +416,34 @@ ble_error_t GattServer::insert_characteristic_value_attribute(
characteristic->isReadAuthorizationEnabled() ||
characteristic->isWriteAuthorizationEnabled()
) {
if (_auth_char_count >= MBED_CONF_BLE_API_IMPLEMENTATION_MAX_CHARACTERISTIC_AUTHORISATION_COUNT) {
if (_auth_callbacks_count >= MBED_CONF_BLE_API_IMPLEMENTATION_MAX_CHARACTERISTIC_AUTHORISATION_COUNT) {
return BLE_ERROR_NO_MEM;
}
_auth_char[_auth_char_count] = characteristic;
++_auth_char_count;

char_auth_callback *new_cb = (char_auth_callback *) alloc_block(sizeof(char_auth_callback));

if (!new_cb) {
return BLE_ERROR_NO_MEM;
}

new_cb->read_cb = characteristic->readAuthorizationCallback;
new_cb->write_cb = characteristic->writeAuthorizationCallback;
new_cb->handle = characteristic->getValueHandle();
new_cb->update_security = characteristic->getUpdateSecurityRequirement();
new_cb->_next = nullptr;

/* add it to the list */
if (_auth_callbacks) {
char_auth_callback *last_cb = _auth_callbacks;
while (last_cb->_next) {
last_cb = last_cb->_next;
}
last_cb->_next = new_cb;
} else {
_auth_callbacks = new_cb;
};

++_auth_callbacks_count;
}

++attribute_it;
Expand Down Expand Up @@ -941,7 +964,8 @@ ble_error_t GattServer::reset(ble::GattServer* server)
currentHandle = 0;
cccd_cnt = 0;

_auth_char_count = 0;
_auth_callbacks_count = 0;
_auth_callbacks = nullptr;
Copy link
Member Author

Choose a reason for hiding this comment

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

memory is freed elsewhere (line 954)


AttsCccRegister(cccd_cnt, (attsCccSet_t *) cccds, cccd_cb);

Expand Down Expand Up @@ -978,8 +1002,8 @@ uint8_t GattServer::atts_read_cb(
attsAttr_t *pAttr
)
{
GattCharacteristic *auth_char = getInstance().get_auth_char(handle);
if (auth_char && auth_char->isReadAuthorizationEnabled()) {
char_auth_callback *auth_cb = getInstance().get_auth_callback(handle);
if (auth_cb && auth_cb->read_cb) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a bit fan of pointer to bool coersion.
I'd prefer a

Suggested change
if (auth_cb && auth_cb->read_cb) {
if ((auth_cb != NULL) && (auth_cb->read_cb != NULL)) {

GattReadAuthCallbackParams read_auth_params = {
connId,
handle,
Expand All @@ -989,9 +1013,10 @@ uint8_t GattServer::atts_read_cb(
AUTH_CALLBACK_REPLY_SUCCESS
};

GattAuthCallbackReply_t ret = auth_char->authorizeRead(&read_auth_params);
if (ret != AUTH_CALLBACK_REPLY_SUCCESS) {
return ret & 0xFF;
auth_cb->read_cb.call(&read_auth_params);
Copy link
Member

Choose a reason for hiding this comment

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

I don't know what FunctionPointerWithContext does, but the test above followed by this by-value access to the member is a bit confusing.


if (read_auth_params.authorizationReply != AUTH_CALLBACK_REPLY_SUCCESS) {
return read_auth_params.authorizationReply & 0xFF;
}

pAttr->pValue = read_auth_params.data;
Expand Down Expand Up @@ -1021,8 +1046,8 @@ uint8_t GattServer::atts_write_cb(
attsAttr_t *pAttr
)
{
GattCharacteristic* auth_char = getInstance().get_auth_char(handle);
if (auth_char && auth_char->isWriteAuthorizationEnabled()) {
char_auth_callback* auth_cb = getInstance().get_auth_callback(handle);
if (auth_cb && auth_cb->write_cb) {
GattWriteAuthCallbackParams write_auth_params = {
connId,
handle,
Expand All @@ -1032,9 +1057,10 @@ uint8_t GattServer::atts_write_cb(
AUTH_CALLBACK_REPLY_SUCCESS
};

GattAuthCallbackReply_t ret = auth_char->authorizeWrite(&write_auth_params);
if (ret!= AUTH_CALLBACK_REPLY_SUCCESS) {
return ret & 0xFF;
auth_cb->write_cb.call(&write_auth_params);

if (write_auth_params.authorizationReply != AUTH_CALLBACK_REPLY_SUCCESS) {
return write_auth_params.authorizationReply & 0xFF;
}
}

Expand Down Expand Up @@ -1329,14 +1355,16 @@ void *GattServer::alloc_block(size_t block_size)
return block->data;
}

GattCharacteristic *GattServer::get_auth_char(uint16_t value_handle)
GattServer::char_auth_callback *GattServer::get_auth_callback(uint16_t value_handle)
{
for (size_t i = 0; i < _auth_char_count; ++i) {
if (_auth_char[i]->getValueHandle() == value_handle) {
return _auth_char[i];
GattServer::char_auth_callback* current = _auth_callbacks;
while (current) {
if (current->handle == value_handle) {
break;
}
current = current->_next;
}
return nullptr;
return current;
}

bool GattServer::get_cccd_index_by_cccd_handle(GattAttribute::Handle_t cccd_handle, uint8_t &idx) const
Expand Down Expand Up @@ -1364,13 +1392,12 @@ bool GattServer::is_update_authorized(
GattAttribute::Handle_t value_handle
)
{
GattCharacteristic *auth_char = get_auth_char(value_handle);
if (!auth_char) {
char_auth_callback *auth_cb = get_auth_callback(value_handle);
if (!auth_cb) {
return true;
}

att_security_requirement_t sec_req =
auth_char->getUpdateSecurityRequirement();
const att_security_requirement_t sec_req = auth_cb->update_security;

if (sec_req == att_security_requirement_t::NONE) {
return true;
Expand Down Expand Up @@ -1426,8 +1453,8 @@ GattServer::GattServer() :
cccd_values(),
cccd_handles(),
cccd_cnt(0),
_auth_char(),
_auth_char_count(0),
_auth_callbacks(nullptr),
_auth_callbacks_count(0),
generic_access_service(),
generic_attribute_service(),
registered_service(nullptr),
Expand Down
19 changes: 16 additions & 3 deletions connectivity/FEATURE_BLE/source/cordio/source/GattServerImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,19 @@ class GattServer : public PalSigningMonitor {
GapAdvertisingData::Appearance getAppearance();

#endif // Disabled until reworked and reintroduced to GattServer API
private:
struct char_auth_callback {
/** The registered callback handler for read authorization reply. */
FunctionPointerWithContext<GattReadAuthCallbackParams *> read_cb;
/** The registered callback handler for write authorization reply. */
FunctionPointerWithContext<GattWriteAuthCallbackParams *> write_cb;
/** built in list */
char_auth_callback *_next = nullptr;
/** Characteristic handle the callbacks belong to. */
ble::attribute_handle_t handle = 0;
/** security requirement of update operations */
ble::att_security_requirement_t update_security = ble::att_security_requirement_t::NONE;
};

public:
/**
Expand Down Expand Up @@ -274,7 +287,7 @@ class GattServer : public PalSigningMonitor {

void *alloc_block(size_t block_size);

GattCharacteristic *get_auth_char(uint16_t value_handle);
char_auth_callback *get_auth_callback(uint16_t value_handle);

bool get_cccd_index_by_cccd_handle(GattAttribute::Handle_t cccd_handle, uint8_t &idx) const;

Expand Down Expand Up @@ -354,8 +367,8 @@ class GattServer : public PalSigningMonitor {
uint16_t cccd_handles[MBED_CONF_BLE_API_IMPLEMENTATION_MAX_CCCD_COUNT];
uint8_t cccd_cnt;

GattCharacteristic *_auth_char[MBED_CONF_BLE_API_IMPLEMENTATION_MAX_CHARACTERISTIC_AUTHORISATION_COUNT];
uint8_t _auth_char_count;
char_auth_callback *_auth_callbacks;
uint8_t _auth_callbacks_count;

struct {
attsGroup_t service;
Expand Down