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/atmega_common/periph_timer: fix spurious IRQs #18978

Merged

Conversation

maribu
Copy link
Member

@maribu maribu commented Nov 25, 2022

Contribution description

As the tile says

Testing procedure

tests/periph_timer from #18963 and tests/periph_timer_periodic should pass. (They do on the arduino-mega2560.)

Issues/PRs references

#18976

@maribu maribu 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 25, 2022
@maribu maribu requested a review from kYc0o as a code owner November 25, 2022 12:13
@maribu maribu mentioned this pull request Nov 25, 2022
20 tasks
@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: AVR Platform: This PR/issue effects AVR-based platforms labels Nov 25, 2022
@@ -158,7 +158,9 @@ int timer_set_absolute(tim_t tim, int channel, unsigned int value)
unsigned state = irq_disable();

ctx[tim].dev->OCR[channel] = (uint16_t)value;
*ctx[tim].flag &= ~(1 << (channel + OCF1A));
/* clear spurious IRQs, if any */
*ctx[tim].flag |= (1 << (channel + OCF1A));
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
*ctx[tim].flag |= (1 << (channel + OCF1A));
*ctx[tim].flag = (1 << (channel + OCF1A));

I think this should also work. @kfessel: You're the AVR expert, do I read the datasheet correctly?

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact, "would also work" is incorrect: The current version would unset all flags and is buggy.

Copy link
Contributor

@kfessel kfessel Nov 25, 2022

Choose a reason for hiding this comment

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

i was about to write that:
doc tmega328p:
"Alternatively, can be cleared by writing a logic one to its bit location."

so if you flags = flags it will clear all flags that are 1.

*ctx[tim].flag |= (1 << (channel + OCF1A)); will clear all flags that are read as 1 and OCF1A in any case.

but the compiler should make *ctx[tim].flag |= (1 << (channel + OCF1A)); work anyway since all flags are placed in io-address-space -> it will become SBI <FLAGSioaddres>, (channel + OCF1A) writing a one to that specific bit location.

Copy link
Member Author

@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.

see inline

@maribu maribu force-pushed the cpu/atmega_common/periph_timer/spurious_irqs branch from 7d4815f to 787884a Compare November 25, 2022 13:46
@riot-ci
Copy link

riot-ci commented Nov 26, 2022

Murdock results

✔️ PASSED

787884a cpu/atmega_common/periph_timer: fix spurious IRQs

Success Failures Total Runtime
117858 0 117858 02h:19m:32s

Artifacts

@maribu maribu merged commit 476dca2 into RIOT-OS:master Nov 26, 2022
@maribu maribu deleted the cpu/atmega_common/periph_timer/spurious_irqs branch November 26, 2022 15:56
@maribu
Copy link
Member Author

maribu commented Nov 26, 2022

Thx :)

@maribu
Copy link
Member Author

maribu commented Nov 26, 2022

Backport provided in #18981

@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: AVR Platform: This PR/issue effects AVR-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.

5 participants