-
Notifications
You must be signed in to change notification settings - Fork 4
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
Clock feature #191
Clock feature #191
Conversation
IFX-Anusha
commented
Oct 24, 2024
- machine.freq() implementation for setting clock by the user
- Remove clock init from I2S and modified tests
- Documentation
e55dd56
to
51872b6
Compare
f41411a
to
d087653
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.
Nice!🚀
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 good! 😃 A couple of things that we should refine 📏
@@ -366,21 +323,6 @@ static void mp_machine_i2s_init_helper(machine_i2s_obj_t *self, mp_arg_val_t *ar | |||
mp_raise_ValueError(MP_ERROR_TEXT("invalid format")); | |||
} | |||
|
|||
// is valid clock freq? | |||
uint32_t audio_clock_freq_hz; | |||
uint32_t rate = args[ARG_rate].u_int; |
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.
We should keep this part checking that the rate is a valid one or?
Otherwise, the use could pass any integer. But actually only the ones filtered where valid?
Additionally, here I would check if the clock is properly for that rate, and raise exception if that is not the case.
ports/psoc6/modmachine.c
Outdated
uint32_t reset_cause; | ||
bool clock_set_i2s = false; |
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.
These flags would not be sufficient to properly check the current clock configuration, because there aren´t deinited anywhere or?
Think of the following case:
- you set the machine.freq(AUDIO_I2S_98_MHZ)
- the clock_set_i2s = true;
- then you set machine.freq(AUDIO_PDM_22_579_200_HZ)
- then clock_set_pdm = true;
- then you try to use machine.I2S. It will consider that the i2s clock is set, but actually isn´t.
I would say inside the i2s or the pdm we need to check the right configuration (also for the given rates). A function to get freq setup can be called by each module (i2s, pdm, ...) and they can check if that is the right one. Raise an exception otherwise.
Alternatively, you have to keep track of those 2 flags (and any future one), and set them to false, when other functions reconfigure those clocks. I think that is less maintainable because every function needs to keep track of "which setup an I going to break".
2b34a61
to
1786828
Compare
9e41e1d
to
2f66332
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.
Looks good!
1786828
to
659139d
Compare
Signed-off-by: IFX-Anusha <[email protected]>
Signed-off-by: IFX-Anusha <[email protected]>
Signed-off-by: IFX-Anusha <[email protected]>
Signed-off-by: IFX-Anusha <[email protected]>
2f66332
to
ed2b7ac
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.
Nice 👯