refactor(twai): adjust source file layout

Author: suda-morris

components/esp_driver_twai/CMakeLists.txt

@@ -10,8 +10,7 @@ set(priv_req esp_driver_gpio esp_pm)
 
 # Currently support only FD targets
 if(CONFIG_SOC_TWAI_SUPPORT_FD)
-    list(APPEND srcs "onchip/esp_twai_onchip.c")
-    list(APPEND public_include "onchip/include")
+    list(APPEND srcs "esp_twai_onchip.c")
 endif()
 
 idf_component_register(

components/esp_driver_twai/Kconfig

@@ -3,6 +3,7 @@ menu "ESP-Driver:TWAI Configurations"
 
     config TWAI_ISR_INTO_IRAM
         bool "Place TWAI ISR in IRAM to reduce latency"
+        select TWAI_OBJ_CACHE_SAFE
         default n
         help
             Place ISR functions to IRAM to increase performance
@@ -15,4 +16,21 @@ menu "ESP-Driver:TWAI Configurations"
             Allow TWAI works under Cache disabled, to enabled this config,
             all callbacks and user_ctx should also place in IRAM
 
+    config TWAI_OBJ_CACHE_SAFE
+        bool
+        default n
+        help
+            This will ensure the TWAI driver object will not be allocated from a memory region
+            where its cache can be disabled.
+
+    config TWAI_ENABLE_DEBUG_LOG
+        bool "Force enable debug log"
+        default n
+        help
+            If enabled, TWAI driver component will:
+            1. ignore the global logging settings
+            2. compile all log messages into the binary
+            3. set the runtime log level to VERBOSE
+            Please enable this option by caution, as it will increase the binary size.
+
 endmenu # TWAI Configuration

components/esp_driver_twai/esp_twai.c

@@ -4,13 +4,10 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-#include <sys/param.h>
-#include "esp_check.h"
 #include "esp_twai.h"
 #include "esp_private/twai_interface.h"
-#include "esp_private/twai_priv.h"
-
-static const char *TAG = "esp_twai";
+#include "esp_private/twai_utils.h"
+#include "twai_private.h"
 
 /**
  * @brief Calculate twai timing param by giving bitrate and hardware limit.
@@ -44,7 +41,7 @@ uint32_t twai_node_timing_calc_param(const uint32_t source_freq, const twai_timi
     }
 
     uint16_t default_point = (in_param->bitrate >= 800000) ? 750 : ((in_param->bitrate >= 500000) ? 800 : 875);
-    uint16_t sample_point = in_param->sample_point ? in_param->sample_point : default_point;  // default sample point based on bitrate if not configured
+    uint16_t sample_point = in_param->sp_permill ? in_param->sp_permill : default_point;  // default sample point based on bitrate if not configured
     uint16_t tseg_1 = (tseg * sample_point) / 1000;
     tseg_1 = MAX(hw_limit->tseg1_min, MIN(tseg_1, hw_limit->tseg1_max));
     uint16_t tseg_2 = tseg - tseg_1 - 1;
@@ -107,9 +104,9 @@ esp_err_t twai_node_config_range_filter(twai_node_handle_t node, uint8_t filter_
 esp_err_t twai_node_reconfig_timing(twai_node_handle_t node, const twai_timing_advanced_config_t *bit_timing, const twai_timing_advanced_config_t *data_timing)
 {
     ESP_RETURN_ON_FALSE(node && (bit_timing || data_timing), ESP_ERR_INVALID_ARG, TAG, "invalid argument: null");
-    ESP_RETURN_ON_FALSE(node->timing_reconfig, ESP_ERR_NOT_SUPPORTED, TAG, "timing_reconfig func null");
+    ESP_RETURN_ON_FALSE(node->reconfig_timing, ESP_ERR_NOT_SUPPORTED, TAG, "reconfig_timing func null");
 
-    return node->timing_reconfig(node, bit_timing, data_timing);
+    return node->reconfig_timing(node, bit_timing, data_timing);
 }
 
 esp_err_t twai_node_register_event_callbacks(twai_node_handle_t node, const twai_event_callbacks_t *cbs, void *user_data)
@@ -151,3 +148,11 @@ esp_err_t twai_node_receive_from_isr(twai_node_handle_t node, twai_frame_header_
 
     return node->receive_isr(node, header, rx_buffer, buf_sz, received_len);
 }
+
+#if CONFIG_TWAI_ENABLE_DEBUG_LOG
+__attribute__((constructor))
+static void twai_override_default_log_level(void)
+{
+    esp_log_level_set(TAG, ESP_LOG_VERBOSE);
+}
+#endif

components/esp_driver_twai/onchip/esp_twai_onchip.c renamed to components/esp_driver_twai/esp_twai_onchip.c

@@ -4,33 +4,11 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-#include <stdatomic.h>
-#include <stdint.h>
-#include "soc/soc_caps.h"
-#include "soc/twai_periph.h"
-#include "soc/io_mux_reg.h"
-#include "hal/twai_hal.h"
-#include "freertos/FreeRTOS.h"
-#include "esp_check.h"
-#include "esp_pm.h"
-#include "esp_heap_caps.h"
-#include "esp_intr_alloc.h"
-#include "esp_clk_tree.h"
 #include "esp_twai.h"
 #include "esp_twai_onchip.h"
 #include "esp_private/twai_interface.h"
-#include "esp_private/twai_priv.h"
-#include "esp_private/gpio.h"
-#include "esp_private/esp_gpio_reserve.h"
-#include "esp_private/periph_ctrl.h"
-
-static const char *TAG = "twai";
-
-#ifdef CONFIG_TWAI_ISR_INTO_IRAM
-#define TWAI_MALLOC_CAPS    (MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT)
-#else
-#define TWAI_MALLOC_CAPS    MALLOC_CAP_DEFAULT
-#endif  //CONFIG_TWAI_ISR_INTO_IRAM
+#include "esp_private/twai_utils.h"
+#include "twai_private.h"
 
 #if !SOC_RCC_IS_INDEPENDENT
 #define TWAI_RCC_ATOMIC() PERIPH_RCC_ATOMIC()
@@ -203,10 +181,10 @@ static void _node_isr_main(void *arg)
     if (int_stat & (TWAIFD_LL_INTR_BUS_ERR | TWAIFD_LL_INTR_ARBI_LOST)) {
         uint32_t err_reason = twaifd_ll_get_err_reason_code(twai_ctx->hal.dev);
         twai_error_event_data_t e_data = {0};
-        e_data.err_type.arb_lost = !!(int_stat & TWAIFD_LL_INTR_ARBI_LOST);
-        e_data.err_type.bit_err = (err_reason == TWAIFD_LL_ERR_BIT_ERR);
-        e_data.err_type.form_err = (err_reason == TWAIFD_LL_ERR_FRM_ERR);
-        e_data.err_type.stuff_err = (err_reason == TWAIFD_LL_ERR_STUF_ERR);
+        e_data.err_code.arb_lost = !!(int_stat & TWAIFD_LL_INTR_ARBI_LOST);
+        e_data.err_code.bit_err = (err_reason == TWAIFD_LL_ERR_BIT_ERR);
+        e_data.err_code.form_err = (err_reason == TWAIFD_LL_ERR_FRM_ERR);
+        e_data.err_code.stuff_err = (err_reason == TWAIFD_LL_ERR_STUF_ERR);
 
         twai_ctx->history.bus_err_num ++;
         if (twai_ctx->cbs.on_error) {
@@ -233,7 +211,7 @@ static void _node_isr_main(void *arg)
         // only call tx_done_cb when tx without error, otherwise on_error_cb should triggered if it is registered
         if (twai_ctx->cbs.on_tx_done && (int_stat & TWAIFD_LL_INTR_TX_DONE)) {
             twai_tx_done_event_data_t tx_ev = {
-                .done_trans_tx = (twai_frame_t *)twai_ctx->p_curr_tx,
+                .done_tx_frame = twai_ctx->p_curr_tx,
             };
             do_yield |= twai_ctx->cbs.on_tx_done(&twai_ctx->api_base, &tx_ev, twai_ctx->user_data);
         }
@@ -268,22 +246,31 @@ static void _node_isr_main(void *arg)
     }
 }
 
+static void _node_destroy(twai_onchip_ctx_t *twai_ctx)
+{
+    if (twai_ctx->pm_lock) {
+        esp_pm_lock_delete(twai_ctx->pm_lock);
+    }
+    if (twai_ctx->intr_hdl) {
+        esp_intr_free(twai_ctx->intr_hdl);
+    }
+    if (twai_ctx->tx_mount_queue) {
+        vQueueDeleteWithCaps(twai_ctx->tx_mount_queue);
+    }
+    if (twai_ctx->ctrlr_id != -1) {
+        _ctrlr_release(twai_ctx->ctrlr_id);
+    }
+    free(twai_ctx);
+}
+
 static esp_err_t _node_delete(twai_node_handle_t node)
 {
     twai_onchip_ctx_t *twai_ctx = __containerof(node, twai_onchip_ctx_t, api_base);
     ESP_RETURN_ON_FALSE(atomic_load(&twai_ctx->state) == TWAI_ERROR_BUS_OFF, ESP_ERR_INVALID_STATE, TAG, "delete node must when node stopped");
 
     _node_release_io(twai_ctx);
-    _ctrlr_release(twai_ctx->ctrlr_id);
     _twai_rcc_clock_ctrl(twai_ctx->ctrlr_id, false);
-#if CONFIG_PM_ENABLE
-    if (twai_ctx->pm_lock) {
-        ESP_RETURN_ON_ERROR(esp_pm_lock_delete(twai_ctx->pm_lock), TAG, "delete power manager failed");
-    }
-#endif //CONFIG_PM_ENABLE
-    esp_intr_free(twai_ctx->intr_hdl);
-    vQueueDeleteWithCaps(twai_ctx->tx_mount_queue);
-    free(twai_ctx);
+    _node_destroy(twai_ctx);
     return ESP_OK;
 }
 
@@ -339,16 +326,18 @@ static esp_err_t _node_set_bit_timing(twai_node_handle_t node, const twai_timing
     if (timing) {
         twaifd_ll_set_nominal_bitrate(twai_ctx->hal.dev, timing);
         if (timing->ssp_offset) {
-            ssp_offset = timing->ssp_offset;
+            // the underlying hardware calculates the ssp in system clock cycles, not in quanta time
+            ssp_offset = timing->ssp_offset * timing->brp;
         }
     }
 #if SOC_TWAI_SUPPORT_FD
     if (timing_fd) {
         twai_ctx->valid_fd_timing = true;
         twaifd_ll_set_fd_bitrate(twai_ctx->hal.dev, timing_fd);
+        // Note, the ssp_offset set for the data phase can override the nominal phase, because ssp is more necessary for a high bitrate communication
         if (timing_fd->ssp_offset) {
-            // prefer to config ssp by fd param
-            ssp_offset = timing_fd->ssp_offset;
+            // the underlying hardware calculates the ssp in system clock cycles, not in quanta time
+            ssp_offset = timing_fd->ssp_offset * timing_fd->brp;
         }
     }
 #endif
@@ -475,12 +464,10 @@ static esp_err_t _node_config_range_filter(twai_node_handle_t node, uint8_t filt
     ESP_RETURN_ON_FALSE(filter_id < SOC_TWAI_RANGE_FILTER_NUM, ESP_ERR_INVALID_ARG, TAG, "Invalid range filter id %d", filter_id);
     ESP_RETURN_ON_FALSE(atomic_load(&twai_ctx->state) == TWAI_ERROR_BUS_OFF, ESP_ERR_INVALID_STATE, TAG, "config filter must when node stopped");
 
-    bool full_open = (range_cfg->range_high == 0) && (range_cfg->range_low == 0);
-    bool full_close = (range_cfg->range_high == UINT32_MAX) && (range_cfg->range_low == UINT32_MAX);
-    bool cc_ext = full_open || (!full_close && range_cfg->is_ext && !range_cfg->no_classic);
-    bool fd_ext = full_open || (!full_close && range_cfg->is_ext && !range_cfg->no_fd);
-    bool cc_std = full_open || (!full_close && !range_cfg->is_ext && !range_cfg->no_classic);
-    bool fd_std = full_open || (!full_close && !range_cfg->is_ext && !range_cfg->no_fd);
+    bool cc_ext = range_cfg->is_ext && !range_cfg->no_classic;
+    bool fd_ext = range_cfg->is_ext && !range_cfg->no_fd;
+    bool cc_std = !range_cfg->is_ext && !range_cfg->no_classic;
+    bool fd_std = !range_cfg->is_ext && !range_cfg->no_fd;
     twaifd_ll_filter_enable_basic_ext(twai_ctx->hal.dev, filter_id, true, cc_ext);
     twaifd_ll_filter_enable_fd_ext(twai_ctx->hal.dev, filter_id, true, fd_ext);
     twaifd_ll_filter_enable_basic_std(twai_ctx->hal.dev, filter_id, true, cc_std);
@@ -575,21 +562,24 @@ esp_err_t twai_new_node_onchip(const twai_onchip_node_config_t *node_config, twa
     // Allocate TWAI node object memory
     twai_onchip_ctx_t *node = heap_caps_calloc(1, sizeof(twai_onchip_ctx_t), TWAI_MALLOC_CAPS);
     ESP_RETURN_ON_FALSE(node, ESP_ERR_NO_MEM, TAG, "No mem");
+    node->ctrlr_id = -1;
     // Acquire controller
     int ctrlr_id = _ctrlr_acquire(node);
-    ESP_GOTO_ON_FALSE(ctrlr_id != -1, ESP_ERR_NOT_FOUND, ctrlr_err, TAG, "Controller not available");
+    ESP_GOTO_ON_FALSE(ctrlr_id != -1, ESP_ERR_NOT_FOUND, err, TAG, "Controller not available");
     node->ctrlr_id = ctrlr_id;
 
     // state is in bus_off before enabled
     atomic_store(&node->state, TWAI_ERROR_BUS_OFF);
     node->tx_mount_queue = xQueueCreateWithCaps(node_config->tx_queue_depth, sizeof(twai_frame_t *), TWAI_MALLOC_CAPS);
-    ESP_GOTO_ON_FALSE(node->tx_mount_queue, ESP_ERR_NO_MEM, create_err, TAG, "no_mem");
-    uint32_t intr_flags = node_config->intr_priority ? BIT(node_config->intr_priority) | ESP_INTR_FLAG_INTRDISABLED : ESP_INTR_FLAG_LOWMED | ESP_INTR_FLAG_INTRDISABLED;
-#if CONFIG_TWAI_ISR_CACHE_SAFE
-    intr_flags |= ESP_INTR_FLAG_IRAM;
-#endif
+    ESP_GOTO_ON_FALSE(node->tx_mount_queue, ESP_ERR_NO_MEM, err, TAG, "no_mem");
+    uint32_t intr_flags = TWAI_INTR_ALLOC_FLAGS;
+    if (node_config->intr_priority > 0) {
+        intr_flags |= BIT(node_config->intr_priority);
+    } else {
+        intr_flags |= ESP_INTR_FLAG_LOWMED;
+    }
     ESP_GOTO_ON_ERROR(esp_intr_alloc(twai_controller_periph_signals.controllers[ctrlr_id].irq_id, intr_flags, _node_isr_main, (void *)node, &node->intr_hdl),
-                      create_err, TAG, "Alloc interrupt failed");
+                      err, TAG, "Alloc interrupt failed");
 
     // Enable bus clock and reset controller
     _twai_rcc_clock_ctrl(ctrlr_id, true);
@@ -599,20 +589,20 @@ esp_err_t twai_new_node_onchip(const twai_onchip_node_config_t *node_config, twa
         .intr_mask = DRIVER_DEFAULT_INTERRUPTS,
         .enable_listen_only = node_config->flags.enable_listen_only,
     };
-    ESP_GOTO_ON_FALSE(twai_hal_init(&node->hal, &hal_config), ESP_ERR_INVALID_STATE, config_err, TAG, "hardware not in reset state");
+    ESP_GOTO_ON_FALSE(twai_hal_init(&node->hal, &hal_config), ESP_ERR_INVALID_STATE, err, TAG, "hardware not in reset state");
     twaifd_ll_set_mode(node->hal.dev, node_config->flags.enable_listen_only, node_config->flags.enable_self_test, node_config->flags.enable_loopback);
     twaifd_ll_set_tx_retrans_limit(node->hal.dev, node_config->fail_retry_cnt);
     twaifd_ll_filter_block_rtr(node->hal.dev, node_config->flags.no_receive_rtr);
     twaifd_ll_enable_filter_mode(node->hal.dev, true);  // each filter still has independent enable control
     twaifd_ll_enable_fd_mode(node->hal.dev, true);      // fd frame still controlled by `header.fdf`
 
     // Configure bus timing
-    ESP_GOTO_ON_ERROR(_node_calc_set_bit_timing(&node->api_base, node_config->clk_src, &node_config->bit_timing, &node_config->data_timing), config_err, TAG, "bitrate error");
+    ESP_GOTO_ON_ERROR(_node_calc_set_bit_timing(&node->api_base, node_config->clk_src, &node_config->bit_timing, &node_config->data_timing), err, TAG, "bitrate error");
 
     // Configure GPIO
-    ESP_GOTO_ON_ERROR(_node_config_io(node, node_config), config_err, TAG, "gpio config failed");
+    ESP_GOTO_ON_ERROR(_node_config_io(node, node_config), err, TAG, "gpio config failed");
 #if CONFIG_PM_ENABLE
-    ESP_GOTO_ON_ERROR(esp_pm_lock_create(ESP_PM_NO_LIGHT_SLEEP, 0, twai_controller_periph_signals.controllers[ctrlr_id].module_name, &node->pm_lock), config_err, TAG, "init power manager failed");
+    ESP_GOTO_ON_ERROR(esp_pm_lock_create(ESP_PM_NO_LIGHT_SLEEP, 0, twai_controller_periph_signals.controllers[ctrlr_id].module_name, &node->pm_lock), err, TAG, "init power manager failed");
 #endif //CONFIG_PM_ENABLE
 
     node->api_base.enable = _node_enable;
@@ -621,7 +611,7 @@ esp_err_t twai_new_node_onchip(const twai_onchip_node_config_t *node_config, twa
     node->api_base.recover = _node_recover;
     node->api_base.config_mask_filter = _node_config_mask_filter;
     node->api_base.config_range_filter = _node_config_range_filter;
-    node->api_base.timing_reconfig = _node_set_bit_timing;
+    node->api_base.reconfig_timing = _node_set_bit_timing;
     node->api_base.register_cbs = _node_register_callbacks;
     node->api_base.transmit = _node_queue_tx;
     node->api_base.receive_isr = _node_parse_rx;
@@ -630,16 +620,9 @@ esp_err_t twai_new_node_onchip(const twai_onchip_node_config_t *node_config, twa
     *node_ret = &node->api_base;
     return ESP_OK;
 
-config_err:
-    if (node->intr_hdl) {
-        esp_intr_free(node->intr_hdl);
-    }
-    if (node->tx_mount_queue) {
-        vQueueDeleteWithCaps(node->tx_mount_queue);
+err:
+    if (node) {
+        _node_destroy(node);
     }
-create_err:
-    _ctrlr_release(ctrlr_id);
-ctrlr_err:
-    free(node);
     return ret;
 }

components/esp_driver_twai/include/esp_private/twai_interface.h

@@ -77,7 +77,7 @@ struct twai_node_base {
      *      - ESP_OK: Success
      *      - ESP_ERR_INVALID_ARG: Invalid timing configuration
      */
-    esp_err_t (*timing_reconfig)(struct twai_node_base *node, const twai_timing_advanced_config_t *bit_timing, const twai_timing_advanced_config_t *data_timing);
+    esp_err_t (*reconfig_timing)(struct twai_node_base *node, const twai_timing_advanced_config_t *bit_timing, const twai_timing_advanced_config_t *data_timing);
 
     /**
      * @brief Transmit a TWAI frame through the TWAI node

components/esp_driver_twai/include/esp_private/twai_priv.h renamed to components/esp_driver_twai/include/esp_private/twai_utils.h

@@ -60,6 +60,8 @@ esp_err_t twai_node_register_event_callbacks(twai_node_handle_t node, const twai
 /**
  * @brief Reconfigure the timing settings of the TWAI node
  *
+ * @note You can reconfigure the timing for the arbitration and data phase, separately or together.
+ *
  * @param node Handle to the TWAI node
  * @param bit_timing Optional,pointer to new twai cc(classic) or arbitration phase of twai fd timing configuration
  * @param data_timing Optional, pointer to new twai fd timing configuration
@@ -110,7 +112,9 @@ esp_err_t twai_node_get_info(twai_node_handle_t node, twai_node_status_t *status
 esp_err_t twai_node_transmit(twai_node_handle_t node, const twai_frame_t *frame, int timeout_ms);
 
 /**
- * @brief Receive a TWAI frame from 'rx_done_cb' (ONLY FROM 'rx_done_cb')
+ * @brief Receive a TWAI frame from 'rx_done_cb'
+ *
+ * @note This function can only be called from the `rx_done_cb` callback, you can't call it from a task.
  *
  * @param[in] node Handle to the TWAI node
  * @param[out] header Where to store frame header

components/esp_driver_twai/include/esp_twai.h

@@ -21,9 +21,9 @@ typedef struct twai_node_base *twai_node_handle_t;
  * @brief TWAI bitrate timing config basic (simple) mode
  */
 typedef struct {
-    uint32_t bitrate;               /**< Expected TWAI bus baud_rate/bitrate in bits/second */
-    uint16_t sample_point;          /**< Optional, sampling point in permill (1/1000) of the entire bit time */
-    uint16_t ssp_permill;           /**< Optional, secondary sample point(ssp) in permill (1/1000) of the entire bit time */
+    uint32_t bitrate;     /**< Expected TWAI bus baud_rate/bitrate in bits/second */
+    uint16_t sp_permill;  /**< Optional, sampling point in permill (1/1000) of the entire bit time */
+    uint16_t ssp_permill; /**< Optional, secondary sample point(ssp) in permill (1/1000) of the entire bit time */
 } twai_timing_basic_config_t;
 
 /**
@@ -52,8 +52,6 @@ typedef struct {
 
 /**
  * @brief Range-based filter configuration structure
- *
- * @note Set both range_low and range_high to `0` to receive ALL frames, both `0xFFFFFFFF` to receive NONE frames
  */
 typedef struct {
     uint32_t range_low;             /**< Lower bound of the filtering range */
@@ -87,7 +85,7 @@ typedef struct {
  * @brief TWAI "TX done" event data
  */
 typedef struct {
-    twai_frame_t *done_trans_tx;
+    const twai_frame_t *done_tx_frame;  /**< Pointer to the frame that has been transmitted */
 } twai_tx_done_event_data_t;
 
 /**
@@ -122,7 +120,7 @@ typedef union {
  * @brief TWAI "error" event data
  */
 typedef struct {
-    twai_error_code_t err_type;
+    twai_error_code_t err_code; /**< Error code indicating the type of the error */
 } twai_error_event_data_t;
 
 /**

components/esp_driver_twai/onchip/include/esp_twai_onchip.h renamed to components/esp_driver_twai/include/esp_twai_onchip.h

@@ -6,3 +6,8 @@ set(COMPONENTS main)
 
 include($ENV{IDF_PATH}/tools/cmake/project.cmake)
 project(twaifd_test)
+
+message(STATUS "Checking TWAI registers are not read-write by half-word")
+include($ENV{IDF_PATH}/tools/ci/check_register_rw_half_word.cmake)
+check_register_rw_half_word(SOC_MODULES "twai*" "pcr" "hp_sys_clkrst"
+                            HAL_MODULES "twai*")