cpu/nrf5x_common: fix uart_poweroff() #19926
Conversation
|
bors merge |
19923: boards: add Silabs EFM32 Giant Gecko GG11 Starter Kit r=benpicco a=gschorcht ### Contribution description The PR adds the support for the EFM32GG11B family and the Silabs EFM32 Giant Gecko GG11 Starter Kit board. The Silabs EFM32 Giant Gecko GG11 has the following on-board features: - EFM32GG11B MCU with 2 MB flash and 512 kB RAM - J-Link USB debugger - 176x176 RGB LCD (not supported) - 2 user buttons, 2 user RGB LEDs and a touch slider - Si7021 Relative Humidity and Temperature Sensor - Si7210 Hall-Effect Sensor (not supported) - USB OTG interface (Device mode supported) - 32 MByte Quad-SPI Flash (not supported yet) - SD card slot (not supported yet, follow-up PR based on PR #19760) - RJ-45 Ethernet (not supported) - Dual microphones (not supported) ### Testing procedure Basic tests should work. ### Issues/PRs references 19926: cpu/nrf5x_common: fix uart_poweroff() r=benpicco a=maribu ### Contribution description Previously, uart_poweroff() and uart_poweron() were no-ops. This replaces them with the logic to indeed power on and power off the UART device. ### Testing procedure - ideally, power consumption should be less after a call to `uart_poweroff()` - beware: this is untested. however, at least a call to `uart_write()` while powered off would get stuck if not for the special handling, while before the UART remained fully operational when "powered off" - regular operation should resume after again calling `uart_poweron()` - this is tested ### Issues/PRs references None Co-authored-by: Gunar Schorcht <gunar@schorcht.net> Co-authored-by: Marian Buschsieweke <marian.buschsieweke@posteo.net>
|
Build failed (retrying...): |
19926: cpu/nrf5x_common: fix uart_poweroff() r=benpicco a=maribu
### Contribution description
Previously, uart_poweroff() and uart_poweron() were no-ops. This replaces them with the logic to indeed power on and power off the UART device.
### Testing procedure
- ideally, power consumption should be less after a call to `uart_poweroff()`
- beware: this is untested. however, at least a call to `uart_write()` while powered off would get stuck if not for the special handling, while before the UART remained fully operational when "powered off"
- regular operation should resume after again calling `uart_poweron()`
- this is tested
### Issues/PRs references
None
Co-authored-by: Marian Buschsieweke <marian.buschsieweke@posteo.net>
|
bors cancel The build failure seemingly was caused by this PR :-/ |
|
Canceled. |
e70e5c0 to
3ddfeee
Compare
3ddfeee to
0ac856d
Compare
|
Probably not worth backporting but I always like fixes, Is it still WIP? |
|
To test this PR, I modified the tests/periph/uart test a little bit and added significantly longer delays so a slow human (me) can read off the multimeter in time. This is from the tests/periph/uart/main.c file, lines 203ff: static void sleep_test(int num, uart_t uart)
{
printf("UARD_DEV(%i): test uart_poweron() and uart_poweroff() -> ", num);
uart_poweroff(uart);
//xtimer_usleep(POWEROFF_DELAY);
xtimer_msleep(4000);
uart_poweron(uart);
xtimer_msleep(4000);
puts("[OK]");
}When the UART is powered off, the nRF52832 on my nRF52DK consumes 0.474mA and with the UART powered on it consumes 0.530mA. IMO this PR is well worth backporting, especially since most of the changes regarding the register accesses are already done in the mainline code, so this PR should shrink to only a handful of lines changed. |
0ac856d to
a4bd18c
Compare
|
OK, rebased. I don't have the hardware at hands, so this is yet untested. The looks reasonable after resolving the merge conflict, though. |
|
I can't add that as a comment, but you can delete the ENABLE_ON and ENABLE_OFF macros from lines 44, 45, 52 and 53. They are now unused. This is the expansion of the test code in tests/periph/uart: static void sleep_test(int num, uart_t uart)
{
printf("UARD_DEV(%i): test uart_poweron() and uart_poweroff() -> ", num);
xtimer_msleep(5);
uart_poweroff(uart);
uint8_t data[8] = "asdf";
uart_write(uart, data, sizeof(data)); // this should not output any data but not hang either
//xtimer_usleep(POWEROFF_DELAY);
xtimer_msleep(4000);
uart_poweron(uart);
xtimer_msleep(4000);
puts("[OK]");
}The additional 5ms sleep is there to make sure the data is sent before the UART goes to sleep. This looks good from my side, thank you for addressing this so quickly :) |
Indeed. But I wonder if those were actually more readable than what I did when mergeing 🤣 |
126d5e8 to
4c4f996
Compare
|
I was so free to squash directly and I changed the code to instead make use of |
|
It probably won't help if I say "works for me" 👀 I can't really say if the board is actually sending something, but it looks convincing: For some reason though the ble subsystem is not added for the nRF52840DK and I did not figure out why. This is what the log looks like for the nRF52DK: But neither of these things have anything to do with what you're seeing I guess? |
No. I also fail to reproduce the issue on a different nrf52840-dk (one that has "nRF52840-Preview-DK" on the silkscreen where the affected board only has "nRF52840-DK"). Maybe I should check the errata. |
|
The board I have here is a PCA10056 Revision 3.0.1 from 2023.6. The nRF52840 has the following text written on it: QIAAF0 means that the chip is Revision 3. We have some chips here with Revision CKAAD0, which is Revision 2. But they are not on a Devboard, but integrated in another project. |
|
OK, here is what I have found so far:
I'm almost certain it is an issue with RIOT not implementing the locks newlib requires to correctly implement reentrant functions, but only protects In any case, I'm 99.999% certain that the issues are unrelated to this PR. |
de99c64 to
f20f795
Compare
|
I just rebased this. It does now indeed still fix the test in |
cpu/nrf5x_common/periph/uart.c
Outdated
| @@ -242,6 +260,7 @@ void uart_poweron(uart_t uart) | |||
|
|
|||
| if (isr_ctx[uart].rx_cb) { | |||
| uart_config[uart].dev->TASKS_STARTRX = 1; | |||
| set_power(uart, true); | |||
There was a problem hiding this comment.
So if only tx is configured the UART stays powered down?
There was a problem hiding this comment.
I probably wanted to do the same as we do on MSP430 here: In TX-Only, only power up the UART right before sending and back off afterwards, so that between log messages, a device can go to deeper power mode.
But let's do this in a separate PR. (And properly, with actually turning on the device during uart_write() in case we are in on-demand mode.)
f20f795 to
864182f
Compare
cpu/nrf5x_common/periph/uart.c
Outdated
| if (isr_ctx[uart].rx_cb) { | ||
| uart_config[uart].dev->TASKS_STARTRX = 1; | ||
| } | ||
| uart_config[uart].dev->TASKS_STARTRX = 1; |
There was a problem hiding this comment.
I think this should not be turned on unconditionally. If there is no RX callback and data in the RX buffer is overwritten, an error event will be generated. Or a lot of error events.
There was a problem hiding this comment.
Before I forget: with the current code, the TASK will be started before the UART is enabled, I wonder if that's allowed by the specs?
There was a problem hiding this comment.
I have no idea why I did that. I just reversed it. Would you mind giving it another spin to be sure that this also works?
864182f to
a57857d
Compare
|
Thanks for getting back to this. I don't have any nRF52 stuff at home unfortunately, so I'll give this a try on Monday back in the office :) |
|
No problem :) This PR was stale so long, I guess there is no need for a rush now :) |
|
So I gave this a short test and the power on and power off seems to work as it should: However the test itself does not: With This seems to be an issue that was fixed in |
|
I should probably rebase this prior merging in case someone needs to bisect things. Otherwise it would be highly confusing why the merge commit works, but the commit that was merged does not. |
a57857d to
a5d61a8
Compare
Previously, uart_poweroff() and uart_poweron() were no-ops. This replaces them with the logic to indeed power on and power off the UART device. Co-authored-by: crasbe <crasbe@gmail.com>
a5d61a8 to
6c0d7fc
Compare
|
Thanks a lot :) |
Contribution description
Previously, uart_poweroff() and uart_poweron() were no-ops. This replaces them with the logic to indeed power on and power off the UART device.
Testing procedure
uart_poweroff()uart_write()while powered off would get stuck if not for the special handling, while before the UART remained fully operational when "powered off"uart_poweron()Issues/PRs references
None