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

Rtc tests refactor #134

Merged
merged 1 commit into from
May 27, 2024
Merged

Rtc tests refactor #134

merged 1 commit into from
May 27, 2024

Conversation

jaenrig-ifx
Copy link
Member

@jaenrig-ifx jaenrig-ifx commented May 24, 2024

By creating this pull request you agree to the terms in CONTRIBUTING.md.
https://github.com/Infineon/.github/blob/master/CONTRIBUTING.md
--- DO NOT DELETE ANYTHING ABOVE THIS LINE ---

CONTRIBUTING.md also tells you what to expect in the PR process.

@NikhitaR-IFX here are the changes.

  • Moved the rtc_init.c to soft_reset: in main.c (The only real necessary change).
  • Unified alarm setup, which is now called within alarm() and if repeat in the internal irq.
  • irq() is only setting the callback
  • Replaced sleep() in test for condition check with timeout in tests (this would be less sensitive to time fluctuation and minimize that waiting time).
  • Added __ del __() (** this might be removed again).

Things that are still missing:

  1. consider cyhal_init()/free() to be delegated to the main(), as the rtc() is required by the time module. Which means, this RTC API does not provide any chance of calling those functions. It will be always inited in the main() and freed when we do a soft reset.
  2. check arguments in irq() -> wake is unused, and we might inform about it? trigger can only be RTC.ALARM0 (even if there is no other).
  3. Remove commented initialization of rtc_obj in constructor? Let´s imagine we want to call a second time RTC() constructor. What should be the behavior? If it is a singleton, it should complain... Otherwise it should return the same object in the same state? That would not make much sense to me. Shall it return again the RTC() but fresh and clean to reconfigure? We have the deinit() cancel()... So I would say they should use that...
    I would say: second call to RTC() should return error. What do you think? Anything I am missing?
  4. Maybe refactor the modtime function in a way that they reuse functions of rtc. Instead of directly calling cyhal_rtc functions.

@NikhitaR-IFX
Copy link
Member

  1. init() and free() are handled in main right now also no or? Only a check is made in respective modules if RTC is already inited through rtc_enable() to ensure there is no access to RTC without initializing. Did I get your point right?
  2. Agreed.
  3. So I tried calling init twice from mtb and it does it fine without any complaints. But ideally if there is only 1RTC, I would expect second init() to fail. The datasheet does not mention of more RTC's available neither it says there is only 1. But with diagram in "Block and Functionality" section in Page 6, I would believe there is only 1. So on MPY side, the constructor should be ideally called only once and we can keep a track of that with a flag, return error on second call. BTW, I did not find "commented initialization of rtc_obj in constructor".
  4. Yes we can. Probably we might need to add/extend some helper functions but all doable.

Copy link
Member

@NikhitaR-IFX NikhitaR-IFX left a comment

Choose a reason for hiding this comment

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

I am happy with the rest of the changes. Just some improvements/questions we can discuss over.


void rtc_irq_handler(void *self_in, cyhal_rtc_event_t event) {
machine_rtc_obj_t *self = (machine_rtc_obj_t *)self_in;
if (self->callback != NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

But where is this initialized as NULL? Should be done in cases where there is no alarm or there is alarm but no callback associated.

{ MP_QSTR_wake, MP_ARG_KW_ONLY | MP_ARG_INT, {.u_int = 0} }
};

mp_arg_val_t args[MP_ARRAY_SIZE(allowed_args)];
mp_arg_parse_all(n_args - 1, pos_args + 1, kw_args, MP_ARRAY_SIZE(allowed_args), allowed_args, args);
machine_rtc_obj_t *self = pos_args[0];
self->callback = args[ARG_handler].u_obj;
// TODO: Shall we do some argument validation here?
Copy link
Member

Choose a reason for hiding this comment

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

Ideally callback can be NULL or a pointer to handler function. For ex: A NULL is valid, when wake-up condition is set. You only want to wake up the system from deep-sleep/hibernate modes and set the wakeup source as RTC ALARM and then you do not need user-level cback. The only difference is if handler is NULL, we don't need to call register function. Rest remains the same.

Signed-off-by: enriquezgarc <enriquezgarcia.external@infineon.com>
@jaenrig-ifx jaenrig-ifx merged commit 2c1d0bb into rtc_tests May 27, 2024
27 checks passed
@jaenrig-ifx jaenrig-ifx deleted the rtc-tests-refactor branch May 27, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants