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

[sam] Add driver for SAMG timer channels #823

Merged
merged 1 commit into from
Feb 9, 2022

Conversation

mcbridejc
Copy link
Contributor

No description provided.

Copy link
Member

@salkinium salkinium left a 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();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(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.

Copy link
Contributor Author

@mcbridejc mcbridejc Feb 8, 2022

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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().

Copy link
Contributor Author

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.

Copy link
Member

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.

@chris-durand chris-durand self-requested a review February 8, 2022 10:16
Copy link
Member

@chris-durand chris-durand left a comment

Choose a reason for hiding this comment

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

Very nice!

@salkinium salkinium added the ci:hal Triggers the exhaustive HAL compile CI jobs label Feb 9, 2022
@salkinium salkinium merged commit f5cdf6a into modm-io:develop Feb 9, 2022
@salkinium salkinium added this to the 2022q1 milestone Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
advanced 🤯 ci:hal Triggers the exhaustive HAL compile CI jobs feature 🚧
Development

Successfully merging this pull request may close these issues.

None yet

3 participants