firmware: port main loop to rust #1690
Conversation
NickeZ
force-pushed
the
maint/rust-main-loop
branch
10 times, most recently
from
ec75283 to
1d4efaa
Compare
NickeZ
force-pushed
the
maint/rust-main-loop
branch
from
1d4efaa to
983b51d
Compare
NickeZ
force-pushed
the
maint/rust-main-loop
branch
from
5beb172 to
d5700b0
Compare
Instead of relying on the caller to not modify the buffer a copy is made to static memory. This enables the caller to use a stack allocated buffer.
NickeZ
force-pushed
the
maint/rust-main-loop
branch
from
d5700b0 to
b287f43
Compare
NickeZ
force-pushed
the
maint/rust-main-loop
branch
from
4e671ce to
e110364
Compare
|
@benma ready for review again |
benma
left a comment
benma
left a comment
There was a problem hiding this comment.
Left another round of comments after reading through the new main_loop function line by line, not looked very closely at all the C wrappers yet.
| if let Some(ref mut task) = orientation_task { | ||
| if let Poll::Ready(_orientation) = util::bb02_async::spin(task) { |
There was a problem hiding this comment.
Just fyi, you can now add more conditions, like if let ... && let .... Above you also have some double nestings becaue of that, which you could flatten.
| crate::workflow::orientation_screen::orientation_screen(), | ||
| )); | ||
|
|
||
| let mut hww_data = None; |
There was a problem hiding this comment.
Helpful to annotate the type (: Option<[u8; 64]>) for clarity
| ) { | ||
| let (uart_read_buf, cap) = if let Some(uart_read_buf) = uart_read_buf { | ||
| ( | ||
| uart_read_buf as *mut _ as *mut _, |
There was a problem hiding this comment.
&mut [u8] can't be cast to *mut u8 in one go, wow. That's crazy.
| uart_read_buf: Option<&mut [u8]>, | ||
| uart_read_buf_len: Option<&mut u16>, |
There was a problem hiding this comment.
These should be inside of one Option, otherwise this function is not safe if you pass Some() for the buffer but None for the len.
| } | ||
| crate::async_usb::spin(); | ||
|
|
||
| if let Some(ref mut task) = orientation_task { |
There was a problem hiding this comment.
in the PR where you added this in the C mainloop I asked you to move this into a separate function to keep the mainloop smaller 😄 Same here please, move it to a local fn orientation_screen_poll?
There was a problem hiding this comment.
no problem, but you really think that is more readable? jumping to another function for like 5 lines of code?
There was a problem hiding this comment.
Yeah, a function also has the benefit that it does not have the full mainloop function body in its scope. Not a very strong opinion though.
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| pub fn product() -> (&'static str, u16) { |
There was a problem hiding this comment.
you don't need to return the size, the resulting str type contains it and you can call .len() on it if needed.
the callsite however just passes it on to set_product(), but that function also takes the string and also does not need the extra len param.
| } | ||
| } | ||
|
|
||
| pub fn set_product(product: &'static str, len: u16) { |
There was a problem hiding this comment.
len param not needed, product.len() is the same
| #[test] | ||
| fn ble_disable_disables_ble() { | ||
| unsafe { | ||
| bitbox02_sys::fake_memory_factoryreset(); |
There was a problem hiding this comment.
Use crate::memory::fake_memory_factoryreset(), which already exists
2 participants
This PR ports the main loop to rust. To keep it simple it tries to stick as much as possible to the c implementation. Rustification will happen later.