-
Notifications
You must be signed in to change notification settings - Fork 130
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
[sam] Add driver for SAMG timer channels #823
Conversation
d4d8409
to
4ca32ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Literally LGTM, since I cannot judge the technical correctness in detail ;-P
MODM_ISR(TC3) | ||
{ | ||
// Clear pending interrupts by reading them | ||
(void)TimerChannel3::getInterruptFlags(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(void)TimerChannel3::getInterruptFlags(); | |
TimerChannel3::acknowledgeInterruptFlags(); |
At least this is the informal naming scheme for the interrupt flags in modm.
Can be implemented like that though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think I had to think about this a bit. On STM32, you clear the flag with a write. On SAMG5x you clear it with a read, so the acknowledgeInterruptFlags()
function, if I added it, would be the same as the getInterruptFlags()
function, except I guess not returning the read value. Adding the extra method does allow for more similar API I suppose, and it's likely to be inlined as a single instruction anyway so I guess it doesn't hurt to have both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it won't match the STM32 API anyway, because acknowledgeInterruptFlags
takes a mask argument, but there's no way to acknowledge interrupt flags individually on this chip.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that I do not really like about the name getInterruptFlags()
is that functions named get...
usually perform a read operation without side effects. From its name it's not obvious it clears any flags. That's why the code above needs the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't come up with a better name despite some overly verbose alternatives like readAndClearInterruptFlags()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with renaming getInterruptFlags to readAndClearInterruptFlags, adding an acknowledgeInterruptFlags that is basically a wrapper on getInterruptFlags, or leaving it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave it as is and fix it later if it becomes an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
4ca32ab
to
f5cdf6a
Compare
No description provided.