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

[fanoush]

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

[gfwilliams]

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

[fanoush]

assuming this doesn't break stuff?

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

[gfwilliams]

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 :)

[fanoush]

So I reworked it to reuse the jsble_advertising_update also when starting the advertising. I tested with 52840 S140 6.1.1 and 52832 S132 3.1.0. I also tried to set array of advertising data so that it changes. I also tried to set the connectable and scannable to false. For some reason when setting scannable true, connectable false it does look in nrfconnect as connectable. Maybe it is BLE thing and invalid combination? Anyway it seems to do the same before the change. Only connectable:false,scannable:false looks as not connectable but then I could not set non empty scan response, empty one worked NRF.setScanResponse([]). So not sure if you can have non connectable device with scan response to keep more data there.

AFAIK the change in behaviour in this PR is

• non_conectable EDIT: non_scannable now does not set scan response for all SD versions, previously only >5
• NRF.setScanResponse now does not start/stop advertising (and seem to work without it), it also does not take advertising data from jswrap_ble_getCurrentAdvertisingData for SD>5 but keeps previous static buffer

[gfwilliams]

Thanks! That looks like a massive improvement.

My only worry is:

non_connectable now does not set scan response for all SD versions, previously only >5

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+)

[fanoush]

Is there any way to change that easily?

oh, ok, I asked about that change here https://github.com/espruino/Espruino/pull/2367#discussion_r1190792913 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. It is true that NRF.setScanResponse call with nonempty array does indeed throw ERR 07 on SD6.x in non scannable case and it did not show that arror on 52832 but still I think the scanresponse data was not there, will check again.

[gfwilliams]

Ahh sorry - I thought that was about non_scannable, but you were talking about non_connectable?

I just tried NRF.setAdvertising({},{connectable:false, scannable:true}); on an nRF52DK with SDK12 and it appears to work. There is scan data, and while there is a 'Connect' button still, if you click it you can't actually connect.

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).

[fanoush]

I thought that was about non_scannable, but you were talking about non_connectable?

oh sorry, my fault, I meant this part https://github.com/espruino/Espruino/pull/2367/files#diff-8f0dae0b60b86ecb69f6cefe4e62627d352767d25e0dd09f422b58a680406865R2633 so really non_scannable variable

[fanoush]

so 2.18 is out, maybe it is time to get this in? is current code ok?

My only worry is:

non_connectable now does not set scan response for all SD versions, previously only >5

it was my mistake, it is actually non_scannable now does not set scan response for all SD versions, previously only >5, is that OK? It is about line mentioned in previous comment , when non_scannable is true it uses NULL,0 for scan response parameters

[gfwilliams]

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