Consolidate and unify state change handling

Signed-off-by Andri Yngvason <[email protected]>

---
 drivers/net/can/flexcan.c | 66
+++--------------------------------------------
 1 file changed, 3 insertions(+), 63 deletions(-)

drivers/net/can/flexcan.c +0 -7

index 2700865..96a0755 100644
@@ -559,74 +559,15 @@ static int flexcan_poll_bus_err(struct net_device
 *dev, u32 reg_esr)
 static void do_state(struct net_device *dev,
              struct can_frame *cf, enum can_state new_state)
 {
-    struct flexcan_priv *priv = netdev_priv(dev);
     struct can_berr_counter bec;
 
     __flexcan_get_berr_counter(dev, &bec);
 
-    switch (priv->can.state) {
-    case CAN_STATE_ERROR_ACTIVE:
-        /*
-         * from: ERROR_ACTIVE
-         * to  : ERROR_WARNING, ERROR_PASSIVE, BUS_OFF
-         * =>  : there was a warning int

drivers/net/can/flexcan.c +0 -17

-        }
-    case CAN_STATE_ERROR_WARNING:    /* fallthrough */
-        /*
-         * from: ERROR_ACTIVE, ERROR_WARNING
-         * to  : ERROR_PASSIVE, BUS_OFF
-         * =>  : error passive int
-         */
-        if (new_state >= CAN_STATE_ERROR_PASSIVE &&
-            new_state <= CAN_STATE_BUS_OFF) {
-            netdev_dbg(dev, "Error Passive IRQ\n");
-            priv->can.can_stats.error_passive++;
-
-            cf->can_id |= CAN_ERR_CRTL;
-            cf->data[1] = (bec.txerr > bec.rxerr) ?
-                CAN_ERR_CRTL_TX_PASSIVE :
-                CAN_ERR_CRTL_RX_PASSIVE;
-        }

drivers/net/can/flexcan.c +1 -21

-    /* process state changes depending on the new state */
-    switch (new_state) {
-    case CAN_STATE_ERROR_WARNING:
-        netdev_dbg(dev, "Error Warning\n");
-        cf->can_id |= CAN_ERR_CRTL;
-        cf->data[1] = (bec.txerr > bec.rxerr) ?
-            CAN_ERR_CRTL_TX_WARNING :
-            CAN_ERR_CRTL_RX_WARNING;
-        break;
-    case CAN_STATE_ERROR_ACTIVE:
-        netdev_dbg(dev, "Error Active\n");
-        cf->can_id |= CAN_ERR_PROT;
-        cf->data[2] = CAN_ERR_PROT_ACTIVE;
-        break;
-    case CAN_STATE_BUS_OFF:
-        cf->can_id |= CAN_ERR_BUSOFF;
+    if (unlikely(new_state == CAN_STATE_BUS_OFF))
         can_bus_off(dev);
-        break;
-    default:
-        break;
-    }
 }
 
 static int flexcan_poll_state(struct net_device *dev, u32 reg_esr)

@@ -658,7 +599,6 @@ static int flexcan_poll_state(struct net_device
 *dev, u32 reg_esr)
         return 0;
 
     do_state(dev, cf, new_state);
-    priv->can.state = new_state;
     netif_receive_skb(skb);
 

     dev->stats.rx_packets++;

drivers/net/can/flexcan.c static int flexcan_poll_bus_err(struct net_device *dev, u32 reg_esr) · line 582

Quoted 7 lines · Andri Yngvason

Signed-off-by Andri Yngvason <[email protected]>

---
...

drivers/net/can/flexcan.c +0 -4

-            cf->can_id |= CAN_ERR_CRTL;
-            cf->data[1] = (bec.txerr > bec.rxerr) ?
-                CAN_ERR_CRTL_TX_WARNING :
-                CAN_ERR_CRTL_RX_WARNING;

Hm, can_change_state() handles the equal case differently. In the SJA1000 manual I found:

"Errors detected during reception or transmission will affect the error counters according to the CAN 2.0B protocol
specification. The error status bit is set when at least one of the error counters has reached or exceeded the CPU
warning limit of 96. An error interrupt is generated, if enabled."

If both are equal we do not known if rx or tx has caused the state change and therefore setting "CAN_ERR_CRTL_TX_WARNING | CAN_ERR_CRTL_RX_WARNING" seems more logical, indeed. But maybe it simply does not happen. Any other opinions?

Quoted 17 lines

drivers/net/can/flexcan.c +0 -17

-        }
-    case CAN_STATE_ERROR_WARNING:    /* fallthrough */
-        /*
-         * from: ERROR_ACTIVE, ERROR_WARNING
-         * to  : ERROR_PASSIVE, BUS_OFF
-         * =>  : error passive int
-         */
-        if (new_state >= CAN_STATE_ERROR_PASSIVE &&
-            new_state <= CAN_STATE_BUS_OFF) {
-            netdev_dbg(dev, "Error Passive IRQ\n");
-            priv->can.can_stats.error_passive++;
-
-            cf->can_id |= CAN_ERR_CRTL;
-            cf->data[1] = (bec.txerr > bec.rxerr) ?
-                CAN_ERR_CRTL_TX_PASSIVE :
-                CAN_ERR_CRTL_RX_PASSIVE;
-        }

