RFC: ble: utils: Add some utils when advertising only Hubble data#41
RFC: ble: utils: Add some utils when advertising only Hubble data#41ceolin wants to merge 4 commits intoHubbleNetwork:mainfrom
Conversation
Add a helper macro that provides a Hubble BLE payload ready to advertise. Signed-off-by: Flavio Ceolin <flavio@hubblenetwork.com>
Simplify the BLE logic using the recently added macro, HUBBLE_BLE_ADV_*. Signed-off-by: Flavio Ceolin <flavio@hubblenetwork.com>
Simplify the BLE logic using the recently added macro, HUBBLE_BLE_ADV_*. Signed-off-by: Flavio Ceolin <flavio@hubblenetwork.com>
Simplify the BLE logic using the recently added macro, HUBBLE_BLE_ADV_*. Signed-off-by: Flavio Ceolin <flavio@hubblenetwork.com>
|
|
||
| #include <zephyr/bluetooth/bluetooth.h> | ||
|
|
||
| #define HUBBLE_BLE_UTILS_BUFFER_SIZE 25 |
There was a problem hiding this comment.
I'm wondering if I'd be better if there are headers common to both zephyr and other platforms. It might be confusing when on zephyr the utils buffer is 25 and others are 31. Maybe something like this:
#define BLE_ADV_LEN 31
#define HUBBLE_BLE_ADV_HEADER_SIZE 6
#ifdef __ZEPHYR__
#define HUBBLE_BLE_UTILS_BUFFER_SIZE (BLE_ADV_LEN - HUBBLE_BLE_ADV_HEADER_SIZE)
#endif
something like that.
There was a problem hiding this comment.
So, that is the thing this symbol does not need to be exported and should be irrelevant for the user, Zephyr will never need 31 bytes but others yes since we return the raw buffer. I could make it a config option changing the default according with the system but it is one more symbol to deal with. Alternatively we can make explicit it is internal ...
| ({ \ | ||
| int _name##_err; \ | ||
| memset(&_name[6], 0, HUBBLE_BLE_UTILS_BUFFER_SIZE - 6); \ | ||
| *(_out_len) = HUBBLE_BLE_UTILS_BUFFER_SIZE - 6; \ |
There was a problem hiding this comment.
similar to my comment on common constants. Instead of the magic number 6, we could have done like:
*(_out_len) = HUBBLE_BLE_UTILS_BUFFER_SIZE - HUBBLE_BLE_ADV_HEADER_SIZE;
or maybe factor this (HUBBLE_BLE_UTILS_BUFFER_SIZE - HUBBLE_BLE_ADV_HEADER_SIZE) out as constant as well since it's common.
There was a problem hiding this comment.
So ... the problem is that you start to leak symbols that are irrelevant for the application and this is a public header. We need to minimize exported symbols that are not usable by the application. I rather have a comment explaining it than a symbol that is not even used in all ports ...
There was a problem hiding this comment.
ah I forgot about the part that this is a utility header. Yeah I guess an explanation comment would suffice then.
| _name##_err = hubble_ble_advertise_get(_data, _len, &_name[6], \ | ||
| _out_len); \ | ||
| if (_name##_err == 0) { \ | ||
| _name[4] = *(_out_len) + 1; \ |
There was a problem hiding this comment.
maybe also let 4 be #define BLE_SERVICE_DATA_POSTION 4 as well?
There was a problem hiding this comment.
Same answer from above ... is this usable by the application ? So why export it in a public header ? Is it used anywhere else ?
| if (status) { | ||
| return; | ||
| } | ||
| HUBBLE_BLE_ADV_DATA_SET(advData, NULL, 0, &len); |
There was a problem hiding this comment.
this doesn't check return status?
| NULL, 0, &advData[HUBBLE_BLE_ADV_HEADER_SIZE], &len) != 0) { | ||
| return (FAILURE); | ||
| } | ||
| HUBBLE_BLE_ADV_DATA_SET(advData, NULL, 0, &len); |
There was a problem hiding this comment.
same here, no return status check?
| _name[1].data_len = *(_out_len); \ | ||
| _name[1].type = BT_DATA_SVC_DATA16; \ | ||
| _name[1].data = _name##_adv_buf; \ | ||
| *(_out_len) = 2; \ |
There was a problem hiding this comment.
Everything looks good to me except this line. What happened if the bt_entries is more than 2 (like the current zephyr ble_beacon sample)?
There was a problem hiding this comment.
that will not the use case for this macro. I think I have commented but the idea is that this macro is used only when the only advertise service is ours. The reason is because it is not only one more entry but you need to add the service list as well ...
The idea here was to simplify for the easy (maybe common) case where the only service is Hubble. Anyone that needs something more complex should use regular API.
Does it make sense ?
There was a problem hiding this comment.
ok that makes sense. This probably needs to be updated/mentioned in the docs and README as well
2 participants
Add some helpers to facilitate when advertising data only using Hubble service.