twai: TWAI_ALERT_RX_DATA indicates that frames were received (IDFGH-5664)

[crackwitz]

related to #7374

the #define has to be inserted among the first few entries because ALERT_LOG_LEVEL_WARNING/ALERT_LOG_LEVEL_ERROR go by value. that necessarily requires all following #defines to be rewritten

this patch was made based on esp-idf v4.2.1, then applied to current master branch. I'll have a chance to verify next week.

[Alvin1Zhang]

Thanks for your contribution.

[Dazza0]

@crackwitz The changes look good. It's just missing documentation and support for when SOC_TWAI_SUPPORTS_RX_STATUS is not supported. I'll push an merge request with those extra changes shortly

[crackwitz]

Interesting. I was unaware of that value's significance and I see now my patch was incomplete.

[igrr]

I'm curious, is there any reason to add TWAI_ALERT_RX_DATA with mask 0x4, redefining all subsequent values? Would it be safer to add it at the end (0x00020000)? This way there is lower chance of introducing a breaking change for existing projects, especially if any pre-built libraries are involved.

[crackwitz]

yes there's a reason. look at how ALERT_LOG_LEVEL_WARNING is used.

esp-idf/components/driver/twai.c

Line 53 in 83956eb

#define ALERT_LOG_LEVEL_WARNING TWAI_ALERT_ARB_LOST //Alerts above and including this level use ESP_LOGW

this change can't be done differently.

what you propose would actually violate code in esp-idf. this takes precedence over hypothetical third party libraries.

doing the whole alerts system differently is a different discussion.

[igrr]

There seems to be only one usage of alert_code >= ALERT_LOG_LEVEL_WARNING, so how about we refactor it roughly as follows:

/* instead of "define ALERT_LOG_LEVEL_WARNING ..." */
static inline bool twai_alert_is_warning(uint32_t alert_code) {
  return alert_code >= TWAI_ALERT_ARB_LOST && alert_code != TWAI_ALERT_RX_DATA;
}

/* where ALERT_LOG_LEVEL_WARNING was used */
else if (twai_alert_is_warning(alert_code)) {
   /* ... */
}

[crackwitz]

I find that to be an entirely unjustifiable special case.

continue that line of thought. you'll arrive at a set of constants of arbitrary order, and your function has to check many of them.

you should find that complexity convoluted and unbearable.

an ordered list of values is clean and expected.

if you want to improve something about that code, convert the hex constants into (1 << $bitposition) expressions. that makes explicit the bit field nature (1-hot) of these constants and it makes order trivial to see.

I'd go even further and suggest moving the #define for ALERT_LOG_LEVEL_WARNING and ALERT_LOG_LEVEL_ERROR into the header. they are part of the public API, their meanings and behavior are documented and specified. then, that list of alert defines could contain a few blank lines and comments that explicitly say "the following are logged at level $something"

// the following are logged with ESP_LOGI
#define TWAI_ALERT_TX_IDLE                  (1 <<  0)  /**< No more messages to transmit */
#define TWAI_ALERT_TX_SUCCESS               (1 <<  1)  /**< The previous transmission was successful */
#define TWAI_ALERT_RX_DATA                  (1 <<  2)  /**< A frame has been received and added to the RX queue */
#define TWAI_ALERT_BELOW_ERR_WARN           (1 <<  3)  /**< Both error counters have dropped below error warning limit */
#define TWAI_ALERT_ERR_ACTIVE               (1 <<  4)  /**< TWAI controller has become error active */
#define TWAI_ALERT_RECOVERY_IN_PROGRESS     (1 <<  5)  /**< TWAI controller is undergoing bus recovery */
#define TWAI_ALERT_BUS_RECOVERED            (1 <<  6)  /**< TWAI controller has successfully completed bus recovery */
// the following are logged with ESP_LOGW
#define TWAI_ALERT_ARB_LOST                 (1 <<  7)  /**< The previous transmission lost arbitration */
#define TWAI_ALERT_ABOVE_ERR_WARN           (1 <<  8)  /**< One of the error counters have exceeded the error warning limit */
#define TWAI_ALERT_BUS_ERROR                (1 <<  9)  /**< A (Bit, Stuff, CRC, Form, ACK) error has occurred on the bus */
// the following are logged with ESP_LOGE
#define TWAI_ALERT_TX_FAILED                (1 << 10)  /**< The previous transmission has failed (for single shot transmission) */
#define TWAI_ALERT_RX_QUEUE_FULL            (1 << 11)  /**< The RX queue is full causing a frame to be lost */
#define TWAI_ALERT_ERR_PASS                 (1 << 12)  /**< TWAI controller has become error passive */
#define TWAI_ALERT_BUS_OFF                  (1 << 13)  /**< Bus-off condition occurred. TWAI controller can no longer influence bus */
#define TWAI_ALERT_RX_FIFO_OVERRUN          (1 << 14)  /**< An RX FIFO overrun has occurred */
#define TWAI_ALERT_TX_RETRIED               (1 << 15)  /**< An message transmission was cancelled and retried due to an errata workaround */
#define TWAI_ALERT_PERIPH_RESET             (1 << 16)  /**< The TWAI controller was reset */

#define TWAI_ALERT_ALL                      ((1 << 17)-1)  /**< Bit mask to enable all alerts during configuration */
#define TWAI_ALERT_NONE                     0x00000000  /**< Bit mask to disable all alerts during configuration */
#define TWAI_ALERT_AND_LOG                  (1 << 17)  /**< Bit mask to enable alerts to also be logged when they occur. Note that logging from the ISR is disabled if CONFIG_TWAI_ISR_IN_IRAM is enabled (see docs). */

[crackwitz]

This is great! Thanks!