[MCXA] cdog: refactor code watchdog driver#5490
[MCXA] cdog: refactor code watchdog driver#5490felipebalbi wants to merge 1 commit intoembassy-rs:mainfrom
Conversation
jamesmunns
left a comment
There was a problem hiding this comment.
I'm OK with this as an improvement, we could potentially make this driver a bit more idiomatic though and have async methods for waiting for interrupt events, etc.
| } | ||
| } | ||
|
|
||
| impl AddAssign<u32> for SecureCounter { |
There was a problem hiding this comment.
I'd prefer to have these as explicit increment and decrement methods, rather than operator overloading.
| debug_halt: PauseControl::PauseTimer, | ||
| lock: LockControl::Unlocked, | ||
| }; | ||
| let mut config = hal::cdog::Config::default(); |
There was a problem hiding this comment.
I have a weak preference to actually hide these settings, and expose these as async methods, or have different Mode type states.
There was a problem hiding this comment.
I did consider that, yeah. However, we don't get a warning interrupt with this one. We need to feed before the interrupt triggers. There are three possibilities, actually: either you want an interrupt, or a reset, or neither.
So, let's say we want an interrupt, you seem to imply an API similar to what follows:
loop {
cdog.instruction_counter().update(200);
let status = cdog.wait_for_interrupt_and_get_status().await;
do_something_with(status);
}Sadly, that won't work. The whole idea of the code watchdog is that if the interrupt triggers, it should imply unauthorized code modification (an attack) which should be treated as sort of a critical situation. That is, we don't want the interrupt to ever trigger. Maybe we could split() the instruction counter into a writer and a waiter and dedicate a task to wait for that interrupt forever, such that applications can decide what to do if that interrupt ever comes. Or, the application could just bind their own interrupt handler instead?
Closes OpenDevicePartnership/embassy-mcxa#139
Trying something new for the
cdog. Some of the changes are highly desirable (now we can support bothcdoginstances), others are up for discussion. Maybe it looks too "abusive" of rust operators. Let me know