Skip to content

cpu/atmega_common#12790

Merged
aabadie merged 3 commits intoRIOT-OS:masterfrom
maribu:atmega_isr_thread
Nov 24, 2019
Merged

cpu/atmega_common#12790
aabadie merged 3 commits intoRIOT-OS:masterfrom
maribu:atmega_isr_thread

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Nov 22, 2019 •
edited
Loading

Contribution description

  • Minor cleanups in cpu/atmega_common:
    • No longer using reserved identifiers
    • Use a C11 atomic instead of volatile + __asm__ volatile("" ::: "memory"); for the flag used to check if the ATmega is in IRQ context
  • Reworked the code that is run at the end of an ISR
    • The current code behaved like thread_yield() is executed at the end of an ISR, but correct behavior is thread_yield_higher(); This PR changes this to behave like thread_yield_higher()
    • With this, tests/isr_yield_higher passes
    • This also save some lines of code an ROM :-)

Testing procedure

  1. Confirm that tests/isr_yield_higher now really passes
  2. Run a bunch of other tests related to context switching, scheduling and interrupts to verify that no regressions are introduced. (E.g. tests/thread_cooperation/, tests/thread_basic/, tests/periph_timer, tests/sched_testing, ...)

Issues/PRs references

Tick tests/isr_yield_higher in #12651

@maribu maribu added Platform: AVR Platform: This PR/issue effects AVR-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Nov 22, 2019
@maribu maribu requested a review from aabadie November 22, 2019 22:27
@aabadie aabadie mentioned this pull request Nov 22, 2019
Closed
13 tasks
@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 23, 2019
aabadie
aabadie reviewed Nov 23, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Nice one! Thanks!
I ran the compile_and_test_for_board.py on atmega256rfr2-xpro and this PR is fixing the isr_yield_higher test. No regressions in tests were found by the script.

I agree that using an std atomic variable is nicer than the previous solution.

So I would say that once my tiny comments are addressed, this is good to go.

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Nov 23, 2019

Using the C11 atomics is inherently incompatible with C++. I was aware of that, but I wasn't aware that cpu.h also gets included from C++.

I could use the preprocessor to only provide the atmega_enter_irq() and atmega_exit_irq() only to C. Or move the static inline atmega_enter_isr() into a C file, so that the #include <stdatomic> from cpu.h could be dropped. But I now I'm no longer sure if this really is an improvement in readability.

I just revert the change to C11 atomics now, so that the actual bugfix can get in swiftly.

@maribu
cpu/atmega_common: Stop using reserved names
2b1bee7 
 Names with two leading underscores are reserved in any context of the c
 standard, and thus must not be used. This ATmega platform used it however for
 defining internal stuff. This commit fixes this.
@maribu
cpu/atmega_common: Clean up & fix IRQ handling
606d72f 
 At the end of an ISR, the ATmega code was doing an `thread_yield()` instead of
 a `thread_yield_higher()`. This resulted in tests/isr_yield_higher failing.
 Fixing this saves a few lines of code, some ROM, and solves the issue.
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Nov 23, 2019

OK. I updated to remove the C11 atomics and squash. The commits were a bit mere entangled that I expected, so I had to do some manual merging. I hope I didn't mess up anything :-D

@maribu
drivers/at86rf2xx: Update to new ATmega API
4347f72 
Replace __enter_isr() with atmega_enter_isr() and __exit_isr() with
atmega_exit_isr()
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Nov 23, 2019

Murdock is super helpful of detecting all the leftovers :-)

The fact that atmega specific code is called in drviers/at86rf2xx only confirms that the change in names to add the atmega_ prefix is making this more readable.

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Nov 23, 2019

@kaspar030: The Murdock failure seems unrelated, but nasty. Like a bug that is only triggered when the stars are aligned in a specific way. Murdock doesn't provide core dumps for tests failed on native, does it?

@maribu maribu mentioned this pull request Nov 23, 2019
Closed
@maribu maribu 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, 2019
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Nov 23, 2019

I triggered a rebuild now. The issue is documented here, in case this is needed. Because a second similar issue occurred on the same machine, I now think that too much load resulting in the timeouts in both cases is the most plausible explanation, rather than a latent bug locking up RIOT on native every now and then.

aabadie
aabadie approved these changes Nov 24, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

ACK

No regression in other tests on atmega256rfr2-xpro and isr_yield_higher is fixed. Code changes are justified and documented.

@aabadie aabadie merged commit 41e29e3 into RIOT-OS:master Nov 24, 2019
@maribu maribu deleted the atmega_isr_thread branch November 25, 2019 09:52
@maribu maribu mentioned this pull request Nov 27, 2019
Merged
@fjmolinas fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019
@jue89 jue89 mentioned this pull request Mar 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aabadie aabadie aabadie approved these changes

Assignees

No one assigned

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: AVR Platform: This PR/issue effects AVR-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

Release 2020.01

Development

Successfully merging this pull request may close these issues.

3 participants

@maribu @aabadie @fjmolinas