From a93bc954c5cbce71325181ae9a9351ef1e8f07bf Mon Sep 17 00:00:00 2001 From: Adeeb Shihadeh Date: Wed, 17 May 2023 17:13:12 -0700 Subject: [PATCH] SPI HITL tests + cleanup (#1417) * start comms hitl tests * pull that out * revert that * more test * fix warnings * fix linter * another simple case --------- Co-authored-by: Comma Device --- Jenkinsfile | 2 +- board/drivers/spi.h | 75 +++++++++++++++++++++++++------------------ board/stm32fx/llspi.h | 8 +++-- board/stm32h7/llspi.h | 4 +-- python/spi.py | 13 +++++--- requirements.txt | 1 + tests/hitl/8_spi.py | 58 +++++++++++++++++++++++++++++++++ 7 files changed, 121 insertions(+), 40 deletions(-) create mode 100644 tests/hitl/8_spi.py diff --git a/Jenkinsfile b/Jenkinsfile index 9614d67f..51684cea 100644 --- a/Jenkinsfile +++ b/Jenkinsfile @@ -94,7 +94,7 @@ pipeline { phone_steps("panda-tres", [ ["build", "scons -j4"], ["flash", "cd tests/ && ./ci_reset_internal_hw.py"], - ["test", "cd tests/hitl && HW_TYPES=9 pytest --durations=0 2*.py [5-7]*.py -k 'not test_fan_controller and not test_fan_overshoot'"], + ["test", "cd tests/hitl && HW_TYPES=9 pytest --durations=0 2*.py [5-9]*.py -k 'not test_fan_controller and not test_fan_overshoot'"], ]) } } diff --git a/board/drivers/spi.h b/board/drivers/spi.h index 6a52cb4e..f78bd0b5 100644 --- a/board/drivers/spi.h +++ b/board/drivers/spi.h @@ -64,8 +64,9 @@ bool check_checksum(uint8_t *data, uint16_t len) { return checksum == 0U; } -void spi_handle_rx(void) { - uint8_t next_rx_state = SPI_STATE_HEADER; +void spi_rx_done(void) { + uint16_t response_len = 0U; + uint8_t next_rx_state = SPI_STATE_HEADER_NACK; bool checksum_valid = false; // parse header @@ -79,17 +80,17 @@ void spi_handle_rx(void) { // response: ACK and start receiving data portion spi_buf_tx[0] = SPI_HACK; next_rx_state = SPI_STATE_HEADER_ACK; + response_len = 1U; } else { // response: NACK and reset state machine print("- incorrect header sync or checksum "); hexdump(spi_buf_rx, SPI_HEADER_SIZE); spi_buf_tx[0] = SPI_NACK; next_rx_state = SPI_STATE_HEADER_NACK; + response_len = 1U; } - llspi_miso_dma(spi_buf_tx, 1); } else if (spi_state == SPI_STATE_DATA_RX) { // We got everything! Based on the endpoint specified, call the appropriate handler - uint16_t response_len = 0U; - bool reponse_ack = false; + bool response_ack = false; checksum_valid = check_checksum(&(spi_buf_rx[SPI_HEADER_SIZE]), spi_data_len_mosi + 1U); if (checksum_valid) { if (spi_endpoint == 0U) { @@ -97,24 +98,24 @@ void spi_handle_rx(void) { ControlPacket_t ctrl; (void)memcpy(&ctrl, &spi_buf_rx[SPI_HEADER_SIZE], sizeof(ControlPacket_t)); response_len = comms_control_handler(&ctrl, &spi_buf_tx[3]); - reponse_ack = true; + response_ack = true; } else { print("SPI: insufficient data for control handler\n"); } } else if ((spi_endpoint == 1U) || (spi_endpoint == 0x81U)) { if (spi_data_len_mosi == 0U) { response_len = comms_can_read(&(spi_buf_tx[3]), spi_data_len_miso); - reponse_ack = true; + response_ack = true; } else { print("SPI: did not expect data for can_read\n"); } } else if (spi_endpoint == 2U) { comms_endpoint2_write(&spi_buf_rx[SPI_HEADER_SIZE], spi_data_len_mosi); - reponse_ack = true; + response_ack = true; } else if (spi_endpoint == 3U) { if (spi_data_len_mosi > 0U) { comms_can_write(&spi_buf_rx[SPI_HEADER_SIZE], spi_data_len_mosi); - reponse_ack = true; + response_ack = true; } else { print("SPI: did expect data for can_write\n"); } @@ -123,42 +124,54 @@ void spi_handle_rx(void) { } } else { // Checksum was incorrect - reponse_ack = false; - print("- incorrect data checksum\n"); + response_ack = false; + print("- incorrect data checksum "); + puth2(spi_data_len_mosi); + print(" "); + hexdump(&(spi_buf_rx[SPI_HEADER_SIZE]), MIN(spi_data_len_mosi, 64)); } - // Setup response header - spi_buf_tx[0] = reponse_ack ? SPI_DACK : SPI_NACK; - spi_buf_tx[1] = response_len & 0xFFU; - spi_buf_tx[2] = (response_len >> 8) & 0xFFU; + if (!response_ack) { + spi_buf_tx[0] = SPI_NACK; + next_rx_state = SPI_STATE_HEADER_NACK; + response_len = 1U; + } else { + // Setup response header + spi_buf_tx[0] = SPI_DACK; + spi_buf_tx[1] = response_len & 0xFFU; + spi_buf_tx[2] = (response_len >> 8) & 0xFFU; - // Add checksum - uint8_t checksum = SPI_CHECKSUM_START; - for(uint16_t i = 0U; i < (response_len + 3U); i++) { - checksum ^= spi_buf_tx[i]; + // Add checksum + uint8_t checksum = SPI_CHECKSUM_START; + for(uint16_t i = 0U; i < (response_len + 3U); i++) { + checksum ^= spi_buf_tx[i]; + } + spi_buf_tx[response_len + 3U] = checksum; + response_len += 4U; + + next_rx_state = SPI_STATE_DATA_TX; } - spi_buf_tx[response_len + 3U] = checksum; - - // Write response - llspi_miso_dma(spi_buf_tx, response_len + 4U); - - next_rx_state = SPI_STATE_DATA_TX; } else { print("SPI: RX unexpected state: "); puth(spi_state); print("\n"); } + // send out response + if (response_len == 0U) { + print("SPI: no response\n"); + spi_buf_tx[0] = SPI_NACK; + spi_state = SPI_STATE_HEADER_NACK; + response_len = 1U; + } + llspi_miso_dma(spi_buf_tx, response_len); + spi_state = next_rx_state; if (!checksum_valid && (spi_checksum_error_count < __UINT16_MAX__)) { spi_checksum_error_count += 1U; } } -void spi_handle_tx(bool timed_out) { - if (timed_out) { - print("SPI: TX timeout\n"); - } - - if ((spi_state == SPI_STATE_HEADER_NACK) || timed_out) { +void spi_tx_done(bool reset) { + if ((spi_state == SPI_STATE_HEADER_NACK) || reset) { // Reset state spi_state = SPI_STATE_HEADER; llspi_mosi_dma(spi_buf_rx, SPI_HEADER_SIZE); diff --git a/board/stm32fx/llspi.h b/board/stm32fx/llspi.h index 3aa7236b..2ee7ffa3 100644 --- a/board/stm32fx/llspi.h +++ b/board/stm32fx/llspi.h @@ -36,7 +36,7 @@ void DMA2_Stream2_IRQ_Handler(void) { ENTER_CRITICAL(); DMA2->LIFCR = DMA_LIFCR_CTCIF2; - spi_handle_rx(); + spi_rx_done(); EXIT_CRITICAL(); } @@ -60,7 +60,11 @@ void DMA2_Stream3_IRQ_Handler(void) { (void)dat; SPI1->DR = 0U; - spi_handle_tx(timed_out); + if (timed_out) { + print("SPI: TX timeout\n"); + } + + spi_tx_done(timed_out); } // ***************************** SPI init ***************************** diff --git a/board/stm32h7/llspi.h b/board/stm32h7/llspi.h index 1e259597..8d640bed 100644 --- a/board/stm32h7/llspi.h +++ b/board/stm32h7/llspi.h @@ -48,7 +48,7 @@ void DMA2_Stream2_IRQ_Handler(void) { ENTER_CRITICAL(); DMA2->LIFCR = DMA_LIFCR_CTCIF2; - spi_handle_rx(); + spi_rx_done(); EXIT_CRITICAL(); } @@ -77,7 +77,7 @@ void SPI4_IRQ_Handler(void) { volatile uint8_t dat = SPI4->TXDR; (void)dat; - spi_handle_tx(false); + spi_tx_done(false); } EXIT_CRITICAL(); diff --git a/python/spi.py b/python/spi.py index c753bf4d..e427bea8 100644 --- a/python/spi.py +++ b/python/spi.py @@ -58,7 +58,14 @@ class SpiDevice: """ Provides locked, thread-safe access to a panda's SPI interface. """ - def __init__(self, speed): + + # 50MHz is the max of the 845. older rev comma three + # may not support the full 50MHz + MAX_SPEED = 50000000 + + def __init__(self, speed=MAX_SPEED): + assert speed <= self.MAX_SPEED + if not os.path.exists(DEV_PATH): raise PandaSpiUnavailable(f"SPI device not found: {DEV_PATH}") if spidev is None: @@ -87,9 +94,7 @@ class PandaSpiHandle(BaseHandle): A class that mimics a libusb1 handle for panda SPI communications. """ def __init__(self): - # 50MHz is the max of the 845. older rev comma three - # may not support the full 50MHz - self.dev = SpiDevice(50000000) + self.dev = SpiDevice() # helpers def _calc_checksum(self, data: List[int]) -> int: diff --git a/requirements.txt b/requirements.txt index 6b063e91..1cb0faf3 100644 --- a/requirements.txt +++ b/requirements.txt @@ -14,3 +14,4 @@ pylint==2.15.4 scons==4.4.0 flaky spidev +pytest-mock diff --git a/tests/hitl/8_spi.py b/tests/hitl/8_spi.py new file mode 100644 index 00000000..da3e5641 --- /dev/null +++ b/tests/hitl/8_spi.py @@ -0,0 +1,58 @@ +import pytest +import random +from unittest.mock import patch + +from panda import Panda +from panda.python.spi import PandaSpiNackResponse + +pytestmark = [ + pytest.mark.test_panda_types((Panda.HW_TYPE_TRES, )) +] + +class TestSpi: + def _ping(self, mocker, panda): + # should work with no retries + spy = mocker.spy(panda._handle, '_wait_for_ack') + panda.health() + assert spy.call_count == 2 + mocker.stop(spy) + + def test_all_comm_types(self, mocker, p): + spy = mocker.spy(p._handle, '_wait_for_ack') + + # controlRead + controlWrite + p.health() + p.can_clear(0) + assert spy.call_count == 2*2 + + # bulkRead + bulkWrite + p.can_recv() + p.can_send(0x123, b"somedata", 0) + assert spy.call_count == 2*4 + + def test_bad_header(self, mocker, p): + with patch('panda.python.spi.SYNC', return_value=0): + with pytest.raises(PandaSpiNackResponse): + p.health() + self._ping(mocker, p) + + def test_bad_checksum(self, mocker, p): + cnt = p.health()['spi_checksum_error_count'] + with patch('panda.python.spi.PandaSpiHandle._calc_checksum', return_value=0): + with pytest.raises(PandaSpiNackResponse): + p.health() + self._ping(mocker, p) + assert (p.health()['spi_checksum_error_count'] - cnt) > 0 + + def test_non_existent_endpoint(self, mocker, p): + for _ in range(10): + ep = random.randint(4, 20) + with pytest.raises(PandaSpiNackResponse): + p._handle.bulkRead(ep, random.randint(1, 1000)) + + self._ping(mocker, p) + + with pytest.raises(PandaSpiNackResponse): + p._handle.bulkWrite(ep, b"abc") + + self._ping(mocker, p)