Comprehensive I2CSlave test

CI-Shield-Tests/I2CSlaveCommsTest.cpp

@@ -28,9 +28,9 @@
 
 using namespace utest::v1;
 
-// 7-bit I2C address of the Mbed MCU
-#define MBED_I2C_ADDRESS 0x72
-#define MBED_I2C_ADDRESS_STR "0x72"
+// 8-bit I2C address of the Mbed MCU
+#define MBED_I2C_ADDRESS 0xE4
+#define MBED_I2C_ADDRESS_STR "0xE4"
 
 
 // Single instance of I2C slave used in the test.
@@ -48,6 +48,7 @@ void host_start_i2c_logging()
     assert_next_message_from_host("start_recording_i2c", "complete");
 }
 
+
 #if STATIC_PINMAP_READY
 // Must be declared globally as I2C stores the pointer
 constexpr auto i2cPinmap = get_i2c_pinmap(PIN_I2C_SDA, PIN_I2C_SCL);
@@ -61,7 +62,7 @@ void create_i2c_object()
 #else	
     i2cSlave = new I2CSlave(PIN_I2C_SDA, PIN_I2C_SCL);
 #endif
-    i2cSlave->address(MBED_I2C_ADDRESS << 1);
+    i2cSlave->address(MBED_I2C_ADDRESS);
 	i2cSlave->frequency(400000);
 }
 
@@ -78,18 +79,202 @@ void test_write_one_byte_to_slave()
         auto event = i2cSlave->receive();
         if(event == I2CSlave::WriteAddressed)
         {
-            i2cSlave->read(reinterpret_cast<char*>(&byteRxed), sizeof(uint8_t));
+            TEST_ASSERT_EQUAL_INT(sizeof(uint8_t), i2cSlave->read(reinterpret_cast<char*>(&byteRxed), sizeof(uint8_t)));
             break;
         }
     }
 
     TEST_ASSERT_EQUAL_UINT8(byteRxed, 0x1);
