pkg/nimble: Update to 1.8.0

[chrysn]

Contribution description

This updates nimble, with all the adjustments that need to go with it.

Testing procedure

• make -C examples/nimble_heart_rate_sensor all flash term on any supported board (I'm using on nrf52dk)
• Use a Chrome based browser on https://webbluetoothcg.github.io/demos/heart-rate-sensor/ or any other BLE tool to read out some heart rate.

Issues/PRs references

Updating this is a prerequisite to using ready-made Rust wrappers on nimble (eg. https://github.com/embassy-rs/trouble/), which is preferable over safely wrapping the whole API surface just once ore.

Current status

Not getting it to build yet; we'll need some patches for upstream problems (not following IWYU) and our dead-code strictness -- and then (currently: after fixing them manually), a few symbols are missing, which should be easy to fix but is still a tedious task.

[edit: It builds now and is tested]

All patches should be upstreamed.

[edit: All applicable patches are filled upstream.]

[riot-ci]

Murdock results

:heavy_check_mark: PASSED

16692a9375ad89ad38291fbe11427667477f54a5 DO NOT MERGE: Disable nimble on ESP32

Success	Failures	Total	Runtime
10222	0	10222	16m:15s

Artifacts

• Documentation preview

[chrysn]

@sjanc, could you help me here? I'm always running into g_ble_ll_data.ll_evq being uninitialized; the place where it would be initialized is ble_ll_init, but that is called from nowhere. Should I just call that (using an exern void ble_ll_init(void) because it's not even in headers), and should it come before or after nimble_port_init?

[sjanc]

@sjanc, could you help me here? I'm always running into g_ble_ll_data.ll_evq being uninitialized; the place where it would be initialized is ble_ll_init, but that is called from nowhere. Should I just call that (using an exern void ble_ll_init(void) because it's not even in headers), and should it come before or after nimble_port_init?

ble_ll_init() is initializing controller part and on Mynewt it is called from generated sysinit calls, so yes, you'll have to use extern in port (at least for now). It shall be called before ble_transport_hs_init() and ble_transport_ll_init() calls [1][2]

I think you should update nimble_port_init() to adjust for those changes as controller port is very HW/OS specific... So call to ble_ll_init() should be from nimble_port_init() with proper orders to keep init order requirements.

[1] https://github.com/apache/mynewt-nimble/blob/master/nimble/controller/pkg.yml#L45 [2] https://github.com/apache/mynewt-nimble/pull/1648

[chrysn]

Thanks; moved into a patch while we're aiming for 1.8.0, and filed as https://github.com/apache/mynewt-nimble/pull/1956. All patches that are now applied are submitted upstream (one even already merged), with the exception of a dead code one where I'm not sure whether this shouldn't better be handled by a less pedantic flag set through our build system.

Builds pass, and I've not only used this with the heart rate example described in tests, but also to set up an IPSP interface.

I'm placing this as ready-for-review -- if the ESP thing can be sorted out quickly, I have some remaining hope to get this in before soft freeze.

[chrysn]

Seems the ESP port is maintained by the ESP vendor, who announced updating weeks ago, but we'll need an ESP update.

[maribu]

I recall that for lvgl we had two packages for some time when upstream did a major release (with breaking API changes).

I wonder if that approach to keep both versions as separate packages and let ESP only work with the old one would be the way to unblock this here.

[gschorcht]

Seems the ESP port is maintained by the ESP vendor, who announced updating weeks ago, but we'll need an ESP update.

So far we are not using the Nimble package for ESP as provided by Espressif in RIOT at all :thinking: We only use the low-level BLE controller driver from ESP-IDF.

But with the migration to ESP-IDF v5.4 and the support of the ESP32-H2 and ESP32-C6, the low-level BLE Controller driver is mixed with some functions from the nimble porting layer from their package (as usual :worried:) so that I will have to contribute some small changes to RIOT's Nimble package for the migration to ESP-IDF 5.4 and the support of the ESP32-H2 and ESP32-C6, see commit

[gschorcht]

@chrysn I played a bit with this PR. If I comment out the ble_ll_init call and the ble_transport_ll_init call in nimble_port_init, it works with ESP32x after increasing the stack size a bit. The latter problem might be related to the verbose log output.

The question is whether you plan to upgrade to the newer nimble version where all your patches are already merged in upstream. This would also fix the problem with ble_ll_init.

[gschorcht]

When I tried to find out why the complete debugging output is generated with the new Nimble version, although only the info level is defined as log level, I realized that they changed their logging system with PR https://github.com/apache/mynewt-nimble/pull/1570.

Analyzing the preprocessor output for pkg/nimble/nimble/host/src/ble_hs_hci.c shows that the macros in following lines

        BLE_HS_LOG(DEBUG, "ble_hs_hci_acl_tx(): ");
        ble_hs_log_mbuf(frag);
        BLE_HS_LOG(DEBUG, "\n");

