da14531: port da14531_protocol_format to Rust #1723
Conversation
The asserts are changed as they were off by one (in the safe conversative direction).
benma
force-pushed
the
da14531-2
branch
2 times, most recently
from
4939c69 to
ac811f1
Compare
NickeZ
left a comment
NickeZ
left a comment
There was a problem hiding this comment.
Very nice, but I think the naming is wrong, it should probably be in a crate called "serial link protocol" or something more like that. Maybe "bitbox interchip serial protocol". The serial link protocol is unrelated to da14531. (technically it is the "SAMD51 to DA14531 serial link protocol").
I also saw an excessive amount of similar tests (1 for every single-byte sequence?) instead of some variance, like various lengths.
There are no negative tests (which isn't a blocker), so no invalid sequences are tested.
I would also prefer if we could handle failure by resetting the communication stack rather than panicking. But this means that we need to return Result instead of panic.
Using the CRC dep we already have in Rust. We expose a helper rust_da14531_crc function too for an unrelated call to CRC so we can get rid of crc.c/crc.h. C unit tests added for da14531_protocol_format still pass, showing the Rust port is correct for the cases tested.
Renamed the crate
There can be no negative tests, right? It's encoding, not decoding - one can encode any payload.
Can we postpone that to another PR? For this PR I'd like to keep it refactor-only, not change functionality. |
| let len_bytes = payload_len.to_le_bytes(); | ||
|
|
||
| let mut idx = 0usize; | ||
| assert!(idx < out.len()); |
There was a problem hiding this comment.
asf4 ASSERT is the same as rust debug_assert.
There was a problem hiding this comment.
Indexing in rust is checked, so you don't need out-of-bounds checks.
Can check what happens if out-buf is to small.
👍 |
It will run into the assert, not sure how to test that in C. In Rust, I could add a test to check if it panics, but I don't see how that is helpful. |
I didn't even see the c-tests initially when I reviewed. |
Should have read the PR comment 🤓 😄 |
Yeah, I was very confused when I only saw rust unit tests :P |
|
Split first commit into separate PR so we can see full CI passing on it: #1726 |
2 participants
C unit tests for the C impl are added.
Then the C function is ported to Rust, while still passing the C tests.
Then the C tests are ported.
This way we can be reasonably sure that the Rust port is correct.