+
+    assert_next_message_from_host("write_bytes_to_slave", "complete");
+}
+
+void test_write_one_byte_to_general_call()
+{
+    host_start_i2c_logging();
+
+    // Kick off the host test doing an I2C transaction.
+    // Some Mbed devices which implement I2C, e.g. LPC1768, can only receive 1 byte to the general call address.
+    // Some values are reserved in the I2C spec, so we write 0x70 which is not reserved.
+    greentea_send_kv("write_bytes_to_slave", "addr 0x0 data 0x70");
+
+    uint8_t byteRxed = 0;
+    while(true)
+    {
+        auto event = i2cSlave->receive();
+        if(event == I2CSlave::WriteGeneral)
+        {
+            TEST_ASSERT_EQUAL_INT(sizeof(uint8_t), i2cSlave->read(reinterpret_cast<char*>(&byteRxed), sizeof(uint8_t)));
+            break;
+        }
+    }
+
+    TEST_ASSERT_EQUAL_UINT8(byteRxed, 0x70);
+
+    assert_next_message_from_host("write_bytes_to_slave", "complete");
+}
+
+void test_doesnt_ack_other_slave_address()
+{
+    host_start_i2c_logging();
+
+    // Kick off the host test doing an I2C transaction
+    greentea_send_kv("try_write_to_wrong_address", "0xE6");
+
+    Timer timeoutTimer;
+    timeoutTimer.start();
+
+    uint8_t byteRxed = 0;
+    while(timeoutTimer.elapsed_time() < 250ms) // Ballpark guess, give the host some time to start the transaction
+    {
+        auto event = i2cSlave->receive();
+        if(event == I2CSlave::WriteAddressed)
+        {
+            i2cSlave->read(reinterpret_cast<char*>(&byteRxed), sizeof(uint8_t));
+            TEST_FAIL_MESSAGE("Write received for wrong address!");
+            break;
+        }
+    }
+
+    assert_next_message_from_host("try_write_to_wrong_address", "complete");
+
+    // We still shouldn't have gotten anything
+    TEST_ASSERT_EQUAL_INT(I2CSlave::NoData, i2cSlave->receive());
 }
 
 void test_destroy_recreate_object()
 {
     delete i2cSlave;
     create_i2c_object();
+
+    // In testing, when we release the I2C pins, it can cause weirdness that prevents the I2C bridge from the host PC from working.
+    // So, we tell the host to reinitialize the bridge.
+    greentea_send_kv("reinit_i2c_bridge", "please");
+    assert_next_message_from_host("reinit_i2c_bridge", "complete");
+}
+
+void test_write_multiple_bytes_to_slave()
+{
+    host_start_i2c_logging();
+
+    // Kick off the host test doing an I2C transaction
+    greentea_send_kv("write_bytes_to_slave", "addr " MBED_I2C_ADDRESS_STR " data 0x4 0x5 0x6 0x7");
+
+    uint8_t bytesRxed[4]{};
+
+    while(true)
+    {
+        auto event = i2cSlave->receive();
+        if(event == I2CSlave::WriteAddressed)
+        {
+            TEST_ASSERT_EQUAL_INT(sizeof(bytesRxed), i2cSlave->read(reinterpret_cast<char*>(bytesRxed), sizeof(bytesRxed)));
+            break;
+        }
+    }
+
+    TEST_ASSERT_EQUAL_UINT8(bytesRxed[0], 0x4);
+    TEST_ASSERT_EQUAL_UINT8(bytesRxed[1], 0x5);
+    TEST_ASSERT_EQUAL_UINT8(bytesRxed[2], 0x6);
+    TEST_ASSERT_EQUAL_UINT8(bytesRxed[3], 0x7);
+
+    assert_next_message_from_host("write_bytes_to_slave", "complete");
+}
+
+/*
+* Tests that if the master writes less bytes than we expect, the actual number of bytes is returned
+*/
+void test_write_less_than_expected_bytes_to_slave()
+{
+    host_start_i2c_logging();
+
+    // Kick off the host test doing an I2C transaction
+    greentea_send_kv("write_bytes_to_slave", "addr " MBED_I2C_ADDRESS_STR " data 0x8 0x9");
+
+    uint8_t bytesRxed[4]{};
+
+    while(true)
+    {
+        auto event = i2cSlave->receive();
+        if(event == I2CSlave::WriteAddressed)
+        {
+            TEST_ASSERT_EQUAL_INT(2, i2cSlave->read(reinterpret_cast<char*>(bytesRxed), sizeof(bytesRxed)));
+            break;
+        }
+    }
+
+    TEST_ASSERT_EQUAL_UINT8(bytesRxed[0], 0x8);
+    TEST_ASSERT_EQUAL_UINT8(bytesRxed[1], 0x9);
+
+    assert_next_message_from_host("write_bytes_to_slave", "complete");
+}
+
+void test_read_one_byte_from_slave()
+{
+    host_start_i2c_logging();
+
+    // Kick off the host test doing an I2C transaction
+    greentea_send_kv("read_bytes_from_slave", "addr " MBED_I2C_ADDRESS_STR " expected-data 0x10");
+
+    const uint8_t byteToSend = 0x10;
+    while(true)
+    {
+        auto event = i2cSlave->receive();
+        if(event == I2CSlave::ReadAddressed)
+        {
+            TEST_ASSERT_EQUAL_INT(0, i2cSlave->write(reinterpret_cast<char const *>(&byteToSend), sizeof(uint8_t)));
+            break;
+        }
+    }
+
+    assert_next_message_from_host("read_bytes_from_slave", "complete");
+}
+
+void test_read_multiple_bytes_from_slave()
+{
+    host_start_i2c_logging();
+
+    // Kick off the host test doing an I2C transaction
+    greentea_send_kv("read_bytes_from_slave", "addr " MBED_I2C_ADDRESS_STR " expected-data 0x11 0x12 0x13 0x14");
+
+    const uint8_t bytesToSend[4] = {0x11, 0x12, 0x13, 0x14};
+    while(true)
+    {
+        auto event = i2cSlave->receive();
+        if(event == I2CSlave::ReadAddressed)
+        {
+            TEST_ASSERT_EQUAL_INT(0, i2cSlave->write(reinterpret_cast<char const *>(&bytesToSend), sizeof(bytesToSend)));
+            break;
+        }
+    }
+
+    assert_next_message_from_host("read_bytes_from_slave", "complete");
+}
+
+/*
+* Test that, if the master tries to read less bytes from us than we expect, 
+* write() returns an error and the master sees the correct data.
+*/
+void test_read_less_bytes_than_expected_from_slave()
+{
+    host_start_i2c_logging();
+
+    // Kick off the host test doing an I2C transaction
+    greentea_send_kv("read_bytes_from_slave", "addr " MBED_I2C_ADDRESS_STR " expected-data 0x15 0x16");
+
+    const uint8_t bytesToSend[4] = {0x15, 0x16, 0x17, 0x18};
+    while(true)
+    {
+        auto event = i2cSlave->receive();
+        if(event == I2CSlave::ReadAddressed)
+        {
+            // Unfortunately there's no specification about the return value from write() in this situation other than that it's
+            // supposed to be nonzero.
+            TEST_ASSERT_NOT_EQUAL(0, i2cSlave->write(reinterpret_cast<char const *>(&bytesToSend), sizeof(bytesToSend)));
+            break;
+        }
+    }
+
+    assert_next_message_from_host("read_bytes_from_slave", "complete");
 }
 
 utest::v1::status_t test_setup(const size_t number_of_cases)
