Skip to content
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

Bluetooth: Avoid use of array compound literals in API #24829

Merged
merged 2 commits into from
May 2, 2020
Merged

Bluetooth: Avoid use of array compound literals in API #24829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f55cf9a
Bluetooth: test: Fix redefinition of BT_BUF_ACL_SIZE
joerchan Apr 29, 2020
ecfd6d0
Bluetooth: host: Avoid use of array compound literals in API
joerchan Apr 30, 2020
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
66 changes: 46 additions & 20 deletions include/bluetooth/bluetooth.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ struct bt_data {
/** @brief Helper to declare elements of bt_data arrays
*
* This macro is mainly for creating an array of struct bt_data
* elements which is then passed to bt_le_adv_start().
* elements which is then passed to e.g. @ref bt_le_adv_start().
*
* @param _type Type of advertising data field
* @param _data Pointer to the data field payload
Expand All @@ -293,7 +293,7 @@ struct bt_data {
/** @brief Helper to declare elements of bt_data arrays
*
* This macro is mainly for creating an array of struct bt_data
* elements which is then passed to bt_le_adv_start().
* elements which is then passed to e.g. @ref bt_le_adv_start().
*
* @param _type Type of advertising data field
* @param _bytes Variable number of single-byte parameters
Expand Down Expand Up @@ -495,6 +495,25 @@ struct bt_le_adv_param {
const bt_addr_le_t *peer;
};

/** @brief Initialize advertising parameters
*
* @param _options Advertising Options
* @param _int_min Minimum advertising interval
* @param _int_max Maximum advertising interval
* @param _peer Peer address, set to NULL for undirected advertising or
* address of peer for directed advertising.
*/
#define BT_LE_ADV_PARAM_INIT(_options, _int_min, _int_max, _peer) \
{ \
.id = BT_ID_DEFAULT, \
.sid = 0, \
.secondary_max_skip = 0, \
.options = (_options), \
.interval_min = (_int_min), \
.interval_max = (_int_max), \
.peer = (_peer), \
}

/** @brief Helper to declare advertising parameters inline
*
* @param _options Advertising Options
Expand All @@ -504,15 +523,9 @@ struct bt_le_adv_param {
* address of peer for directed advertising.
*/
#define BT_LE_ADV_PARAM(_options, _int_min, _int_max, _peer) \
((struct bt_le_adv_param[]) { { \
.id = BT_ID_DEFAULT, \
.sid = 0, \
.secondary_max_skip = 0, \
.options = (_options), \
.interval_min = (_int_min), \
.interval_max = (_int_max), \
.peer = (_peer), \
} })
((struct bt_le_adv_param[]) { \
BT_LE_ADV_PARAM_INIT(_options, _int_min, _int_max, _peer) \
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't work; the cast is still of a temporary. Using a version of samples/bluetooth/peripheral converted to C++:

/mnt/nordic/zp/zephyr/include/bluetooth/bluetooth.h:526:30: error: taking address of temporary array
  526 |  ((struct bt_le_adv_param[]) { \
      |  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
  527 |   BT_LE_ADV_PARAM_INIT(_options, _int_min, _int_max, _peer) \
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  528 |   })
      |   ~~                          
/mnt/nordic/zp/zephyr/include/bluetooth/bluetooth.h:539:29: note: in expansion of macro 'BT_LE_ADV_PARAM'
  539 | #define BT_LE_ADV_CONN_NAME BT_LE_ADV_PARAM(BT_LE_ADV_OPT_CONNECTABLE | \
      |                             ^~~~~~~~~~~~~~~
../src/main.cxx:260:24: note: in expansion of macro 'BT_LE_ADV_CONN_NAME'
  260 |  err = bt_le_adv_start(BT_LE_ADV_CONN_NAME, ad, ARRAY_SIZE(ad), NULL, 0);
      |                        ^~~~~~~~~~~~~~~~~~~

The diagnostic goes away if the cast type is changed to const struct bt_le_adv_param[]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pabigot Thanks. I didn't add the const-qualifier, because the recommendation stated that was best to avoid these constructs altogether for C++. Which is why I added _INIT macros and removed the use in the static inline functions.
C++ code can then include the header, but it must avoid these specific macros.

If I added const to all the places that used this I get a ripple effect of const qualifier being discarded.

If you change your C++ peripheral like this:

struct bt_le_adv_param param = BT_LE_ADV_PARAM_INIT(BT_LE_ADV_OPT_CONNECTABLE |
						    BT_LE_ADV_OPT_USE_NAME,
						    BT_GAP_ADV_FAST_INT_MIN_2,
						    BT_GAP_ADV_FAST_INT_MAX_2,
						    NULL);
err = bt_le_adv_start(&param, ad, ARRAY_SIZE(ad), NULL, 0);

You should be good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I accept that's a workaround so I'll remove my NAK. It'd be nice to have uses of those macros in C++ code produce a diagnostic, but that's not in scope here.

The Zephyr Bluetooth examples are awash in uses of the original (non-init) macro as an argument to functions. A goal of the original solution was to keep the recommended calling code the same for both C and C++ code. Clearly that's not feasible.

We should probably add or convert one of the bluetooth samples to C++ to show how it's supposed to be done.

FWIW include/bluetooth/gatt.h at BT_GATT_CHARACTERISTIC also has an explicit cast of a temporary compound literal being cast to an array. That may not be as easy to fix, but also need not be part of this PR.

})

#define BT_LE_ADV_CONN_DIR(_peer) BT_LE_ADV_PARAM(BT_LE_ADV_OPT_CONNECTABLE | \
BT_LE_ADV_OPT_ONE_TIME, 0, 0,\
Expand Down Expand Up @@ -878,6 +891,25 @@ struct bt_le_scan_cb {
sys_snode_t node;
};

/** @brief Initialize scan parameters
*
* @param _type Scan Type, BT_LE_SCAN_TYPE_ACTIVE or
* BT_LE_SCAN_TYPE_PASSIVE.
* @param _options Scan options
* @param _interval Scan Interval (N * 0.625 ms)
* @param _window Scan Window (N * 0.625 ms)
*/
#define BT_LE_SCAN_PARAM_INIT(_type, _options, _interval, _window) \
{ \
.type = (_type), \
.options = (_options), \
.interval = (_interval), \
.window = (_window), \
.timeout = 0, \
.interval_coded = 0, \
.window_coded = 0, \
}

/** @brief Helper to declare scan parameters inline
*
* @param _type Scan Type, BT_LE_SCAN_TYPE_ACTIVE or
Expand All @@ -887,15 +919,9 @@ struct bt_le_scan_cb {
* @param _window Scan Window (N * 0.625 ms)
*/
#define BT_LE_SCAN_PARAM(_type, _options, _interval, _window) \
((struct bt_le_scan_param[]) { { \
.type = (_type), \
.options = (_options), \
.interval = (_interval), \
.window = (_window), \
.timeout = 0, \
.interval_coded = 0, \
.window_coded = 0, \
} })
((struct bt_le_scan_param[]) { \
BT_LE_SCAN_PARAM_INIT(_type, _options, _interval, _window) \
})

/** Helper macro to enable active scanning to discover new devices. */
#define BT_LE_SCAN_ACTIVE BT_LE_SCAN_PARAM(BT_LE_SCAN_TYPE_ACTIVE, \
Expand Down
79 changes: 60 additions & 19 deletions include/bluetooth/conn.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,21 @@ struct bt_le_conn_param {
u16_t timeout;
};

/** @brief Initialize connection parameters
*
* @param int_min Minimum Connection Interval (N * 1.25 ms)
* @param int_max Maximum Connection Interval (N * 1.25 ms)
* @param lat Connection Latency
* @param to Supervision Timeout (N * 10 ms)
*/
#define BT_LE_CONN_PARAM_INIT(int_min, int_max, lat, to) \
{ \
.interval_min = (int_min), \
.interval_max = (int_max), \
.latency = (lat), \
.timeout = (to), \
}

/** Helper to declare connection parameters inline
*
* @param int_min Minimum Connection Interval (N * 1.25 ms)
Expand All @@ -47,12 +62,9 @@ struct bt_le_conn_param {
* @param to Supervision Timeout (N * 10 ms)
*/
#define BT_LE_CONN_PARAM(int_min, int_max, lat, to) \
((struct bt_le_conn_param[]) { { \
.interval_min = (int_min), \
.interval_max = (int_max), \
.latency = (lat), \
.timeout = (to), \
} })
((struct bt_le_conn_param[]) { \
BT_LE_CONN_PARAM_INIT(int_min, int_max, lat, to) \
})

/** Default LE connection parameters:
* Connection Interval: 30-50 ms
Expand Down Expand Up @@ -334,21 +346,32 @@ struct bt_conn_le_create_param {
u16_t timeout;
};

/** @brief Initialize create connection parameters
*
* @param _options Create connection options.
* @param _interval Create connection scan interval (N * 0.625 ms).
* @param _window Create connection scan window (N * 0.625 ms).
*/
#define BT_CONN_LE_CREATE_PARAM_INIT(_options, _interval, _window) \
{ \
.options = (_options), \
.interval = (_interval), \
.window = (_window), \
.interval_coded = 0, \
.window_coded = 0, \
.timeout = 0, \
}

/** Helper to declare create connection parameters inline
*
* @param _options Create connection options.
* @param _interval Create connection scan interval (N * 0.625 ms).
* @param _window Create connection scan window (N * 0.625 ms).
*/
#define BT_CONN_LE_CREATE_PARAM(_options, _interval, _window) \
((struct bt_conn_le_create_param[]) { { \
.options = (_options), \
.interval = (_interval), \
.window = (_window), \
.interval_coded = 0, \
.window_coded = 0, \
.timeout = 0, \
} })
((struct bt_conn_le_create_param[]) { \
BT_CONN_LE_CREATE_PARAM_INIT(_options, _interval, _window) \
})

/** Default LE create connection parameters.
* Scan continuously by setting scan interval equal to scan window.
Expand Down Expand Up @@ -393,8 +416,12 @@ struct bt_conn *bt_conn_create_le(const bt_addr_le_t *peer,
const struct bt_le_conn_param *conn_param)
{
struct bt_conn *conn;
struct bt_conn_le_create_param param = BT_CONN_LE_CREATE_PARAM_INIT(
BT_LE_CONN_OPT_NONE,
BT_GAP_SCAN_FAST_INTERVAL,
BT_GAP_SCAN_FAST_INTERVAL);

if (bt_conn_le_create(peer, BT_CONN_LE_CREATE_CONN, conn_param,
if (bt_conn_le_create(peer, &param, conn_param,
&conn)) {
return NULL;
}
Expand Down Expand Up @@ -423,7 +450,12 @@ int bt_conn_le_create_auto(const struct bt_conn_le_create_param *create_param,
__deprecated static inline
int bt_conn_create_auto_le(const struct bt_le_conn_param *conn_param)
{
return bt_conn_le_create_auto(BT_CONN_LE_CREATE_CONN_AUTO, conn_param);
struct bt_conn_le_create_param param = BT_CONN_LE_CREATE_PARAM_INIT(
BT_LE_CONN_OPT_NONE,
BT_GAP_SCAN_FAST_INTERVAL,
BT_GAP_SCAN_FAST_WINDOW);

return bt_conn_le_create_auto(&param, conn_param);
}

/** @brief Stop automatic connect creation.
Expand Down Expand Up @@ -1151,14 +1183,23 @@ struct bt_br_conn_param {
bool allow_role_switch;
};

/** @brief Initialize BR/EDR connection parameters
*
* @param role_switch True if role switch is allowed
*/
#define BT_BR_CONN_PARAM_INIT(role_switch) \
{ \
.allow_role_switch = (role_switch), \
}

/** Helper to declare BR/EDR connection parameters inline
*
* @param role_switch True if role switch is allowed
*/
#define BT_BR_CONN_PARAM(role_switch) \
((struct bt_br_conn_param[]) { { \
.allow_role_switch = (role_switch), \
} })
((struct bt_br_conn_param[]) { \
BT_BR_CONN_PARAM_INIT(role_switch) \
})

/** Default BR/EDR connection parameters:
* Role switch allowed
Expand Down
5 changes: 0 additions & 5 deletions tests/bluetooth/bsim_bt/edtt_ble_test_app/hci_test_app/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,6 @@ static void error_response(int error)
waiting_opcode = 0;
}

#if defined(CONFIG_BT_CTLR_DATA_LENGTH_MAX)
#define BT_BUF_ACL_SIZE BT_L2CAP_BUF_SIZE(CONFIG_BT_CTLR_DATA_LENGTH_MAX)
#else
#define BT_BUF_ACL_SIZE BT_L2CAP_BUF_SIZE(60)
#endif
NET_BUF_POOL_FIXED_DEFINE(hci_cmd_pool, CONFIG_BT_HCI_CMD_COUNT,
BT_BUF_RX_SIZE, NULL);
NET_BUF_POOL_FIXED_DEFINE(hci_data_pool, CONFIG_BT_CTLR_TX_BUFFERS + 4,
Expand Down