firmware: specialize protocol read/write functions.

Before this commit, proto::io::{read,write}_* functions were taking
a &mut {Read,Write}. This means a lot of virtual dispatch.
After this commit, all these functions are specialized for
the specific IO trait.

This could be achieved with just changing the signature from
  fn read_x(reader: &mut Read)
to
  fn read_x<R: Read>(reader: &mut R)
but the functions were also grouped into ReadExt and WriteExt
traits as a refactoring.

Initially, it was expected that the generic traits from
the byteorder crate could be used, but they require endianness
to be specified on every call and thus aren't very ergonomic.
They also lack the equivalent to our read_string and read_bytes.

Thus, it seems fine to just define a slightly different extension
trait.

This also optimized the test_rpc_timing test: 1.7ms→1.2ms.

artiq/firmware/ksupport/lib.rs

@@ -126,7 +126,7 @@ extern fn rpc_send_async(service: u32, tag: CSlice<u8>, data: *const *const ())
             rpc_proto::send_args(&mut writer, service, tag.as_ref(), data)?;
             writer.position()
         };
-        proto::io::write_u32(&mut slice, length as u32)
+        proto::WriteExt::write_u32(&mut slice, length as u32)
     }).unwrap_or_else(|err| {
         assert!(err.kind() == std::io::ErrorKind::WriteZero);
 

artiq/firmware/libproto/analyzer_proto.rs

@@ -1,5 +1,5 @@
 use std::io::{self, Write};
-use io::*;
+use WriteExt;
 
 #[derive(Debug)]
 pub struct Header {
@@ -12,11 +12,11 @@ pub struct Header {
 
 impl Header {
     pub fn write_to(&self, writer: &mut Write) -> io::Result<()> {
-        write_u32(writer, self.sent_bytes)?;
-        write_u64(writer, self.total_byte_count)?;
-        write_u8(writer, self.overflow_occurred as u8)?;
-        write_u8(writer, self.log_channel)?;
-        write_u8(writer, self.dds_onehot_sel as u8)?;
+        writer.write_u32(self.sent_bytes)?;
+        writer.write_u64(self.total_byte_count)?;
+        writer.write_u8(self.overflow_occurred as u8)?;
+        writer.write_u8(self.log_channel)?;
+        writer.write_u8(self.dds_onehot_sel as u8)?;
         Ok(())
     }
 }

artiq/firmware/libproto/io.rs

@@ -1,75 +0,0 @@
-use std::io::{self, Read, Write};
-use std::vec::Vec;
-use std::string::String;
-use byteorder::{ByteOrder, NetworkEndian};
-
-// FIXME: replace these with byteorder core io traits once those are in
-pub fn read_u8(reader: &mut Read) -> io::Result<u8> {
-    let mut bytes = [0; 1];
-    reader.read_exact(&mut bytes)?;
-    Ok(bytes[0])
-}
-
-pub fn write_u8(writer: &mut Write, value: u8) -> io::Result<()> {
-    let bytes = [value; 1];
-    writer.write_all(&bytes)
-}
-
-pub fn read_u16(reader: &mut Read) -> io::Result<u16> {
-    let mut bytes = [0; 2];
-    reader.read_exact(&mut bytes)?;
-    Ok(NetworkEndian::read_u16(&bytes))
-}
-
-pub fn write_u16(writer: &mut Write, value: u16) -> io::Result<()> {
-    let mut bytes = [0; 2];
-    NetworkEndian::write_u16(&mut bytes, value);
-    writer.write_all(&bytes)
-}
-
-pub fn read_u32(reader: &mut Read) -> io::Result<u32> {
-    let mut bytes = [0; 4];
-    reader.read_exact(&mut bytes)?;
-    Ok(NetworkEndian::read_u32(&bytes))
-}
-
-pub fn write_u32(writer: &mut Write, value: u32) -> io::Result<()> {
-    let mut bytes = [0; 4];
-    NetworkEndian::write_u32(&mut bytes, value);
-    writer.write_all(&bytes)
-}
-
-pub fn read_u64(reader: &mut Read) -> io::Result<u64> {
-    let mut bytes = [0; 8];
-    reader.read_exact(&mut bytes)?;
-    Ok(NetworkEndian::read_u64(&bytes))
-}
-
-pub fn write_u64(writer: &mut Write, value: u64) -> io::Result<()> {
-    let mut bytes = [0; 8];
-    NetworkEndian::write_u64(&mut bytes, value);
-    writer.write_all(&bytes)
-}
-
-pub fn read_bytes(reader: &mut Read) -> io::Result<Vec<u8>> {
-    let length = read_u32(reader)?;
-    let mut value = Vec::new();
-    value.resize(length as usize, 0);
-    reader.read_exact(&mut value)?;
-    Ok(value)
-}
-
-pub fn write_bytes(writer: &mut Write, value: &[u8]) -> io::Result<()> {
-    write_u32(writer, value.len() as u32)?;
-    writer.write_all(value)
-}
-
-pub fn read_string(reader: &mut Read) -> io::Result<String> {
-    let bytes = read_bytes(reader)?;
-    String::from_utf8(bytes)
-           .map_err(|_| io::Error::new(io::ErrorKind::InvalidData, "invalid UTF-8"))
-}
-
-pub fn write_string(writer: &mut Write, value: &str) -> io::Result<()> {
-    write_bytes(writer, value.as_bytes())
-}

artiq/firmware/libproto/lib.rs

@@ -9,8 +9,6 @@ extern crate log;
 extern crate dyld;
 extern crate std_artiq as std;
 
-pub mod io;
-
 // Internal protocols.
 pub mod kernel_proto;
 
@@ -19,3 +17,86 @@ pub mod analyzer_proto;
 pub mod moninj_proto;
 pub mod session_proto;
 pub mod rpc_proto;
+
+use std::io::{Read, Write, Result, Error, ErrorKind};
+use std::vec::Vec;
+use std::string::String;
+use byteorder::{ByteOrder, NetworkEndian};
+
+pub trait ReadExt: Read {
+    fn read_u8(&mut self) -> Result<u8> {
+        let mut bytes = [0; 1];
+        self.read_exact(&mut bytes)?;
+        Ok(bytes[0])
+    }
+
+    fn read_u16(&mut self) -> Result<u16> {
+        let mut bytes = [0; 2];
+        self.read_exact(&mut bytes)?;
+        Ok(NetworkEndian::read_u16(&bytes))
+    }
+
+    fn read_u32(&mut self) -> Result<u32> {
+        let mut bytes = [0; 4];
+        self.read_exact(&mut bytes)?;
+        Ok(NetworkEndian::read_u32(&bytes))
+    }
+
+    fn read_u64(&mut self) -> Result<u64> {
+        let mut bytes = [0; 8];
+        self.read_exact(&mut bytes)?;
+        Ok(NetworkEndian::read_u64(&bytes))
+    }
+
+    fn read_bytes(&mut self) -> Result<Vec<u8>> {
+        let length = self.read_u32()?;
+        let mut value = Vec::new();
+        value.resize(length as usize, 0);
+        self.read_exact(&mut value)?;
+        Ok(value)
+    }
+
+    fn read_string(&mut self) -> Result<String> {
+        let bytes = self.read_bytes()?;
+        String::from_utf8(bytes)
+               .map_err(|_| Error::new(ErrorKind::InvalidData, "invalid UTF-8"))
+    }
+}
+
+impl<R: Read + ?Sized> ReadExt for R {}
+
+pub trait WriteExt: Write {
+    fn write_u8(&mut self, value: u8) -> Result<()> {
+        let bytes = [value; 1];
+        self.write_all(&bytes)
+    }
+
+    fn write_u16(&mut self, value: u16) -> Result<()> {
+        let mut bytes = [0; 2];
+        NetworkEndian::write_u16(&mut bytes, value);
+        self.write_all(&bytes)
+    }
+
+    fn write_u32(&mut self, value: u32) -> Result<()> {
+        let mut bytes = [0; 4];
+        NetworkEndian::write_u32(&mut bytes, value);
+        self.write_all(&bytes)
+    }
+
+    fn write_u64(&mut self, value: u64) -> Result<()> {
+        let mut bytes = [0; 8];
+        NetworkEndian::write_u64(&mut bytes, value);
+        self.write_all(&bytes)
+    }
+
+    fn write_bytes(&mut self, value: &[u8]) -> Result<()> {
+        self.write_u32(value.len() as u32)?;
+        self.write_all(value)
+    }
+
+    fn write_string(&mut self, value: &str) -> Result<()> {
+        self.write_bytes(value.as_bytes())
+    }
+}
+
+impl<W: Write + ?Sized> WriteExt for W {}

artiq/firmware/libproto/moninj_proto.rs

@@ -1,5 +1,5 @@
 use std::io::{self, Read, Write};
-use io::*;
+use {ReadExt, WriteExt};
 
 #[derive(Debug)]
 pub enum HostMessage {
@@ -16,20 +16,20 @@ pub enum DeviceMessage {
 
 impl HostMessage {
     pub fn read_from(reader: &mut Read) -> io::Result<HostMessage> {
-        Ok(match read_u8(reader)? {
+        Ok(match reader.read_u8()? {
             0 => HostMessage::Monitor {
-                enable: if read_u8(reader)? == 0 { false } else { true },
-                channel: read_u32(reader)?,
-                probe: read_u8(reader)?
+                enable: if reader.read_u8()? == 0 { false } else { true },
+                channel: reader.read_u32()?,
+                probe: reader.read_u8()?
             },
             1 => HostMessage::Inject {
-                channel: read_u32(reader)?,
-                overrd: read_u8(reader)?,
-                value: read_u8(reader)?
+                channel: reader.read_u32()?,
+                overrd: reader.read_u8()?,
+                value: reader.read_u8()?
             },
             2 => HostMessage::GetInjectionStatus {
-                channel: read_u32(reader)?,
-                overrd: read_u8(reader)?
+                channel: reader.read_u32()?,
+                overrd: reader.read_u8()?
             },
             _ => return Err(io::Error::new(io::ErrorKind::InvalidData, "unknown packet type"))
         })
@@ -40,16 +40,16 @@ impl DeviceMessage {
     pub fn write_to(&self, writer: &mut Write) -> io::Result<()> {
         match *self {
             DeviceMessage::MonitorStatus { channel, probe, value } => {
-                write_u8(writer, 0)?;
-                write_u32(writer, channel)?;
-                write_u8(writer, probe)?;
-                write_u32(writer, value)?;
+                writer.write_u8(0)?;
+                writer.write_u32(channel)?;
+                writer.write_u8(probe)?;
+                writer.write_u32(value)?;
             },
             DeviceMessage::InjectionStatus { channel, overrd, value } => {
-                write_u8(writer, 1)?;
-                write_u32(writer, channel)?;
-                write_u8(writer, overrd)?;
-                write_u8(writer, value)?;
+                writer.write_u8(1)?;
+                writer.write_u32(channel)?;
+                writer.write_u8(overrd)?;
+                writer.write_u8(value)?;
             }
         }
         Ok(())

artiq/firmware/libproto/rpc_proto.rs

@@ -1,7 +1,7 @@
 use std::io::{self, Read, Write};
 use std::str;
 use cslice::{CSlice, CMutSlice};
-use io::*;
+use {ReadExt, WriteExt};
 use self::tag::{Tag, TagIterator, split_tag};
 
 unsafe fn recv_value(reader: &mut Read, tag: Tag, data: &mut *mut (),
@@ -18,19 +18,19 @@ unsafe fn recv_value(reader: &mut Read, tag: Tag, data: &mut *mut (),
         Tag::None => Ok(()),
         Tag::Bool =>
             consume_value!(u8, |ptr| {
-                *ptr = read_u8(reader)?; Ok(())
+                *ptr = reader.read_u8()?; Ok(())
             }),
         Tag::Int32 =>
             consume_value!(u32, |ptr| {
-                *ptr = read_u32(reader)?; Ok(())
+                *ptr = reader.read_u32()?; Ok(())
             }),
         Tag::Int64 | Tag::Float64 =>
             consume_value!(u64, |ptr| {
-                *ptr = read_u64(reader)?; Ok(())
+                *ptr = reader.read_u64()?; Ok(())
             }),
         Tag::String => {
             consume_value!(CMutSlice<u8>, |ptr| {
-                let length = read_u32(reader)? as usize;
+                let length = reader.read_u32()? as usize;
                 *ptr = CMutSlice::new(alloc(length)? as *mut u8, length);
                 reader.read_exact((*ptr).as_mut())?;
                 Ok(())
@@ -47,7 +47,7 @@ unsafe fn recv_value(reader: &mut Read, tag: Tag, data: &mut *mut (),
         Tag::List(it) | Tag::Array(it) => {
             struct List { elements: *mut (), length: u32 };
             consume_value!(List, |ptr| {
-                (*ptr).length = read_u32(reader)?;
+                (*ptr).length = reader.read_u32()?;
 
                 let tag = it.clone().next().expect("truncated tag");
                 (*ptr).elements = alloc(tag.size() * (*ptr).length as usize)?;
@@ -93,24 +93,24 @@ unsafe fn send_value(writer: &mut Write, tag: Tag, data: &mut *const ()) -> io::
         })
     }
 
-    write_u8(writer, tag.as_u8())?;
+    writer.write_u8(tag.as_u8())?;
     match tag {
         Tag::None => Ok(()),
         Tag::Bool =>
             consume_value!(u8, |ptr|
-                write_u8(writer, *ptr)),
+                writer.write_u8(*ptr)),
         Tag::Int32 =>
             consume_value!(u32, |ptr|
-                write_u32(writer, *ptr)),
+                writer.write_u32(*ptr)),
         Tag::Int64 | Tag::Float64 =>
             consume_value!(u64, |ptr|
-                write_u64(writer, *ptr)),
+                writer.write_u64(*ptr)),
         Tag::String =>
             consume_value!(CSlice<u8>, |ptr|
-                write_string(writer, str::from_utf8((*ptr).as_ref()).unwrap())),
+                writer.write_string(str::from_utf8((*ptr).as_ref()).unwrap())),
         Tag::Tuple(it, arity) => {
             let mut it = it.clone();
-            write_u8(writer, arity)?;
+            writer.write_u8(arity)?;
             for _ in 0..arity {
                 let tag = it.next().expect("truncated tag");
                 send_value(writer, tag, data)?
@@ -120,7 +120,7 @@ unsafe fn send_value(writer: &mut Write, tag: Tag, data: &mut *const ()) -> io::
         Tag::List(it) | Tag::Array(it) => {
             struct List { elements: *const (), length: u32 };
             consume_value!(List, |ptr| {
-                write_u32(writer, (*ptr).length)?;
+                writer.write_u32((*ptr).length)?;
                 let tag = it.clone().next().expect("truncated tag");
                 let mut data = (*ptr).elements;
                 for _ in 0..(*ptr).length as usize {
@@ -139,7 +139,7 @@ unsafe fn send_value(writer: &mut Write, tag: Tag, data: &mut *const ()) -> io::
         Tag::Keyword(it) => {
             struct Keyword<'a> { name: CSlice<'a, u8>, contents: () };
             consume_value!(Keyword, |ptr| {
-                write_string(writer, str::from_utf8((*ptr).name.as_ref()).unwrap())?;
+                writer.write_string(str::from_utf8((*ptr).name.as_ref()).unwrap())?;
                 let tag = it.clone().next().expect("truncated tag");
                 let mut data = &(*ptr).contents as *const ();
                 send_value(writer, tag, &mut data)
@@ -150,7 +150,7 @@ unsafe fn send_value(writer: &mut Write, tag: Tag, data: &mut *const ()) -> io::
         Tag::Object => {
             struct Object { id: u32 };
             consume_value!(*const Object, |ptr|
-                write_u32(writer, (**ptr).id))
+                writer.write_u32((**ptr).id))
         }
     }
 }
@@ -166,7 +166,7 @@ pub fn send_args(writer: &mut Write, service: u32, tag_bytes: &[u8],
         trace!("send<{}>({})->{}", service, args_it, return_it);
     }
 
-    write_u32(writer, service)?;
+    writer.write_u32(service)?;
     for index in 0.. {
         if let Some(arg_tag) = args_it.next() {
             let mut data = unsafe { *data.offset(index) };
@@ -175,8 +175,8 @@ pub fn send_args(writer: &mut Write, service: u32, tag_bytes: &[u8],
             break
         }
     }
-    write_u8(writer, 0)?;
-    write_bytes(writer, return_tag_bytes)?;
+    writer.write_u8(0)?;
+    writer.write_bytes(return_tag_bytes)?;
 
     Ok(())
 }

artiq/firmware/libproto/session_proto.rs

@@ -1,12 +1,12 @@
 use std::io::{self, Read, Write};
 use std::vec::Vec;
 use std::string::String;
-use io::*;
+use {ReadExt, WriteExt};
 
 fn read_sync(reader: &mut Read) -> io::Result<()> {
     let mut sync = [0; 4];
     for i in 0.. {
-        sync[i % 4] = read_u8(reader)?;
+        sync[i % 4] = reader.read_u8()?;
         if sync == [0x5a; 4] { break }
     }
     Ok(())
@@ -47,37 +47,37 @@ pub enum Request {
 impl Request {
     pub fn read_from(reader: &mut Read) -> io::Result<Request> {
         read_sync(reader)?;
-        Ok(match read_u8(reader)? {
+        Ok(match reader.read_u8()? {
             1  => Request::Log,
             2  => Request::LogClear,
             3  => Request::SystemInfo,
-            4  => Request::SwitchClock(read_u8(reader)?),
-            5  => Request::LoadKernel(read_bytes(reader)?),
+            4  => Request::SwitchClock(reader.read_u8()?),
+            5  => Request::LoadKernel(reader.read_bytes()?),
             6  => Request::RunKernel,
             7  => Request::RpcReply {
-                tag: read_bytes(reader)?
+                tag: reader.read_bytes()?
             },
             8  => Request::RpcException {
-                name:     read_string(reader)?,
-                message:  read_string(reader)?,
-                param:    [read_u64(reader)? as i64,
-                           read_u64(reader)? as i64,
-                           read_u64(reader)? as i64],
-                file:     read_string(reader)?,
-                line:     read_u32(reader)?,
-                column:   read_u32(reader)?,
-                function: read_string(reader)?
+                name:     reader.read_string()?,
+                message:  reader.read_string()?,
+                param:    [reader.read_u64()? as i64,
+                           reader.read_u64()? as i64,
+                           reader.read_u64()? as i64],
+                file:     reader.read_string()?,
+                line:     reader.read_u32()?,
+                column:   reader.read_u32()?,
+                function: reader.read_string()?
             },
             9  => Request::FlashRead {
-                key: read_string(reader)?
+                key: reader.read_string()?
             },
             10 => Request::FlashWrite {
-                key:   read_string(reader)?,
-                value: read_bytes(reader)?
+                key:   reader.read_string()?,
+                value: reader.read_bytes()?
             },
             11 => Request::FlashErase,
             12 => Request::FlashRemove {
-                key: read_string(reader)?
+                key: reader.read_string()?
             },
             _  => return Err(io::Error::new(io::ErrorKind::InvalidData, "unknown request type"))
         })
@@ -126,77 +126,77 @@ impl<'a> Reply<'a> {
         write_sync(writer)?;
         match *self {
             Reply::Log(ref log) => {
-                write_u8(writer, 1)?;
-                write_string(writer, log)?;
+                writer.write_u8(1)?;
+                writer.write_string(log)?;
             },
 
             Reply::SystemInfo { ident, finished_cleanly } => {
-                write_u8(writer, 2)?;
+                writer.write_u8(2)?;
                 writer.write(b"AROR")?;
-                write_string(writer, ident)?;
-                write_u8(writer, finished_cleanly as u8)?;
+                writer.write_string(ident)?;
+                writer.write_u8(finished_cleanly as u8)?;
             },
             Reply::ClockSwitchCompleted => {
-                write_u8(writer, 3)?;
+                writer.write_u8(3)?;
             },
             Reply::ClockSwitchFailed => {
-                write_u8(writer, 4)?;
+                writer.write_u8(4)?;
             },
 
             Reply::LoadCompleted => {
-                write_u8(writer, 5)?;
+                writer.write_u8(5)?;
             },
             Reply::LoadFailed(reason) => {
-                write_u8(writer, 6)?;
-                write_string(writer, reason)?;
+                writer.write_u8(6)?;
+                writer.write_string(reason)?;
             },
 
             Reply::KernelFinished => {
-                write_u8(writer, 7)?;
+                writer.write_u8(7)?;
             },
             Reply::KernelStartupFailed => {
-                write_u8(writer, 8)?;
+                writer.write_u8(8)?;
             },
             Reply::KernelException {
                 name, message, param, file, line, column, function, backtrace
             } => {
-                write_u8(writer, 9)?;
-                write_string(writer, name)?;
-                write_string(writer, message)?;
-                write_u64(writer, param[0] as u64)?;
-                write_u64(writer, param[1] as u64)?;
-                write_u64(writer, param[2] as u64)?;
-                write_string(writer, file)?;
-                write_u32(writer, line)?;
-                write_u32(writer, column)?;
-                write_string(writer, function)?;
-                write_u32(writer, backtrace.len() as u32)?;
+                writer.write_u8(9)?;
+                writer.write_string(name)?;
+                writer.write_string(message)?;
+                writer.write_u64(param[0] as u64)?;
+                writer.write_u64(param[1] as u64)?;
+                writer.write_u64(param[2] as u64)?;
+                writer.write_string(file)?;
+                writer.write_u32(line)?;
+                writer.write_u32(column)?;
+                writer.write_string(function)?;
+                writer.write_u32(backtrace.len() as u32)?;
                 for &addr in backtrace {
-                    write_u32(writer, addr as u32)?
+                    writer.write_u32(addr as u32)?
                 }
             },
 
             Reply::RpcRequest { async } => {
-                write_u8(writer, 10)?;
-                write_u8(writer, async as u8)?;
+                writer.write_u8(10)?;
+                writer.write_u8(async as u8)?;
             },
 
             Reply::FlashRead(ref bytes) => {
-                write_u8(writer, 11)?;
-                write_bytes(writer, bytes)?;
+                writer.write_u8(11)?;
+                writer.write_bytes(bytes)?;
             },
             Reply::FlashOk => {
-                write_u8(writer, 12)?;
+                writer.write_u8(12)?;
             },
             Reply::FlashError => {
-                write_u8(writer, 13)?;
+                writer.write_u8(13)?;
             },
 
             Reply::WatchdogExpired => {
-                write_u8(writer, 14)?;
+                writer.write_u8(14)?;
             },
             Reply::ClockFailure => {
-                write_u8(writer, 15)?;
+                writer.write_u8(15)?;
             },
         }
         Ok(())

artiq/firmware/runtime/rtio_dma.rs

@@ -2,7 +2,7 @@ use std::vec::Vec;
 use std::string::String;
 use std::btree_map::BTreeMap;
 use std::mem;
-use proto::io;
+use proto::WriteExt;
 
 #[derive(Debug)]
 struct Entry {
@@ -33,14 +33,14 @@ impl Manager {
         let length = /*length*/1 + /*channel*/3 + /*timestamp*/8 + /*address*/2 +
                      /*data*/data.len() * 4;
         let writer = &mut self.recording;
-        io::write_u8(writer, length as u8).unwrap();
-        io::write_u8(writer, (channel >> 24) as u8).unwrap();
-        io::write_u8(writer, (channel >> 16) as u8).unwrap();
-        io::write_u8(writer, (channel >>  8) as u8).unwrap();
-        io::write_u64(writer, timestamp).unwrap();
-        io::write_u16(writer, address as u16).unwrap();
+        writer.write_u8(length as u8).unwrap();
+        writer.write_u8((channel >> 24) as u8).unwrap();
+        writer.write_u8((channel >> 16) as u8).unwrap();
+        writer.write_u8((channel >>  8) as u8).unwrap();
+        writer.write_u64(timestamp).unwrap();
+        writer.write_u16(address as u16).unwrap();
         for &word in data {
-            io::write_u32(writer, word).unwrap();
+            writer.write_u32(word).unwrap();
         }
     }