drivers/net/can/flexcan.c static int flexcan_poll_bus_err(struct net_device *dev, u32 reg_esr) · line 567

Quoted 10 lines

drivers/net/can/flexcan.c +2 -8

-        break;
-    case CAN_STATE_BUS_OFF:
-        netdev_err(dev, "BUG! "
-               "hardware recovered automatically from BUS_OFF\n");
-        break;
-    default:
-        break;
-    }
+    can_change_state(dev, cf, new_state,
+             can_get_err_dir(bec.rxerr, bec.txerr));

Saves a lot of lines :).

drivers/net/can/flexcan.c static int flexcan_poll_bus_err(struct net_device *dev, u32 reg_esr) · line 615

Quoted 8 lines

drivers/net/can/flexcan.c +0 -7

-    /* process state changes depending on the new state */
-    switch (new_state) {
-    case CAN_STATE_ERROR_WARNING:
-        netdev_dbg(dev, "Error Warning\n");
-        cf->can_id |= CAN_ERR_CRTL;
-        cf->data[1] = (bec.txerr > bec.rxerr) ?
-            CAN_ERR_CRTL_TX_WARNING :

To validate the correct behaviour could you please send messages while the cable is disconnected. Then reconnect the cable and see how the error state decreases. You can monitor the behaviour with ""candump -td -e any,0:0,#FFFFFFFF" in another shell.

Quoted 17 lines · Wolfgang Grandegger

drivers/net/can/flexcan.c static int flexcan_poll_bus_err(struct net_device *dev, u32 reg_esr) · line 582

Quoted 7 lines · Andri Yngvason

Signed-off-by Andri Yngvason <[email protected]>

---
...

drivers/net/can/flexcan.c +0 -4

-            cf->can_id |= CAN_ERR_CRTL;
-            cf->data[1] = (bec.txerr > bec.rxerr) ?
-                CAN_ERR_CRTL_TX_WARNING :
-                CAN_ERR_CRTL_RX_WARNING;

Hm, can_change_state() handles the equal case differently. In the SJA1000 manual I found:

"Errors detected during reception or transmission will affect the error counters according to the CAN 2.0B protocol
specification. The error status bit is set when at least one of the error counters has reached or exceeded the CPU
warning limit of 96. An error interrupt is generated, if enabled."

Quoted 4 lines

If both are equal we do not known if rx or tx has caused the state change and therefore setting "CAN_ERR_CRTL_TX_WARNING | CAN_ERR_CRTL_RX_WARNING" seems more logical, indeed. But maybe it simply does not happen. Any other opinions?

I think that not specifically handling the equal case would be wrong. Let's consider the following sequence of events:
* txerr reaches warning level
* rxerr reaches warning level
If they are both equal at this point, you will only get a second CAN_ERR_CRTL_TX_WARNING in the current implementation, whereas in the proposed
implementation, the user would get
CAN_ERR_CRTL_TX_WARNING | CAN_ERR_CRTL_RX_WARNING and because the user can know
the prior error state message, he can find out which state actually changed.

But this is all based on the premise that txerr hasn't progressed since. In fact,
because we cannot assume that txerr stays in place until rxerr catches up, this
is what we should be doing:
enum can_state errcount_to_state(unsigned int count)

{
    if (unlikely(count > 127))
        return CAN_STATE_ERROR_PASSIVE;

    if (unlikely(count > 96))
        return CAN_STATE_ERROR_WARNING;

    return CAN_STATE_ERROR_ACTIVE;
}

enum can_err_dir can_get_err_dir(unsigned int txerr, unsigned int rxerr)

{
    enum can_err_dir dir;

    enum can_state tx_state = errcount_to_state(txerr);
    enum can_state rx_state = errcount_to_state(rxerr);

    if (tx_state > rx_state)
        return CAN_ERR_DIR_TX;

    if (tx_state < rx_state)
        return CAN_ERR_DIR_RX;

    return CAN_ERR_DIR_TX | CAN_ERR_DIR_RX;
}

However, now that we've introduced errcount_to_state(), it seems to me that it would
be simpler to dump the proposed CAN_ERR_DIR enum in favour of passing the two states
directly to can_change_state().

Quoted 27 lines

Quoted 17 lines

drivers/net/can/flexcan.c +0 -17

-        }
-    case CAN_STATE_ERROR_WARNING:    /* fallthrough */
-        /*
-         * from: ERROR_ACTIVE, ERROR_WARNING
-         * to  : ERROR_PASSIVE, BUS_OFF
-         * =>  : error passive int
-         */
-        if (new_state >= CAN_STATE_ERROR_PASSIVE &&
-            new_state <= CAN_STATE_BUS_OFF) {
-            netdev_dbg(dev, "Error Passive IRQ\n");
-            priv->can.can_stats.error_passive++;
-
-            cf->can_id |= CAN_ERR_CRTL;
-            cf->data[1] = (bec.txerr > bec.rxerr) ?
-                CAN_ERR_CRTL_TX_PASSIVE :
-                CAN_ERR_CRTL_RX_PASSIVE;
-        }

