pkg/nimble: Fix compilation with USEMODULE += nimble_svc_bas

[zvecr]

Contribution description

pkg/nimble/Makefile.include includes logic for the module, however the corresponding Make target is missing.

This produces the following error:

make[3]: *** No rule to make target 'nimble_svc_bas', needed by 'all'.  Stop.

[maribu]

Hi, thanks for the PR.

I sadly have little prior knowledge about nimBLE, but I will review and help you get this upstream anyway.

At first I thought that this might just be a new feature, but given that we already have

pkg/nimble/Makefile.include
61:ifneq (,$(filter nimble_svc_bas,$(USEMODULE)))

in the master tree, we can assume that this was meant to work. It is a pity, that our documentation is a bit sparse.

I can confirm the issue. In master I get the same error you get:

$ USEMODULE=nimble_svc_bas make BOARD=nrf52840dk -C examples/nimble_scanner 
make: Entering directory '/home/[email protected]/Repos/software/RIOT/master/examples/nimble_scanner'
Building application "nimble_scanner" for "nrf52840dk" with CPU "nrf52".

[...]
"make" -C /home/[email protected]/Repos/software/RIOT/master/pkg/nimble/scanner
make[1]: *** No rule to make target 'nimble_svc_bas', needed by 'all'.  Stop.
make: *** [/home/[email protected]/Repos/software/RIOT/master/examples/nimble_scanner/../../Makefile.include:811: pkg-build] Error 2
make: Leaving directory '/home/[email protected]/Repos/software/RIOT/master/examples/nimble_scanner'

However, with this PR I get

$ USEMODULE=nimble_svc_bas make BOARD=nrf52840dk -C examples/nimble_scanner 
make: Entering directory '/home/[email protected]/Repos/software/RIOT/master/examples/nimble_scanner'
Building application "nimble_scanner" for "nrf52840dk" with CPU "nrf52".

[...]

In file included from /home/[email protected]/.cache/RIOT/pkg/nimble/nimble/host/services/bas/src/ble_svc_bas.c:24:
/home/[email protected]/.cache/RIOT/pkg/nimble/porting/npl/riot/include/syscfg/syscfg.h:15:49: error: 'MYNEWT_VAL_BLE_SVC_BAS_BATTERY_LEVEL_READ_PERM' undeclared here (not in a function); did you mean 'MYNEWT_VAL_BLE_SVC_GAP_APPEARANCE_WRITE_PERM'?
   15 | #define MYNEWT_VAL(_name)                       MYNEWT_VAL_ ## _name
      |                                                 ^~~~~~~~~~~
/home/[email protected]/.cache/RIOT/pkg/nimble/nimble/host/services/bas/src/ble_svc_bas.c:58:22: note: in expansion of macro 'MYNEWT_VAL'
   58 |                      MYNEWT_VAL(BLE_SVC_BAS_BATTERY_LEVEL_READ_PERM),
      |                      ^~~~~~~~~~

Something along the lines of

diff --git a/pkg/nimble/patches/0001-port-npl-riot-add-svc_bas-config.patch b/pkg/nimble/patches/0001-port-npl-riot-add-svc_bas-config.patch
new file mode 100644
index 0000000000..89f52f6cd1
--- /dev/null
+++ b/pkg/nimble/patches/0001-port-npl-riot-add-svc_bas-config.patch
@@ -0,0 +1,36 @@
+From 3cd2f7dc9f2c17ffb83499ab569a291988cbbc08 Mon Sep 17 00:00:00 2001
+From: Marian Buschsieweke <[email protected]>
+Date: Sat, 8 Feb 2025 13:54:20 +0100
+Subject: [PATCH] port/npl/riot: add svc_bas config
+
+---
+ porting/npl/riot/include/syscfg/syscfg.h | 13 +++++++++++++
+ 1 file changed, 13 insertions(+)
+
+diff --git a/porting/npl/riot/include/syscfg/syscfg.h b/porting/npl/riot/include/syscfg/syscfg.h
+index 84bdea5..a0b49c8 100644
+--- a/porting/npl/riot/include/syscfg/syscfg.h
++++ b/porting/npl/riot/include/syscfg/syscfg.h
+@@ -670,6 +670,19 @@
+ #define MYNEWT_VAL_BASELIBC_PRESENT (1)
+ #endif
+ 
++/*** @apache-mynewt-nimble/nimble/host/services/bas */
++#ifndef MYNEWT_VAL_BLE_SVC_BAS_BATTERY_LEVEL_NOTIFY_ENABLE
++#define MYNEWT_VAL_BLE_SVC_BAS_BATTERY_LEVEL_NOTIFY_ENABLE (1)
++#endif
++
++#ifndef MYNEWT_VAL_BLE_SVC_BAS_BATTERY_LEVEL_READ_PERM
++#define MYNEWT_VAL_BLE_SVC_BAS_BATTERY_LEVEL_READ_PERM (0)
++#endif
++
++#ifndef MYNEWT_VAL_BLE_SVC_BAS_SYSINIT_STAGE
++#define MYNEWT_VAL_BLE_SVC_BAS_SYSINIT_STAGE (303)
++#endif
++
+ /*** @apache-mynewt-core/sys/console/stub */
+ #ifndef MYNEWT_VAL_CONSOLE_UART_BAUD
+ #define MYNEWT_VAL_CONSOLE_UART_BAUD (115200)
+-- 
+2.43.0
+

