From cbde47fe7985e49cc94e7fb2c408b7327d6c15a8 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Wed, 14 Feb 2024 10:37:43 +0100 Subject: [PATCH] fix(eppp_link): SPI transport refactored --- components/eppp_link/eppp_link.c | 290 ++++++++++-------- .../eppp_link/examples/host/main/app_main.c | 6 +- 2 files changed, 167 insertions(+), 129 deletions(-) diff --git a/components/eppp_link/eppp_link.c b/components/eppp_link/eppp_link.c index 44fe3089cf9..1d3e89f8dc0 100644 --- a/components/eppp_link/eppp_link.c +++ b/components/eppp_link/eppp_link.c @@ -32,6 +32,34 @@ static const char *TAG = "eppp_link"; static int s_retry_num = 0; static int s_eppp_netif_count = 0; // used as a suffix for the netif key + +struct packet { + size_t len; + uint8_t *data; +}; + +#if CONFIG_EPPP_LINK_DEVICE_SPI +#define MAX_PAYLOAD 1500 +#define MIN_TRIGGER_US 20 +#define SPI_HEADER_MAGIC 0x1234 + +struct header { + uint16_t magic; + uint16_t size; + uint16_t next_size; + uint16_t check; +} __attribute__((packed)); + +enum blocked_status { + NONE, + MASTER_BLOCKED, + MASTER_WANTS_READ, + SLAVE_BLOCKED, + SLAVE_WANTS_WRITE, +}; + +#endif // CONFIG_EPPP_LINK_DEVICE_SPI + struct eppp_handle { #if CONFIG_EPPP_LINK_DEVICE_SPI QueueHandle_t out_queue; @@ -39,6 +67,12 @@ struct eppp_handle { spi_device_handle_t spi_device; spi_host_device_t spi_host; int gpio_intr; + uint16_t next_size; + uint16_t transaction_size; + struct packet outbound; + enum blocked_status blocked; + uint32_t slave_last_edge; + esp_timer_handle_t timer; #elif CONFIG_EPPP_LINK_DEVICE_UART QueueHandle_t uart_event_queue; uart_port_t uart_port; @@ -50,21 +84,16 @@ struct eppp_handle { bool netif_stop; }; -struct packet { - size_t len; - uint8_t *data; -}; + static esp_err_t transmit(void *h, void *buffer, size_t len) { struct eppp_handle *handle = h; #if CONFIG_EPPP_LINK_DEVICE_SPI -#define MAX_PAYLOAD 1600 struct packet buf = { }; - uint8_t *current_buffer = buffer; size_t remaining = len; - do { // TODO: Refactor this loop to allocate only once and perform + do { // TODO(IDF-9194): Refactor this loop to allocate only once and perform // fragmentation after receiving from the queue (applicable only if MTU > MAX_PAYLOAD) size_t batch = remaining > MAX_PAYLOAD ? MAX_PAYLOAD : remaining; buf.data = malloc(batch); @@ -82,6 +111,16 @@ static esp_err_t transmit(void *h, void *buffer, size_t len) return ESP_ERR_NO_MEM; } } while (remaining > 0); + + if (handle->role == EPPP_SERVER && handle->blocked == SLAVE_BLOCKED) { + uint32_t now = esp_timer_get_time(); + uint32_t diff = now - handle->slave_last_edge; + if (diff < MIN_TRIGGER_US) { + esp_rom_delay_us(MIN_TRIGGER_US - diff); + } + gpio_set_level(handle->gpio_intr, 0); + } + #elif CONFIG_EPPP_LINK_DEVICE_UART ESP_LOG_BUFFER_HEXDUMP("ppp_uart_send", buffer, len, ESP_LOG_VERBOSE); uart_write_bytes(handle->uart_port, buffer, len); @@ -89,6 +128,14 @@ static esp_err_t transmit(void *h, void *buffer, size_t len) return ESP_OK; } +static void IRAM_ATTR timer_callback(void *arg) +{ + struct eppp_handle *h = arg; + if (h->blocked == SLAVE_WANTS_WRITE) { + gpio_set_level(h->gpio_intr, 0); + } +} + static void netif_deinit(esp_netif_t *netif) { if (netif == NULL) { @@ -119,7 +166,7 @@ static void netif_deinit(esp_netif_t *netif) static esp_netif_t *netif_init(eppp_type_t role) { - if (s_eppp_netif_count > 9) { + if (s_eppp_netif_count > 9) { // Limit to max 10 netifs, since we use "EPPPx" as the unique key (where x is 0-9) ESP_LOGE(TAG, "Cannot create more than 10 instances"); return NULL; } @@ -147,6 +194,24 @@ static esp_netif_t *netif_init(eppp_type_t role) return NULL; } } + h->transaction_size = 0; + h->outbound.data = NULL; + h->outbound.len = 0; + if (role == EPPP_SERVER) { + esp_timer_create_args_t args = { + .callback = &timer_callback, + .arg = h, + .name = "timer" + }; + if (esp_timer_create(&args, &h->timer) != ESP_OK) { + ESP_LOGE(TAG, "Failed to create the packet queue"); + vQueueDelete(h->out_queue); + vSemaphoreDelete(h->ready_semaphore); + free(h); + return NULL; + } + } + #endif esp_netif_driver_ifconfig_t driver_cfg = { @@ -266,41 +331,37 @@ static void on_ip_event(void *arg, esp_event_base_t base, int32_t event_id, void #if CONFIG_EPPP_LINK_DEVICE_SPI #define SPI_ALIGN(size) (((size) + 3U) & ~(3U)) - -#define TRANSFER_SIZE SPI_ALIGN((MAX_PAYLOAD + 4)) -#define SHORT_PAYLOAD (0) -#define CONTROL_SIZE (SHORT_PAYLOAD + 4) - -#define CONTROL_MASTER 0xA5 -#define CONTROL_MASTER_WITH_DATA 0xA6 -#define CONTROL_SLAVE 0x5A -#define CONTROL_SLAVE_WITH_DATA 0x5B -#define DATA_MASTER 0xAF -#define DATA_SLAVE 0xFA - +#define TRANSFER_SIZE SPI_ALIGN((MAX_PAYLOAD + 6)) #define MAX(a,b) (((a)>(b))?(a):(b)) -struct header { - uint16_t size; - uint8_t magic; - uint8_t crc; -} __attribute__((packed)); - static void IRAM_ATTR gpio_isr_handler(void *arg) { static uint32_t s_last_time; uint32_t now = esp_timer_get_time(); uint32_t diff = now - s_last_time; - if (diff < 20) { // debounce + if (diff < MIN_TRIGGER_US) { // debounce return; } s_last_time = now; - - BaseType_t yield = false; struct eppp_handle *h = arg; - xSemaphoreGiveFromISR(h->ready_semaphore, &yield); - if (yield) { - portYIELD_FROM_ISR(); + BaseType_t yield = false; + + // Positive edge means SPI slave prepared the data + if (gpio_get_level(h->gpio_intr) == 1) { + xSemaphoreGiveFromISR(h->ready_semaphore, &yield); + if (yield) { + portYIELD_FROM_ISR(); + } + return; + } + + // Negative edge (when master blocked) means that slave wants to transmit + if (h->blocked == MASTER_BLOCKED) { + struct packet buf = { .data = NULL, .len = -1 }; + xQueueSendFromISR(h->out_queue, &buf, &yield); + if (yield) { + portYIELD_FROM_ISR(); + } } } @@ -350,7 +411,7 @@ static esp_err_t init_master(struct eppp_config_spi_s *config, esp_netif_t *neti //GPIO config for the handshake line. gpio_config_t io_conf = { - .intr_type = GPIO_INTR_POSEDGE, + .intr_type = GPIO_INTR_ANYEDGE, .mode = GPIO_MODE_INPUT, .pull_up_en = 1, .pin_bit_mask = BIT64(config->intr), @@ -358,19 +419,31 @@ static esp_err_t init_master(struct eppp_config_spi_s *config, esp_netif_t *neti gpio_config(&io_conf); gpio_install_isr_service(0); - gpio_set_intr_type(config->intr, GPIO_INTR_POSEDGE); + gpio_set_intr_type(config->intr, GPIO_INTR_ANYEDGE); gpio_isr_handler_add(config->intr, gpio_isr_handler, esp_netif_get_io_driver(netif)); return ESP_OK; } static void post_setup(spi_slave_transaction_t *trans) { - gpio_set_level((int)trans->user, 1); + struct eppp_handle *h = trans->user; + h->slave_last_edge = esp_timer_get_time(); + gpio_set_level(h->gpio_intr, 1); + if (h->transaction_size == 0) { // If no transaction planned: + if (h->outbound.len == 0) { // we're blocked if we don't have any data + h->blocked = SLAVE_BLOCKED; + } else { + h->blocked = SLAVE_WANTS_WRITE; // we notify the master that we want to write + esp_timer_start_once(h->timer, MIN_TRIGGER_US); + } + } } static void post_trans(spi_slave_transaction_t *trans) { - gpio_set_level((int)trans->user, 0); + struct eppp_handle *h = trans->user; + h->blocked = NONE; + gpio_set_level(h->gpio_intr, 0); } static esp_err_t deinit_slave(esp_netif_t *netif) @@ -433,15 +506,13 @@ static esp_err_t perform_transaction_master(struct eppp_handle *h, size_t len, c t.length = len * 8; t.tx_buffer = tx_buffer; t.rx_buffer = rx_buffer; - - xSemaphoreTake(h->ready_semaphore, portMAX_DELAY); // Wait until slave is ready return spi_device_transmit(h->spi_device, &t); } static esp_err_t perform_transaction_slave(struct eppp_handle *h, size_t len, const void *tx_buffer, void *rx_buffer) { spi_slave_transaction_t t = {}; - t.user = (void *)(h->gpio_intr); + t.user = h; t.length = len * 8; t.tx_buffer = tx_buffer; t.rx_buffer = rx_buffer; @@ -455,107 +526,79 @@ esp_err_t eppp_perform(esp_netif_t *netif) struct eppp_handle *h = esp_netif_get_io_driver(netif); - // Three types of frames (control, control+data, data), two roles (master, slave) - const uint8_t FRAME_OUT_CTRL = h->role == EPPP_CLIENT ? CONTROL_MASTER : CONTROL_SLAVE; - const uint8_t FRAME_OUT_CTRL_EX = h->role == EPPP_CLIENT ? CONTROL_MASTER_WITH_DATA : CONTROL_SLAVE_WITH_DATA; - const uint8_t FRAME_OUT_DATA = h->role == EPPP_CLIENT ? DATA_MASTER : DATA_SLAVE; - const uint8_t FRAME_IN_CTRL = h->role == EPPP_SERVER ? CONTROL_MASTER : CONTROL_SLAVE; - const uint8_t FRAME_IN_CTRL_EX = h->role == EPPP_SERVER ? CONTROL_MASTER_WITH_DATA : CONTROL_SLAVE_WITH_DATA; - const uint8_t FRAME_IN_DATA = h->role == EPPP_SERVER ? DATA_MASTER : DATA_SLAVE; - // Perform transaction for these two roles (master, slave) + // Perform transaction for master and slave const perform_transaction_t perform_transaction = h->role == EPPP_CLIENT ? perform_transaction_master : perform_transaction_slave; if (h->stop) { return ESP_ERR_TIMEOUT; } - struct packet buf = { .len = 0 }; - struct header *head = (void *)out_buf; - bool need_data_frame = false; - size_t out_long_payload = 0; - head->magic = FRAME_OUT_CTRL_EX; - head->size = 0; - head->crc = 0; - BaseType_t tx_queue_stat = xQueueReceive(h->out_queue, &buf, 0); // h->role == EPPP_CLIENT ? pdMS_TO_TICKS(10): 0); - if (tx_queue_stat == pdTRUE && buf.data) { - if (buf.len > SHORT_PAYLOAD) { - head->magic = FRAME_OUT_CTRL; - printf("O"); - head->size = buf.len; - out_long_payload = buf.len; - need_data_frame = true; - } else { - head->magic = FRAME_OUT_CTRL_EX; - printf("o"); - head->size = buf.len; - memcpy(out_buf + sizeof(struct header), buf.data, buf.len); - free(buf.data); + BaseType_t tx_queue_stat; + bool allow_test_tx = false; + uint16_t next_tx_size = 0; + if (h->role == EPPP_CLIENT) { + // SPI MASTER only code + if (xSemaphoreTake(h->ready_semaphore, pdMS_TO_TICKS(1000)) != pdTRUE) { + // slave might not be ready, but maybe we just missed an interrupt + allow_test_tx = true; + } + if (h->outbound.len == 0 && h->transaction_size == 0 && h->blocked == NONE) { + h->blocked = MASTER_BLOCKED; + xQueueReceive(h->out_queue, &h->outbound, portMAX_DELAY); + h->blocked = NONE; + if (h->outbound.len == -1) { + h->outbound.len = 0; + h->blocked = MASTER_WANTS_READ; + } + } else if (h->blocked == MASTER_WANTS_READ) { + h->blocked = NONE; } } - head->crc = esp_rom_crc8_le(0, out_buf, sizeof(struct header) - 1); - printf("+"); - esp_err_t ret = perform_transaction(h, CONTROL_SIZE, out_buf, in_buf); - if (ret != ESP_OK) { - ESP_LOGE(TAG, "spi_device_transmit failed"); - return ESP_FAIL; - } - head = (void *)in_buf; - uint8_t crc = esp_rom_crc8_le(0, in_buf, sizeof(struct header) - 1); - if (crc != head->crc) { - ESP_LOGE(TAG, "Wrong checksum"); - return ESP_FAIL; - } - if (head->magic != FRAME_IN_CTRL && head->magic != FRAME_IN_CTRL_EX) { - ESP_LOGE(TAG, "Wrong magic"); - return ESP_FAIL; - } - if (head->magic == FRAME_IN_CTRL_EX && head->size > 0) { - printf("i"); - esp_netif_receive(netif, in_buf + sizeof(struct header), head->size, NULL); - } - size_t in_long_payload = 0; - if (head->magic == FRAME_IN_CTRL) { - printf("I"); - need_data_frame = true; - in_long_payload = head->size; - } - if (!need_data_frame) { - return ESP_OK; - } - - // now, we need data frame - head = (void *)out_buf; - head->magic = FRAME_OUT_DATA; - head->size = out_long_payload; -// head->crc = 0; - head->crc = esp_rom_crc8_le(0, out_buf, sizeof(struct header) - 1); - if (head->size > 0) { - memcpy(out_buf + sizeof(struct header), buf.data, buf.len); - ESP_LOG_BUFFER_HEXDUMP(TAG, out_buf + sizeof(struct header), head->size, ESP_LOG_VERBOSE); - free(buf.data); + struct header *head = (void *)out_buf; + if (h->outbound.len <= h->transaction_size && allow_test_tx == false) { + // sending outbound + head->size = h->outbound.len; + if (h->outbound.len > 0) { + memcpy(out_buf + sizeof(struct header), h->outbound.data, h->outbound.len); + free(h->outbound.data); + ESP_LOG_BUFFER_HEXDUMP(TAG, out_buf + sizeof(struct header), head->size, ESP_LOG_VERBOSE); + h->outbound.data = NULL; + h->outbound.len = 0; + } + do { + tx_queue_stat = xQueueReceive(h->out_queue, &h->outbound, 0); + } while (tx_queue_stat == pdTRUE && h->outbound.len == -1); + if (h->outbound.len == -1) { // used as a signal only, no actual data + h->outbound.len = 0; + } + } else { + // outbound is bigger, need to transmit in another transaction (keep this empty) + head->size = 0; } - printf("*"); - ret = perform_transaction(h, MAX(in_long_payload, out_long_payload) + sizeof(struct header), out_buf, in_buf); + next_tx_size = head->next_size = h->outbound.len; + head->magic = SPI_HEADER_MAGIC; + head->check = esp_rom_crc16_le(0, out_buf, sizeof(struct header) - sizeof(uint16_t)); + esp_err_t ret = perform_transaction(h, sizeof(struct header) + h->transaction_size, out_buf, in_buf); if (ret != ESP_OK) { ESP_LOGE(TAG, "spi_device_transmit failed"); + h->transaction_size = 0; // need to start with HEADER only transaction return ESP_FAIL; } head = (void *)in_buf; - crc = esp_rom_crc8_le(0, in_buf, sizeof(struct header) - 1); - if (crc != head->crc) { - ESP_LOGE(TAG, "Wrong checksum"); - return ESP_FAIL; - } - if (head->magic != FRAME_IN_DATA) { - ESP_LOGE(TAG, "Wrong magic"); + uint16_t check = esp_rom_crc16_le(0, in_buf, sizeof(struct header) - sizeof(uint16_t)); + if (check != head->check || head->magic != SPI_HEADER_MAGIC) { + h->transaction_size = 0; // need to start with HEADER only transaction + if (allow_test_tx) { + return ESP_OK; + } + ESP_LOGE(TAG, "Wrong checksum or magic"); return ESP_FAIL; } - printf("/%d|%d/", out_long_payload, head->size); - if (head->size > 0) { ESP_LOG_BUFFER_HEXDUMP(TAG, in_buf + sizeof(struct header), head->size, ESP_LOG_VERBOSE); esp_netif_receive(netif, in_buf + sizeof(struct header), head->size, NULL); } + h->transaction_size = MAX(next_tx_size, head->next_size); return ESP_OK; } @@ -641,6 +684,9 @@ static void remove_handlers(void) void eppp_deinit(esp_netif_t *netif) { + if (netif == NULL) { + return; + } #if CONFIG_EPPP_LINK_DEVICE_SPI struct eppp_handle *h = esp_netif_get_io_driver(netif); if (h->role == EPPP_CLIENT) { @@ -671,10 +717,6 @@ esp_netif_t *eppp_init(eppp_type_t role, eppp_config_t *config) #if CONFIG_EPPP_LINK_DEVICE_SPI if (role == EPPP_CLIENT) { init_master(&config->spi, netif); - - // as a client, try to actively connect (not waiting for server's interrupt) - struct eppp_handle *h = esp_netif_get_io_driver(netif); - xSemaphoreGive(h->ready_semaphore); } else { init_slave(&config->spi, netif); diff --git a/components/eppp_link/examples/host/main/app_main.c b/components/eppp_link/examples/host/main/app_main.c index 7ea2d45eaec..b320cf4ffa3 100644 --- a/components/eppp_link/examples/host/main/app_main.c +++ b/components/eppp_link/examples/host/main/app_main.c @@ -78,7 +78,7 @@ static void mqtt_event_handler(void *args, esp_event_base_t base, int32_t event_ static void mqtt_app_start(void) { esp_mqtt_client_config_t mqtt_cfg = { - .broker.address.uri = CONFIG_EXAMPLE_BROKER_URL, + .broker.address.uri = "mqtt://mqtt.eclipseprojects.io", }; esp_mqtt_client_handle_t client = esp_mqtt_client_init(&mqtt_cfg); @@ -104,10 +104,6 @@ void app_main(void) eppp_config_t config = EPPP_DEFAULT_CLIENT_CONFIG(); #if CONFIG_EPPP_LINK_DEVICE_SPI config.transport = EPPP_TRANSPORT_SPI; -// config.spi.freq = 20*1000*1000; -// config.spi.input_delay_ns = 5; -// config.spi.cs_ena_posttrans = 3; -// config.spi.cs_ena_pretrans = 3; #else config.transport = EPPP_TRANSPORT_UART; config.uart.tx_io = CONFIG_EXAMPLE_UART_TX_PIN;