Skip to content
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

cpu/nrf5x/uart: run STOPTX task after finished tx #18954

Merged
merged 1 commit into from
Nov 24, 2022

Conversation

jue89
Copy link
Contributor

@jue89 jue89 commented Nov 22, 2022

Contribution description

The examples/hello-world shows a power consumption of 400uA on the nrf52dk board, even though stdin isn't wired and, thus, RX of the periph_uart isn't wired.

This PR makes sure that TASKS_STARTTX is called before a transmission. After completion the TASKS_STOPTX is run allowing for entering low power mode.

Testing procedure

nrf51 and nrf52 CPUs should be able to transmit octets using periph_uart with and without the uart_nonblocking feature.

Issues/PRs references

@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Nov 22, 2022
@jue89 jue89 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 22, 2022
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

lgtm, trusting your testing. I have one style suggestion. Please squash right away.

cpu/nrf5x_common/periph/uart.c Outdated Show resolved Hide resolved
This reduces power consumption for UARTs that are configured in tx-only mode.
@jue89 jue89 force-pushed the fix/nrf5x-uart-lowpower branch from 9b4f684 to 3da1fac Compare November 22, 2022 20:33
@kaspar030
Copy link
Contributor

is there any way to test if this affects parallel printing? (two threads printing at the same time, one stopping tx after finishing, killing tx for the other)

@jue89
Copy link
Contributor Author

jue89 commented Nov 22, 2022

is there any way to test if this affects parallel printing? (two threads printing at the same time, one stopping tx after finishing, killing tx for the other)

Well, parallel printing would crash anyway - for the nrf52 the driver is using DMA. To remove this race, we would need mutexes, I guess?

@jue89
Copy link
Contributor Author

jue89 commented Nov 22, 2022

I'll try to come up with a solution. Something like this:

  • nrf51: reference count the parallel uart_write() calls. Call run TASKS_STOPTX only if all calls have returned.
  • nrf52: spin-lock if another DMA process is currently on-going.

What do you think? Do you have better ideas?

@maribu
Copy link
Member

maribu commented Nov 23, 2022

Note that UART is slow. If the DMA is still busy with shuffling many many bytes out of the slow UART, it likely is better to just switch to another thread.

I'd just go for a mutex. They are super-duper fast in the hot path, and still well optimized when they actually need to block.

I'd also say that parallel UART access is a rare corner case. So most of the time the mutex will be in the super-duper fast hot path.

@haukepetersen
Copy link
Contributor

LGTM as well.

Regarding the Mutex: IMHO there is no need for the (low-level) UART driver to be thread safe. This should be handled by the module acutually using the UART, e.g. STDIO or something like a Modbus library etc. However this should of course be documented (if it is not already) :-)

@maribu
Copy link
Member

maribu commented Nov 23, 2022

This should be handled by the module acutually using the UART

Agreed! Having this implemented once e.g. in stdio rather than for each and every UART driver makes much more sense.

@haukepetersen
Copy link
Contributor

Also one might want to use periph_uart in ISR context, and that won't work too well with mutexes involved, right?!

@aabadie aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 23, 2022
@jue89
Copy link
Contributor Author

jue89 commented Nov 23, 2022

This should be handled by the module acutually using the UART

Agreed! Having this implemented once e.g. in stdio rather than for each and every UART driver makes much more sense.

Is stdio thread-safe? I see exactly this case: a thread is interrupted while printing to stdout and the ISR also logs something to stdout.

@jue89
Copy link
Contributor Author

jue89 commented Nov 23, 2022

I'd just go for a mutex. They are super-duper fast in the hot path, and still well optimized when they actually need to block.

But how to handle ISRs calling uart_write()? Afaik are mutex_lock() and ISRs no friends ...

@maribu
Copy link
Member

maribu commented Nov 23, 2022

stdio from ISR is only allowed for debugging. And in that case, one wants to use stdio_rtt (which is now also supported with OpenOCD, btw.) anyway to reduce the risk of delaying the ISR too much to reproduce the bug.

@jue89
Copy link
Contributor Author

jue89 commented Nov 23, 2022

So keep this PR like it is and wait for Murdock?

@riot-ci
Copy link

riot-ci commented Nov 23, 2022

Murdock results

✔️ PASSED

3da1fac cpu/nrf5x/uart: run STOPTX task after finished tx

Success Failures Total Runtime
117849 0 117849 01h:56m:03s

Artifacts

@jue89
Copy link
Contributor Author

jue89 commented Nov 24, 2022

Murdock is green. I would merge, if nobody objects?

The nrf5x periph_uart wasn't thread-safe before this PR and this PR doesn't change this.
If this needs to be changed, I'd do that in a follow-up PR.

@maribu maribu merged commit 020c6ff into RIOT-OS:master Nov 24, 2022
@jue89
Copy link
Contributor Author

jue89 commented Nov 24, 2022

Thank you all :)

@jue89 jue89 deleted the fix/nrf5x-uart-lowpower branch November 24, 2022 13:21
@kaspar030 kaspar030 added this to the Release 2023.01 milestone Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants