Merge pull request #22 from atlas-aero/21-add-check-for-ram-memory-ac…

…cess

#21 add additional checks for RAM memory access

src/can.rs

@@ -62,6 +62,8 @@ pub enum Error<B, CS> {
     InvalidPayloadLength(usize),
     /// Invalid Ram Address region error
     InvalidRamAddress(u16),
+    /// Payload buffer length not a multiple of 4 bytes
+    InvalidBufferSize(usize),
 }
 
 impl<B, CS> From<BusError<B, CS>> for Error<B, CS> {
@@ -316,7 +318,7 @@ impl<B: Transfer<u8>, CS: OutputPin, CLK: Clock> Controller<B, CS, CLK> {
     }
 
     /// Receive CAN Message
-    pub fn receive(&mut self, data: &mut [u8]) -> Result<(), Error<B::Error, CS::Error>> {
+    pub fn receive<const L: usize>(&mut self, data: &mut [u8; L]) -> Result<(), Error<B::Error, CS::Error>> {
         let fifo_status_reg = Self::fifo_status_register(FIFO_RX_INDEX);
 
         let mut rxfifo_status_byte0 = self.read_register(fifo_status_reg)?;
@@ -357,7 +359,7 @@ impl<B: Transfer<u8>, CS: OutputPin, CLK: Clock> Controller<B, CS, CLK> {
 
         // copy message data into mutable buffer
         let mut data = [0u8; L];
-        data.copy_from_slice(&message.buff);
+        data[..message.buff.len()].copy_from_slice(&message.buff);
 
         buffer[0] = (command >> 8) as u8;
         buffer[1] = (command & 0xFF) as u8;
@@ -377,9 +379,18 @@ impl<B: Transfer<u8>, CS: OutputPin, CLK: Clock> Controller<B, CS, CLK> {
     }
 
     /// Read message from RX FIFO
-    fn read_fifo(&mut self, register: u16, data: &mut [u8]) -> Result<(), Error<B::Error, CS::Error>> {
+    pub(crate) fn read_fifo<const L: usize>(
+        &mut self,
+        register: u16,
+        data: &mut [u8; L],
+    ) -> Result<(), Error<B::Error, CS::Error>> {
+        if L % 4 != 0 {
+            return Err(Error::InvalidBufferSize(L));
+        }
+
         let payload_address = register + 8;
         let mut buffer = [0u8; 2];
+
         let command = (payload_address & 0x0FFF) | ((Operation::Read as u16) << 12);
 
         buffer[0] = (command >> 8) as u8;

src/message.rs

@@ -34,14 +34,17 @@ pub enum DLC {
     SixtyFour,
 }
 
-/// Invalid data length code error
+/// Possible errors when creating a [TxMessage] object
 #[derive(Debug, Eq, PartialEq, Ord, PartialOrd)]
-pub enum DLCError {
+pub enum MessageError {
+    /// Payload length invalid
     InvalidLength(usize),
+    /// Message type Length argument not divisble by 4
+    InvalidTypeSize(usize),
 }
 
 impl DLC {
-    fn from_length(value: usize) -> Result<Self, DLCError> {
+    fn from_length(value: usize) -> Result<Self, MessageError> {
         match value {
             0 => Ok(Self::Zero),
             1 => Ok(Self::One),
@@ -59,7 +62,7 @@ impl DLC {
             32 => Ok(Self::ThirtyTwo),
             48 => Ok(Self::FortyEight),
             64 => Ok(Self::SixtyFour),
-            val => Err(DLCError::InvalidLength(val)),
+            val => Err(MessageError::InvalidLength(val)),
         }
     }
 }
@@ -98,36 +101,58 @@ pub struct TxHeader {
 
 pub trait MessageType<const L: usize> {
     /// Setup CAN message header depending on message type
-    fn setup_header(&self, header: &mut TxHeader, payload_length: usize) -> Result<(), DLCError>;
+    fn setup_header(&self, header: &mut TxHeader, payload_length: usize) -> Result<(), MessageError>;
 }
 
-/// CAN 2.0 message type where L is the number  //  if payload_length > MAX_LENGTH {
+/// CAN 2.0 message type where `L` is the max number of payload bytes.
+/// For CAN2.0, `L` can either be 4 or 8.
 #[derive(Debug, Copy, Clone)]
 pub struct Can20<const L: usize> {}
 
 impl<const L: usize> MessageType<L> for Can20<L> {
-    fn setup_header(&self, _header: &mut TxHeader, payload_length: usize) -> Result<(), DLCError> {
+    fn setup_header(&self, _header: &mut TxHeader, payload_length: usize) -> Result<(), MessageError> {
         if L > 8 || payload_length > 8 {
             let max = payload_length.max(L);
-            debug!("Maximum of 8 bytes allowed. Current size: {max} bytes");
-            return Err(DLCError::InvalidLength(max));
+            debug!("Maximum of 64 bytes allowed. Current size: {max} bytes");
+            return Err(MessageError::InvalidLength(max));
+        }
+
+        if payload_length > L {
+            debug!("Payload length {payload_length} must be less than or equal {L}");
+            return Err(MessageError::InvalidLength(payload_length));
         }
+
+        if L % 4 != 0 {
+            debug!("CAN2.0 generic argument must be 4 or 8");
+            return Err(MessageError::InvalidTypeSize(L));
+        }
+
         Ok(())
     }
 }
-
-/// CAN FD message type where L is number data bytes
+/// CAN FD message type where `L` is the max number of payload bytes.
+/// `L` must be a multiple of 4 and `L` >= actual payload length in bytes
 #[derive(Debug, Copy, Clone)]
 pub struct CanFd<const L: usize> {
     pub bitrate_switch: bool,
 }
 
 impl<const L: usize> MessageType<L> for CanFd<L> {
-    fn setup_header(&self, header: &mut TxHeader, payload_length: usize) -> Result<(), DLCError> {
+    fn setup_header(&self, header: &mut TxHeader, payload_length: usize) -> Result<(), MessageError> {
         if L > 64 || payload_length > 64 {
             let max = payload_length.max(L);
             debug!("Maximum of 64 bytes allowed. Current size: {max} bytes");
-            return Err(DLCError::InvalidLength(max));
+            return Err(MessageError::InvalidLength(max));
+        }
+
+        if payload_length > L {
+            debug!("Payload length {payload_length} must be less than or equal {L}");
+            return Err(MessageError::InvalidLength(payload_length));
+        }
+
+        if L % 4 != 0 {
+            debug!("CANFD generic argument must be a multiple of 4");
+            return Err(MessageError::InvalidTypeSize(L));
         }
 
         header.set_bit_rate_switch(self.bitrate_switch);
@@ -148,15 +173,15 @@ pub struct TxMessage<T: MessageType<L>, const L: usize> {
 }
 
 impl<T: MessageType<L>, const L: usize> TxMessage<T, L> {
-    pub fn new(message_type: T, data: Bytes, identifier: Id) -> Result<Self, DLCError> {
+    pub fn new(message_type: T, data: Bytes, identifier: Id) -> Result<Self, MessageError> {
         let mut header = TxHeader::new();
 
         let mut payload_length = data.len();
 
         message_type.setup_header(&mut header, payload_length)?;
 
         // length used to choose the next supported DLC
-        while let Err(DLCError::InvalidLength(_)) = DLC::from_length(payload_length) {
+        while let Err(MessageError::InvalidLength(_)) = DLC::from_length(payload_length) {
             payload_length += 1;
         }
 

src/tests/can.rs

@@ -1,4 +1,4 @@
-use crate::can::{BusError, ConfigError, Controller};
+use crate::can::{BusError, ConfigError, Controller, Error};
 use crate::config::{
     BitRateConfig, ClockConfiguration, ClockOutputDivisor, Configuration, FifoConfiguration, PLLSetting, PayloadSize,
     RequestMode, RetransmissionAttempts, SystemClockDivisor,
@@ -285,6 +285,113 @@ fn test_transmit_can20() {
     mocks.into_controller().transmit(&tx_message_copy).unwrap();
 }
 
+#[test]
+fn test_transmit_can20_3_bytes() {
+    let mut mocks = Mocks::default();
+    let mut seq = Sequence::new();
+    let payload: [u8; 3] = [1, 2, 3];
+    let payload_bytes = Bytes::copy_from_slice(&payload);
+
+    let msg_type = Can20::<8> {};
+
+    let identifier = ExtendedId::new(EXTENDED_ID).unwrap();
+    let tx_message = TxMessage::new(msg_type, payload_bytes, Id::Extended(identifier)).unwrap();
+    let tx_message_copy = tx_message.clone();
+
+    // mock fifo status register read byte 0 (1st attempt) -> tx fifo full
+    mocks.mock_register_read::<0b0000_0000>([0x30, 0x6C], &mut seq);
+
+    // mock fifo status register read byte 0 (2nd attempt) -> tx fifo not full
+    mocks.mock_register_read::<0b0000_0001>([0x30, 0x6C], &mut seq);
+
+    // mock read operation status
+    mocks.mock_register_read::<0b1100_0000>([0x30, 0x2], &mut seq);
+
+    // mock fifo user address register read (reading 32 bits) --> address = 0x4A2
+    mocks.mock_read32::<0x00_00_04_A2>([0x30, 0x70], &mut seq);
+
+    // mock writing message in RAM specified by fifo user address (0x4A2)
+    // transfer cmd+tx_header
+    mocks
+        .pin_cs
+        .expect_set_low()
+        .times(1)
+        .return_const(Ok(()))
+        .in_sequence(&mut seq);
+
+    mocks
+        .bus
+        .expect_transfer()
+        .times(1)
+        .returning(move |data| {
+            let mut cmd_and_header_buffer = [0u8; 10];
+            cmd_and_header_buffer[0] = 0x28;
+            cmd_and_header_buffer[1] = 0xA2;
+
+            cmd_and_header_buffer[2..].copy_from_slice(&tx_message.header.into_bytes());
+
+            for chunk in cmd_and_header_buffer[2..].chunks_exact_mut(4) {
+                let num = BigEndian::read_u32(chunk);
+                LittleEndian::write_u32(chunk, num);
+            }
+
+            assert_eq!(cmd_and_header_buffer, data);
+            Ok(&[0u8; 10])
+        })
+        .in_sequence(&mut seq);
+
+    // transfer payload
+    mocks
+        .bus
+        .expect_transfer()
+        .times(1)
+        .returning(move |data| {
+            assert_eq!(payload, data[..payload.len()]);
+            Ok(&[0u8; 8])
+        })
+        .in_sequence(&mut seq);
+
+    mocks
+        .pin_cs
+        .expect_set_high()
+        .times(1)
+        .return_const(Ok(()))
+        .in_sequence(&mut seq);
+
+    // mock setting of bits txreq and uinc
+    mocks
+        .pin_cs
+        .expect_set_low()
+        .times(1)
+        .return_const(Ok(()))
+        .in_sequence(&mut seq);
+
+    mocks
+        .bus
+        .expect_transfer()
+        .times(1)
+        .returning(move |data| {
+            assert_eq!([0x20, 0x69, 0x03], data);
+            Ok(&[0u8; 3])
+        })
+        .in_sequence(&mut seq);
+
+    mocks
+        .pin_cs
+        .expect_set_high()
+        .times(1)
+        .return_const(Ok(()))
+        .in_sequence(&mut seq);
+
+    // mock reading of fifo control register
+    // 1st attempt -> txreq still set ->not all messages inside tx fifo have been transmitted
+    mocks.mock_register_read::<0x02>([0x30, 0x69], &mut seq);
+    // 2nd attempt -> txreq cleared -> all messages inside tx fifo have been transmitted
+    mocks.mock_register_read::<0x00>([0x30, 0x69], &mut seq);
+
+    mocks.into_controller().transmit(&tx_message_copy).unwrap();
+}
+
 #[test]
 fn test_transmit_can_fd() {
     let mut mocks = Mocks::default();
@@ -391,6 +498,15 @@ fn test_transmit_can_fd() {
     mocks.into_controller().transmit(&tx_message_copy).unwrap();
 }
 
+#[test]
+fn test_read_fifo_invalid_payload_buffer_size() {
+    let mocks = Mocks::default();
+    let mut buff = [0u8; 3];
+
+    let result = mocks.into_controller().read_fifo(0x123, &mut buff);
+    assert_eq!(result.unwrap_err(), Error::InvalidBufferSize(3));
+}
+
 #[test]
 fn test_receive() {
     let mut mocks = Mocks::default();

src/tests/message.rs

@@ -1,4 +1,4 @@
-use crate::message::{Can20, CanFd, DLCError, TxMessage, DLC};
+use crate::message::{Can20, CanFd, MessageError, TxMessage, DLC};
 use bytes::Bytes;
 use embedded_can::Id;
 use embedded_can::{ExtendedId, StandardId};
@@ -40,7 +40,7 @@ fn test_dlc_success() {
     let payload_bytes = Bytes::copy_from_slice(&[0u8; 13]);
     let standard_id = StandardId::new(STANDARD_ID).unwrap();
 
-    let msg_type = CanFd::<13> { bitrate_switch: false };
+    let msg_type = CanFd::<16> { bitrate_switch: false };
 
     let message = TxMessage::new(msg_type, payload_bytes, Id::Standard(standard_id)).unwrap();
 
@@ -68,6 +68,46 @@ fn test_dlc_error() {
     let message_2_0 = TxMessage::new(can_msg_20, payload_bytes_2_0, Id::Standard(standard_id));
     let message_fd = TxMessage::new(can_msg_fd, payload_bytes_fd, Id::Standard(standard_id));
 
-    assert_eq!(message_2_0.unwrap_err(), DLCError::InvalidLength(10));
-    assert_eq!(message_fd.unwrap_err(), DLCError::InvalidLength(65));
+    assert_eq!(message_2_0.unwrap_err(), MessageError::InvalidLength(10));
+    assert_eq!(message_fd.unwrap_err(), MessageError::InvalidLength(65));
+}
+
+#[test]
+fn test_message_size_divisible_by_four_error() {
+    let data_2_0 = [0u8; 6];
+    let data_fd = [0u8; 26];
+
+    let payload_bytes_2_0 = Bytes::copy_from_slice(&data_2_0);
+    let payload_bytes_fd = Bytes::copy_from_slice(&data_fd);
+
+    let can_msg_20 = Can20::<6> {};
+    let can_msg_fd = CanFd::<26> { bitrate_switch: false };
+
+    let standard_id = StandardId::new(STANDARD_ID).unwrap();
+
+    let message_2_0 = TxMessage::new(can_msg_20, payload_bytes_2_0, Id::Standard(standard_id));
+    let message_fd = TxMessage::new(can_msg_fd, payload_bytes_fd, Id::Standard(standard_id));
+
+    assert_eq!(message_2_0.unwrap_err(), MessageError::InvalidTypeSize(6));
+    assert_eq!(message_fd.unwrap_err(), MessageError::InvalidTypeSize(26));
+}
+
+#[test]
+fn test_payload_greater_than_generic_type_args() {
+    let data_2_0 = [0u8; 5];
+    let data_fd = [0u8; 23];
+
+    let payload_bytes_2_0 = Bytes::copy_from_slice(&data_2_0);
+    let payload_bytes_fd = Bytes::copy_from_slice(&data_fd);
+
+    let can_msg_20 = Can20::<4> {};
+    let can_msg_fd = CanFd::<20> { bitrate_switch: false };
+
+    let standard_id = StandardId::new(STANDARD_ID).unwrap();
+
+    let message_2_0 = TxMessage::new(can_msg_20, payload_bytes_2_0, Id::Standard(standard_id));
+    let message_fd = TxMessage::new(can_msg_fd, payload_bytes_fd, Id::Standard(standard_id));
+
+    assert_eq!(message_2_0.unwrap_err(), MessageError::InvalidLength(5));
+    assert_eq!(message_fd.unwrap_err(), MessageError::InvalidLength(23));
 }