-
-
Notifications
You must be signed in to change notification settings - Fork 739
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
for SD>5 use static buffers for advertising and scan response data #2367
Conversation
S140 6.1.x and up no longer makes its own copy of advertising data, provided buffers need to be valid all the time, also any change needs new set of buffers (otherwise it fails with NRF_ERROR_INVALID_STATE)
targets/nrf5x/bluetooth.c
Outdated
} | ||
|
||
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good - thanks!
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Thanks! Looks really good to me - happy to merge as-is (assuming this doesn't break stuff?) but it could also be a good opportunity to really tidy this up |
I tested only nrf52840 with SD 6.0.0 and 6.1.1 so far, so not really sure yet, I also tried only whether device is still connectable and then tried NRF.getAdvertisingData and used changed array from that for NRF.setAdvertising. I did not actually try setScanResponse as there is no getScanResponse to get sensible data, I will test a bit more |
Looks good - thanks! Let me know when you've had a chance to have a bit of a play with it and are happy and I'll merge. What I really need to do is come up with an automated test suite for Bluetooth that can be run on all the different device builds, and then I would feel a lot more confident about making some big changes to clean stuff up :) |
So I reworked it to reuse the jsble_advertising_update also when starting the advertising. AFAIK the change in behaviour in this PR is
|
Thanks! That looks like a massive improvement. My only worry is:
Is there any way to change that easily? I can definitely imagine a case where someone wants to have a beacon where it broadcasts data (and a scan response) but it's not connectable. Since the majority of people using advertising with Espruino are still going to be on SDK 12 (API<5?) I imagine they'd still expect the old behaviour (which I think worked on those builds? Just not on SDK15+) |
oh, ok, I asked about that change here #2367 (comment) was not sure why there is such code, BTW it does not affect NRF.setResponse so maybe it still covers your case? Only when starting advertising it does that, can you actually provide scan response to that? And as mentioned in previous comment this combination does not work properly, at least nrfconnect still shows this as connectable in flags and shows connect button even with current firmware so not sure if this is even valid combination. Anyway I can ifdef that bit too if needed. |
Ahh sorry - I thought that was about non_scannable, but you were talking about non_connectable? I just tried I'm not 100% sure what could be done to make it advertise as not connectable since as far as I can tell from the advertising data, it's not advertising anything saying it's connectable or not (there's nothing in the BLE flags for it). |
oh sorry, my fault, I meant this part https://github.com/espruino/Espruino/pull/2367/files#diff-8f0dae0b60b86ecb69f6cefe4e62627d352767d25e0dd09f422b58a680406865R2633 so really |
so 2.18 is out, maybe it is time to get this in? is current code ok?
it was my mistake, it is actually |
Ahh - thanks! Yes, that makes more sense - yes, if it's nonscannable we shouldn't have to worry about scan response. Thanks for this! I'll merge and hopefully we'll have some time to see if any issues surface |
S140 6.1.x and up no longer makes its own copy of advertising data, provided buffers need to be valid all the time, also any change needs new set of buffers (otherwise it fails with NRF_ERROR_INVALID_STATE)
fixes #2000