coded phy works (better) with S140 6.1.x #2465
Comments
|
BTW, it is probably about extended advertising since even with passive scan the data array returned is 34 bytes so over normal advertising limit of 31 bytes so I guess that is why the https://github.com/espruino/BangleApps/tree/master/apps/shownearby works fine with 6.0.0. and the thermometer is not visible. |
|
Ahh, that's interesting - thanks! I wonder if there isn't something else that could be done to help us receive bigger advertising on the 6.0.0 softdevice, if that's the underlying issue? Sorry but just to check, you say you swapped the softdevice hex and made no other changes to the compile and it was all ok? Definitely moving to the new SD would be good if it does improve the situation - but I'm a bit anxious about doing OTA updates for Bangle.js given how much grief I get from users every time there's even a minor app update :( |
yes except that I am building with 2048 instead of 4096 as mentioned here #2000 (comment) so that internal flash writing is stable on both. Then you can upgrade/downgrade softdevices and it works on both. So we actually don't need to force people to upgrade. But if we put 6.1.1 to the repo and build with that then 6.0.0 won't be tested much.
Not sure if nrf52 to nrf52 (i.e. Bangle2 to Bangle2) could do better with 6.0.0 or it s not there at all, that is another thing that is mentioned in 6.1.0 release notes ( And btw did scanning with phy set to "both" or "2mbps" worked for you at some point? like However with 6.1.x I tried to redefine "both" to use } else if (jsvIsStringEqual(advPhy,"both")) {
m_scan_param.scan_phys = BLE_GAP_PHY_1MBPS|BLE_GAP_PHY_CODED;
if (m_scan_param.interval<m_scan_param.window*2) m_scan_param.interval=m_scan_param.window*2;
m_scan_param.extended = 1;
}with interval being twice the window as suggested in migration guide pdf and this works very nice, I get one list with both normal and coded phy advertisements when scanning. |
|
Thanks - I've just moved to 2048b writes - probably a good idea anyway. Does that affect DFU do you know? If that was writing 4096b then we might be in a situation where you wrote the new softdevice but the device was then unable to update itself? I guess mostly DFU works in 2k blocks so maybe there is no point where it writes more.
That's a tricky one, because when you
I'm not honestly sure - I found it quite hard to test when I was doing it, so maybe it never worked. |
So I was testing this and changed the code here https://github.com/espruino/Espruino/blob/master/targets/nrf5x/bluetooth.c#L2990 so all three bits could be set independently #if NRF_SD_BLE_API_VERSION>5
if (jsvObjectGetBoolChild(options, "extended"))
m_scan_param.extended = 1;
JsVar *advPhy = jsvObjectGetChildIfExists(options, "phy");
if (jsvIsUndefined(advPhy)) {
// default
} else {
char phys[18]; // up to "1mbps,2mbps,coded"
size_t len=jsvGetString(advPhy,phys,18);
phys[len]=0;
int phycount=0;
if (strstr(phys,"2mbps")!=NULL) {
m_scan_param.scan_phys |= BLE_GAP_PHY_2MBPS;
m_scan_param.extended = 1;
phycount++;
}
if (strstr(phys,"1mbps")!=NULL) {
m_scan_param.scan_phys |= BLE_GAP_PHY_1MBPS;
phycount++;
}
if (strstr(phys,"coded")!=NULL) {
m_scan_param.scan_phys |= BLE_GAP_PHY_CODED;
m_scan_param.extended = 1;
phycount++;
}
if (strstr(phys,"both")!=NULL) {
m_scan_param.scan_phys = BLE_GAP_PHY_1MBPS|BLE_GAP_PHY_CODED;
m_scan_param.extended = 1;
phycount=2;
}
if (phycount==0)
jsWarn("Unknown phy %q\n", advPhy);
else
if (m_scan_param.interval<m_scan_param.window*phycount) m_scan_param.interval=m_scan_param.window*phycount;
}
jsvUnLock(advPhy);
#endifand the result is that very few combinations work , only
According to this https://infocenter.nordicsemi.com/topic/com.nordic.infocenter.s140.api.v6.1.1/structble__gap__scan__params__t.html?cp=5_7_4_4_2_1_4_10_6#a369859ca03e34c8220452ae95d1aa02f (while the explanation is a bit confusing) I understand it like this
Maybe some of those combinations may make sense only when connecting, not scanning (as per documentation) I am still not sure about extending the window when scanning more phys. Why for coded the interval should be window*2 as per migration guide and why not for 1+2mbps. I don't know why it would not simply switch PHY in next rounds since e.g. window 100,interval 100, timeout 2000 should cycle through three advertising channels every 100 units so why not change phy then and do it again (in same way for 1mbps+coded and 1+2mbps. Anyway, thank for changing the block size. As for the scanning stuff mentioned above I think it would make sense to redefine Or if "both" is not descriptive enough the code above can work too with 2mbps option removed, but that would break current documentation. I also looked into setAdvertising with coded phy (including enabling
I never had any issue with DFU update on 6.1.x so maybe as you say it does not use such big block. |
|
And BTW the double interval size when scanning for both phys on 6.1.x is needed, I have added overriding window/interval like jsvUnLock(advPhy);
#endif
uint32_t scan_window=jsvObjectGetIntegerChild(options, "window");
if (scan_window>=4 && scan_window<=16384) m_scan_param.window=scan_window;
uint32_t scan_interval=jsvObjectGetIntegerChild(options, "interval");
if (scan_interval>=4&& scan_interval<=16384) m_scan_param.interval=scan_interval;
if (m_scan_param.interval<m_scan_param.window) m_scan_param.interval=m_scan_param.window;
}And when setting scanning to both 1mbps and coded and set interval to less than 2* window (like |
You did mention under "...very few combinations work , only..." that all 3 work, and I believe that's what BLE_GAP_PHYS_SUPPORTED does which is what we use for But maybe that should be Good idea about specify window and interval, I'll add that - I think it needs a MSEC_TO_UNITS around it? |
yes, was just quick hack to have something there as any number is good no matter the unit. also was thinking why we have everything BLE related in milliseconds when the (((TIME) * 1000) / (625)) conversion makes it inexact and there is extra multiplication and division bloating the code but yes people are probably used to miliseconds :-)
Yes but that actually always gives me error. As it is now I get error for |
Yes, and it makes more sense if we support other platforms - the inaccuracy is a bit annoying though I know. Just tested and it doesn't work for me either, so just pushing a change now |
|
Just to follow up with coded phy vs connectable (and scannable). By default connectable+coded phy does not work with current build. Found out it is because of NRF_SDH_BLE_GAP_EVENT_LENGTH being 3 in Another issue is about connectable+scannable - that I found out is invalid combination for coded phy (or extended advertising) in BLE. So now I remember and know why there is no And BTW, there is nice info about it here https://devzone.nordicsemi.com/f/nordic-q-a/47073/general-questions-about-notifications-low-level-ble-packets-and-softdevice-phy-connection-interval-connection-event-length-att-mtu-and-dle What I get from all of this is that if we want to switch device to be connectable over coded phy (worthwhile even for Bangle 2 IMO, I hate when I leave my phone on a desk, go to kitchen and it gets disconnected from gadgetbridge) the best is probably also lowering MTU and restarting SoftDevice to have smaller memory requirements (i,e same as now) because maybe larger MTU over coded phy is not that great idea anyway. So something like we already have for other cases when softdevice restart is needed. And when switching back to normal mode we may have to restart again and increase MTU (and possibly also decrease NRF_SDH_BLE_GAP_EVENT_LENGTH - which should be also tunable at runtime) And as the scannable mode is not supported with coded phy+connectable we would probably also need to support extended advertising with larger size to fit everything in without scan response packet. That can be 'hacked' to keep same size by actually joining the m_adv_data and m_scan_rsp_data to be one single bigger buffer instead of 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]; // 31and reuse that area more dynamically so even with 1mbps with extended advertising one could make device non scannable and have larger advertising packet instead of scan response. So far I only tested connectable+coded phy with larger memory requirements, all other stuff is just ideas for now. I know I probably won't have time for this for next 14 days so just updating this with the info I know. |
|
Sorry about the delay:
That's a pain - so we're using all the RAM for a 131b MTU but actually it's not really needed when doing long range (even though we need the bigger NRF_SDH_BLE_GAP_EVENT_LENGTH?). It looks like a 1.2kb extra RAM (unless we switch at runtime as you say!)
I feel like maybe this should just be enabled by default on Bangle/Jolt.js? Most people aren't going to know to be able to extend it and would really appreciate the long range out of the box. The only devices where we can add it are nRF52840 right now, and on those we do have 256k RAM. While usually I'm pretty precious about it I do wonder whether it's better just to suck up the 1.2k extra used for the sake of making our lives easier? I doubt anyone will miss 0.5% of the RAM.
Honestly I don't have a problem just bumping |
except that flash block size bug that is already fixed it works for me with other nrf52840 devices, same application binary will start with any of them. so I just switch between them interchangeably
If you wish, you can try softdevice DFU packages here There is nothing Magic3 specific in them, they are just nordic dfu softdevice packages that work with nordic DFU bootloader. Quite likely they will just work with Bangle.js/Jolt.js however they will probably overwrite the app and then stay in DFU bootloader so you need to reupload the application zip again over DFU - not sure if that is user friendly enough when compared to the SPI flashing you have there now. So not sure if the SPI file method could be usable/preferable for updating softdevice too. DFU bootloader does some magic with MBR so it is MBR that overwrites softdevice but if current SPI flashing code have softdevice still disabled (or can have), overwriting it may be not much harder than overwriting application. Will answer the rest later. |
|
Thanks! I've just pushed a change which should move up to 6.1.1 builds.
Maybe for people who care about upgrading to get those features? But yes, the SPI flash bootloader may well do it - we do have writing to softdevice enabled I think (I had to so we could self-update the bootloader, since AFAIK you can't turn it off once it's on). I'll have to try and see! |
Maybe it is quickest way to try it and see how much coded phy connection helps with the range and how stable/fast it is. Also how usable and widely supported it is - I am not sure how many new but cheaper phones can do code phy. Recently (can be 2 years already?) I bough two new phones in a family - Motorola G72 and Edge 30, the G72 was lower end and slightly newer than Edge 30 and it still cannot do coded phy, only the Edge 30 (somewhat mid range phone) has coded phy. So they don't put it into every new phone.
Yes that is the easiest too, I am just not sure how practical is scan response at all (in normal phy modes) when you can put all the data into bigger extended advertising packet, so that is why I suggested to join the array as maybe one may want to use scan response only with normal advertising. But for keeping the code as it is now, just enlarging m_adv_data is the way too. The minimum needed for now was just to allow extended advertising packet big enough to fit what we currently put to scan response (the nordic uart GUID?) because coded phy does not allow scan response. And also it is needed to change the code that currently splits the data into the default advertising/scan response to put everything into extended advertising. This is to get the nordic uart connection working. However maybe for official devices where you have device names hardcoded and don't need the UART GUID to be there, even that is not important? So maybe gadgetbridge connection to Bangle named device would work even now over coded phy by having just that enabled without enlarging advertising packet. |
True - and it uses up more battery. My only concern really is the support by devices that are scanning - you note how bad new phones are at coded phy, I wonder how well devices cope with extended advertising. I can't find it now but at some point there was a suggestion that Espruino devices just advertise the Espruino Manufacturer ID by default and maybe drop the Nordic UART unless it's requested. It's a bit of a side issue - I created a new issue at #2628 |
I just tried it with Bangle.js 2 and S140-6.1.1-magic3.zip and it works. It does overwrite existing app however so then the device is in constant reboot so I need to hold the button to get the progress bar next reboot and enter dfu again. Then I can upload application DFU zip again and it works again. So maybe there is a bug in the bootloader that it does not detect that after softdevice dfu update the application is not valid as it says "BOOTING" and then it reboots later (watchdog? some fault exception?). Here nrf_dfu_app_is_valid , I don't see this called in our version, there is this instead Espruino/targets/nrf5x_dfu/main.c Line 149 in 7cdebab bool nrf_dfu_app_is_valid(bool do_crc)
{
NRF_LOG_DEBUG("Enter nrf_dfu_app_is_valid");
if (s_dfu_settings.bank_0.bank_code != NRF_DFU_BANK_VALID_APP)
{
// Bank 0 has no valid app. Nothing to bootand there are also values like So it works thanks to the watchdog or some fault that causes reboot but is not completely safe. |
|
However I can report nice initial success with my phone and Gadgetbridge over coded phy. After building latest code with this additional patch diff --git a/targets/nrf5x/bluetooth.c b/targets/nrf5x/bluetooth.c
index b5af65033..9cccd6fd7 100644
--- a/targets/nrf5x/bluetooth.c
+++ b/targets/nrf5x/bluetooth.c
@@ -2778,6 +2779,12 @@ uint32_t jsble_advertising_start() {
} else if (jsvIsStringEqual(advPhy,"coded")) {
adv_params.primary_phy = BLE_GAP_PHY_CODED; // must use 1mbps phy if connectable?
adv_params.secondary_phy = BLE_GAP_PHY_CODED;
+ } else if (jsvIsStringEqual(advPhy,"coded,1mbps")) {
+ adv_params.primary_phy = BLE_GAP_PHY_CODED; // must use 1mbps phy if connectable?
+ adv_params.secondary_phy = BLE_GAP_PHY_1MBPS;
+ } else if (jsvIsStringEqual(advPhy,"1mbps,coded")) {
+ adv_params.primary_phy = BLE_GAP_PHY_1MBPS; // must use 1mbps phy if connectable?
+ adv_params.secondary_phy = BLE_GAP_PHY_CODED;
} else jsWarn("Unknown phy %q\n", advPhy);
jsvUnLock(advPhy);
}I can start advertising as and then I can connect both from Web IDE (over normal 1mbps) and when disconnected I can also force connection over coded phy from nrfConnect - dots next to connect button and 'connect with preferred phy' and selecting coded phy and my connection is over coded phy - there is "read PHY' item in the menu next to 'connected | client | server |' and it shows in the log current TX/RX PHY as coded. Then when already connected I can go to Gadgetbridge and click connect and it reuses existing connection so it is still over coded phy. Then I took the phone and went far away to the room behind several walls where the RSSI is typically over 100 and the connection worked fine. I went to the application loader and upgraded several apps and everything worked quite smoothly over this connection. As I was still connected also in nrfconnect app I could see in its log all the BLE traffic Gadgetbridge is doing. Then I disconnected from both gadgetbridge and nrfconnect and could not find the watch in nrfconnect on phone at all. So I was thinking it is far away and now advertises over 1mbps only (as current watchface exited after app upgrades so the advertising could be reset) however when I went back to same room I could not find it either and had to restart watch to get it advertising correctly again. Anyway, this was too complicated. Then I tried easier way. It looks like it is enough to advertise over coded phy only - so patch above is not needed. Then I went to the far away room again and saw the watch advertising over coded phy with RSSI between 106-111(!), then I simply went to gadgetbridge and clicked connect without connecting in nrfconnect first and it connected to Bangle just fine and it worked. I went to app loader again and it detected all my apps upgraded so couldn't do much but it was quite stable - no disconnects or significant delays. Then I double checked - went back to the watch, rebooted it to advertise over 1mbps only, went back to the room far away, closed all doors behind me and double checked I could not see Bangle in nrfConnect advertising at all. Then went back set advertising to coded phy, want back again and in the very same spot I could see Bangle adversing with RSSI like 109 and could connect in gadgetbridge just fine, went to app loader, it was getting device info from the watch and it just worked. Maybe it was slower but not bad at all :-) So I can confirm it does improve range. And at least with my (Motorola Edge 30) phone Gadgetbridge can connect when device is advertising only over coded phy, at least when it is already configured there and it is enough to click connect. |
|
Also I just found that in the end advertising does not matter for Gadgetbridge connection. What matters is the increased NRF_SDH_BLE_GAP_EVENT_LENGTH = posibility of connection over coded phy that we have now. We can stil keep old legacy advertising and nrfConnect android app can switch to coded phy later even if not advertised. Here is screenshot of nrfConnect connecting with normal Bangle legacy advertising
When first connecting in nrfConnect via the menu with preferred phy it can be seen that it first connected over 2mbps (not sure why, maybe I clicked wrong checkbox) and then I could set preferred phy to coded and it switched. Then while keeping the connection I clicked connect in gadgetbridge and it was working over coded phy. I have put the watch outside to table on terrace which is farther away from room I had it before, closed door, then went to same spot on the the other side behind several walls and closed doors and the connection still worked. I went to apploader inside gadgetbridge few times as it pulls quite a lot of data from the watch and it still worked. So advertising over coded phy is nice too, but maybe some extra code or setting on android side could set the phy dynamically (possibly based on RSSI?) so when the phone is near the watch it uses normal connection but when you go a bit farther it could switch to coded phy before connection breaks. That could give us best of both worlds, faster data rate and lower battery draw when near the phone and connection still working when leavin phone or watch on desk and going a bit farther. |
looks like upgrade to 6.1.1 works, even downgrade to 6.1.0 works but downgrade to 6.0.0 bricks the device, see conversation here https://github.com/orgs/espruino/discussions/7736#discussioncomment-13045972 After softdevice DFU the device restarts into bangle bootloader with some initial messages but then the display goes dark and holding button does not trigger watchdog and reboot. This is pretty strange since this downgrade was the procedure that almost everyone does with this file when flashing Espruino to Magic3 as it comes with 6.1.x by default and Espruino previously did not run on 6.1.x. I even did the |
|
Thanks for all the checking! So if I add an option to Bangle.js Settings which lets you set: On 6.1.1 and latest firmwares we should be fine to use the coded connections as-is? The downgrade is frustrating - I wonder what the issue there is - maybe it's not updating the block of flash at the end of memory properly? But either way I think it's probably not unreasonable to ask users to upgrade to 6.1.1 and then stick with it? I know the downgrade was done because it seems there was some instability uploading apps, but I haven't seen this on 6.1.1 yet - I think must have been something else. |
Yes (also with showName = true), that is how it works with Gadgetbridge on my phone that can scan devices over coded phy only. And even 6.0.0 would be enough for that. Alternatively no changes are needed whatsoever if you now do the connection via nrfconnect - force coded phy there after connection, then connect in gadgetbridge and then disconnect in nrfconnect (it logs all traffic otherwise). This is something that we could in future do from gadgetbridge android code (possibly based on rssi) - maybe this comment could help with that. Then we could keep it advertising normally in legacy mode over 1mbit rate. EDIT: because typically you are not far away when you want to connect, you just want to keep the connection when you don't take the phone or watch with you to some near places. Also some phones can do coded phy during connection but can't scan when device is advertising over it (as discussed in that 166 issue linked in this comment). |
|
Here are my current changes as per comment https://github.com/orgs/espruino/discussions/7736#discussioncomment-13049257 diff --git a/targets/nrf5x/bluetooth.c b/targets/nrf5x/bluetooth.c
index b5af65033..e770284aa 100644
--- a/targets/nrf5x/bluetooth.c
+++ b/targets/nrf5x/bluetooth.c
@@ -2655,7 +2656,8 @@ void jsble_setup_advdata(ble_advdata_t *advdata) {
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
+// for extended advertising with no scan response m_adv_data may overflow into m_scan_rsp_data
+static uint8_t m_scan_rsp_data[BLE_GAP_ADV_SET_DATA_SIZE_MAX];
#endif
uint32_t jsble_advertising_update(uint8_t *advPtr, unsigned int advLen, uint8_t *rspPtr, unsigned int rspLen, ble_gap_adv_params_t *adv_params) {
@@ -2685,7 +2687,8 @@ uint32_t jsble_advertising_update(uint8_t *advPtr, unsigned int advLen, uint8_t
// now we can modify data and switch back to it
if (advPtr){
if (advLen){
- advLen=MIN(advLen,BLE_GAP_ADV_SET_DATA_SIZE_MAX);
+ unsigned int maxlen = (rspLen==0) ? BLE_GAP_ADV_SET_DATA_SIZE_MAX*2 : BLE_GAP_ADV_SET_DATA_SIZE_MAX;
+ advLen=MIN(advLen, maxlen); // allow to overflow into scan response if missing
memcpy(m_adv_data, advPtr, advLen);
}
m_ble_gap_adv_data.adv_data.p_data = m_adv_data;
@@ -2778,10 +2781,17 @@ uint32_t jsble_advertising_start() {
} else if (jsvIsStringEqual(advPhy,"coded")) {
adv_params.primary_phy = BLE_GAP_PHY_CODED; // must use 1mbps phy if connectable?
adv_params.secondary_phy = BLE_GAP_PHY_CODED;
+ } else if (jsvIsStringEqual(advPhy,"coded,1mbps")) {
+ adv_params.primary_phy = BLE_GAP_PHY_CODED;
+ adv_params.secondary_phy = BLE_GAP_PHY_1MBPS;
+ } else if (jsvIsStringEqual(advPhy,"1mbps,coded")) {
+ adv_params.primary_phy = BLE_GAP_PHY_1MBPS;
+ adv_params.secondary_phy = BLE_GAP_PHY_CODED;
} else jsWarn("Unknown phy %q\n", advPhy);
jsvUnLock(advPhy);
}
- if (adv_params.secondary_phy == BLE_GAP_PHY_AUTO) {
+ bool extended_advertising = (adv_params.secondary_phy != BLE_GAP_PHY_AUTO);
+ if (!extended_advertising) {
// the default...
adv_params.properties.type = non_connectable
? (non_scannable ? BLE_GAP_ADV_TYPE_NONCONNECTABLE_NONSCANNABLE_UNDIRECTED : BLE_GAP_ADV_TYPE_NONCONNECTABLE_SCANNABLE_UNDIRECTED)
@@ -2808,7 +2818,7 @@ uint32_t jsble_advertising_start() {
adv_params.interval = bleAdvertisingInterval;
uint32_t err_code = 0;
- uint8_t m_enc_scan_response_data[31]; // BLE_GAP_ADV_SET_DATA_SIZE_MAX
+ uint8_t m_enc_scan_response_data[BLE_GAP_ADV_SET_DATA_SIZE_MAX];
uint16_t m_enc_scan_response_data_len = sizeof(m_enc_scan_response_data);
#if NRF_SD_BLE_API_VERSION<5
err_code = adv_data_encode(&scanrsp, m_enc_scan_response_data, &m_enc_scan_response_data_len);
@@ -2823,12 +2833,23 @@ uint32_t jsble_advertising_start() {
//jsiConsolePrintf("adv_data_set %d %d\n", advPtr, advLen);
#if NRF_SD_BLE_API_VERSION>5
memset(&m_ble_gap_adv_data, 0, sizeof(m_ble_gap_adv_data));
+
+ if (extended_advertising && non_scannable && m_enc_scan_response_data_len>0){
+ // we may join scan response into the extended advertising data instead of throwing it away
+ unsigned int combined_len = advLen + m_enc_scan_response_data_len;
+ uint8_t *combined_data = alloca(combined_len);
+ memcpy(combined_data, advPtr, advLen);
+ memcpy(&combined_data[advLen], m_enc_scan_response_data, m_enc_scan_response_data_len);
+ err_code = jsble_advertising_update(combined_data, combined_len, NULL, 0, &adv_params);
+ } else
#endif
- err_code = jsble_advertising_update(
- (uint8_t*)advPtr, advLen,
- non_scannable ? NULL : m_enc_scan_response_data, non_scannable ? 0 : m_enc_scan_response_data_len,
- &adv_params
- );
+ {
+ err_code = jsble_advertising_update(
+ (uint8_t*)advPtr, advLen,
+ non_scannable ? NULL : m_enc_scan_response_data, non_scannable ? 0 : m_enc_scan_response_data_len,
+ &adv_params
+ );
+ }
jsble_check_error(err_code);
#if NRF_SD_BLE_API_VERSION>5
if (!err_code) {as said it is a bit of a hack to reuse the scan response as is when we find out we would otherwise throw it away. I guess better would be to rewrite the code to not build it at all as the non_scannable value is known quite early there. I think it works but is not very nice so not sure it is worth making PR with it. And also most likely NRF.setScanResponse will break it as it currently does not check for non_scannable flag so it always rewrites scan respose data even if setting it for non_scannable case probably makes no sense. EDIT: after some more testing there is something wrong with that code. The nordic guid from scan response that goes over 31 bytes is wrong for me. I don't see why that should be. I've removed the alloca and simplified pointer arithmetics but it is still the same. it is probably something inside jsble_advertising_update like memcpy copying less bytes. |
|
Thanks - yes, maybe if the size is 31 it'll try and align them? Maybe something like this would work? ... also afaik I think as in that other conversation having a big array on 52840 where we have more RAM is probably easier. I see you do |
yes, was thinking the same thing, that should work. And yes looks like it gets reordered, the m_scan_rsp_data goes first for some reason
The difference is just the static buffer where to finally copy the merged data. But yes, increasing the m_adv_data instead would prevent this alignment bug. The more complicated issue is the logic of putting stuff into scan response vs (extended) advertising data, so that we don't build the scan response if not needed. But there is not much easier way anyway(?) since we start from advertising data from variable Espruino/targets/nrf5x/bluetooth.c Line 2722 in 0a32718 that we can't easily append to, so the result would be similar as now as there would need to be extra buffer like this Espruino/targets/nrf5x/bluetooth.c Line 2811 in 0a32718 there anyway. Or the logic would need to be moved somewhere else/earlier so the advertising data variable would already have the final data including stuff that otherwise goes to scan response. So does it make sense to continue in this direction of this hack and merge scan response into extended advertising data like this or is it the wrong way?
not sure, maybe not, I have just put your if condition into variable to reuse it later, here is already the same assumption that it is the extended advertising case Espruino/targets/nrf5x/bluetooth.c Line 2789 in 0a32718 |
|
Ahh, I see what you mean about the scan response and putting those uuids into the main advertising packet. I guess that brings up a question of UUIDs in general since right now we put them all in scan response, and it feels like they should all go in the main advertising packet? The whole: Feels a bit broken. Maybe I should add: And then ensure all the services go in advertising by default, and then folks can always do: to force services in the scan response? ... and if I add |
that was it, now it works as expected, nordic uart guid is there and I can also make name much longer and it still fits diff --git a/targets/nrf5x/bluetooth.c b/targets/nrf5x/bluetooth.c
index b5af65033..43a78f371 100644
--- a/targets/nrf5x/bluetooth.c
+++ b/targets/nrf5x/bluetooth.c
@@ -2654,8 +2655,10 @@ void jsble_setup_advdata(ble_advdata_t *advdata) {
#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
+// twice the size so it can also hold scan response or data from it for extended advertising
+static uint8_t m_adv_data[BLE_GAP_ADV_SET_DATA_SIZE_MAX*2];
+// scan response is not used when extended advertising is enabled so let's reuse same space
+static uint8_t *m_scan_rsp_data=&m_adv_data[BLE_GAP_ADV_SET_DATA_SIZE_MAX];
#endif
uint32_t jsble_advertising_update(uint8_t *advPtr, unsigned int advLen, uint8_t *rspPtr, unsigned int rspLen, ble_gap_adv_params_t *adv_params) {
@@ -2664,8 +2667,8 @@ uint32_t jsble_advertising_update(uint8_t *advPtr, unsigned int advLen, uint8_t
// 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];
+ uint8_t tmp_adv_data[BLE_GAP_ADV_SET_DATA_SIZE_MAX*2];
+ uint8_t *tmp_scan_rsp_data = &tmp_adv_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){
@@ -2685,7 +2688,8 @@ uint32_t jsble_advertising_update(uint8_t *advPtr, unsigned int advLen, uint8_t
// now we can modify data and switch back to it
if (advPtr){
if (advLen){
- advLen=MIN(advLen,BLE_GAP_ADV_SET_DATA_SIZE_MAX);
+ unsigned int maxlen = (rspLen == 0) ? (BLE_GAP_ADV_SET_DATA_SIZE_MAX*2) : (BLE_GAP_ADV_SET_DATA_SIZE_MAX);
+ advLen = MIN(advLen, maxlen); // allow to overflow into scan response if missing
memcpy(m_adv_data, advPtr, advLen);
}
m_ble_gap_adv_data.adv_data.p_data = m_adv_data;
@@ -2778,10 +2782,17 @@ uint32_t jsble_advertising_start() {
} else if (jsvIsStringEqual(advPhy,"coded")) {
adv_params.primary_phy = BLE_GAP_PHY_CODED; // must use 1mbps phy if connectable?
adv_params.secondary_phy = BLE_GAP_PHY_CODED;
+ } else if (jsvIsStringEqual(advPhy,"coded,1mbps")) {
+ adv_params.primary_phy = BLE_GAP_PHY_CODED;
+ adv_params.secondary_phy = BLE_GAP_PHY_1MBPS;
+ } else if (jsvIsStringEqual(advPhy,"1mbps,coded")) {
+ adv_params.primary_phy = BLE_GAP_PHY_1MBPS;
+ adv_params.secondary_phy = BLE_GAP_PHY_CODED;
} else jsWarn("Unknown phy %q\n", advPhy);
jsvUnLock(advPhy);
}
- if (adv_params.secondary_phy == BLE_GAP_PHY_AUTO) {
+ bool extended_advertising = (adv_params.secondary_phy != BLE_GAP_PHY_AUTO);
+ if (!extended_advertising) {
// the default...
adv_params.properties.type = non_connectable
? (non_scannable ? BLE_GAP_ADV_TYPE_NONCONNECTABLE_NONSCANNABLE_UNDIRECTED : BLE_GAP_ADV_TYPE_NONCONNECTABLE_SCANNABLE_UNDIRECTED)
@@ -2808,7 +2819,7 @@ uint32_t jsble_advertising_start() {
adv_params.interval = bleAdvertisingInterval;
uint32_t err_code = 0;
- uint8_t m_enc_scan_response_data[31]; // BLE_GAP_ADV_SET_DATA_SIZE_MAX
+ uint8_t m_enc_scan_response_data[BLE_GAP_ADV_SET_DATA_SIZE_MAX];
uint16_t m_enc_scan_response_data_len = sizeof(m_enc_scan_response_data);
#if NRF_SD_BLE_API_VERSION<5
err_code = adv_data_encode(&scanrsp, m_enc_scan_response_data, &m_enc_scan_response_data_len);
@@ -2823,12 +2834,23 @@ uint32_t jsble_advertising_start() {
//jsiConsolePrintf("adv_data_set %d %d\n", advPtr, advLen);
#if NRF_SD_BLE_API_VERSION>5
memset(&m_ble_gap_adv_data, 0, sizeof(m_ble_gap_adv_data));
+
+ if (extended_advertising && non_scannable && m_enc_scan_response_data_len>0){
+ // we may join scan response into the extended advertising data instead of throwing it away
+ unsigned int combined_len = advLen + m_enc_scan_response_data_len;
+ uint8_t combined_data[combined_len];
+ memcpy(combined_data, advPtr, advLen);
+ memcpy(combined_data + advLen, m_enc_scan_response_data, m_enc_scan_response_data_len);
+ err_code = jsble_advertising_update(combined_data, combined_len, NULL, 0, &adv_params);
+ } else
#endif
- err_code = jsble_advertising_update(
- (uint8_t*)advPtr, advLen,
- non_scannable ? NULL : m_enc_scan_response_data, non_scannable ? 0 : m_enc_scan_response_data_len,
- &adv_params
- );
+ {
+ err_code = jsble_advertising_update(
+ (uint8_t*)advPtr, advLen,
+ non_scannable ? NULL : m_enc_scan_response_data, non_scannable ? 0 : m_enc_scan_response_data_len,
+ &adv_params
+ );
+ }
jsble_check_error(err_code);
#if NRF_SD_BLE_API_VERSION>5
if (!err_code) {And BTW now when Nordic UART guid is OK Chrome in Ubuntu 22.04 can find and connect to it with Here sis ome output from |
well with device name even single custom service may not fit and only very few standard 16bit ones will.
Maybe yes, the first is magic, the second says exactly what one wants
What about having exactly the same thing for setScanResponse as is for setAdvertising? NRF.setScanResponse({}, { that would be symmetrical (including putting name or manufacturer or service data there) and would not need to exclude flags from raw advertising data. Why there are not symmetrical now? ok maybe setAdvertising is meant for both advertising packet and scan response? and (missing) setAdvertisingData and setScanResponse would be symmetrical EDIT: so if setAdvertising is meant for setting both advertising data and scan response maybe it could be optionally nested? having same structure as now under extra "data" and "response" nodes. If those are there no extra magic would be done what goes where like is done now? |
|
Thanks! And good point about I think it's better to keep Just checking - do you think the |
I don't know, maybe some devices could use it as a hint that this device can also do coded phy and switch to it later? nrfConnect on android doesn't care and allows you to do the switch even if just legacy advertising is there but maybe other OSes may care? Also this currently forces switch to extended advertising even if still advertising primarily over 1mbps (but not sure, the extended:1 flag may do this too?) |
|
Thanks. I just pushed some changes to bring in I spotted that actually we already handle advertising services to the main advertising, so it should work pretty well as-is: #2631 |
I am using xaomi thermometer with custom firmware https://github.com/pvvx/ATC_MiThermometer that is able to advertise over coded phy. I can find it in nrfconnect on my phone but I could never find it with Espruino on 52840 (with S140 6.0.0).
Now I tried with 6.1.1 (and 6.1.0) and just by updating softdevice it works with same espruino binary - suddenly the thermometer can be found!
with 6.0.0
with 6.1.1
I checked S140 6.1.0 release notes and it actually starts with
the "new features" section also mentions
a single call to sd_ble_gap_scan_start() or sd_ble_gap_connect() (DRGN-8668).
by the SoftDevice (DRGN-9802)
There is quite a lot of bugfixes and there is also migration document with code describing how to scan on both PHYs at once (basically double the interval) I am attaching both here
s140_nrf52_6.1.0_migration-document.pdf
s140_nrf52_6.1.0_release-notes.pdf
I also noticed one mentioned change which may be related to the 4096 block writing bug to internal flash, looks like maybe they reduced it too much :-)
So basically I am suggesting to eventually move to 6.1.0 or 6.1.1 to support coded phy/advertising extensions better. The only issue I see that need fixing with 6.1.x is reducing flash write block size from 4096 to 2048 as per that comment.
The text was updated successfully, but these errors were encountered: