Skip to content

Conversation

HendrikVE
Copy link
Contributor

@HendrikVE HendrikVE commented Aug 15, 2019

Contribution description

Implement a new module stdio_nimble, which uses UART and nimble in parallel for stdio. The characteristic for stdin is writable and the characteristic for stdout uses the indicate mechanism to publish the system's output to a connected device. Calls of "_send_from_ringbuffer" to send data via nimBLE are chained together using a callback in nimBLE called "_gap_event_cb".

Module can be enabled with USEMODULE += stdio_nimble

UART can be disabled with CFLAGS += -DSTDIO_NIMBLE_DISABLE_UART

For automatic advertising a new module is provided: "auto_nimble_advertise". It will take care to enable advertising on disconnect and disable on connect.

Module can be enabled with USEMODULE += auto_nimble_advertise

Advertised device name can be configured with
CFLAGS += -DAUTO_NIMBLE_ADVERTISE_DEVICE_NAME='"name"'

Testing procedure

You can download the matching android app (source) which was developed in parallel to this contribution at the Google Playstore for your smartphone, but you can also use any other bluetooth developement app, like Nordics "nRF Connect"-App, available for Android and iOS.

All you have to do is adding USEMODULE += stdio_nimble to the Makefile of an application, for example default and run it on a nrf52dk board. Now you can open the app, connect to the device and input commands to the shell and receive its output via bluetooth. UART input and output are still working in parallel to this.

Issues/PRs references

@HendrikVE HendrikVE force-pushed the nimble_shell_module branch 2 times, most recently from a50ee27 to 99e7f3a Compare August 15, 2019 13:05
@miri64 miri64 added Area: BLE Area: Bluetooth Low Energy support Area: sys Area: System Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Aug 28, 2019
Copy link
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not look into the code in too much detail (yet), but there are some issues that we should think about before going into the fine details:

  • additional thread: this is only needed because of the 'bug/behavior' of NimBLE, where the hacky delay is added, right? When actually merging this, I would prefer to gain more insights on this behavior and save that memory. Or at least as a work around, I would prefer to post delayed events to NimBLEs event queue instead...
  • static memory allocation: there is (i) no need for that here, and (ii) the way malloc is used is extremely dangerous (return value is not checked at all)
  • use of msg: the way the type field is used to encode a buffers length is a nice big old security nightmare: if you send an invalid message from any other thread to this thread, it enables someone to print out a lot of otherwise hidden memory...
  • BLE behavior: I have not made up my mind yet, but I think the advertising behavior coded in this PR should at least be optional or made in a way it can be disabled (e.g. a sub-module?). I guess we have two typical use-cases: (i) this code is used in an otherwise BLE-unrelated context (e.g. hello-world example on nrf boards), or (ii) it is used with some BLE application (e.g. nimlbe_gatt). For the first case, is makes sense to tell the node to advertise at some point. But for the second option, there is potential trouble when the user application will also try to start advertising...

@HendrikVE
Copy link
Contributor Author

@haukepetersen This PR includes now completely rewritten code. Instead of an additional thread, it's using nimBLE's callback architecture, so most of your previous concerns are invalid by now. There is also a second new module auto_nimble_advertise, which only handles advertising, which is not mandatory for stdio_nimble to work.

@HendrikVE
Copy link
Contributor Author

HendrikVE commented Sep 24, 2019

@haukepetersen
Copy link
Contributor

Coll, will have another look

@haukepetersen
Copy link
Contributor

Finally I got to reviewing this PR, sorry for the delay. First to my test results:

  1. the NimBLE shell does not seem to work for me, using the default example @ nrf52dk, compiling with USEMODULE="stdio_nimble auto_nimble_advertise". The shell over uart works as expected. Also I was able to connect from my node using the RIOT shell app from the Playstore. However, when issuing shell commands (like ps or help), the output in the app is scrambled and incomplete:
    Screenshot_20200207-162046_RIOT_OS_Bluetooth_Shell

  2. Running the same configuration without the additional uart shell (CFLAGS += -DSTDIO_NIMBLE_DISABLE_UART) behaves exactly the same. Output for unkown shell commands looks fine, but the output for known shell commands (ps, saul, help) is broken.

