tests: uart_basic_api: Don't assume we can drink from IRQ firehose. #42
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There're (at least) 2 UART TX interrupt causes: "tx fifo has more room" and "transmission of tx fifo complete". Zephyr API has only one function to enable TX interrupts, uart_irq_tx_enable(), so it's fair to assume it enables interrupt for both conditions. But then immediately after enabling TX IRQ, it will be fired with "tx fifo has more room" cause. If ISR doesn't do anything to fill FIFO, on some architectures, immediately after return from ISR, it will be fired again (with no instruction progress in the main application thread). That's exactly the situation with this test, and on ARM, it leads to inifnite IRQ loop. So, instead move call to uart_fifo_fill() inside ISR, and be sure to disable TX IRQ after we transmitted enough characters. Change-Id: Ibbd05acf96b01fdb128f08054819fd104d6dfae8 Signed-off-by: Paul Sokolovsky <[email protected]>
nashif
added
the
In progress
|
Migrated from https://gerrit.zephyrproject.org/r/#/c/12919/ (which has a lot of discussion!) |
|
Let this be the happy pull request, it hanged in gerrit enough already... (# 42) |
|
@lag-linaro : please review |
nashif
approved these changes
May 2, 2017
nashif
removed
the
In progress
|
Thanks! I consider that as an approval of the general direction of work on #49 (of which this change is a small part). |
|
For reference, here's the latest test matrix of this test, with this patch merged: https://docs.google.com/spreadsheets/d/1e4hx9iVmAp5pBSTd2namF_VdsiMBS-OmlJUI4UPBMN4/edit#gid=2127015340 |
nagineni
pushed a commit
to nagineni/zephyr
that referenced
this pull request
Nov 20, 2017
[General] Set JERRY_BASE in zjs-env.sh to point to the copy in deps/
ulfalizer
pushed a commit
to ulfalizer/zephyr
that referenced
this pull request
May 13, 2019
PR zephyrproject-rtos#42 / Commit 4a2a1e17a488 "Use zephyr_module.py to generate Kconfig.modules" totally failed to handle exceptions by trying to re-use bits of existing code which was meant for something completely different: - exception name mismatch ("ex" vs "e"); - irrelevant sh.CommandNotFound exception left over; - use of the low-level popen()/pcommunicate() which rarely ever raise exceptions; ... To reproduce the issue, temporarily rename 'scripts/zephyr_module.py' to something else and observe the Kconfig check silently passing despite the lack of the main script and thanks to a /tmp/Kconfig.modules file left behind by some previous run. Clean-up the mess and simplify the code thanks to the higher-level and recommended subprocess.check_output(). Maybe any /tmp/Kconfig.modules leftover should also be cleaned-up but that's a different issue. (This confirms the theory that error handling tends to have the worst test coverage) Signed-off-by: Marc Herbert <[email protected]>
ulfalizer
pushed a commit
to ulfalizer/zephyr
that referenced
this pull request
Oct 23, 2019
PR zephyrproject-rtos#42 / Commit 4a2a1e17a488 "Use zephyr_module.py to generate Kconfig.modules" totally failed to handle exceptions by trying to re-use bits of existing code which was meant for something completely different: - exception name mismatch ("ex" vs "e"); - irrelevant sh.CommandNotFound exception left over; - use of the low-level popen()/pcommunicate() which rarely ever raise exceptions; ... To reproduce the issue, temporarily rename 'scripts/zephyr_module.py' to something else and observe the Kconfig check silently passing despite the lack of the main script and thanks to a /tmp/Kconfig.modules file left behind by some previous run. Clean-up the mess and simplify the code thanks to the higher-level and recommended subprocess.check_output(). Maybe any /tmp/Kconfig.modules leftover should also be cleaned-up but that's a different issue. (This confirms the theory that error handling tends to have the worst test coverage) Signed-off-by: Marc Herbert <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There're (at least) 2 UART TX interrupt causes: "tx fifo has more
room" and "transmission of tx fifo complete". Zephyr API has only
one function to enable TX interrupts, uart_irq_tx_enable(), so it's
fair to assume it enables interrupt for both conditions. But then
immediately after enabling TX IRQ, it will be fired with "tx fifo
has more room" cause. If ISR doesn't do anything to fill FIFO, on
some architectures, immediately after return from ISR, it will be
fired again (with no instruction progress in the main application
thread). That's exactly the situation with this test, and on ARM,
it leads to inifnite IRQ loop.
So, instead move call to uart_fifo_fill() inside ISR, and be sure
to disable TX IRQ after we transmitted enough characters.
Change-Id: Ibbd05acf96b01fdb128f08054819fd104d6dfae8
Signed-off-by: Paul Sokolovsky [email protected]
This change is