-
Notifications
You must be signed in to change notification settings - Fork 2.1k
sys/stdio_nimble: add new stdio module using nimble #12012
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
a50ee27
to
99e7f3a
Compare
There was a problem hiding this 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 thetype
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...
99e7f3a
to
0c232fc
Compare
@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 |
0c232fc
to
89a1d09
Compare
Coll, will have another look |
89a1d09
to
d933865
Compare
d933865
to
6f018f6
Compare
6f018f6
to
2709a9a
Compare
There was a problem hiding this 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).
sys/auto_init/auto_init.c
Outdated
@@ -196,6 +196,16 @@ void auto_init(void) | |||
extern void nimble_riot_init(void); | |||
nimble_riot_init(); | |||
#endif | |||
#ifdef MODULE_STDIO_NIMBLE |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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))) |
There was a problem hiding this comment.
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)))
?
There was a problem hiding this comment.
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.
sys/auto_nimble_advertise/Makefile
Outdated
@@ -0,0 +1,5 @@ | |||
ifeq (gnu, $(TOOLCHAIN)) | |||
CFLAGS += -Wno-cast-function-type |
There was a problem hiding this comment.
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?!
There was a problem hiding this comment.
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.
sys/include/stdio_nimble.h
Outdated
/** | ||
* @brief UUID for stdio service (value: e6d54866-0292-4779-b8f8-c52bbec91e71) | ||
*/ | ||
static const ble_uuid128_t gatt_svr_svc_stdio_uuid |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
sys/include/stdio_nimble.h
Outdated
/** | ||
* @brief Dummy access callback, because nimble requires one | ||
*/ | ||
static int gatt_svr_chr_access_noop( |
There was a problem hiding this comment.
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?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved in to .c
sys/stdio_nimble/stdio_nimble.c
Outdated
|
||
/* copy-pasted from stdio_uart module */ | ||
#ifndef STDIO_NIMBLE_DISABLE_UART | ||
static void _init_stdio_uart(isrpipe_t *isrpipe) |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
sys/stdio_nimble/stdio_nimble.c
Outdated
} | ||
#endif | ||
|
||
static void _stdio_nimble_write(const void* buffer, size_t len) |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
sys/stdio_nimble/stdio_nimble.c
Outdated
switch (event->type) { | ||
|
||
case BLE_GAP_EVENT_CONNECT: | ||
_clear_ringbuffer(); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
sys/stdio_nimble/stdio_nimble.c
Outdated
_send_stdio = false; | ||
} | ||
_conn_handle = event->connect.conn_handle; | ||
ble_att_set_preferred_mtu(MTU_MAX_NIMBLE); |
There was a problem hiding this comment.
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?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fixed now. Thanks!
sys/stdio_nimble/stdio_nimble.c
Outdated
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); |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
43c044c
to
78f939f
Compare
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. Well, I don't know why the output is broken four you, it's working on my phone ... classic ... *sigh* |
0ed647f
to
0ab2234
Compare
3c956ef
to
c66a928
Compare
9d04046
to
6583c61
Compare
#17446 is now a dependency that should be merged first. |
The dependency is now in! |
54a5b69
to
e488836
Compare
Rebased to master |
There was a problem hiding this 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.
sys/stdio_nimble/stdio_nimble.c
Outdated
ssize_t stdio_write(const void* buffer, size_t len) | ||
{ | ||
#if IS_USED(MODULE_STDIO_NIMBLE_DEBUG) | ||
mutex_lock(&_debug_print_mutex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the same concern as https://github.com/RIOT-OS/RIOT/pull/12012/files#r792789326
#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 | ||
|
There was a problem hiding this comment.
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.
Unfortunately I don't have the hardware to try |
But since my bluetooth adapter seems to be a bit crappy I can't differentiate whose fault it is. |
Please squash @HendrikVE! |
6b4f3a3
to
b5a8c75
Compare
While checking my squashed commits I noticed my changes in |
Found out that |
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>
b5a8c75
to
f91751e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
@haukepetersen your comments were quite some time ago, do you mind taking a look? |
Or dismissing your review, whatever feels more pertinent. |
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! |
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. |
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 exampledefault
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