Additional, some thoughts about the structure of this PR:

  • I (still) don't like to combine uart with stdio_nimble. I understand, that this might have helped with the implementation, but structural it is a mess. Further, it does not help, that parts are (quote) copy-pasted from stdio_uart module . I think this is screwing with our (current) STDIO concept of only having a given STDIO device at any time. If we want to multiplex over more than a single STDIO device, this should be implemented generically on a systems level, not be hacked into specific STDIO implementations... So I would strongly opt for removing the complete uart support from this module!
  • I am not a 100% happy with the auto_nimble_advertise module. It implements partly generic and partly specific behavior, without allowing for more advanced configuration. E.g. it is in the current state designed for connectable advertisements only, supporting specifically a single connection, and no further AD fields... All this is not really implied by the name?! I would propose to generify this module by encapsulating a more generic and configurable core module with a simplified wrapper that then exposes an option to use it via auto_init. And having this in a separate PR is probably not a bad idea...
  • the buffering seems to be odd to me: if I read the code correctly, it is for stdio_write and stdio_read blocking, if there is not enough space in the buffers. IMO, it would be more straight forward to simply drop data in case of filled buffers. Of course the buffer sizes should be configurable, so that applications with a lot of STDIO thoughput can simply up their buffers to mitigate data loss. Actually, e.g. stdio_uart is also implemented this way.

Copy link
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more findings in the code. I think there is some room for improvement concerning the buffer handling in particular (which would also allow for significant code simplification).

@@ -196,6 +196,16 @@ void auto_init(void)
extern void nimble_riot_init(void);
nimble_riot_init();
#endif
#ifdef MODULE_STDIO_NIMBLE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to put the initialization of stdio_nimble and auto_nimble_advertise into the generic NimBLE initialization in pkg/nimble/contrib/nimble_riot.c. This would be more in line with other nimble-based modules, and even more important it would ensure the initialization order and make sure that NimBLE is always initialized before stdio_nimble_init() is called...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks for the hint!

Makefile.dep Outdated
@@ -457,7 +466,9 @@ endif

