Hi Oliver,
On 02/15/2011 08:29 PM, Oliver Hartkopp wrote:
> On 15.02.2011 20:18, Wolfgang Grandegger wrote:
>> On 02/15/2011 05:52 PM, Oliver Hartkopp wrote:
>>> On 15.02.2011 10:34, Wolfgang Grandegger wrote:
>>>
>
>>>>
>>>> and move CAN_ERR_PROT_ACTIVE to
>>>>
>>>> /* error status of CAN-controller / data[1] */
>>>> #define CAN_ERR_CRTL_ACTIVE 0x40 /* back to error active */
>>>>
>>>
>>> Hm. So far only 'problems' have been encoded into this byte. We could define
>>> 'zero' as having 'no problems' ... but adding a new 'good situation flag'
>>> doesn't fit to me.
>>
>> Well, data[1] holds all our state changes. When we start to handle
>> decreasing state changes as well.
>
> I strongly assumed the data[] bytes hold 'states' and not 'state changes' ...
Yep, but we treat error active as state as well. Anyway, I'm fine with
the CAN_ERR_ACTIVE class (in can_id).
> Therefore missing bits would indicate that everything is fine.
>
>>> IMHO i would leave this data[1] as-is and better add a new flag into the
>>> CAN-ID:
>>>
>>> #define CAN_ERR_BUSOFF 0x00000040U /* bus off */
>>> #define CAN_ERR_BUSERROR 0x00000080U /* bus error (may flood!) */
>>> #define CAN_ERR_RESTARTED 0x00000100U /* controller restarted */
>>> #define CAN_ERR_ACTIVE 0x00000200U /* controller (self) recovered */
>>>
>>> Analogue to the bus-off flags you can filter for a self recovering
>>> controller
>>> then.
>>
>> Yes, to wait for "back-to-error-active" events when bus-off recovery has
>> completed might be useful.
>
> fine.
>
>>>> Then the error message for the "no-ack-on-tx" error would be
>>>> composed the following way:
>>>>
>>>> cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>>>
>>> Better
>>> cf->can_id |= CAN_ERR_PROT | CAN_ERR_ACK;
>>> ???
>>
>> Why, it's a bus error just like a crc or frame error.
>
> ?? I _removed_ the CAN_ERR_BUSERR flag in my suggestion, as i don't think it
> is correct here.
>
>> CAN_ERR_ACK might
>> just be useful for backward compatibility but I don't think anybody uses
>> it. I would like to clean that up.
>
> Hm - my intention was that most of the interesting states are available in the
> CAN-ID directly (e.g. _BUSOFF, _RESTARTED, _ACK and now _ACTIVE)
Why is especially _ACK interesting. As I said, it belongs to the bus
errors. There are 5 different error types (which are not mutually
exclusive):
- BIT ERROR
- STUFF ERROR
- CRC ERROR
- FORM ERROR
- ACKNOWLEDGMENT ERROR
It does make little sense to me to just have a bit for the last error in
the CAN id (and not the others as well). They all below to CAN_ERR_BUSERROR.
> These bits are only enablers for the additional information in data[..] :
>
> #define CAN_ERR_LOSTARB 0x00000002U /* lost arbitration / data[0] */
> #define CAN_ERR_CRTL 0x00000004U /* controller problems / data[1] */
> #define CAN_ERR_PROT 0x00000008U /* protocol violations / data[2..3] */
> #define CAN_ERR_TRX 0x00000010U /* transceiver status / data[4] */
The problem we have is that the errors are mainly derived from the
reporting capabilities of the SJA1000. Then we added some other flags
for errors from other controllers. There are other inconsistencies.
We do not have a bit for CRC errors. Therefore the at91_can.c just sets:
if (reg_sr & AT91_IRQ_CERR) {
...
cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
}
and in ti_hecc.c I find:
if (err_status & HECC_CANES_CRCE) {
hecc_set_bit(priv, HECC_CANES, HECC_CANES_CRCE);
cf->data[2] |= CAN_ERR_PROT_LOC_CRC_SEQ |
CAN_ERR_PROT_LOC_CRC_DEL;
}
It uses the data[3] fields (data[2] is wrong). And furthermore I find:
if (err_status & HECC_CANES_ACKE) {
hecc_set_bit(priv, HECC_CANES, HECC_CANES_ACKE);
cf->data[2] |= CAN_ERR_PROT_LOC_ACK |
CAN_ERR_PROT_LOC_ACK_DEL;
}
which is the mapping for the CAN_ERR_ACK. And in flexcan.c I find
another coding for the CRC error:
if (reg_esr & FLEXCAN_ESR_CRC_ERR) {
dev_dbg(dev->dev.parent, "CRC_ERR irq\n");
cf->data[2] |= CAN_ERR_PROT_BIT;
cf->data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ;
rx_errors = 1;
}
Puh, what a mess. OK, I think what we need is:
/* error in CAN protocol (type) / data[2] */
#define CAN_ERR_PROT_UNSPEC 0x00 /* unspecified */
#define CAN_ERR_PROT_BIT 0x01 /* single bit error */
#define CAN_ERR_PROT_FORM 0x02 /* frame format error */
#define CAN_ERR_PROT_STUFF 0x04 /* bit stuffing error */
#define CAN_ERR_PROT_BIT0 0x08 /* unable to send dominant bit */
#define CAN_ERR_PROT_BIT1 0x10 /* unable to send recessive bit */
#define CAN_ERR_PROT_CRC 0x20 /* crc error */
#define CAN_ERR_PROT_ACK 0x40 /* active error announcement */
#define CAN_ERR_PROT_TX 0x80 /* error occured on transmission
Apart from CAN_ERR_PROT_ACTIVE I removed CAN_ERR_PROT_OVERLOAD which is
*not* used in any driver and added the missing two bus error types
CAN_ERR_PROT_CRC, CAN_ERR_PROT_ACK. For BIT errors we have three bits
and in principle we could set (CAN_ERR_PROT_BIT0 ¦ CAN_ERR_PROT_BIT1)
for CAN controllers not distinguishing BIT0 and BIT1 errors. data[3]
would only be filled by the SJA1000. CAN_ERR_PROT and CAN_ERR_BUSERROR
are more or less redundant. It's getting more and more clear now what to do.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
