-
Notifications
You must be signed in to change notification settings - Fork 13
drivers: LPUARTE #202
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
drivers: LPUARTE #202
Conversation
7047e7e
to
356f6a2
Compare
Should the LPUART maybe just be a sample rather than a driver? Don't we expect users to use NRFX directly? Maybe it would be more efficient to show users how to do LP UART using NRFX rather than "hiding" that inside a driver behind a new API :) By extension, the only new config would be |
Good question. A bit of clarification on the decision to use nrfx directly. This means that in BM context we use nrfx drivers directly and do not involve zephyr drivers etc. that depends on DTS. This does not mean that customers must always interact directly with nrfx, we can make libraries and drivers on top that which use nrfx under the hood. End goal is that we want to make it easy for users to add LPUART to their application. Hence, we should have a sample to show that, and that alone. We also would like to have support for using LPUARTE in the NUS sample. Instead of having duplicate code in both the LPUART and NUS sample, I found it easier and more maintainable to have this as a driver rather than in the sample directly. This also makes it so that if we do changes to the driver, the user "gets that for free" and don't need to do the exact same changes to the code they copied into their application, which would be the case if it was part of the sample.
That is a question for @nrfconnect/ncs-ll-izera. |
Overview | ||
******** | ||
|
||
The sample initializes the application LPUARTE instance, specified in the :file:`board-config.h` file in the board. | ||
It then implements a simple loopback using a single LPUARTE instance. | ||
By default, the console and logging are disabled to demonstrate low power consumption when UART is active. |
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.
Please add a detailed description, or link to one, of what "LPUART" is supposed to do. I have never heard of it, but it must be documented somewhere with some state diagrams or something since it exists in NCS :)
This would help me and other users understand what the implementation is meant to do
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 agree that it should be introduced as a Low Power alternative to UART.
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.
Added documentation for LPUART driver based on NCS documentation. In NCS (https://docs.nordicsemi.com/bundle/ncs-3.0.2/page/nrf/drivers/uart_nrf_sw_lpuart.html#uart-nrf-sw-lpuart) the documentation is referencing Device Tree and Kconfig symbols that are not present in BM, so I am afraid linking the documentation would be confusing.
Absolutely, we do that for the logging, console and if #205 is merged, the shell backend :)
Understood. The LPUART implementation here has the same restrictions as the logging, console and shell backends (single instance "hardcoded" to board-config). I thought since it was placed in drivers it was meant to be "fully" portable like the NRFX drivers, which looking at the code it clearly isn't. Would it make sense to move the LPUART implementation to subsys to make this clear, and/or document that clearly by for example not having
Looking at the implementation in NCS of sw_lpuart, this is definately not something that should be in NRFX :D I was thinking of "Supports return to the IDLE state between transactions (when using HW flow control)" mentioned here https://docs.nordicsemi.com/bundle/ps_nrf54L15/page/uarte.html but that is clearly nothing like what sw_lpuart is doing. |
It is meant to be possible to have multiple instances, though then the user must specify the pins and instance themselves (e.g. in their custom board definition). For NCS BM samples we will probably stick to one instance. PR is still in draft, there is a lot of cleanup to be done! |
Out of curiosity, who are the target users of BLE_NUS and SW_LPUART with NCS BM? |
include/lpuarte.h
Outdated
nrfx_gpiote_pin_t rdy_pin; | ||
}; | ||
|
||
nrfx_err_t bm_lpuarte_init(struct bm_lpuarte *lpu, |
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 not NRF_ERRORS? I believe all of the Bare Metal libraries should return those.
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 used nrfx errors as this is similar to the nrfx uarte driver, just with some extra logic. So it is easy to switch from one to another. Have a look at the NUS sample.
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 can understand the reasoning, but makes for an inconsistent API when presented alongside the others. Not sure what I would do differently to address this though.
907f8a7
to
1704e06
Compare
9da0082
to
95c7fa1
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.
Looks good!
ed43ed3
to
28d63f8
Compare
de048f5
to
70c3c9c
Compare
@nrfconnect/ncs-co-boards , @nrfconnect/ncs-co-build-system, @nrfconnect/ncs-bm-doc Please review! |
case 0: | ||
return &gpiote30_instance; | ||
case 1: | ||
return &gpiote20_instance; |
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.
but they can actually use any pin on any port?
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, we need a better solution for GPIOTE in general. @anhmolt is looking into a separate module.
samples/peripherals/lpuarte/Kconfig
Outdated
menu "LPUARTE sample" | ||
|
||
config LPUARTE_DATA_LEN_MAX | ||
int "maximum UARTE data length" |
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.
int "maximum UARTE data length" | |
int "Maximum UARTE data length" |
samples/bluetooth/ble_nus/Kconfig
Outdated
|
||
endif #BLE_UART_HWFC | ||
endif #BLE_LPUART |
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.
endif #BLE_LPUART | |
endif # BLE_LPUART |
samples/bluetooth/ble_nus/Kconfig
Outdated
endif #BLE_LPUART | ||
|
||
config NUS_UART_HWFC | ||
bool "BLE UART use HWFC" |
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.
not really BLE UART, it's NUS UART
samples/bluetooth/ble_nus/README.rst
Outdated
Building and running with LPUARTE | ||
********************************* | ||
|
||
The **overlay-lpuarte.conf** file configures the sample to use the :ref:`lpuarte` driver for the NUS Service. |
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.
use
:zephyr:file:`<blah>`
syntax
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 don't see that anywhere? Assume you mean :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.
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.
Replaced with : 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.
:file:`filename`
is the correct approach.
samples/bluetooth/ble_nus/src/main.c
Outdated
|
||
/* Maximum length of data (in bytes) that can be transmitted to the peer by the | ||
* Nordic UART service module. | ||
*/ | ||
static volatile uint16_t ble_nus_max_data_len = BLE_NUS_MAX_DATA_LEN_CALC(BLE_GATT_ATT_MTU_DEFAULT); | ||
|
||
/* Receive buffer used in UART ISR callback */ | ||
static uint8_t uarte_rx_buf[4]; | ||
static uint8_t uarte_rx_buf[IS_ENABLED(CONFIG_NUS_LPUARTE) ? 128 : 2]; |
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.
surely this value should come from a Kconfig?
241bcc7
to
25e8b5d
Compare
38efc3e
to
773ffa1
Compare
samples/bluetooth/ble_nus/Kconfig
Outdated
if BLE_UART_HWFC | ||
|
||
config BLE_UART_PIN_CTS | ||
int "BLE UART CTS pin" | ||
default 7 if SOC_SERIES_NRF52X | ||
default 3 | ||
|
||
config BLE_UART_PIN_RTS | ||
int "BLE UART RTS pin" | ||
default 5 if SOC_SERIES_NRF52X | ||
default 2 | ||
|
||
endif #BLE_UART_HWFC | ||
|
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 BLE_UART_HWFC | |
config BLE_UART_PIN_CTS | |
int "BLE UART CTS pin" | |
default 7 if SOC_SERIES_NRF52X | |
default 3 | |
config BLE_UART_PIN_RTS | |
int "BLE UART RTS pin" | |
default 5 if SOC_SERIES_NRF52X | |
default 2 | |
endif #BLE_UART_HWFC |
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 is in the first commit)
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 in first commit.
@@ -19,6 +19,8 @@ | |||
#include <zephyr/logging/log_ctrl.h> | |||
#include <zephyr/sys/util.h> | |||
|
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.
there is no separation in include types above
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.
Removed.
add_subdirectory_ifdef(CONFIG_CONSOLE console) | ||
add_subdirectory_ifdef(CONFIG_CLOCK_CONTROL clock_control) | ||
add_subdirectory_ifdef(CONFIG_FLASH flash) | ||
add_subdirectory_ifdef(CONFIG_BM_SW_LPUARTE lpuarte) |
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.
@nordicjm to go through and add missing sort lines
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.
As I understand it, this will be a separate PR.
@@ -0,0 +1,8 @@ | |||
# This file enables Low Power UARTE (LPUARTE) driver for the NUS UART instance. |
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 is not a (dtc) overlay file, this is a Kconfig fragment, rename to lpuarte.conf
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.
changed to lpuarte.conf, here and others.
platform_allow: | ||
- bm_nrf54l15dk/nrf54l05/cpuapp/s115_softdevice | ||
- bm_nrf54l15dk/nrf54l10/cpuapp/s115_softdevice | ||
- bm_nrf54l15dk/nrf54l15/cpuapp/s115_softdevice | ||
- bm_nrf54l15dk/nrf54l05/cpuapp/s115_softdevice/mcuboot | ||
- bm_nrf54l15dk/nrf54l10/cpuapp/s115_softdevice/mcuboot | ||
- bm_nrf54l15dk/nrf54l15/cpuapp/s115_softdevice/mcuboot | ||
extra_args: EXTRA_CONF_FILE="overlay-lpuarte.conf" |
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.
and adjust here
samples/bluetooth/ble_nus/README.rst
Outdated
Building and running with LPUARTE | ||
********************************* | ||
|
||
The :file:`overlay-lpuarte.conf` file configures the sample to use the :ref:`LPUARTE <driver_lpuarte>` driver for the NUS Service. |
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.
and here
config NUS_UART_RX_BUF_SIZE | ||
int "NUS UART receive buffer size" | ||
default 128 if NUS_LPUARTE | ||
default 1 |
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 help help text saying that this is double buffered so the actual buffer size is 2* this
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.
Added help text.
samples/bluetooth/ble_nus/src/main.c
Outdated
if (err != NRFX_SUCCESS) { | ||
LOG_ERR("UARTE TX failed, nrfx err %d", err); | ||
goto idle; | ||
return -1; |
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.
main must only ever return 0
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.
Corrected back to goto idle.
773ffa1
to
636960a
Compare
0b4b43a
to
16f133c
Compare
Use board-config.h for uarte pin and instance config. Signed-off-by: Eivind Jølsgard <[email protected]>
Add low power uarte driver and sample. Based on NCS LPUARTE driver and sample, nrfconnect/sdk-nrf/commit/d33871cc92bc50034a6d9b7bdcac94c0ff358390 Co-authored-by: Andreas Moltumyr <[email protected]> Signed-off-by: Eivind Jølsgard <[email protected]>
Add support for LPUARTE to NUS sample. Co-authored-by: Andreas Moltumyr <[email protected]> Signed-off-by: Eivind Jølsgard <[email protected]>
16f133c
to
80ff0d6
Compare
Add LPUARTE driver and sample.
Add LPUARTE support for NUS sample.
Depends on #221 for lowest power consumption.