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

include: uart: Another pass on improving docstrings #10761

Merged
merged 1 commit into from Oct 31, 2018

Conversation

pfalcon
Copy link
Contributor

@pfalcon pfalcon commented Oct 22, 2018

  1. Avoid outdated references to registers of a particular hardware
    in the generic API.

  2. Propagate specifications/clarifications of ISR behavior to
    docstrings of more functions which guaranteed to work only in ISR.

This continues work previously done in:

38f78e8
0fdc9b5
etc.

Signed-off-by: Paul Sokolovsky paul.sokolovsky@linaro.org

@pfalcon pfalcon added the area: UART Universal Asynchronous Receiver-Transmitter label Oct 22, 2018
@codecov-io
Copy link

codecov-io commented Oct 22, 2018

Codecov Report

Merging #10761 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10761   +/-   ##
=======================================
  Coverage   53.08%   53.08%           
=======================================
  Files         215      215           
  Lines       25820    25820           
  Branches     5692     5692           
=======================================
  Hits        13707    13707           
  Misses       9803     9803           
  Partials     2310     2310

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24da652...34a3541. Read the comment docs.

@pfalcon
Copy link
Contributor Author

pfalcon commented Oct 23, 2018

@gon1332: FYI, feel free to have a look/review.

include/uart.h Outdated
@@ -578,6 +596,7 @@ static inline int _impl_uart_irq_update(struct device *dev)
*
* This sets up the callback for IRQ. When an IRQ is triggered,
* the specified function will be called with specified user data.
* See description of uart_urq_update() for requirements on the ISR.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo in "uart_irq_update()"

@pfalcon
Copy link
Contributor Author

pfalcon commented Oct 23, 2018 via email

1. Avoid outdated references to registers of a particular hardware
in the generic API.

2. Propagate specifications/clarifications of ISR behavior to
docstrings of more functions which guaranteed to work only in ISR.

This continues work previously done in:

38f78e8
0fdc9b5
etc.

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
@pfalcon
Copy link
Contributor Author

pfalcon commented Oct 23, 2018

This came as a side effect of reviewing UART driver in #9042, and seeing that the bunch of docstrings don't mention constraints specified in other docstrings. And going over this for Nth, with everything else happening inbetween (like, people spawning more drivers with raw ISRs for UART access, weird bugreports), following thoughts are dawning:

  1. The UART driver API is too low-level, and despite all attempts to specify constraints, following which it's intended to be possible to write "cross-platform ISRs", there's always chance of some corner case not covered.
  2. Even more surprising was that bunch of IRQ-related functionality of UART API was made userspace syscalls. @andrewboie, how did we get to that? Even with reading the docstring, it's pretty clear that uart_irq_tx_enable() is IRQ related. What's the point of exporting it userspace, like, user calls uart_irq_tx_enable(), what happens next? It's not possible to define ISR callback from userspace anyway. And most of the UART API is strictly related to interrupt-driven mode. Only uart_poll_in()/uart_poll_out() could be exposed to userspace (and perhaps err_check()). They're also not too useful, but why not? IMHO, we should fix that ASAP.

@pfalcon pfalcon requested a review from galak October 29, 2018 15:46
@pfalcon
Copy link
Contributor Author

pfalcon commented Oct 29, 2018

Ping.

@nashif nashif merged commit d850b3a into zephyrproject-rtos:master Oct 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: UART Universal Asynchronous Receiver-Transmitter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants