YWF: Convert many more drivers#568
Conversation
7295c63 to
b0c05bd
Compare
Using YWF with alarm is more nuanced because users might reasonably expect to have both periodic timers and do a delay_ms.
|
I reverted the changes to alarm.c as alarm needs more thought. |
|
Ok I've tested a bunch of these, including the sensors using the hail app and the kv test app. |
brghena
left a comment
brghena
left a comment
There was a problem hiding this comment.
Okay, I read through all of this.
One tiny nit. Also two places where I'm very confused about the resulting behavior and I think something is wrong (although not necessarily this PR's fault or responsibility to fix).
| @@ -21,13 +13,8 @@ bool libtocksync_app_state_exists(void) { | |||
| returncode_t libtocksync_app_state_save(void) { | |||
| returncode_t err; | |||
|
|
|||
| result.fired = false; | |||
|
|
|||
| err = libtock_app_state_save(app_state_cb); | |||
| err = libtock_app_state_save(app_state_noop_cb); | |||
| if (err != RETURNCODE_SUCCESS) return err; | |||
|
|
|||
| // Wait for the callback. | |||
| yield_for(&result.fired); | |||
|
|
|||
| return result.ret; | |||
| return libtocksync_app_state_yield_wait_for(); | |||
There was a problem hiding this comment.
This seems wrong to me. There's a return code that's getting dropped here. Shouldn't that be in the ywf data?
There was a problem hiding this comment.
I believe nothing is dropped because there is no return code.
https://github.com/tock/tock/blob/master/capsules/extra/src/app_flash_driver.rs#L160C33-L160C48
https://github.com/tock/libtock-c/blob/master/libtock/storage/app_state.c#L14
There was a problem hiding this comment.
Okay, so this won't likely result in a bug. But it still seems weird to me that we're both registering for a callback and calling yield_wait_for.
Shouldn't we just be calling libtock_app_state_command_save() directly? https://github.com/tock/libtock-c/blob/master/libtock/storage/app_state.c#L61 (I guess with the init check before it)
There was a problem hiding this comment.
I can't suggest here, but I think the code would be:
returncode_t libtocksync_app_state_save(void) {
returncode_t err;
if (!_app_state_inited) {
err = app_state_init();
if (err != RETURNCODE_SUCCESS) return err;
}
err = libtock_app_state_command_save((uint32_t) _app_state_flash_pointer);
if (err != RETURNCODE_SUCCESS) return err;
return libtocksync_app_state_yield_wait_for();
}
There was a problem hiding this comment.
But app_state_init is static. Using the libtock function with a noop callback avoids copying or exposing the init function.
There was a problem hiding this comment.
Good point. I think that a refactor would improve the app state driver, but that's out-of-scope for this PR
Co-authored-by: Branden Ghena <branden.ghena@gmail.com>
4 participants
Mostly done with Claude.
Want to see what happens with CI, and then try to test some of these changes.