tock / libtock-rs

Rust userland library for Tock
Apache License 2.0
160 stars 105 forks source link

Raw 802.15.4 support #551

Open wprzytula opened 2 weeks ago

wprzytula commented 2 weeks ago

I built this upon libtock-c implementation.

I focused on providing safe abstractions.

Feedback much welcome.

Example is complete. Unit tests are done. Haven't tested this on hardware as well.

alevy commented 2 weeks ago

Looks good!

alevy commented 2 weeks ago

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.

tyler-potyondy commented 2 weeks ago

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:

Exciting to see some 15.4 support making its way into libtock-rs!

wprzytula commented 2 weeks ago

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!

tyler-potyondy commented 2 weeks ago

@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!

wprzytula commented 1 week ago

Finished implementation. I did some minor fixes to the logic, wrote a very decent test suite and polished documentation.

wprzytula commented 1 week ago

Applied clippy fixes.

alevy commented 1 week ago

@wprzytula what's the state of this?

It says still

Haven't tested this on hardware as well.

Is this still accurate?

wprzytula commented 1 week ago

@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.

wprzytula commented 1 week ago

I need help with understanding the Miri check failure. I'm not sure what is exactly believed to be UB and why.