tock
Raw 802.15.4 support
I built this upon libtock-c implementation.
I focused on providing safe abstractions.
- Tx part is quite bare, i.e. just passing a byte slice. Should it be more structured?
- Rx part is sophisticated. There are:
-
Frame,RxRingBuffer- these build up the ring buffer that is shared with kernel; -
RxSingleBufferOperator,RxBufferAlternatingOperator- these encapsulate the buffer and provide safe operations on them. The former uses a single buffer, which makes it possible to lose some frames when the buffer is not allowed to the kernel. The latter alternates between two buffers, which guarantees no frame loss at cost of doubled memory usage.
-
Feedback much welcome.
Example is complete. Unit tests are done. Haven't tested this on hardware as well.
Tx part is quite bare, i.e. just passing a byte slice. Should it be more structured?
I do not think it should be more structured.
Overall looks good to me (from the 15.4 side of the world; I'm less familiar with libtock-rs and can't speak too much on that front)! A few comments:
- The ring buffer size should be the 15.4 frame size + some meta data (this is explained in the 15.4 capsule driver). Looking through this, it seems your ring buffer is just 127 bytes (15.4 frame size).
- It appears you are not implementing the ring buffer (just swapping two buffers). This is entirely fine (and will work for most applications). You may need to implement handling multiple packets within each ring buffer if you face workloads with heavy/frequent packet reception (e.g. OpenThread). Swapping buffers is better than just using one buffer, but can still experience dropped/overwritten packets in the case of multiple packets being received prior to the swap.
Exciting to see some 15.4 support making its way into libtock-rs!
Overall looks good to me (from the 15.4 side of the world; I'm less familiar with libtock-rs and can't speak too much on that front)! A few comments:
- The ring buffer size should be the 15.4 frame size + some meta data (this is explained in the 15.4 capsule driver). Looking through this, it seems your ring buffer is just 127 bytes (15.4 frame size).
According to phy_driver.rs:
//! The ring buffer provided by the process must be of the form:
//!
//! ```text
//! | read index | write index | user_frame 0 | user_frame 1 | ... | user_frame n |
//! ```
//!
//! `user_frame` denotes the 15.4 frame in addition to the relevant 3 bytes of
//! metadata (offset to data payload, length of data payload, and the MIC len).
//! The capsule assumes that this is the form of the buffer. Errors or deviation
//! in the form of the provided buffer will likely result in incomplete or
//! dropped packets.
I created the following structs:
#[repr(C)]
pub struct Frame {
pub header_len: u8,
pub payload_len: u8,
pub mic_len: u8,
pub body: [u8; MAX_MTU],
}
#[repr(C)]
pub struct RxRingBuffer<const N: usize> {
/// From where the next frame will be read by process.
/// Updated by process only.
read_index: u8,
/// Where the next frame will be written by kernel.
/// Updated by kernel only.
write_index: u8,
/// Slots for received frames.
frames: [Frame; N],
}
I can't see any deviations from the kernel docs. Could you point them out?
- It appears you are not implementing the ring buffer (just swapping two buffers).
I do not understand why you believe so. RxRingBuffer has both read and write indices, which are used to establish ring buffer semantics on the buffer. Both Operators are written in a way that makes them return frames one by one, using ring buffer pop operation. Or am I mistaken?
Exciting to see some 15.4 support making its way into libtock-rs!
Hooray!
@wprzytula my mistake. I missed the RxRingBuffer fields when skimming through this earlier and did not see the popping from the ring buffer etc. Thanks for the clarification!
Finished implementation. I did some minor fixes to the logic, wrote a very decent test suite and polished documentation.
@wprzytula what's the state of this?
It says still
Haven't tested this on hardware as well.
Is this still accurate?
@wprzytula what's the state of this?
It says still
Haven't tested this on hardware as well.
Is this still accurate?
I still haven't succeeded in implementing HIL on cc2650 (this is WIP), so yes. I'd be very grateful if someone tried this out on another piece of hardware, e.g. Nordic boards.
I need help with understanding the Miri check failure. I'm not sure what is exactly believed to be UB and why.
@wprzytula, sorry for the extended silence on this PR. A bunch of us are extensively discussing how to resolve the soundness issues correctly (e.g. https://github.com/tock/tock/blob/master/doc/wg/network/notes/network-notes-2024-07-15.md#154-libtock-rs-driver). Sorry this is not getting more attention while that's happening.
It turns out not to be obvious what the right fix would be, so no specific actionable recommendation just yet.
@wprzytula, sorry for the extended silence on this PR. A bunch of us are extensively discussing how to resolve the soundness issues correctly (e.g. https://github.com/tock/tock/blob/master/doc/wg/network/notes/network-notes-2024-07-15.md#154-libtock-rs-driver). Sorry this is not getting more attention while that's happening.
It turns out not to be obvious what the right fix would be, so no specific actionable recommendation just yet.
I have read the discussion you refer to. I understand your doubts and the current point now, thank you.
@wprzytula what's the state of this? It says still
Haven't tested this on hardware as well.
Is this still accurate?
I still haven't succeeded in implementing HIL on cc2650 (this is WIP), so yes. I'd be very grateful if someone tried this out on another piece of hardware, e.g. Nordic boards.
I finally succeeded in writing HIL for cc2650 802.15.4 radio, and so I ran tests of this PR on it. Everything seems to work correctly, both for RxSingleOperator and RxBufferAlternatingOperator.
When I think of it, it's only the RxRingBufferInKernel, required for RxBufferAlternatingOperator to work, that causes UB according to Miri.
RxSingleBufferOperator is accepted. As it is good enough (RxBufferAlternatingOperator is only better in the way that it does not miss frames if a frame comes when user is examining the buffer, preventing it from being accessed by kernel), perhaps we could merge this PR without RxBufferAlternatingOperator-related code? This would give libtock-rs's users access to basic networking!
WDYT @alevy @jrvanwhy @bradjc @tyler-potyondy ?
As it is good enough (
RxBufferAlternatingOperatoris only better in the way that it does not miss frames if a frame comes when user is examining the buffer, preventing it from being accessed by kernel), perhaps we could merge this PR withoutRxBufferAlternatingOperator-related code? This would give libtock-rs's users access to basic networking!
I think that is a great move forward! Nonetheless, we do want to figure out a way to support Tock's buffer swapping semantics for "lossless" kernel -- userspace communication in the long term; it's one of the primary advantages of this kernel API. That being said, I agree that having something is better than nothing, and in particular if the potential loss of packets is sufficiently small to not cause havoc or be recoverable in practice.
I've removed RxBufferAlternatingOperator and its controversial code. Please run CI, it should pass now.
@wprzytula I kicked off CI, there's still some unresolved imports but feel free to keep pushing to this branch and CI should run from this point onward.
@wprzytula I kicked off CI, there's still some unresolved imports but feel free to keep pushing to this branch and CI should run from this point onward.
Unfortunately, an approval is required again.
Unfortunately, an approval is required again.
It'd seem like GitHub changed something. We used to only need to approve once, and the repository is set to require approval only for first-time contributors. In any case, keep posting here to have someone hit the button as much as you'd like, we can hide these comments later.
It is very unclear why it keeps requiring approval to run actions. The repo and org are set to require it for first-time contributors, but I really thought that was once-only.
To try to "fix" it, I have added @wprzytula as a read-only contributor. The should result in zero gained permissions they didn't have before since the repo is public, but I'm hoping that listing them as a collaborator maybe lets CI run? Maybe?
To try to "fix" it, I have added @wprzytula as a read-only contributor. The should result in zero gained permissions they didn't have before since the repo is public, but I'm hoping that listing them as a collaborator maybe lets CI run? Maybe?
Hooray! This works.
The should result in zero gained permissions they didn't have before since the repo is public, but I'm hoping that listing them as a collaborator maybe lets CI run?
It's great that this works, but I believe this is a bug on GitHub's end. It used to only require approval once. I don't like adding contributors to repositories as a permanent fix; we'll for sure forget to maintain / clean that list up.
The current CI failure seems to be about MSRV. Can someone confirm? What are we doing about that?
The current CI failure seems to be about MSRV. Can someone confirm? What are we doing about that?
Our policy is to bump the MSRV whenever doing so is convenient. You may need to increase the nightly toolchain version as well. See https://github.com/tock/libtock-rs/pull/541 for an example of how to do so.
Oh, and I suggest increasing the toolchain versions in a separate PR, just in case the toolchain bump causes compilation failures for somebody else' PR. If the toolchain version increase passes CI we can merge it quickly.
@lschuermann @jrvanwhy Why is this still not merged? Are there still some action points to be resolved?