Initial networking stack with software timestamping #52
Conversation
rnijveld
changed the title
| fn recv( | ||
| socket: &std::net::UdpSocket, | ||
| buf: &mut [u8], | ||
| recv_ts: &mut Option<NtpTimestamp>, |
There was a problem hiding this comment.
Using a &mut Option as an outparam seems pretty unidiomatic. I think it would definitely be better to return a tuple (why do we think callers will be interested in the length of the read?).
There was a problem hiding this comment.
Definitely! So the final version already changed that! The length of the read would mostly be important to ensure that the buffer was filled with the expected number of bytes and not fewer than expected.
| @@ -24,7 +24,7 @@ async fn setup_connection() -> std::io::Result<UdpSocket> { | |||
|
|
|||
| #[tokio::main] | |||
| async fn main() -> Result<(), Box<dyn Error>> { | |||
| let socket = setup_connection().await?; | |||
| let socket = ntp_udp::UdpSocket::from_tokio(setup_connection().await?)?; | |||
There was a problem hiding this comment.
Why not add a basic constructor to ntp_udp::UdpSocket that takes the source and destination addresses, and move that functionality out of this crate's setup_connection()? That seems a simpler design. (I would probably skip having the from_std() and from_tokio() constructors unless they add value for testing.)
Right now the receive timestamp is returned via an output parameter. I think it could be made to return as a return value just as easily, but this might feel more natural in places where a normal recv could also be used.
See #14