@@ -102,7 +287,7 @@ utest::v1::status_t test_setup(const size_t number_of_cases)
     funcSelPins = 0b001;
 
 	// Setup Greentea using a reasonable timeout in seconds
-	GREENTEA_SETUP(20, "i2c_slave_comms");
+	GREENTEA_SETUP(30, "i2c_slave_comms");
 	return verbose_test_setup_handler(number_of_cases);
 }
 
@@ -112,10 +297,28 @@ void test_teardown(const size_t passed, const size_t failed, const failure_t fai
     return greentea_test_teardown_handler(passed, failed, failure);
 }
 
+// TODO test what happens if the master writes more bytes to a slave than the length of the buffer passed to read().
+// The current I2CSlave API does not specify what is supposed to happen in this case -- does the slave NACK, or does it
+// accept bytes and then discard them?
+
+// TODO test what happens if the master reads more bytes from a slave than the length of the buffer passed to write().
+// The slave cannot NACK the master in this situation.
+// Does the slave write junk to the bus?  What error code is returned from write()?
+// The current I2CSlave API does not specify what is supposed to happen in this case.
+
+// Note: Sadly, the i2c bridge chip does not support zero-length reads or writes, so we cannot test those automatically.
 
 // Test cases
 Case cases[] = {
 	Case("Write one byte to slave", test_write_one_byte_to_slave),
+    Case("Does not acknowledge other slave address", test_doesnt_ack_other_slave_address),
+    Case("Destroy & recreate I2C object", test_destroy_recreate_object),
+    Case("Write multiple bytes to slave", test_write_multiple_bytes_to_slave),
+    Case("Write less bytes than expected to slave", test_write_less_than_expected_bytes_to_slave),
+    Case("Read one byte from slave", test_read_one_byte_from_slave),
+    Case("Destroy & recreate I2C object", test_destroy_recreate_object),
+    Case("Read multiple bytes from slave", test_read_multiple_bytes_from_slave),
+    Case("Read less bytes than expected from slave", test_read_less_bytes_than_expected_from_slave),
 };
 
 Specification specification(test_setup, cases, test_teardown, greentea_continue_handlers);

CI-Shield-Tests/host_test_utils/sigrok_interface.py

@@ -16,6 +16,8 @@
 from typing import List, cast, Optional, Tuple
 from dataclasses import dataclass
 
+from mbed_host_tests.host_tests_logger import HtrunLogger
+
 LOGIC_ANALYZER_FREQUENCY = 4 # MHz
 
 if sys.platform == "win32":
@@ -238,9 +240,39 @@ def pretty_print_i2c_data(data: List[I2CBusData]) -> str:
         if isinstance(bus_data, I2CStop):
             result += " ".join(strings_this_transaction) + "\n"
             strings_this_transaction = []
+
+    # Grab anything after the last stop
+    if len(strings_this_transaction):
+        result += " ".join(strings_this_transaction) + "\n"
 
     return result
 