drivers/net/can/flexcan.c static int flexcan_poll_bus_err(struct net_device *dev, u32 reg_esr) · line 567

Quoted 10 lines

drivers/net/can/flexcan.c +2 -8

-        break;
-    case CAN_STATE_BUS_OFF:
-        netdev_err(dev, "BUG! "
-               "hardware recovered automatically from BUS_OFF\n");
-        break;
-    default:
-        break;
-    }
+    can_change_state(dev, cf, new_state,
+             can_get_err_dir(bec.rxerr, bec.txerr));

Quoted 1 line

Saves a lot of lines :).

Indeed ;)

Quoted 8 lines

drivers/net/can/flexcan.c static int flexcan_poll_bus_err(struct net_device *dev, u32 reg_esr) · line 615

Quoted 8 lines

drivers/net/can/flexcan.c +0 -7

-    /* process state changes depending on the new state */
-    switch (new_state) {
-    case CAN_STATE_ERROR_WARNING:
-        netdev_dbg(dev, "Error Warning\n");
-        cf->can_id |= CAN_ERR_CRTL;
-        cf->data[1] = (bec.txerr > bec.rxerr) ?
-            CAN_ERR_CRTL_TX_WARNING :

Quoted 5 lines

To validate the correct behaviour could you please send messages while the cable is disconnected. Then reconnect the cable and see how the error state decreases. You can monitor the behaviour with ""candump -td -e any,0:0,#FFFFFFFF" in another shell.

I'm using PCAN-USB Pro to generate errors on the bus. It works quite well.

I can generate tx errors by sending from the device and then have the pcan ruin a few frames. rx errors can be generated by having an other device on the bus outputting random data and then let the pcan corrupt the frames.

Sadly the error generation mechanism only works on windows. :(

I've tried the "disconnected cable" method too in the past. It usually puts mscan into bus-off quite fast.

Quoted 22 lines · Andri Yngvason

Quoted 17 lines · Wolfgang Grandegger

Review

Quoted 7 lines · Andri Yngvason

Signed-off-by Andri Yngvason <[email protected]>

---
...

(quoted diff) +0 -4

-            cf->can_id |= CAN_ERR_CRTL;
-            cf->data[1] = (bec.txerr > bec.rxerr) ?
-                CAN_ERR_CRTL_TX_WARNING :
-                CAN_ERR_CRTL_RX_WARNING;

Hm, can_change_state() handles the equal case differently. In the SJA1000 manual I found:

"Errors detected during reception or transmission will affect the error counters according to the CAN 2.0B protocol
specification. The error status bit is set when at least one of the error counters has reached or exceeded the CPU
warning limit of 96. An error interrupt is generated, if enabled."

Quoted 4 lines

If both are equal we do not known if rx or tx has caused the state change and therefore setting "CAN_ERR_CRTL_TX_WARNING | CAN_ERR_CRTL_RX_WARNING" seems more logical, indeed. But maybe it simply does not happen. Any other opinions?

Quoted 12 lines

I think that not specifically handling the equal case would be wrong. Let's consider the following sequence of events:
* txerr reaches warning level
* rxerr reaches warning level
If they are both equal at this point, you will only get a second CAN_ERR_CRTL_TX_WARNING in the current implementation, whereas in the proposed
implementation, the user would get
CAN_ERR_CRTL_TX_WARNING | CAN_ERR_CRTL_RX_WARNING and because the user can know
the prior error state message, he can find out which state actually changed.

The question is what error (rx or tx) error did triger the error state change interrupt. I doubt that such an interrupt is triggered when one error counter catches up, .e.g. txer was > 128 and rxerr exceeded 128. It's even not sure that all the controllers act the same way. Therefore also keeping the current behaviour would be fine for me.

Quoted 32 lines

But this is all based on the premise that txerr hasn't progressed since. In fact,
because we cannot assume that txerr stays in place until rxerr catches up, this
is what we should be doing:
enum can_state errcount_to_state(unsigned int count)

{
     if (unlikely(count > 127))
         return CAN_STATE_ERROR_PASSIVE;

     if (unlikely(count > 96))
         return CAN_STATE_ERROR_WARNING;

     return CAN_STATE_ERROR_ACTIVE;
}

enum can_err_dir can_get_err_dir(unsigned int txerr, unsigned int rxerr)

{
     enum can_err_dir dir;

     enum can_state tx_state = errcount_to_state(txerr);
     enum can_state rx_state = errcount_to_state(rxerr);

     if (tx_state > rx_state)
         return CAN_ERR_DIR_TX;

     if (tx_state < rx_state)
         return CAN_ERR_DIR_RX;

     return CAN_ERR_DIR_TX | CAN_ERR_DIR_RX;
}

Quoted 5 lines

However, now that we've introduced errcount_to_state(), it seems to me that it would
be simpler to dump the proposed CAN_ERR_DIR enum in favour of passing the two states
directly to can_change_state().

D'accord.

Quoted 6 lines

Quoted 6 lines

To validate the correct behaviour could you please send messages while the cable is disconnected. Then reconnect the cable and see how the error state decreases. You can monitor the behaviour with ""candump -td -e any,0:0,#FFFFFFFF" in another shell.

Quoted 4 lines

I'm using PCAN-USB Pro to generate errors on the bus. It works quite well. I can generate tx errors by sending from the device and then have the pcan ruin a few frames. rx errors can be generated by having an other device on the bus outputting random data and then let the pcan corrupt the frames.

Short-circuiting the CAN low and high lines is a simple method to

Quoted 4 lines

Sadly the error generation mechanism only works on windows. :(

I've tried the "disconnected cable" method too in the past. It usually puts mscan into bus-off quite fast.

Sending a message whithout cable should never trigger an bus-off. The tx error counter never exceeds 128.

Here is an example output of "candump -candump -td -e any,0:0,#FFFFFFFF" for a recovery from error passive state due to no ack/cable (reconnect after 5s) for a SJA1000 on an on EMS PCI card:

(000.201913) can0 1C [0]
(000.212241) can0 20000204 [8] 00 08 00 00 00 00 60 00 ERRORFRAME

         controller-problem{tx-error-warning}
         state-change{tx-error-warning}
         error-counter-tx-rx{{96}{0}}
  (000.003544)  can0  20000204  [8] 00 20 00 00 00 00 80 00   ERRORFRAME
         controller-problem{tx-error-passive}
         state-change{tx-error-passive}
         error-counter-tx-rx{{128}{0}}
  (004.901842)  can0   1D  [7] 1D F6 33 52 31 4B DE
  (000.000116)  can0  20000200  [8] 00 08 00 00 00 00 7F 00   ERRORFRAME
         state-change{tx-error-warning}
         error-counter-tx-rx{{127}{0}}
  (000.000678)  can0   1E  [6] 42 05 14 82 23 B6
  ...
  (000.201927)  can0   49  [4] 2F 1A 97 25
  (000.000096)  can0  20000200  [8] 00 40 00 00 00 00 5F 00   ERRORFRAME
         state-change{back-to-error-active}
         error-counter-tx-rx{{95}{0}}

(000.202184) can0 4A [8] 7F 87 0E FE 03 BA 78 91

This is from my related patch-set.

Quoted 37 lines · Wolfgang Grandegger

Quoted 23 lines · Andri Yngvason

Quoted 17 lines · Wolfgang Grandegger

Review

Quoted 7 lines · Andri Yngvason

Signed-off-by Andri Yngvason <[email protected]>

---
...

(quoted diff) +0 -4

-            cf->can_id |= CAN_ERR_CRTL;
-            cf->data[1] = (bec.txerr > bec.rxerr) ?
-                CAN_ERR_CRTL_TX_WARNING :
-                CAN_ERR_CRTL_RX_WARNING;

Hm, can_change_state() handles the equal case differently. In the SJA1000 manual I found:

"Errors detected during reception or transmission will affect the error counters according to the CAN 2.0B protocol
specification. The error status bit is set when at least one of the error counters has reached or exceeded the CPU
warning limit of 96. An error interrupt is generated, if enabled."

Quoted 5 lines

If both are equal we do not known if rx or tx has caused the state change and therefore setting "CAN_ERR_CRTL_TX_WARNING | CAN_ERR_CRTL_RX_WARNING" seems more logical, indeed. But maybe it simply
does not happen. Any other opinions?

Quoted 13 lines

I think that not specifically handling the equal case would be wrong. Let's
consider the following sequence of events:
* txerr reaches warning level
* rxerr reaches warning level
If they are both equal at this point, you will only get a second CAN_ERR_CRTL_TX_WARNING in the current implementation, whereas in the proposed
implementation, the user would get
CAN_ERR_CRTL_TX_WARNING | CAN_ERR_CRTL_RX_WARNING and because the user can know
the prior error state message, he can find out which state actually changed.

Quoted 5 lines

The question is what error (rx or tx) error did triger the error state change interrupt. I doubt that such an interrupt is triggered when one error counter catches up, .e.g. txer was > 128 and rxerr exceeded 128. It's even not sure that all the controllers act the same way. Therefore also keeping the current behaviour would be fine for me.

Also, because of the state != priv->state assert, the equal case won't happen
when the state increases, but it might happen when it goes down. Perhaps that should be changed?

But in the case where the state goes down, there will definitely be an interrupt generated. E.g. rx_state = warn, tx_state = passive and then when tx_state -> warn, we will have the controller's state go to warn from passive,
and then rx_state == tx_state. So, if we only want to send which state changed, we actually have to keep copies of each counter's current (last) state, as is done in priv->state, for the whole controller.

I think it would be easier, simpler and more useful to just send the current,
state of each counter whenever the state changes. Consider this:

drivers/net/can/dev.c +61 -0

index 02492d2..6199571 100644
@@ -273,6 +273,118 @@ static int can_get_bittiming(struct net_device
 *dev, struct can_bittiming *bt,
     return err;
 }
 
+static void can_update_error_counters(enum can_state new_state)
+{
+    if (state < priv->state)
+        return;
+
+    switch (new_state) {
+    case CAN_STATE_ERROR_ACTIVE:
+        netdev_warn(dev, "%s: oops, did we come from a state less than
 error-active?",
+                __func__);
+        break;
...
+}
+
+static int can_txstate_to_frame(enum can_state state)
+{
+    switch(state)
+    {
+    case CAN_STATE_ERROR_ACTIVE:
+        return CAN_ERR_CRTL_TX_ACTIVE;
...
+}
+
+static int can_rxstate_to_frame(enum can_state state)
+{
+    switch(state)
+    {
+    case CAN_STATE_ERROR_ACTIVE:
+        return CAN_ERR_CRTL_RX_ACTIVE;
...
+}
+
+void can_change_state(struct net_device *dev, struct can_frame *cf,
+              enum can_state new_state, enum can_state tx_state,
+              enum can_state rx_state)
+{
+    struct can_priv *priv = netdev_priv(dev);
+
+    if (unlikely(state == priv->state)) {
+        netdev_warn(dev, "%s: oops, state did not change", __func__);
+        return;
+    }
+
+    can_update_error_counters(new_state);
+
+    if (unlikely(state == CAN_STATE_BUS_OFF)) {
+        cf->can_id |= CAN_ERR_BUSOFF;
+    } else {
+        cf->can_id |= CAN_ERR_CRTL;
+        /* Absolute: */
+        cf->data[1] |= can_txstate_to_frame(tx_state)
+                 | can_rxstate_to_frame(rx_state);
+        /* Alternatively, the difference:
+         *  if (tx_state > rx_state)
+         *     cf->data[1] |= can_txstate_to_frame(tx_state);
+         *  if (tx_state < rx_state)
+         *     cf->data[1] |= can_rxstate_to_frame(rx_state);
+         *  else
+         *      cf->data[1] |= can_txstate_to_frame(tx_state)
+         *               | can_rxstate_to_frame(rx_state);
+         * Or even, disregarding the equal case:
+         *  cf->data[1] |= (tx_state > rx_state) ?
+         *      can_txstate_to_frame(tx_state) :
+         *      can_rxstate_to_frame(rx_state);
+         */

Quoted 45 lines

Quoted 32 lines

But this is all based on the premise that txerr hasn't progressed since. In fact,
because we cannot assume that txerr stays in place until rxerr catches up, this
is what we should be doing:
enum can_state errcount_to_state(unsigned int count)

{
     if (unlikely(count > 127))
         return CAN_STATE_ERROR_PASSIVE;

     if (unlikely(count > 96))
         return CAN_STATE_ERROR_WARNING;

     return CAN_STATE_ERROR_ACTIVE;
}

enum can_err_dir can_get_err_dir(unsigned int txerr, unsigned int rxerr)

{
     enum can_err_dir dir;

     enum can_state tx_state = errcount_to_state(txerr);
     enum can_state rx_state = errcount_to_state(rxerr);

     if (tx_state > rx_state)
         return CAN_ERR_DIR_TX;

     if (tx_state < rx_state)
         return CAN_ERR_DIR_RX;

     return CAN_ERR_DIR_TX | CAN_ERR_DIR_RX;
}

Quoted 5 lines

However, now that we've introduced errcount_to_state(), it seems to me that it would
be simpler to dump the proposed CAN_ERR_DIR enum in favour of passing the two states
directly to can_change_state().

D'accord.

Quoted 6 lines

Quoted 6 lines

To validate the correct behaviour could you please send messages while the cable is disconnected. Then reconnect the cable and see how the error state decreases. You can monitor the behaviour with ""candump -td -e any,0:0,#FFFFFFFF" in another shell.

Quoted 8 lines

Quoted 7 lines

I'm using PCAN-USB Pro to generate errors on the bus. It works quite well.
I can generate tx errors by sending from the device and then have the pcan
ruin a few frames. rx errors can be generated by having an other device on
the bus outputting random data and then let the pcan corrupt the frames.

Short-circuiting the CAN low and high lines is a simple method to

Ahh, yes, I tried that too. That's what triggered bus-off. I got it mixed up in
my head. :)

Quoted 30 lines

Quoted 4 lines

Sadly the error generation mechanism only works on windows. :(

I've tried the "disconnected cable" method too in the past. It usually puts mscan into bus-off quite fast.

Sending a message whithout cable should never trigger an bus-off. The tx error counter never exceeds 128.

Here is an example output of "candump -candump -td -e any,0:0,#FFFFFFFF" for a recovery from error passive state due to no ack/cable (reconnect after 5s) for a SJA1000 on an on EMS PCI card:

(000.201913) can0 1C [0]
(000.212241) can0 20000204 [8] 00 08 00 00 00 00 60 00 ERRORFRAME

         controller-problem{tx-error-warning}
         state-change{tx-error-warning}
         error-counter-tx-rx{{96}{0}}
  (000.003544)  can0  20000204  [8] 00 20 00 00 00 00 80 00   ERRORFRAME
         controller-problem{tx-error-passive}
         state-change{tx-error-passive}
         error-counter-tx-rx{{128}{0}}
  (004.901842)  can0   1D  [7] 1D F6 33 52 31 4B DE
  (000.000116)  can0  20000200  [8] 00 08 00 00 00 00 7F 00   ERRORFRAME
         state-change{tx-error-warning}
         error-counter-tx-rx{{127}{0}}
  (000.000678)  can0   1E  [6] 42 05 14 82 23 B6
  ...
  (000.201927)  can0   49  [4] 2F 1A 97 25
  (000.000096)  can0  20000200  [8] 00 40 00 00 00 00 5F 00   ERRORFRAME
         state-change{back-to-error-active}
         error-counter-tx-rx{{95}{0}}

Quoted 4 lines

(000.202184) can0 4A [8] 7F 87 0E FE 03 BA 78 91

This is from my related patch-set.

Okay, I'll try that but the -e flag won't help much because candump expects the PROT abuse.

Andri

PS.: I must admit that I don't actually know why it's useful to know which error counter changed; tx or rx. I think it would be much simpler to send the max of both and be done with it. Can anyone point out a case where this helps?

Quoted 4 lines · Andri Yngvason

PS.: I must admit that I don't actually know why it's useful to know which error counter changed; tx or rx. I think it would be much simpler to send the max of both and be done with it. Can anyone point out a case where this helps?

I agree that it would be much simpler not to distinguish between rx and tx state changes. This is for historical reasons. Oliver, do you remember why we adapted that solution?

Quoted 11 lines · Wolfgang Grandegger

Quoted 7 lines · Andri Yngvason

PS.: I must admit that I don't actually know why it's useful to know which
error counter changed; tx or rx. I think it would be much simpler to send
the max of both and be done with it. Can anyone point out a case where this
helps?

Quoted 4 lines

I agree that it would be much simpler not to distinguish between rx and tx state changes. This is for historical reasons. Oliver, do you remember why we adapted that solution?

No. Indeed I was not even aware of the fact that error counters should

set
into any kind of relation.

When the error counters change, the error message should be fired. And when the thresholds e.g. for CAN_ERR_CRTL_*X_WARNING are triggered these
flags should be set accordingly.

So can_state errcount_to_state() makes perfectly sense.

But I don't know why to compare tx error counters to rx error counters either.