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

Jump to bottom

for SD>5 use static buffers for advertising and scan response data #2367

Merged
merged 4 commits into from
May 26, 2023
Merged

for SD>5 use static buffers for advertising and scan response data #2367

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a6c4118
for SD>5 use static buffers for advertising and scan response data
fanoush May 10, 2023
00396b5
jsble_advertising_update_scanresponse stub for ESP32
fanoush May 10, 2023
80eb114
Merge branch 'master' into f-SD6.1-advertising
gfwilliams May 10, 2023
d29146f
reuse jsble_advertising_update also for advertising start
fanoush May 11, 2023
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
1 change: 1 addition & 0 deletions libs/bluetooth/bluetooth.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ void jsble_restart_softdevice(JsVar *jsFunction);

uint32_t jsble_advertising_start();
uint32_t jsble_advertising_update_advdata(char *dPtr, unsigned int dLen);
uint32_t jsble_advertising_update_scanresponse(char *dPtr, unsigned int dLen);
void jsble_advertising_stop();

/** Is BLE connected to any device at all? */
Expand Down
20 changes: 1 addition & 19 deletions libs/bluetooth/jswrap_bluetooth.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1164,25 +1164,7 @@ void jswrap_ble_setScanResponse(JsVar *data) {
jsvObjectSetOrRemoveChild(execInfo.hiddenRoot, BLE_NAME_SCAN_RESPONSE_DATA, data);

#ifdef NRF5X
#if NRF_SD_BLE_API_VERSION<5
err_code = sd_ble_gap_adv_data_set(NULL, 0, (uint8_t *)respPtr, respLen);
#else
extern uint8_t m_adv_handle;
// Get existing advertising data as on SDK15 we have to be able to re-supply this
// when changing the scan response
JsVar *advData = jswrap_ble_getCurrentAdvertisingData();
JSV_GET_AS_CHAR_ARRAY(advPtr, advLen, advData);

ble_gap_adv_data_t d;
d.adv_data.p_data = (uint8_t *)advPtr;
d.adv_data.len = advLen;
d.scan_rsp_data.p_data = (uint8_t *)respPtr;
d.scan_rsp_data.len = respLen;
if (bleStatus & BLE_IS_ADVERTISING) sd_ble_gap_adv_stop(m_adv_handle);
err_code = sd_ble_gap_adv_set_configure(&m_adv_handle, &d, NULL);
if (bleStatus & BLE_IS_ADVERTISING) sd_ble_gap_adv_start(m_adv_handle, APP_BLE_CONN_CFG_TAG);
jsvUnLock(advData);
#endif
err_code=jsble_advertising_update_scanresponse((uint8_t *)respPtr, respLen);
Copy link
Member

Choose a reason for hiding this comment

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

Any chance we could just remove the NRF5X define as well, and just dump a jsble_advertising_update_scanresponse placeholder in for ESP32?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, could do that, I can just put that DEAD FIXME stuff into esp32 implementation of that method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, but it is reported as conflict as I branched from older version and next unrelated lines below are different, not sure how to fix that except moving it elsewhere, I don't want to update this branch now, I'll try to move it

Copy link
Member

Choose a reason for hiding this comment

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

no problem - i merged it here (I hope) - github actions seem broken at the moment

#else
err_code = 0xDEAD;
jsiConsolePrintf("FIXME\n");
Expand Down
74 changes: 57 additions & 17 deletions targets/nrf5x/bluetooth.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2419,6 +2419,13 @@ void jsble_setup_advdata(ble_advdata_t *advdata) {
advdata->flags = BLE_GAP_ADV_FLAGS_LE_ONLY_GENERAL_DISC_MODE;
}

#if NRF_SD_BLE_API_VERSION>5
static ble_gap_adv_data_t m_ble_gap_adv_data;
// SoftDevice >= 6.1.0 needs this as static buffers
static uint8_t m_adv_data[BLE_GAP_ADV_SET_DATA_SIZE_MAX];
static uint8_t m_scan_rsp_data[BLE_GAP_ADV_SET_DATA_SIZE_MAX]; // 31
#endif

uint32_t jsble_advertising_start() {
// try not to call from IRQ as we might want to allocate JsVars
if (bleStatus & BLE_IS_ADVERTISING) return 0;
Expand Down Expand Up @@ -2524,16 +2531,18 @@ uint32_t jsble_advertising_start() {

//jsiConsolePrintf("adv_data_set %d %d\n", advPtr, advLen);
#if NRF_SD_BLE_API_VERSION>5
ble_gap_adv_data_t d;
memset(&d, 0, sizeof(d));
d.adv_data.p_data = (uint8_t*)advPtr;
d.adv_data.len = advLen;
advLen=MIN(advLen,BLE_GAP_ADV_SET_DATA_SIZE_MAX);
memset(&m_ble_gap_adv_data, 0, sizeof(m_ble_gap_adv_data));
memcpy(m_adv_data, advPtr, advLen);
m_ble_gap_adv_data.adv_data.p_data = m_adv_data;
m_ble_gap_adv_data.adv_data.len = advLen;
if (!non_scannable) {
d.scan_rsp_data.p_data = m_enc_scan_response_data;
d.scan_rsp_data.len = m_enc_scan_response_data_len;
memcpy(m_scan_rsp_data, m_enc_scan_response_data, m_enc_scan_response_data_len);
m_ble_gap_adv_data.scan_rsp_data.p_data = m_scan_rsp_data;
m_ble_gap_adv_data.scan_rsp_data.len = m_enc_scan_response_data_len;
}

err_code = sd_ble_gap_adv_set_configure(&m_adv_handle, &d, &adv_params);
err_code = sd_ble_gap_adv_set_configure(&m_adv_handle, &m_ble_gap_adv_data, &adv_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 know this is the first use so jsble_advertising_update wouldn't work as-is, but it'd be nice if we could find a way to remove the duplication here (and in the ifdefs below where we do sd_ble_gap_adv_data_set as well).

I guess if we m_ble_gap_adv_data was all zero'd out it'd be good enough.

Copy link
Contributor Author

@fanoush fanoush May 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

m_ble_gap_adv_data is in bss so is zeroed out on startup but then it is not after stopping advertising or any time later. BTW this one does not need to be static for SD>6.1 to work but it keeps enough data for making the temporary copy and when you want to set just the other half. so maybe keeping it as js variable is still an option too (which I didn't like much, I'd better get rid of those variables than this structure)

Copy link
Member

Choose a reason for hiding this comment

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

Leaving the structure is fine by me - also the JS vars do get tricky when we're trying to configure stuff as we tear down and restart the JS engine :)

What I meant was just whether we could use jsble_advertising_update here again to reduce some duplication since they are almost identical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, got it, but don't know how, it does set it twice with temporary copy because the guess is that it is already advertising at that point but that may not be true. when properly knowing it is not advertising it could skip that and then it could be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could probably use bleStatus & BLE_IS_ADVERTISING, will test

Copy link
Member

Choose a reason for hiding this comment

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

Or I was thinking if both the lengths of data are zero (eg m_ble_gap_adv_data was memset(0) or cleared by reboot) then it could assume there was no need for that first call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

had a look yesterday but did not finish, there is also the adv_params bit for sd_ble_gap_adv_set_configure which needs to be set only if not advertising and there is also the if (!non_scannable) which does not set response only for SD>5 (should be there for all versions?). But anyway it is definitely worth reusing, I found out my newly added code is actually not used in typical NRF.GetAdvertisingData, NRF.setAdvertising() calls, it is only called from interrupt when having multiple advertising packets so after reuse it will be.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good - thanks!

jsble_check_error(err_code);
if (!err_code) {
jsble_check_error(sd_ble_gap_tx_power_set(BLE_GAP_TX_POWER_ROLE_ADV, m_adv_handle, m_tx_power));
Expand All @@ -2559,21 +2568,52 @@ uint32_t jsble_advertising_start() {
return err_code;
}

uint32_t jsble_advertising_update_advdata(char *dPtr, unsigned int dLen) {
uint32_t jsble_advertising_update(char *advPtr, unsigned int advLen,char *rspPtr, unsigned int rspLen) {
#if NRF_SD_BLE_API_VERSION>5
ble_gap_adv_data_t d;
d.adv_data.p_data = (uint8_t *)dPtr;
d.adv_data.len = dLen;
d.scan_rsp_data.p_data = NULL;
d.scan_rsp_data.len = 0;

// TODO: scan_rsp_data? Does not setting this remove it?
return sd_ble_gap_adv_set_configure(&m_adv_handle, &d, NULL);
// first we need to switch away from our static live advertising data buffers
// otherwise we would get NRF_ERROR_INVALID_STATE from sd_ble_gap_adv_set_configure
ble_gap_adv_data_t tmp;
uint8_t tmp_adv_data[BLE_GAP_ADV_SET_DATA_SIZE_MAX];
uint8_t tmp_scan_rsp_data[BLE_GAP_ADV_SET_DATA_SIZE_MAX];

tmp.adv_data.len = m_ble_gap_adv_data.adv_data.len;
if (tmp.adv_data.len){
memcpy(tmp_adv_data, m_adv_data, tmp.adv_data.len);
tmp.adv_data.p_data = tmp_adv_data;
} else tmp.adv_data.p_data = NULL;

tmp.scan_rsp_data.len = m_ble_gap_adv_data.scan_rsp_data.len;
if (tmp.scan_rsp_data.len){
memcpy(tmp_scan_rsp_data, m_scan_rsp_data, tmp.scan_rsp_data.len);
tmp.scan_rsp_data.p_data = tmp_scan_rsp_data;
} else tmp.scan_rsp_data.p_data = NULL;

sd_ble_gap_adv_set_configure(&m_adv_handle, &tmp, NULL);

// now we can modify data and switch back to it
if (advLen){
advLen=MIN(advLen,BLE_GAP_ADV_SET_DATA_SIZE_MAX);
memcpy(m_adv_data,advPtr,advLen);
m_ble_gap_adv_data.adv_data.len = advLen;
}
if (rspLen){
rspLen=MIN(rspLen,BLE_GAP_ADV_SET_DATA_SIZE_MAX);
memcpy(m_scan_rsp_data, rspPtr, rspLen);
m_ble_gap_adv_data.scan_rsp_data.len = rspLen;
}
return sd_ble_gap_adv_set_configure(&m_adv_handle, &m_ble_gap_adv_data, NULL);
#else
return sd_ble_gap_adv_data_set((uint8_t *)dPtr, dLen, NULL, 0);
return sd_ble_gap_adv_data_set((uint8_t *)advPtr, advLen, (uint8_t *)rspPtr, rspLen);
#endif
}

uint32_t jsble_advertising_update_advdata(char *dPtr, unsigned int dLen) {
return jsble_advertising_update(dPtr,dLen,NULL,0);
}
uint32_t jsble_advertising_update_scanresponse(char *dPtr, unsigned int dLen) {
return jsble_advertising_update(NULL,0,dPtr,dLen);
}

void jsble_advertising_stop() {
if (!(bleStatus & BLE_IS_ADVERTISING)) return;
#if NRF_SD_BLE_API_VERSION > 5
Expand Down