Improve central_uart sample application #12537
Conversation
In contrast to the peripheral_uart sample application the central_uart application uses hard-coded values for buffer size, UART Rx wait time and NUS write timeout. This patch replaces the hard-coded values with proper entries in a Kconfig file to make this sample match the peripheral_uart sample application. Signed-off-by: Michael Hunold <michael.hunold@eu.panasonic.com>
The peripheral_uart supports the Kconfig option BT_NUS_SECURITY_ENABLED
which allows the sample application to be built with or without support
for a secure connection.
This option is not present in the central_uart sample, it
unconditionally enforces a secure connection.
If the peripheral_uart sample application is built without support for a
secure connection, this leads to the following error and warning
messages in the log file for the central_uart sample application when a
connection is established:
<err> bt_smp: pairing failed (peer reason 0x5)
<wrn> central_uart: Security failed: xx:xx:xx:xx:xx:xx (random)
level 1 err 5
<wrn> central_uart: Pairing failed conn: xx:xx:xx:xx:xx:xx (random),
reason 5
This patch adds BT_NUS_SECURITY_ENABLED to the central_uart sample
application similarly to the peripheral_uart sample application, but the
default behaviour when building the sample application is unchanged.
Signed-off-by: Michael Hunold <michael.hunold@eu.panasonic.com>
pideu-mh
requested review from
alwa-nordic,
carlescufi and
KAGA164
as code owners
github-actions
bot
added
the
changelog-entry-required
|
Thank you for your contribution! Note: This comment is automatically posted and updated by the Contribs GitHub Action. |
alwa-nordic
added
the
CI-Requested
The peripheral_uart and central_uart sample applications use almost identical code for the UART handling. But one difference is the usage of the UART_RX_TIMEOUT value. It is used in uart_rx_enable() and is expected to be a value in microseconds. The peripheral_uart sample application uses a value of 50000 for UART_RX_TIMEOUT which corresponds to a timeout of 50ms, which is pretty reasonable for a timeout value for the UART. Due to whatever reasons the central_uart application uses a value of 50 for UART_RX_TIMEOUT which corresponds to a timeout of just 50 microseconds, which looks unreasonable. The assumption here now is that this was simply an oversight and really 50000 should have been used right from the beginning. This patch fixes this and makes the handling of peripheral_uart and central_uart identical when it comes to UART_RX_TIMEOUT. Signed-off-by: Michael Hunold <michael.hunold@eu.panasonic.com>
pideu-mh
force-pushed
the
v2.4-central_uart
branch
from
e225c1d
to
2bfdc4c
Compare
github-actions
bot
removed
the
CI-Requested
|
Unfortunately the compliance checks on the patch series failed, because one commend in the commit message exceeded the max. line length. I reformatted the commit message and updated the patch series. No other changes were done. |
alwa-nordic
added
the
CI-Requested
|
Please add entry in sample.yaml: sample.bluetooth.central_uart.security_disabled: |
| default y | ||
| select BT_SMP | ||
| help | ||
| "Enable BLE security for the UART service client" |
There was a problem hiding this comment.
Please add dot an every help line end
| "Enable BLE security for the UART service client" | |
| Enable BLE security for the UART service client |
| @@ -384,12 +384,16 @@ static void connected(struct bt_conn *conn, uint8_t conn_err) | |||
| LOG_WRN("MTU exchange failed (err %d)", err); | |||
| } | |||
|
|
|||
| #ifdef CONFIG_BT_NUS_SECURITY_ENABLED | |||
There was a problem hiding this comment.
You could use here IS_ENABLED macro instead of conditional compilation
| #ifdef CONFIG_BT_NUS_SECURITY_ENABLED | |
| if(IS_ENABLED(CONFIG_BT_NUS_SECURITY_ENABLED)) { |
|
|
||
| int main(void) | ||
| { | ||
| int err; | ||
|
|
||
| #ifdef CONFIG_BT_NUS_SECURITY_ENABLED |
There was a problem hiding this comment.
Please try to use IS_ENABLED macro like it is done in the peripheral_uart sample
|
|
||
| menu "Nordic UART Service Client sample" | ||
|
|
||
| config BT_NUS_UART_BUFFER_SIZE |
There was a problem hiding this comment.
Maybe BT_NUS_C_.... since this is about client side
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
This patch set tries to make the central_uart sample application more similar to the peripheral_uart sample application in the following ways:
Please check out the individual commit messages for more details.