Closed fanoush closed 11 months ago
Oh, it works for SDK12 because PEER_MANAGER_ENABLED
is actually defined few lines above the test just for SDK12!
Not sure if PEER_MANAGER_ENABLED test should be removed for SDK15 as there is no nrf51 with SDK15 anyway or the PEER_MANAGER_ENABLED=1 should be defined there for everything nrf52 and SDK >=12 (as SDK11 and lower does not have it)
Oh, I see you moved it from SDK12 part in https://github.com/espruino/Espruino/commit/e51db236ed51db1f883ed872bcc867cb3f9979c3
I guess the test was there because of microbit1/nrf51 and not peer manager so it can be removed for SDK15? Created PR with this change.
Thanks for tracking this down! I guess my only concern is does this leave PEER_MANAGER_ENABLED
as undefined?
I assume it's probably set somewhere or it likely wouldn't build (maybe it's a default in sdk_config.h), but...
or the PEER_MANAGER_ENABLED=1 should be defined there for everything nrf52 and SDK >=12
I think it probably makes sense doing that to be more explicit, as I think the peer manager is needed for central mode...
Yes it looks undefined, this
diff --git a/targets/nrf5x/bluetooth.c b/targets/nrf5x/bluetooth.c
index 288c799ad..21ab91143 100644
--- a/targets/nrf5x/bluetooth.c
+++ b/targets/nrf5x/bluetooth.c
@@ -69,6 +69,8 @@
#if NRF_SD_BLE_API_VERSION<5
#include "fstorage.h"
#include "fstorage_internal_defs.h"
+#else
+#error PEERMANAGER UNDEFINED
#endif
#include "ble_conn_state.h"
static pm_peer_id_t m_peer_id; /**< Device reference handle to the current bonded central. */
added here https://github.com/espruino/Espruino/blob/master/targets/nrf5x/bluetooth.c#L72 breaks banglejs2 build.
I think the peer manager is needed for central mode.
I think it is needed for bonding and secure conenctions so if this is not used, Peer Manager it is not needed, however it looks like currently it works for BANGLEJS2 insludin bonding without this PEER_MANAGER_ENABLED
define?
Anyway I'd say CENTRAL_COUNT value is unrelated to peer manager being on/off even if currently missing PEER_MANAGER_ENABLED for SDK!=12 may be a bug.
Oh, no, sorry, forget it that is wrong #endif, so looks like PEER_MANAGER_ENABLED is defined
Ok, thanks for checking that out! Is PEER_MANAGER_ENABLED still defined in your SDK15 build without CENTRAL_LINK_COUNT=1
defined?
As you say peer manager may not be needed - however it's possible things may behave a bit strangely without it. I've never tested central with it not enabled
Is PEER_MANAGER_ENABLED still defined in your SDK15 build without
CENTRAL_LINK_COUNT=1
defined?
Yes. When just commenting out 'DEFINES += -DCENTRAL_LINK_COUNT=2 -DNRF_SDH_BLE_CENTRAL_LINK_COUNT=2'
in BANGLEJS2 board and adding the #else
and #error
to line https://github.com/espruino/Espruino/blob/master/targets/nrf5x/bluetooth.c#L72 as mentioned in previous post it prints the error.
And at that place it is inside PEER_MANAGER_ENABLED and NRF_SD_BLE_API_VERSION>=5. So by mistake I put it to right place to test exactly this.
It probably gets defined later from this place https://github.com/espruino/Espruino/blob/master/targetlibs/nrf5x_15/nrf52_config/sdk_config.h#L120
Thanks! Ok, that's good then. I just added a line in to set PEER_MANAGER in app_config - I know it's not needed but I think it might be helpful to have it all in one place
This does not work for some reason https://github.com/espruino/Espruino/blob/62acf92dd9cd718bfb4c1c013f67b0c80fc1a489/targets/nrf5x/app_config.h#L181C3-L181C3
with this patch
I get
Espruino/targets/nrf5x/app_config.h:185:2: error: #error Central is 0
when building for BANGLEJS2Not sure why PEER_MANAGER_ENABLED is undefined there, it is also tested in NRF5X_SDK_12 section above and that maybe works?
Noticed because I have older board files without the -DCENTRAL_LINK_COUNT lines and with fresh build I see no console output when connecting and also
process.env.APP_RAM_BASE.toString(16)
was lower than expected. Adding explicit'DEFINES += -DCENTRAL_LINK_COUNT=1 -DNRF_SDH_BLE_CENTRAL_LINK_COUNT=1',
helped.