+def pretty_diff_i2c_data(logger: HtrunLogger, expected: List[I2CBusData], actual: List[I2CDataByte]) -> bool:
+    """
+    Diff expected I2C data against actual.  Always prints the actual data to the console, and prints the expected
+    too if they don't match.
+    """
+
+    if len(actual) > 0:
+        logger.prn_inf("Saw on the I2C bus:\n" + pretty_print_i2c_data(actual))
+    else:
+        logger.prn_inf("Saw nothing the I2C bus.")
+
+    match = True
+    if len(expected) != len(actual):
+        logger.prn_err("Expected length differs from actual!")
+        match = False
+    else:
+        for index, (expected_item, actual_item) in enumerate(zip(expected, actual)):
+            if expected_item != actual_item:
+                logger.prn_err(f"Data item at index {index}: expected {str(expected_item)} but got {str(actual_item)}")
+                match = False
+
+    if not match:
+        logger.prn_inf("We expected:\n" + pretty_print_i2c_data(expected))
+
+    return match
+
 class SigrokI2CRecorder(SigrokRecorderBase):
 
     # i2c sigrok command
@@ -265,11 +297,15 @@ def record(self, record_time: float):
 
     def get_result(self) -> List[I2CBusData]:
         """
-        Get the data that was recorded
-        :return: Data recorded (list of I2CBusData subclasses)
+        Get the data that was recorded.
+        
+        :return: Data recorded (list of I2CBusData subclasses).  If nothing was seen before the timeout (logic analyzer never triggered), returns [].
         """
 
-        sigrok_output = self._get_sigrok_output()
+        try:
+            sigrok_output = self._get_sigrok_output()
+        except subprocess.TimeoutExpired:
+            return []
 
         i2c_transaction: List[I2CBusData] = []
 

CI-Shield-Tests/host_tests/i2c_basic_test.py

@@ -6,12 +6,13 @@
 import sys
 import os
 import pathlib
+import subprocess
 
 # Unfortunately there's no easy way to make the test runner add a directory to its module path...
 this_script_dir = pathlib.Path(os.path.dirname(__file__))
 sys.path.append(str(this_script_dir / ".." / "host_test_utils"))
 
-from sigrok_interface import I2CStart, I2CRepeatedStart, I2CWriteToAddr, I2CReadFromAddr, I2CDataByte, I2CAck, I2CNack, I2CStop, SigrokI2CRecorder, pretty_print_i2c_data
+from sigrok_interface import I2CStart, I2CRepeatedStart, I2CWriteToAddr, I2CReadFromAddr, I2CDataByte, I2CAck, I2CNack, I2CStop, SigrokI2CRecorder, pretty_print_i2c_data, pretty_diff_i2c_data
 
 
 class I2CBasicTestHostTest(BaseHostTest):
@@ -69,30 +70,9 @@ def _callback_verify_sequence(self, key: str, value: str, timestamp):
         """
         Verify that the current recorded I2C data matches the given sequence
         """
-
         recorded_data = self.recorder.get_result()
-        if len(recorded_data) > 0:
-            self.logger.prn_inf("Saw on the I2C bus:\n" + pretty_print_i2c_data(recorded_data))
-        else:
-            self.logger.prn_inf("Saw nothing the I2C bus.")
-
-        fail = False
-        if len(self.SEQUENCES[value]) != len(recorded_data):
-            self.logger.prn_inf("Expected length differs from actual!")
-            fail = True
-        else:
-            for index, (expected, actual) in enumerate(zip(self.SEQUENCES[value], recorded_data)):
-                if expected != actual:
-                    self.logger.prn_inf(f"Data item at index {index}: expected {str(expected)} but got {str(actual)}")
-                    fail = True
-
-        if not fail:
-            self.logger.prn_inf("PASS")
-            self.send_kv('verify_sequence', 'complete')
-        else:
-            self.logger.prn_inf("We expected: \n" + pretty_print_i2c_data(self.SEQUENCES[value]))
-            self.logger.prn_inf("FAIL")
-            self.send_kv('verify_sequence', 'failed')
+
+        self.send_kv('verify_sequence', 'complete' if pretty_diff_i2c_data(self.logger, self.SEQUENCES[value], recorded_data) else 'failed')
 
     def setup(self):