ifneq (,$(filter stdin,$(USEMODULE)))
ifneq (,$(filter stdio_uart,$(USEMODULE)))
USEMODULE += stdio_uart_rx
ifeq (,$(filter stdio_nimble,$(USEMODULE)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about ifneq (,$(filter stdio_uart stdio_nimble,$(USEMODULE)))?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm reverting to pre-PR state, because UART-in-parallel feature is removed.

@@ -0,0 +1,5 @@
ifeq (gnu, $(TOOLCHAIN))
CFLAGS += -Wno-cast-function-type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe better fix the code instead of suppressing the warning?!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this one. Honestly, I don't even remember anymore the reason why I've added this. There is not even a warning after removing this.

/**
* @brief UUID for stdio service (value: e6d54866-0292-4779-b8f8-c52bbec91e71)
*/
static const ble_uuid128_t gatt_svr_svc_stdio_uuid
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should better be allocated in the C file. If they should be available to other modules, I a define in the header is the better choice.

Having it like this would mean the compiler could possibly allocated this memory once for every compilation unit this header is included...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This header wasn't meant to be included by another module, as stdio_nimble is initiated by the auto_init mechanism. I moved these parts out anyway.

/**
* @brief Dummy access callback, because nimble requires one
*/
static int gatt_svr_chr_access_noop(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this function is implemented in the header file, it should at least be inlined. But why not put it into the *.c file?!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved in to .c


/* copy-pasted from stdio_uart module */
#ifndef STDIO_NIMBLE_DISABLE_UART
static void _init_stdio_uart(isrpipe_t *isrpipe)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why have a separate function for these 2 lines? The code becomes easier to read if they are put directly into stdio_init.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, because UART-in-parallel feature is removed.

}
#endif

static void _stdio_nimble_write(const void* buffer, size_t len)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, there is not really a reason for splitting this function if it is only used once. For a bad compiler this might just increase code size...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because UART-in-parallel feature is removed, I merged _stdio_nimble_write in to stdio_write. I generally like it to put code for different purposes in to their own functions. Both are for writing data, but if you see it at a lower level one is for sending to uart and the other for sending to nimble. That's why I've put them in separated functions.

switch (event->type) {

case BLE_GAP_EVENT_CONNECT:
_clear_ringbuffer();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why clear it? Wouldn't it be nicer to get all the node's output that was produced prio to the connect event? This way one could even see the boot messages (provided the buffer space is configured accordingly).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem I see here is that this is somewhat a strange behaviour. You also wouldn't expect to get old output from your old session when you start a new ssh session on a remote machine. In my opinion it could limit the use cases of this module, because people don't want to get the old output. Either because it's irrelevant output of a previous session or even worse something that should be kept secret and is now sent to someone else. In addition, even if not clearing the buffer, the boot message would be sent only on the first connection. But instead one could think about sending a welcome message after connecting.

_send_stdio = false;
}
_conn_handle = event->connect.conn_handle;
ble_att_set_preferred_mtu(MTU_MAX_NIMBLE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these functions should only be called if event->connect.status == 0, right?!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, fixed now. Thanks!

if (isrpipe_write_one(&_stdin_isrpipe, _stdin_data[i]) == -1) {

/* sleep for 5 ms to give consumer thread time to consume given data */
xtimer_usleep(5000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems bad to me to block the NimBLE host thread for potentially some time. As mentioned above: I think the way to go is simply to drop data in case the buffer(s) run full!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by dropping characters that don't fit into the ring buffer as you suggested.

@HendrikVE
Copy link
Contributor Author

HendrikVE commented Feb 10, 2020

Thank you very much for your detailed review! I've pushed several fixups, but the simpler buffer handling with potential data drop is still on it's way.
Also, I still need to think about a better advertisement module.

Well, I don't know why the output is broken four you, it's working on my phone ... classic ... *sigh*
I guess we will have to test my phone with your compiled application to identify whether it's a bug in the app.

@HendrikVE
Copy link
Contributor Author

Data drop for full buffers is implemented now.

I've outsourced the automated advertisement module. It's now in PR #13425. This PR was rebased to it. Therefore #13425 is now a dependency for this PR.

@HendrikVE
Copy link
Contributor Author

#17446 is now a dependency that should be merged first.

@fjmolinas
Copy link
Contributor

#17446 is now a dependency that should be merged first.

The dependency is now in!

@HendrikVE HendrikVE force-pushed the nimble_shell_module branch from 54a5b69 to e488836 Compare January 19, 2022 15:31
@HendrikVE
Copy link
Contributor Author

Rebased to master

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-tested today and still experience the same issue, I'm running the examples/twr_aloha test between two boards, one with `stdio_nimble. This is what happend:

2022-01-26 16:22:16,128 # twr req CC:95 -c 5
2022-01-26 16:22:16,129 # [twr]: start ranging
2022-01-26 16:22:17,119 # {"t": 89578, "src": "C3:29", "dst": "CC:95", "d_cm": 35}
2022-01-26 16:22:18,109 # {"t": 90570, "src": "C3:29", "dst": "CC:95", "d_cm": 37}
2022-01-26 16:22:19,099 # {"t": 91570, "src": "C3:29", "dst": "CC:95", "d_cm": 36}
2022-01-26 16:22:20,134 # {"t": 92570, "src": "C3:29", "dst": "CC:95", "d_cm": 40}
2022-01-26 16:22:21,124 # {"t": 93570, "src": "C3:29", "dst": "CC:95", "d_cm": 38}
> twr req CC:95 -c 100 -i 100
2022-01-26 16:22:25,759 # twr req CC:95 -c 100 -i 100
2022-01-26 16:22:25,759 # [twr]: start ranging
2022-01-26 16:22:25,849 # {"t": 98309, "src": "C3:29", "dst": "CC:95", "d_cm": 32}
2022-01-26 16:22:25,940 # {"t": 98402, "src": "C3:29", "dst": "CC:95", "d_cm": 31}
2022-01-26 16:22:26,028 # {"t": 98501, "src": "C3:29", "dst": "CC:95", "d_cm": 33}
2022-01-26 16:22:26,164 # {"t": 98601, "src": "C3:29", "dst": "CC:95", "d_cm": 35}
2022-01-26 16:22:26,253 # {"t": 98701, "src": "C3:29", "dst": "CC:95", "d_cm": 34}
2022-01-26 16:22:26,345 # {"t": 98801, "src": "C3:29", "dst": "CC:95", "d_cm": 31}
2022-01-26 16:22:26,434 # {"t": 98902, "src": "C3:29", "dst": "CC:95", "d_cm": 35}
2022-01-26 16:22:26,524 # {"t": 99001, "src": "C3:29", "dst": "CC:95", "d_cm": 35}
2022-01-26 16:22:26,660 # {"t": 99101, "src": "C3:29", "dst": "CC:95", "d_cm": 36}
2022-01-26 16:22:26,750 # {"t": 99201, "src": "C3:29", "dst": "CC:95", "d_cm": 33}
2022-01-26 16:22:26,839 # {"t": 99301, "src": "C3:29", "dst": "CC:95", "d_cm": 36}
2022-01-26 16:22:26,928 # {"t": 99402, "src": "C3:29", "dst": "CC:95", "d_cm": 36}
2022-01-26 16:22:27,065 # {"t": 99501, "src": "C3:29", "dst": "CC:95", "d_cm": 34}
2022-01-26 16:22:27,154 # {"t": 99601, "src": "C3:29", "dst": "CC:95", "d_cm": 35}
2022-01-26 16:22:27,245 # {"t": 99701, "src": "C3:29", "dst": "CC:95", "d_cm": 34}
2022-01-26 16:22:27,335 # {"t": 99801, "src": "C3:29", "dst": "CC:95", "d_cm": 34}
2022-01-26 16:22:27,423 # {"t": 99902, "src": "C3:29", "dst": "CC:95", "d_cm": 31}
2022-01-26 16:22:27,560 # {"t": 100001, "src": "C3:29", "dst": "CC:95", "d_cm": 34}
2022-01-26 16:22:27,650 # {"t": 100101, "src": "C3:29", "dst": "CC:95", "d_cm": 33}
2022-01-26 16:22:27,738 # {"t": 100201, "src": "C3:29", "dst": "CC:95", "d_cm": 36}
2022-01-26 16:22:27,964 # {"t": 100402, "src": "C3:29", "dst": "CC:95", "d_cm": 35}
2022-01-26 16:22:28,054 # {"t": 100501, "src": "C3:29", "dst": "CC:95", "d_cm": 32}
2022-01-26 16:22:28,144 # {"t": 100601, "src": "C3:29", "dst": "CC:95", "d_cm": 34}
2022-01-26 16:22:28,234 # {"t": 100701, "src": "C3:29", "dst": "CC:95", "d_cm": 34}
2022-01-26 16:22:28,323 # {"t": 100801, "src": "C3:29", "dst": "CC:95", "d_cm": 33}
2022-01-26 16:22:28,459 # {"t": 100902, "src": "C3:29", "dst": "CC:95", "d_cm": 35}
2022-01-26 16:22:28,550 # {"t": 101001, "src": "C3:29", "dst": "CC:95", "d_cm": 32}
2022-01-26 16:22:28,639 # {"t": 101101, "src": "C3:29", "dst": "CC:95", "d_cm": 35}
2022-01-26 16:22:28,728 # {"t": 101201, "src": "C3:29", "dst": "CC:95", "d_cm": 34}
2022-01-26 16:22:28,865 # {"t": 101302, "src": "C3:29", "dst": "CC:95", "d_cm": 34}
2022-01-26 16:22:28,955 # {"t": 101401, "src": "C3:29", "dst": "CC:95", "d_cm": 32}
2022-01-26 16:22:29,044 # {"t": 101501, "src": "C3:29", "dst": "CC:95", "d_cm": 35}
2022-01-26 16:22:29,135 # {"t": 101601, "src": "C3:29", "dst": "CC:95", "d_cm": 35}
2022-01-26 16:22:29,223 # {"t": 101702, "src": "C3:29", "dst": "CC:95", "d_cm": 32}
2022-01-26 16:22:29,449 # {"t": 101802, "src": "C3:29", "dst": "CC:95", "d_cm": 32}
2022-01-26 16:22:29,540 # {"t": 101901, "src": "C3:29", "dst": "CC:95", "d_cm": 35}
2022-01-26 16:22:29,629 # {"t": 102001, "src": "C3:29", "dst": "CC:95", "d_cm": 36}
2022-01-26 16:22:29,720 # {"t": 102101, "src": "C3:29", "dst": "CC:95", "d_cm": 30}
help
2022-01-26 16:22:49,001 # Serial port disconnected, waiting to get reconnected...
2022-01-26 16:22:50,003 # Serial port disconnected, waiting to get reconnected...
2022-01-26 16:22:51,004 # Serial port disconnected, waiting to get reconnected...
2022-01-26 16:22:52,005 # Try to reconnect to /tmp/dwafde again...
2022-01-26 16:22:52,006 # Reconnected to serial port /tmp/dwafde
> 2022-01-26 16:22:52,985 #
> 2022-01-26 16:22:52,986 # {"t": 124303, "src": "C3:29", "dst": "CC:95", "d_cm": 37}

The disconnected/reconnected happened at the time where I disconnected ble-serial because it was stuck, and on re-connect the applications resumed. But then I inverted who initiated the requests and it worked fine, event ith more intensive loads. So I guess its more of a combination of both use cases than stdio-nimble itself.

Still worried about mutex_lock in something that can be called in ISR though, could lead to a deadlock I think.

Regarding the test, I would have preferred an example, but will not nitpick on that. The test should be a tests-with-config though.

Once those two issues are addresed I think we can squash this one.

ssize_t stdio_write(const void* buffer, size_t len)
{
#if IS_USED(MODULE_STDIO_NIMBLE_DEBUG)
mutex_lock(&_debug_print_mutex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +179 to +185
#ifdef MODULE_STDIO_NIMBLE
extern void stdio_nimble_init(void);
/* stdio_nimble_init() needs to be called after nimble stack initialization
* and before nimble_autoadv_init() */
stdio_nimble_init();
#endif

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets leave it or a follow up.

@HendrikVE
Copy link
Contributor Author

Unfortunately I don't have the hardware to try examples/twr_aloha but I tried replicating the output behaviour and everything went fine using this configuration for me. When I am sending too many messages then nimble has no time to send the data and then I get a timeout and once in a while also the following message: 16:40:00.395 | ERROR | main.py: Bluetooth connection failed: [org.bluez.Error.Failed] Operation failed with ATT error: 0x11 (Unsupported Feature or Parameter Value)

@HendrikVE
Copy link
Contributor Author

But since my bluetooth adapter seems to be a bit crappy I can't differentiate whose fault it is.

@fjmolinas
Copy link
Contributor

Please squash @HendrikVE!

@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 28, 2022
@HendrikVE HendrikVE force-pushed the nimble_shell_module branch from 6b4f3a3 to b5a8c75 Compare January 28, 2022 16:28
@github-actions github-actions bot removed the Area: examples Area: Example Applications label Jan 28, 2022
@HendrikVE
Copy link
Contributor Author

While checking my squashed commits I noticed my changes in boards/nrf52dk/Makefile.dep. At the time I simply extended the existing rules, but now it seems that this is not necessary anymore as it was removed in the meantime. The test compiles and runs without these changes. So I guess I should remove it?

@HendrikVE
Copy link
Contributor Author

Found out that nordic_softdevice_ble was removed after the 2020.10 release. So these changes do not have any effect => remove

HendrikVE and others added 4 commits January 28, 2022 19:26
Implement a new module stdio_nimble, which uses nimble
for stdio. The characteristic for stdin is writable and
the characteristic for stdout uses the indicate mechanism
to publish the system's output to a connected device.
Data will be sent out asynchronously via callout functions.

The module can be enabled with "USEMODULE += stdio_nimble"

Co-authored-by: Francisco Molina <femolina@uc.cl>
@HendrikVE HendrikVE force-pushed the nimble_shell_module branch from b5a8c75 to f91751e Compare January 28, 2022 18:27
@github-actions github-actions bot removed the Area: boards Area: Board ports label Jan 28, 2022
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@fjmolinas
Copy link
Contributor

@haukepetersen your comments were quite some time ago, do you mind taking a look?

@fjmolinas
Copy link
Contributor

@haukepetersen your comments were quite some time ago, do you mind taking a look?

Or dismissing your review, whatever feels more pertinent.

@fjmolinas
Copy link
Contributor

I discarded @haukepetersen since it's been stale for quite a while. I think there was enough time for him to take a look. Let's get this in and get some usage feedback. Thanks for sticking through for so long @HendrikVE!

@fjmolinas fjmolinas merged commit 14f22c1 into RIOT-OS:master Feb 4, 2022
@HendrikVE
Copy link
Contributor Author

Awesome, thank you so much @fjmolinas for all your work on this! I'm happy to see this merged into master :) I hope to get around to fix my app soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: BLE Area: Bluetooth Low Energy support Area: build system Area: Build system Area: doc Area: Documentation Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: don't stale State: Tell state-bot to ignore this issue Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants