Conversation
In order to handle multiple connection to radios and to be able to close the USB radio once all connections are dropped, the LinkContext was using Arc<SharedCrazyradio> and Weak<SharedCrazyradio>. This was not how the SharedCrazyradio was intended to be used, it was internded to be cloned to share the radio, not access by multiple unmutable references. This commit makes the SharedCrazyradio type encode this requirement by taking &mut self to most methods. It also adds a WeakSharedCrazyradio to handle the USB device management. The Crate version is bumped up since this is a breaking change.
There was a problem hiding this comment.
Pull request overview
This pull request adds Wireshark packet capture functionality to the Crazyradio driver. It introduces a callback-based mechanism that allows external code to intercept and process all packets sent and received by the radio, which is useful for debugging and protocol analysis.
Changes:
- Added a new
capturemodule with a global callback mechanism for packet capture - Integrated capture calls into
send_packetandsend_packet_no_ackmethods to capture TX packets and ACK payloads - Added a
radio_indexfield to identify different radio instances in captures - Added a new "wireshark" feature flag to make this functionality optional
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| Cargo.toml | Adds the "wireshark" feature flag with no dependencies |
| src/capture.rs | New module providing callback registration and packet capture infrastructure using OnceLock and Mutex for thread-safe global state |
| src/lib.rs | Integrates capture calls in packet send methods, adds radio_index field to Crazyradio struct for capture identification |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
evoggy
force-pushed
the
evoggy/wireshark
branch
from
baca7eb to
45f7025
Compare
ataffanel
left a comment
ataffanel
left a comment
There was a problem hiding this comment.
Looks good, some documentation and naming comments, but the functionality is simple and sweet, great job!
| /// | ||
| /// This should be called once at initialization to enable packet capture. | ||
| /// Subsequent calls will be silently ignored. | ||
| pub fn set_callback(callback: CaptureCallback) { |
There was a problem hiding this comment.
Somewhere there should be some documentation about what is expected of this callback. Since it is called synchronously in-line with the packet handling, it should be as quick as possible to execute.
src/lib.rs
Outdated
| //! - **shared_radio** enables [SharedCrazyradio] object that allows to share a radio between threads | ||
| //! - **async** enables async function to create a [Crazyradio] object and use the [SharedCrazyradio] | ||
| //! - **serde** emables [serde](https://crates.io/crates/serde) serialization/deserialization of the [Channel] struct | ||
| //! - **wireshark** enables packet capture to Wireshark |
There was a problem hiding this comment.
nitpick (?): There is nothing related to wireshark in this change. All there is is a way to capture packets. Maybe the documentation and fonctionality should be called something like "packet_capture" to make it clear that wireshark is not involve at this level.
There was a problem hiding this comment.
None of the functionality in this crate or the crazyflie-lib-rs crate is strictly connected to Wireshark, it's not until you reach the extcap module (https://github.com/evoggy/wireshark-crazyflie) it's reformatted to fit Wireshark. So I'll rename it all the way and remove any reference to Wireshark.
Added functionality to sending data to the library and forwarding it to Wireshark.