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

serial: uart_irq_tx_ready() needs better documentation or correction of the test #11535

Closed
anangl opened this issue Nov 20, 2018 · 7 comments
Closed
Assignees
Labels
area: API Changes to public APIs area: UART Universal Asynchronous Receiver-Transmitter Enhancement Changes/Updates/Additions to existing features priority: low Low impact/importance bug

Comments

@anangl
Copy link
Member

anangl commented Nov 20, 2018

During a PR review, some confusion about the expected behavior of the uart_irq_tx_ready function popped out, see this comment and replies to it.

Apparently, some existing Zephyr code expects this function to return true only when the TX interrupt is enabled (see for instance drivers/bluetooth/hci/h4.c or subsys/shell/shell_uart.c) and this function returning true is even treated as an indication that some TX action should be done, while from reading the API description one can conclude that this only indicates that TX action is possible because "UART TX buffer can accept at least one character for transmission".
Indeed, most of UART drivers implement this function in the way described above, but some exceptions seems to be also present (see uart_stm32.c). And the basic UART API test seems to also not follow this approach.

To avoid further confusion, it should be clarified what is the actually required behavior of the function.

@pfalcon pfalcon added the area: UART Universal Asynchronous Receiver-Transmitter label Nov 20, 2018
@carlescufi carlescufi added the area: API Changes to public APIs label Nov 20, 2018
@galak galak added Enhancement Changes/Updates/Additions to existing features priority: low Low impact/importance bug labels Nov 20, 2018
@tautologyclub
Copy link
Contributor

I've tried to get this clarified from Nordic before.

Why should the UART fifo be ready to receive bytes if TX interrupts are disabled? That makes no sense. The API specifies clearly that calling fifo_fill from outside interrupt context is undefined. Thus, if TX interrupts are disabled, clearly the UART fifo can impossibly be in a state where it's ready to accept transfers.

Out of curiosity, would ANY samples/tests relying on interrupt-driven UART currently not fail horribly with the current nRF implementation?

@carlescufi
Copy link
Member

carlescufi commented Feb 12, 2019

@jarz-nordic or @Mierunski could you take a stab at this one please?

@carlescufi carlescufi added this to the v1.14.0 milestone Feb 12, 2019
@tautologyclub
Copy link
Contributor

Well the situation isn't as bad as I originally thought, I was under the impression that Nordic had misimplemented the API and then abandoned a broken implementation with the justification that "a new API is upcoming anyway". But it's not so bad.

@jakub-uC
Copy link
Contributor

@tautologyclub : I apologize once more for being rude before.

Now lets digg into your concerns. First:

Why should the UART fifo be ready to receive bytes if TX interrupts are disabled? That makes no sense.

According to old Zephyr UART API description function uart_irq_tx_ready shall not be called outside of interrupt context. Please let me know why it bothers you what status it will return when called outside of interrupt context?

Thus, if TX interrupts are disabled, clearly the UART fifo can impossibly be in a state where it's ready to accept transfers.

Clearly. But there is no API function to check this readiness outside of interrupt context.

Out of curiosity, would ANY samples/tests relying on interrupt-driven UART currently not fail horribly with the current nRF implementation?

May you please elaborate a it more about that? What example? We are trying to fix all bugs as soon as possible.

@jakub-uC
Copy link
Contributor

Yes, new API has been introduced to avoid future misunderstandings.

@tautologyclub
Copy link
Contributor

@jarz-nordic You can call uart_irq_tx_ready from interrupt context without TX interrupts being enabled, no? From what I can recall, this seemed to be the pattern in plenty of samples. Maybe there's been a purge lately, or I am just misremembering. Anyway, the whole thing is solved by using uart_irq_is_pending(dev) appropriately so I don't think it's a terribly big deal. Documentation could be better regarding the whole UART api, but that seems to be on its way.

@jakub-uC
Copy link
Contributor

jakub-uC commented Feb 12, 2019

@tautologyclub : yes it is inconsistency. uart_irq_tx_ready should not be called when uart tx interrupt is not active because it might return true in a special case. It will be after UART (not applicable for UARTE) initialization and before first transmission.

@nashif nashif removed this from the v1.14.0 milestone Mar 4, 2019
@carlescufi carlescufi changed the title uart_irq_tx_ready() needs better documentation or correction of the test serial: uart_irq_tx_ready() needs better documentation or correction of the test May 28, 2019
@anangl anangl closed this as completed May 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: UART Universal Asynchronous Receiver-Transmitter Enhancement Changes/Updates/Additions to existing features priority: low Low impact/importance bug
Projects
None yet
Development

No branches or pull requests

8 participants