-
Notifications
You must be signed in to change notification settings - Fork 220
EIC Fixes for D5X #837
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
EIC Fixes for D5X #837
Conversation
jbeaurivage
left a 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.
Thanks @rnd-ash. I completely understand why one would want the EIC to run on a faster clock. What surprised me is learning that the OSCULP32K was NOT always enabled? For some reason I thought it couldn't be turned off. But maybe that's only the case on D21 targets.
| /// Do not use this function if you are using the osc32k clock | ||
| /// for the EIC peripheral, instead use [`Eic::new`] | ||
| #[hal_cfg("eic-d5x")] | ||
| pub fn new_gclk(mclk: &mut pac::Mclk, _clock: EicClock, eic: pac::Eic) -> Self { |
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.
Only real suggestion would be to call this function new_with_gclk.
|
@jbeaurivage @ianrrees I had some conversations today about how this API works, and I believe my initial interpretation is wrong. In summary, the EIC actually works like this.
So, forward question to ask. How to implement this in the API? I was thinking of the EIC clock source being an enum pub enum EicClkSrc<'a>{
Gclk(&'a EicClock),
Ulp(&'a UlpClock)
}Then provide this enum to the new() function of EIC so the user can initially set the clocking source. But then also provide Trying to implement this now, but I find that the V1 clock API does not have a struct to represent the OSC32K clock, any suggestions on how to go from here? |
|
@rnd-ash, your suggestion does make sense. IMO the premise behind the v1 clock API is that peripherals are pretty much responsible for setting up the clocking according to their needs. Therefore my suggestion would be this:
|
|
Actually I would expose 3 different constructors:
|
Summary
As part of my work for implementing the Event system for pulse counting, I noticed that there was some issues with the EIC implementation. This PR fixes them.
Document the fact that when using the V2 clocking API, the user must enable the OSC32K clock and wire it up to the EIC Clock, since the HAL assumes that the OSC32K clock will drive the EIC peripheral. Later, I will possibly create a new constructor for the EIC peripheral that can take in a much faster clock source if the user wishes for it.
Create a constructor for the EIC peripheral that does not assume that OSC32K clock source is used. This allows the EIC peripheral to be driven by a much faster clock source, thus, have a faster event detection frequency
The enable_event method on an EIC channel does not actually function, as the EIC peripheral is enabled when the function is called, but the evctrl register is locked due to the peripheral being enabled. So the method now disables the EIC before enabling the bit, and then re-enables the peripheral. This is noted in the function documentation