orientation-screen: Migrate to rust #1631
Conversation
NickeZ
force-pushed
the
maint/migrate-orientation-screen-to-rust
branch
18 times, most recently
from
39c79ca to
781d4b1
Compare
benma
left a comment
benma
left a comment
There was a problem hiding this comment.
(not done reviewing yet, just some quick comments)
NickeZ
force-pushed
the
maint/migrate-orientation-screen-to-rust
branch
from
7d78b67 to
fece219
Compare
NickeZ
force-pushed
the
maint/migrate-orientation-screen-to-rust
branch
3 times, most recently
from
673483d to
d92d6f6
Compare
benma
left a comment
benma
left a comment
There was a problem hiding this comment.
The orientation screen is not actually working, it's always the same orientation no matter which side you tap.
Right, I noticed now that we deliver multiple short taps so the screen is rotated back to normal. In the C-code we popped the screen in the event handler so the screen never sees more than one tap. |
NickeZ
force-pushed
the
maint/migrate-orientation-screen-to-rust
branch
from
1e470d7 to
800cf1c
Compare
Last commit ensures that callback only is called once for the component. |
NickeZ
force-pushed
the
maint/migrate-orientation-screen-to-rust
branch
from
3845db8 to
822a8a7
Compare
NickeZ
force-pushed
the
maint/migrate-orientation-screen-to-rust
branch
5 times, most recently
from
45ed416 to
f2dd8dd
Compare
| option(&result).await | ||
| } | ||
|
|
||
| pub async fn create() -> bool { |
There was a problem hiding this comment.
don't know why that name, it does not mean anything to me. how about process or choose_and_handle?
There was a problem hiding this comment.
The C function is called orientation_screen_create(). I guess I could call it orientation_screen which is inline with confirm. But I do think it looks ugly to have confirm::confirm or orientation_screen::orientation_screen.
There was a problem hiding this comment.
Yeah the create functions there refer to creating component instances, but that does not transfer so well to higher level async Rust workflows.
f you don't like repeating words, the above function could be renamed to choose and this file to orientation, then it becomes worklows::orientation::choose(...).
Could also merge the two funcs again, I asked to split them when you still called the C callback, but now the distinction is a bit fuzzier.
tACK regardless
NickeZ
force-pushed
the
maint/migrate-orientation-screen-to-rust
branch
from
024c886 to
39ad25a
Compare
NickeZ
force-pushed
the
maint/migrate-orientation-screen-to-rust
branch
from
53ab1c9 to
6f2e946
Compare
benma
left a comment
benma
left a comment
There was a problem hiding this comment.
tACK (but please squash before merge)
NickeZ
force-pushed
the
maint/migrate-orientation-screen-to-rust
branch
4 times, most recently
from
c190075 to
752dfdc
Compare
|
@benma I decided to simplify the delay, removed some state and abort if more than 10 concurrently are allocated. PTAL |
NickeZ
force-pushed
the
maint/migrate-orientation-screen-to-rust
branch
from
3fd8452 to
c71bdd0
Compare
src/delay.c
Outdated
| } | ||
| CRITICAL_SECTION_LEAVE() | ||
| if (full) { | ||
| Abort("To many concurrent delays"); |
src/delay.c
Outdated
| if (_tasks[self->id].timer.cb && !_tasks[self->id].done) { | ||
| timer_remove_task(&TIMER_0, &_tasks[self->id].timer); | ||
| } | ||
| CRITICAL_SECTION_LEAVE(); |
There was a problem hiding this comment.
Shouldn't this be one line down, the last thing in the function?
There was a problem hiding this comment.
Yeah, if the api should be re-entrant it should be that. I can fix it.
create an async friendly hardware supported delayin C and wrap it in rust using Future.
NickeZ
force-pushed
the
maint/migrate-orientation-screen-to-rust
branch
from
99d6d66 to
eb9513b
Compare
2 participants
create an async friendly hardware supported delayin C and wrap it in rust using Future.