From 52f96bac685b129c5b90e274fe39386e76da512e Mon Sep 17 00:00:00 2001 From: Adeeb Shihadeh Date: Fri, 19 May 2023 22:43:34 -0700 Subject: [PATCH] spi: nack on can tx endpoint if buffer is full (#1426) * spi: nack on can tx endpoint if buffer is full * handle in python lib * fix timeout * fix timeout * fix linter * cleanup * fix --------- Co-authored-by: Comma Device --- board/can_comms.h | 9 ++++++--- board/config.h | 3 ++- board/drivers/bxcan.h | 2 +- board/drivers/can_common.h | 2 +- board/drivers/fdcan.h | 2 +- board/drivers/spi.h | 18 ++++++++++++++---- board/drivers/usb.h | 7 +++---- board/flasher.h | 2 +- board/pedal/main.c | 2 +- python/__init__.py | 8 +++++--- python/spi.py | 13 +++++++++---- tests/hitl/8_spi.py | 8 ++++---- tests/libpanda/panda.c | 5 +++-- 13 files changed, 51 insertions(+), 30 deletions(-) diff --git a/board/can_comms.h b/board/can_comms.h index 02bc0aef3..93c32c71d 100644 --- a/board/can_comms.h +++ b/board/can_comms.h @@ -110,8 +110,11 @@ void comms_can_reset(void) { } // TODO: make this more general! -void usb_cb_ep3_out_complete(void) { - if (can_tx_check_min_slots_free(MAX_CAN_MSGS_PER_BULK_TRANSFER)) { - usb_outep3_resume_if_paused(); +void refresh_can_tx_slots_available(void) { + if (can_tx_check_min_slots_free(MAX_CAN_MSGS_PER_USB_BULK_TRANSFER)) { + can_tx_comms_resume_usb(); + } + if (can_tx_check_min_slots_free(MAX_CAN_MSGS_PER_SPI_BULK_TRANSFER)) { + can_tx_comms_resume_spi(); } } diff --git a/board/config.h b/board/config.h index 12b514007..903161876 100644 --- a/board/config.h +++ b/board/config.h @@ -13,7 +13,8 @@ #define CAN_INIT_TIMEOUT_MS 500U #define DEEPSLEEP_WAKEUP_DELAY 3U #define USBPACKET_MAX_SIZE 0x40U -#define MAX_CAN_MSGS_PER_BULK_TRANSFER 51U +#define MAX_CAN_MSGS_PER_USB_BULK_TRANSFER 51U +#define MAX_CAN_MSGS_PER_SPI_BULK_TRANSFER 170U #define VIN_READOUT_DIVIDER 11U diff --git a/board/drivers/bxcan.h b/board/drivers/bxcan.h index 815ef0fe1..15e34d038 100644 --- a/board/drivers/bxcan.h +++ b/board/drivers/bxcan.h @@ -151,7 +151,7 @@ void process_can(uint8_t can_number) { can_health[can_number].total_tx_checksum_error_cnt += 1U; } - usb_cb_ep3_out_complete(); + refresh_can_tx_slots_available(); } } diff --git a/board/drivers/can_common.h b/board/drivers/can_common.h index 3f2f0830a..be6b5b2a5 100644 --- a/board/drivers/can_common.h +++ b/board/drivers/can_common.h @@ -147,7 +147,7 @@ void can_clear(can_ring *q) { q->r_ptr = 0; EXIT_CRITICAL(); // handle TX buffer full with zero ECUs awake on the bus - usb_cb_ep3_out_complete(); + refresh_can_tx_slots_available(); } // assign CAN numbering diff --git a/board/drivers/fdcan.h b/board/drivers/fdcan.h index 4dcc3fdf4..a5b97ba13 100644 --- a/board/drivers/fdcan.h +++ b/board/drivers/fdcan.h @@ -119,7 +119,7 @@ void process_can(uint8_t can_number) { can_health[can_number].total_tx_checksum_error_cnt += 1U; } - usb_cb_ep3_out_complete(); + refresh_can_tx_slots_available(); } } diff --git a/board/drivers/spi.h b/board/drivers/spi.h index a18597498..ed88f6ea8 100644 --- a/board/drivers/spi.h +++ b/board/drivers/spi.h @@ -37,7 +37,7 @@ uint8_t spi_endpoint; uint16_t spi_data_len_mosi; uint16_t spi_data_len_miso; uint16_t spi_checksum_error_count = 0; - +bool spi_can_tx_ready = false; #define SPI_HEADER_SIZE 7U @@ -46,6 +46,10 @@ void llspi_init(void); void llspi_mosi_dma(uint8_t *addr, int len); void llspi_miso_dma(uint8_t *addr, int len); +void can_tx_comms_resume_spi(void) { + spi_can_tx_ready = true; +} + void spi_init(void) { // platform init llspi_init(); @@ -114,8 +118,14 @@ void spi_rx_done(void) { 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); - response_ack = true; + if (spi_can_tx_ready) { + spi_can_tx_ready = false; + comms_can_write(&spi_buf_rx[SPI_HEADER_SIZE], spi_data_len_mosi); + response_ack = true; + } else { + response_ack = false; + print("SPI: CAN NACK\n"); + } } else { print("SPI: did expect data for can_write\n"); } @@ -126,7 +136,7 @@ void spi_rx_done(void) { // Checksum was incorrect response_ack = false; print("- incorrect data checksum "); - puth2(spi_data_len_mosi); + puth4(spi_data_len_mosi); print("\n"); hexdump(spi_buf_rx, SPI_HEADER_SIZE); hexdump(&(spi_buf_rx[SPI_HEADER_SIZE]), MIN(spi_data_len_mosi, 64)); diff --git a/board/drivers/usb.h b/board/drivers/usb.h index 469299d74..d10bed15e 100644 --- a/board/drivers/usb.h +++ b/board/drivers/usb.h @@ -27,8 +27,7 @@ bool usb_enumerated = false; uint16_t usb_last_frame_num = 0U; void usb_init(void); -void usb_cb_ep3_out_complete(void); -void usb_outep3_resume_if_paused(void); +void refresh_can_tx_slots_available(void); // **** supporting defines **** @@ -808,7 +807,7 @@ void usb_irqhandler(void) { #endif // NAK cleared by process_can (if tx buffers have room) outep3_processing = false; - usb_cb_ep3_out_complete(); + refresh_can_tx_slots_available(); } else if ((USBx_OUTEP(3)->DOEPINT & 0x2000) != 0) { #ifdef DEBUG_USB print(" OUT3 PACKET WTF\n"); @@ -926,7 +925,7 @@ void usb_irqhandler(void) { //USBx->GINTMSK = 0xFFFFFFFF & ~(USB_OTG_GINTMSK_NPTXFEM | USB_OTG_GINTMSK_PTXFEM | USB_OTG_GINTSTS_SOF | USB_OTG_GINTSTS_EOPF); } -void usb_outep3_resume_if_paused(void) { +void can_tx_comms_resume_usb(void) { ENTER_CRITICAL(); if (!outep3_processing && (USBx_OUTEP(3)->DOEPCTL & USB_OTG_DOEPCTL_NAKSTS) != 0) { USBx_OUTEP(3)->DOEPTSIZ = (32U << 19) | 0x800U; diff --git a/board/flasher.h b/board/flasher.h index 97d81096e..bcd98daf0 100644 --- a/board/flasher.h +++ b/board/flasher.h @@ -110,7 +110,7 @@ int comms_can_read(uint8_t *data, uint32_t max_len) { return 0; } -void usb_cb_ep3_out_complete(void) {} +void refresh_can_tx_slots_available(void) {} void comms_endpoint2_write(uint8_t *data, uint32_t len) { current_board->set_led(LED_RED, 0); diff --git a/board/pedal/main.c b/board/pedal/main.c index e6b197cf7..eb4046610 100644 --- a/board/pedal/main.c +++ b/board/pedal/main.c @@ -54,7 +54,7 @@ void comms_endpoint2_write(uint8_t *data, uint32_t len) { UNUSED(data); UNUSED(len); } -void usb_cb_ep3_out_complete(void) {} +void refresh_can_tx_slots_available(void) {} int comms_control_handler(ControlPacket_t *req, uint8_t *resp) { unsigned int resp_len = 0; diff --git a/python/__init__.py b/python/__init__.py index 0849f0a75..7425ff1f9 100644 --- a/python/__init__.py +++ b/python/__init__.py @@ -401,14 +401,16 @@ class Panda: return [] def reset(self, enter_bootstub=False, enter_bootloader=False, reconnect=True): + # no response is expected since it resets right away + timeout = 5000 if isinstance(self._handle, PandaSpiHandle) else 15000 try: if enter_bootloader: - self._handle.controlWrite(Panda.REQUEST_IN, 0xd1, 0, 0, b'') + self._handle.controlWrite(Panda.REQUEST_IN, 0xd1, 0, 0, b'', timeout=timeout) else: if enter_bootstub: - self._handle.controlWrite(Panda.REQUEST_IN, 0xd1, 1, 0, b'') + self._handle.controlWrite(Panda.REQUEST_IN, 0xd1, 1, 0, b'', timeout=timeout) else: - self._handle.controlWrite(Panda.REQUEST_IN, 0xd8, 0, 0, b'') + self._handle.controlWrite(Panda.REQUEST_IN, 0xd8, 0, 0, b'', timeout=timeout) except Exception: pass if not enter_bootloader and reconnect: diff --git a/python/spi.py b/python/spi.py index e427bea86..8a0fe5cc9 100644 --- a/python/spi.py +++ b/python/spi.py @@ -120,8 +120,11 @@ class PandaSpiHandle(BaseHandle): logging.debug("starting transfer: endpoint=%d, max_rx_len=%d", endpoint, max_rx_len) logging.debug("==============================================") + n = 0 + start_time = time.monotonic() exc = PandaSpiException() - for n in range(MAX_XFER_RETRY_COUNT): + while (time.monotonic() - start_time) < timeout*1e-3: + n += 1 logging.debug("\ntry #%d", n+1) try: logging.debug("- send header") @@ -129,16 +132,18 @@ class PandaSpiHandle(BaseHandle): packet += bytes([reduce(lambda x, y: x^y, packet) ^ CHECKSUM_START]) spi.xfer2(packet) + to = timeout - (time.monotonic() - start_time)*1e3 logging.debug("- waiting for header ACK") - self._wait_for_ack(spi, HACK, timeout, 0x11) + self._wait_for_ack(spi, HACK, int(to), 0x11) # send data logging.debug("- sending data") packet = bytes([*data, self._calc_checksum(data)]) spi.xfer2(packet) + to = timeout - (time.monotonic() - start_time)*1e3 logging.debug("- waiting for data ACK") - self._wait_for_ack(spi, DACK, timeout, 0x13) + self._wait_for_ack(spi, DACK, int(to), 0x13) # get response length, then response response_len_bytes = bytes(spi.xfer2(b"\x00" * 2)) @@ -154,7 +159,7 @@ class PandaSpiHandle(BaseHandle): return dat[:-1] except PandaSpiException as e: exc = e - logging.debug("SPI transfer failed, %d retries left", MAX_XFER_RETRY_COUNT - n - 1, exc_info=True) + logging.debug("SPI transfer failed, retrying", exc_info=True) raise exc # libusb1 functions diff --git a/tests/hitl/8_spi.py b/tests/hitl/8_spi.py index da3e5641e..3d8a33b0b 100644 --- a/tests/hitl/8_spi.py +++ b/tests/hitl/8_spi.py @@ -33,14 +33,14 @@ class TestSpi: def test_bad_header(self, mocker, p): with patch('panda.python.spi.SYNC', return_value=0): with pytest.raises(PandaSpiNackResponse): - p.health() + p._handle.controlRead(Panda.REQUEST_IN, 0xd2, 0, 0, p.HEALTH_STRUCT.size, timeout=50) 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() + p._handle.controlRead(Panda.REQUEST_IN, 0xd2, 0, 0, p.HEALTH_STRUCT.size, timeout=50) self._ping(mocker, p) assert (p.health()['spi_checksum_error_count'] - cnt) > 0 @@ -48,11 +48,11 @@ class TestSpi: for _ in range(10): ep = random.randint(4, 20) with pytest.raises(PandaSpiNackResponse): - p._handle.bulkRead(ep, random.randint(1, 1000)) + p._handle.bulkRead(ep, random.randint(1, 1000), timeout=50) self._ping(mocker, p) with pytest.raises(PandaSpiNackResponse): - p._handle.bulkWrite(ep, b"abc") + p._handle.bulkWrite(ep, b"abc", timeout=50) self._ping(mocker, p) diff --git a/tests/libpanda/panda.c b/tests/libpanda/panda.c index 7f1ecdb13..8efb6de4d 100644 --- a/tests/libpanda/panda.c +++ b/tests/libpanda/panda.c @@ -8,8 +8,9 @@ void process_can(uint8_t can_number) { } //int safety_tx_hook(CANPacket_t *to_send) { return 1; } typedef struct harness_configuration harness_configuration; -void usb_cb_ep3_out_complete(void); -void usb_outep3_resume_if_paused(void) { }; +void refresh_can_tx_slots_available(void); +void can_tx_comms_resume_usb(void) { }; +void can_tx_comms_resume_spi(void) { }; #include "health.h" #include "faults.h"