are now just expanded to

        BLE_HS_LOG_DEBUG("ble_hs_hci_acl_tx(): ");
        ble_hs_log_mbuf(frag);
        BLE_HS_LOG_DEBUG("\n");

where BLE_HS_LOG_DEBUG is simply expanded to

__attribute__((__format__ (__printf__, 1, 0)))
static inline void BLE_HS_LOG_DEBUG(const char *fmt, ...) {
   va_list args; 
   __builtin_va_start(args, fmt);
   vprintf(fmt, args); 
   __builtin_va_end(args);
};

without any conditional compilation so that all debugging output is generated without checking the log level anywhere.

[gschorcht]

With the following patches, we could solve the logging problem:

diff --git a/porting/npl/riot/include/nimble/nimble_npl_os_log.h b/porting/npl/riot/include/nimble/nimble_npl_os_log.h
index f585070dad2..a012d1b98bc 100644
--- a/porting/npl/riot/include/nimble/nimble_npl_os_log.h
+++ b/porting/npl/riot/include/nimble/nimble_npl_os_log.h
@@ -23,16 +23,27 @@
 #include <stdarg.h>
 #include <stdio.h>
 
+#define LOG_LEVEL_DEBUG    (0)
+#define LOG_LEVEL_INFO     (1)
+#define LOG_LEVEL_WARN     (2)
+#define LOG_LEVEL_ERROR    (3)
+#define LOG_LEVEL_CRITICAL (4)
+
 /* Example on how to use macro to generate module logging functions */
 #define BLE_NPL_LOG_IMPL(lvl) \
-        __attribute__((__format__ (__printf__, 1, 0))) \
-        static inline void _BLE_NPL_LOG_CAT(BLE_NPL_LOG_MODULE, \
-                _BLE_NPL_LOG_CAT(_, lvl))(const char *fmt, ...)\
-        {                               \
-            va_list args;               \
-            va_start(args, fmt);        \
-            vprintf(fmt, args);          \
-            va_end(args);               \
+        __attribute__((__format__ (__printf__, 1, 0)))           \
+        static inline void _BLE_NPL_LOG_CAT(BLE_NPL_LOG_MODULE,  \
+                _BLE_NPL_LOG_CAT(_, lvl))(const char *fmt, ...)  \
+        {                                                        \
+            if (LOG_LEVEL_ ## lvl >= MYNEWT_VAL(DFLT_LOG_LVL)) { \
+                va_list args;                                    \
+                va_start(args, fmt);                             \
+                vprintf(fmt, args);                              \
+                va_end(args);                                    \
+            }                                                    \
+            else {                                               \
+                (void)fmt;                                       \
+            }                                                    \
         }
 
 #endif  /* _NIMBLE_NPL_OS_LOG_H_ */

The default logging level should be increased to warning, otherwise the GATT service is a bit too annoying.

diff --git a/porting/npl/riot/include/syscfg/syscfg.h b/porting/npl/riot/include/syscfg/syscfg.h
index 4ec768e8aef..e13179e1e69 100644
--- a/porting/npl/riot/include/syscfg/syscfg.h
+++ b/porting/npl/riot/include/syscfg/syscfg.h
@@ -990,7 +990,7 @@
 #endif
 
 #ifndef MYNEWT_VAL_DFLT_LOG_LVL
-#define MYNEWT_VAL_DFLT_LOG_LVL (1)
+#define MYNEWT_VAL_DFLT_LOG_LVL (2)
 #endif
 
 #ifndef MYNEWT_VAL_DFLT_LOG_MOD

[gschorcht]

With the following patch, Nimble is working on ESP32x SoCs.

diff --git a/porting/nimble/src/nimble_port.c b/porting/nimble/src/nimble_port.c
index cd0cd004b42..20bc951e643 100644
--- a/porting/nimble/src/nimble_port.c
+++ b/porting/nimble/src/nimble_port.c
@@ -36,7 +36,6 @@ extern void ble_ll_init(void);
 void
 nimble_port_init(void)
 {
-    ble_ll_init();
     /* Initialize default event queue */
     ble_npl_eventq_init(&g_eventq_dflt);
     /* Initialize the global memory pool */
@@ -47,6 +46,10 @@ nimble_port_init(void)
     /* Initialize the host */
     ble_transport_hs_init();
 
+#if NIMBLE_CFG_CONTROLLER
+    ble_ll_init();
+#endif
+
 #if NIMBLE_CFG_CONTROLLER
 #ifndef RIOT_VERSION
     hal_timer_init(5, NULL);
@@ -54,8 +57,10 @@ nimble_port_init(void)
 #endif
 #endif
 
+#if !defined(RIOT_VERSION) || !defined(CPU_ESP32)
     /* Initialize the controller */
     ble_transport_ll_init();
+#endif
 }
 
 void

[crasbe]

It would be nice if we could move this forward :)

[BOZHENG001]

Any further plans on this PR?