fixes the build issue for me. Would that make sense in your opinion?

If you agree with that change, please directly squash that into a single commit (or use git commit --amend).

Could you please change the title of the commit to something like:

<SUBSYSTEM>: <description>

(E.g. pkg/nimble: Fix compilation with USEMODULE += nimble_svc_bas.)

[riot-ci]

Murdock results

✔️ PASSED

2b2cc60 pkg/nimble: Fix compilation of 'USEMODULE += nimble_svc_bas'

Success	Failures	Total	Runtime
10271	0	10271	10m:45s

Artifacts

• Documentation preview

[zvecr]

Something along the lines of

Im currently doing the following right now

CFLAGS += -DMYNEWT_VAL_BLE_SVC_BAS_BATTERY_LEVEL_NOTIFY_ENABLE=1
CFLAGS += -DMYNEWT_VAL_BLE_SVC_BAS_BATTERY_LEVEL_READ_PERM=0

So that seems fine to me!

[maribu]

I was aiming for something that would compile by default, so that we could enable compile testing coverage for nimble_svc_bas as a follow up. Without test coverage, things just tend to break.

Judging from the name only, MYNEWT_VAL_BLE_SVC_BAS_BATTERY_LEVEL_NOTIFY_ENABLE would require some interface to be implemented to actually report the battery level, doesn't it?

I assume that this is possible to provide from the application side. But the default would need to be

CFLAGS += -DMYNEWT_VAL_BLE_SVC_BAS_BATTERY_LEVEL_NOTIFY_ENABLE=0

as we cannot assume that each and every app provides battery level info. We would at least need a way to allow the app to overwrite this.

With adding the patch for the the syscfg.h, we would have

#ifndef DMYNEWT_VAL_BLE_SVC_BAS_BATTERY_LEVEL_NOTIFY_ENABLE
#define DMYNEWT_VAL_BLE_SVC_BAS_BATTERY_LEVEL_NOTIFY_ENABLE 0
#endif

which would allow the app to just add

CFLAGS += -DMYNEWT_VAL_BLE_SVC_BAS_BATTERY_LEVEL_NOTIFY_ENABLE=1

to enable battery level reporting, if it has that implemented.

[zvecr]

The service doesn't directly provide any mechanism to read battery level, it only provides an API to set the current value.

https://github.com/apache/mynewt-nimble/blob/c29b277c18f6de67dcae80688d66b0d9df3cc515/nimble/host/services/bas/src/ble_svc_bas.c#L99-L109

ble_svc_bas_battery_level_set with notifications enabled, will send out the value there and then.

[maribu]

Thx for clarifying!

Would you mind to add this to pkg/nimble/Makefile.include as part of this PR?

[zvecr]

I would advice about not making it a default behaviour through CFLAGS.

Adding those lines makes it sort of impossible to change the behaviour if someone wished to do so. Whereas the patch at least allows the values to be redefined. Maybe not such a common scenario for battery, but you might want the encrypted read permission.

so that we could enable compile testing coverage

Hopefully I can get round to submitting some of the example/test apps i've used along the way of getting HID over GATT working. BAS being a optional service for it to function.

[maribu]

OK, if we want the application to be able to set those values, I'd prefer the variant with the patch, as suggested in #21200 (comment)

Having a module selectable that will not compile unless certain CFLAGS are set it a bit too arcane, even if that behaviour were documented. Instead, we need to peovide conservative defaults and allow the app to override, if needed.