On 16.02.2011 21:04, Wolfgang Grandegger wrote:
> On 02/16/2011 08:00 PM, Oliver Hartkopp wrote:
>> On 16.02.2011 09:10, Wolfgang Grandegger wrote:
>>> But wrong termination may result in other errors as well and there I
>>> think it could not be used reliably for the purpose you describe above.
>>
>> Ok. But i think we could leave it as-is. I don't think it's worth to break
>> the
>> definition here.
>
> I prefer the clean solution. I don't think it's worth to keep it in the
> CAN id. It will just confuse users.
Is also OK for me.
Would you suggest to reuse the bit-value from CAN_ERR_ACK (0x0020U) for
CAN_ERR_ACTIVE then? I've done this in my suggested patch below.
>> For that reason i would prefer to have the CAN_ERR_ACTIVE bit *only* in the
>> CAN-ID status bits and not in the detailed data[1] (CAN_ERR_CTRL_ACTIVE) as
>> you and Kurt suggested.
>
> OK, but then I would drop CAN_ERR_CTRL_ACTIVE.
Yes, thanks. That was my intention.
>>> We then could or even should add a version information into the error
>>> message. I think data[5] could be used for that purpose.
>>
>> It's like having the CAN core version information where Dave was complaining
>> when it gone Mainline. Who should check version numbers and maintain all
>> this?
>
> This would allow users already using the error flags to easily migrate
> to the new and changed error types. My idea here is:
>
> cf->data[5] = CAN_ERR_VERSION;
>
> Also candump could use it to avoid displaying rubbish with old kernels.
Hm - maybe then it's better to retrieve the Kernel version with uname(2) ...
>
> Furthermore I would like to use:
>
> s/CAN_ERR_PROT_OVERLOAD/CAN_ERR_PROT_CRC/
>
> for the CRC errors.
ok.
>>
>> CAN_ERR_ACK(can_id) and CAN_ERR_PROT_ACK(data[2])
>>
>> are set together. But who cares?
>
> Me, see above.
ok.
>
>> CAN_ERR_BUSERROR(can_id) could be defined as the or-ed value from
>>
>> (CAN_ERR_LOSTARB | CAN_ERR_PROT | CAN_ERR_CRTL)
>
> What do you mean here. Currently CAN_ERR_BUSERROR behave like
> CAN_ERR_PROT but bus errors are *not* state changes or lostarb errors.
> In principle we could define:
>
> #define CAN_ERR_BUSERROR CAN_ERR_PROT
>
> But maybe we leave it as is to allowing for other protocol violations
> which are not classical bus errors.
ok.
So the final error.h patch would look like this then:
diff --git a/include/linux/can/error.h b/include/linux/can/error.h
index d4127fd..f9245e5 100644
--- a/include/linux/can/error.h
+++ b/include/linux/can/error.h
@@ -22,7 +22,7 @@
#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] */
-#define CAN_ERR_ACK 0x00000020U /* received no ACK on transmission */
+#define CAN_ERR_ACTIVE 0x00000020U /* controller (self) recovered */
#define CAN_ERR_BUSOFF 0x00000040U /* bus off */
#define CAN_ERR_BUSERROR 0x00000080U /* bus error (may flood!) */
#define CAN_ERR_RESTARTED 0x00000100U /* controller restarted */
@@ -49,8 +49,8 @@
#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_OVERLOAD 0x20 /* bus overload */
-#define CAN_ERR_PROT_ACTIVE 0x40 /* active error announcement */
+#define CAN_ERR_PROT_CRC 0x20 /* CRC error */
+#define CAN_ERR_PROT_ACK 0x40 /* received no ACK on transmission */
#define CAN_ERR_PROT_TX 0x80 /* error occured on transmission */
/* error in CAN protocol (location) / data[3] */
Right?
> I started writing the patch and realized that we need some more
> clarification and cleanup. Next point is how we increment the
> stats->[r|t]x_error counters? It's not done consistently and it's not
> even possible to assign the direction of bus errors properly. Have a
> look to the drivers to realizes the mess. Should we do that at all? We
> have our own stats of CAN errors:
>
> struct can_device_stats {
> __u32 bus_error; /* Bus errors */
> __u32 error_warning; /* Changes to error warning state */
> __u32 error_passive; /* Changes to error passive state */
> __u32 bus_off; /* Changes to bus off state */
> __u32 arbitration_lost; /* Arbitration lost errors */
> __u32 restarts; /* CAN controller re-starts */
> };
>
> I tend to drop touching the network stats completely. What do you and
> others think.
Using the common network stats would have the advantage, that you can easily
see problems with standard tools like 'ifconfig' or 'cat /proc/net/dev'
But maybe a cleanup is needed anyway ;-)
Regards,
Oliver
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
