Inconsistent error state transition error frames

Inconsistent error state transition error frames

[Andri Yngvason]

Hello,

I've been working on a userspace program to monitor the CAN bus and I found that it only works as expected on one platform (flexcan). The problem is that on most other platforms, no error frame is emitted when the controller transitions back to error-active state. I see that you've already discussed this on this list here: http://thread.gmane.org/gmane.linux.can/201 

Why hasn't this been resolved?

I already patched the mscan and sja1000 drivers myself, and I was considering submitting them, but since you've already done some work on this (which I believe is superior to mine), my work is unlikely to be of any consequence to this community, except to show that this inconsistency does indeed result in wasted effort and frustration.

Of course, I'll help out if I can, e.g. by testing on the platforms that I have available.

Best regards,
Andri Yngvason

Re: Inconsistent error state transition error frames

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1463 bytes --]

On 09/08/2014 06:26 PM, Andri Yngvason wrote:
> I've been working on a userspace program to monitor the CAN bus and I
> found that it only works as expected on one platform (flexcan). The
> problem is that on most other platforms, no error frame is emitted
> when the controller transitions back to error-active state. I see
> that you've already discussed this on this list here:
> http://thread.gmane.org/gmane.linux.can/201
>
> Why hasn't this been resolved?

I assume no one needed this feature so far, I assume. And/or cared not
enough to upstream these patches.

> I already patched the mscan and sja1000 drivers myself, and I was
> considering submitting them, but since you've already done some work
> on this (which I believe is superior to mine), my work is unlikely to

Wolfgang mentioned in this mail, that he had some patches back than. Do
you still have them, Wolfgang?

> be of any consequence to this community, except to show that this
> inconsistency does indeed result in wasted effort and frustration.

Feel free to post your patches.

> Of course, I'll help out if I can, e.g. by testing on the platforms
> that I have available.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: Inconsistent error state transition error frames

[Wolfgang Grandegger]

On Mon, 08 Sep 2014 18:36:38 +0200, Marc Kleine-Budde <mkl@pengutronix.de>
wrote:
> On 09/08/2014 06:26 PM, Andri Yngvason wrote:
>> I've been working on a userspace program to monitor the CAN bus and I
>> found that it only works as expected on one platform (flexcan). The
>> problem is that on most other platforms, no error frame is emitted
>> when the controller transitions back to error-active state. I see

Also the Flexcan implementation does not correctly change the error
states backwards, IIRC.

>> that you've already discussed this on this list here:
>> http://thread.gmane.org/gmane.linux.can/201
>>
>> Why hasn't this been resolved?
> 
> I assume no one needed this feature so far, I assume. And/or cared not
> enough to upstream these patches.

Yep, little time left for free software development, lack of hardware,
etc. Somebody needs to care and post patches. Anyway, that's a good
occasion to start a migration path, maybe first for the Flexcan and
SJA1000 and MSCAN.

>> I already patched the mscan and sja1000 drivers myself, and I was
>> considering submitting them, but since you've already done some work
>> on this (which I believe is superior to mine), my work is unlikely to
> 
> Wolfgang mentioned in this mail, that he had some patches back than. Do
> you still have them, Wolfgang?

The patches to consolidate bus-off and error-state handling are still
at:

https://gitorious.org/linux-can/wg-linux-can-next/commits/eec921ac28fde243456078a557768808d93d94a3
https://gitorious.org/linux-can/wg-linux-can-next/commit/825f9bcb961aa193bacb9af6fb0d8491ee8fd02e

They introduce the can_id flag CAN_ERR_STATE_CHANGE and the data[1] flag
CAN_ERR_CRTL_ACTIVE, which I think would simplify migration. Also
there is now a common function can_change_state() which should make
the driver code more readable.

>> be of any consequence to this community, except to show that this
>> inconsistency does indeed result in wasted effort and frustration.
> 
> Feel free to post your patches.
> 
>> Of course, I'll help out if I can, e.g. by testing on the platforms
>> that I have available.

That would be very useful. Currently I do not have any CAN hardware at
hand.

Wolfgang.

Re: Inconsistent error state transition error frames

[Andri Yngvason]

On þri 9.sep 2014 10:48, Wolfgang Grandegger wrote:
> On Mon, 08 Sep 2014 18:36:38 +0200, Marc Kleine-Budde <mkl@pengutronix.de>
> wrote:
>> On 09/08/2014 06:26 PM, Andri Yngvason wrote:
>>> I've been working on a userspace program to monitor the CAN bus and I
>>> found that it only works as expected on one platform (flexcan). The
>>> problem is that on most other platforms, no error frame is emitted
>>> when the controller transitions back to error-active state. I see
> Also the Flexcan implementation does not correctly change the error
> states backwards, IIRC.
It does report the state changes backwards, but it uses 
CAN_ERR_PROT_ACTIVE for error-active state, which does have "ACTIVE" in 
the name, but it's probably originally intended for something else and 
isn't at all consistent with other error state can frames.
>>> that you've already discussed this on this list here:
>>> http://thread.gmane.org/gmane.linux.can/201
>>>
>>> Why hasn't this been resolved?
>> I assume no one needed this feature so far, I assume. And/or cared not
>> enough to upstream these patches.
> Yep, little time left for free software development, lack of hardware,
> etc. Somebody needs to care and post patches. Anyway, that's a good
> occasion to start a migration path, maybe first for the Flexcan and
> SJA1000 and MSCAN.
>
>>> I already patched the mscan and sja1000 drivers myself, and I was
>>> considering submitting them, but since you've already done some work
>>> on this (which I believe is superior to mine), my work is unlikely to
>> Wolfgang mentioned in this mail, that he had some patches back than. Do
>> you still have them, Wolfgang?
> The patches to consolidate bus-off and error-state handling are still
> at:
>
> https://gitorious.org/linux-can/wg-linux-can-next/commits/eec921ac28fde243456078a557768808d93d94a3
> https://gitorious.org/linux-can/wg-linux-can-next/commit/825f9bcb961aa193bacb9af6fb0d8491ee8fd02e
>
> They introduce the can_id flag CAN_ERR_STATE_CHANGE and the data[1] flag
> CAN_ERR_CRTL_ACTIVE, which I think would simplify migration. Also
> there is now a common function can_change_state() which should make
> the driver code more readable.
Presumably, we'll need to rebase these changes. Would you like to do 
that or would you like me to do it and post the patches? Admittedly, I 
haven't posted patches to the kernel before, but I've read the 
documentation and hopefully I'll get it right. ;)
>>> be of any consequence to this community, except to show that this
>>> inconsistency does indeed result in wasted effort and frustration.
>> Feel free to post your patches.
>>
>>> Of course, I'll help out if I can, e.g. by testing on the platforms
>>> that I have available.
> That would be very useful. Currently I do not have any CAN hardware at
> hand.
One last thing: The canutils project assumes that the flexcan way of 
error-state reporting is correct, but that might break if we apply these 
changes, so I guess that we'll have to change that too.

Andri.

Re: Inconsistent error state transition error frames

[Wolfgang Grandegger]

On Tue, 9 Sep 2014 11:59:28 +0000, Andri Yngvason
<andri.yngvason@marel.com> wrote:
> On þri 9.sep 2014 10:48, Wolfgang Grandegger wrote:
>> On Mon, 08 Sep 2014 18:36:38 +0200, Marc Kleine-Budde
>> <mkl@pengutronix.de>
>> wrote:
>>> On 09/08/2014 06:26 PM, Andri Yngvason wrote:
>>>> I've been working on a userspace program to monitor the CAN bus and I
>>>> found that it only works as expected on one platform (flexcan). The
>>>> problem is that on most other platforms, no error frame is emitted
>>>> when the controller transitions back to error-active state. I see
>> Also the Flexcan implementation does not correctly change the error
>> states backwards, IIRC.
> It does report the state changes backwards, but it uses 
> CAN_ERR_PROT_ACTIVE for error-active state, which does have "ACTIVE" in 
> the name, but it's probably originally intended for something else and 
> isn't at all consistent with other error state can frames.

Yes, the "error in CAN protocol" flag was misused somehow. It's
misleading,
at least.

>>>> that you've already discussed this on this list here:
>>>> http://thread.gmane.org/gmane.linux.can/201
>>>>
>>>> Why hasn't this been resolved?
>>> I assume no one needed this feature so far, I assume. And/or cared not
>>> enough to upstream these patches.
>> Yep, little time left for free software development, lack of hardware,
>> etc. Somebody needs to care and post patches. Anyway, that's a good
>> occasion to start a migration path, maybe first for the Flexcan and
>> SJA1000 and MSCAN.
>>
>>>> I already patched the mscan and sja1000 drivers myself, and I was
>>>> considering submitting them, but since you've already done some work
>>>> on this (which I believe is superior to mine), my work is unlikely to
>>> Wolfgang mentioned in this mail, that he had some patches back than.
Do
>>> you still have them, Wolfgang?
>> The patches to consolidate bus-off and error-state handling are still
>> at:
>>
>>
https://gitorious.org/linux-can/wg-linux-can-next/commits/eec921ac28fde243456078a557768808d93d94a3
>>
https://gitorious.org/linux-can/wg-linux-can-next/commit/825f9bcb961aa193bacb9af6fb0d8491ee8fd02e
>>
>> They introduce the can_id flag CAN_ERR_STATE_CHANGE and the data[1]
flag
>> CAN_ERR_CRTL_ACTIVE, which I think would simplify migration. Also
>> there is now a common function can_change_state() which should make
>> the driver code more readable.
> Presumably, we'll need to rebase these changes. Would you like to do 
> that or would you like me to do it and post the patches? Admittedly, I 

Would be really nice if you could find time. At least it would be faster
and more efficient (as I do not have hardware at hand). For an RFC patch
to consolidate the error-state handling the generic part and one or two
drivers using it would just be fine. Proper bus-off handling could be
addressed separately.

> haven't posted patches to the kernel before, but I've read the 
> documentation and hopefully I'll get it right. ;)

Don't worry. You will learn it quickly.

>>>> be of any consequence to this community, except to show that this
>>>> inconsistency does indeed result in wasted effort and frustration.
>>> Feel free to post your patches.
>>>
>>>> Of course, I'll help out if I can, e.g. by testing on the platforms
>>>> that I have available.
>> That would be very useful. Currently I do not have any CAN hardware at
>> hand.
> One last thing: The canutils project assumes that the flexcan way of 
> error-state reporting is correct, but that might break if we apply these

> changes, so I guess that we'll have to change that too.

Yes, can-utils needs to be updated as well and maybe we even need some
kind of version number or flag in the CAN error messages to act properly.

Thanks,

Wolfgang.

Re: Inconsistent error state transition error frames

[Andri Yngvason]

On þri 9.sep 2014 13:53, Wolfgang Grandegger wrote:
> On Tue, 9 Sep 2014 11:59:28 +0000, Andri Yngvason
> <andri.yngvason@marel.com> wrote:
>> On þri 9.sep 2014 10:48, Wolfgang Grandegger wrote:
>>> On Mon, 08 Sep 2014 18:36:38 +0200, Marc Kleine-Budde
>>> <mkl@pengutronix.de>
>>> wrote:
>>>> On 09/08/2014 06:26 PM, Andri Yngvason wrote:
>>>>> I've been working on a userspace program to monitor the CAN bus and I
>>>>> found that it only works as expected on one platform (flexcan). The
>>>>> problem is that on most other platforms, no error frame is emitted
>>>>> when the controller transitions back to error-active state. I see
>>> Also the Flexcan implementation does not correctly change the error
>>> states backwards, IIRC.
>> It does report the state changes backwards, but it uses
>> CAN_ERR_PROT_ACTIVE for error-active state, which does have "ACTIVE" in
>> the name, but it's probably originally intended for something else and
>> isn't at all consistent with other error state can frames.
> Yes, the "error in CAN protocol" flag was misused somehow. It's
> misleading,
> at least.
>
>>>>> that you've already discussed this on this list here:
>>>>> http://thread.gmane.org/gmane.linux.can/201
>>>>>
>>>>> Why hasn't this been resolved?
>>>> I assume no one needed this feature so far, I assume. And/or cared not
>>>> enough to upstream these patches.
>>> Yep, little time left for free software development, lack of hardware,
>>> etc. Somebody needs to care and post patches. Anyway, that's a good
>>> occasion to start a migration path, maybe first for the Flexcan and
>>> SJA1000 and MSCAN.
>>>
>>>>> I already patched the mscan and sja1000 drivers myself, and I was
>>>>> considering submitting them, but since you've already done some work
>>>>> on this (which I believe is superior to mine), my work is unlikely to
>>>> Wolfgang mentioned in this mail, that he had some patches back than.
> Do
>>>> you still have them, Wolfgang?
>>> The patches to consolidate bus-off and error-state handling are still
>>> at:
>>>
>>>
> https://gitorious.org/linux-can/wg-linux-can-next/commits/eec921ac28fde243456078a557768808d93d94a3
> https://gitorious.org/linux-can/wg-linux-can-next/commit/825f9bcb961aa193bacb9af6fb0d8491ee8fd02e
>>> They introduce the can_id flag CAN_ERR_STATE_CHANGE and the data[1]
> flag
>>> CAN_ERR_CRTL_ACTIVE, which I think would simplify migration. Also
>>> there is now a common function can_change_state() which should make
>>> the driver code more readable.
>> Presumably, we'll need to rebase these changes. Would you like to do
>> that or would you like me to do it and post the patches? Admittedly, I
> Would be really nice if you could find time. At least it would be faster
> and more efficient (as I do not have hardware at hand). For an RFC patch
> to consolidate the error-state handling the generic part and one or two
> drivers using it would just be fine. Proper bus-off handling could be
> addressed separately.
>
>> haven't posted patches to the kernel before, but I've read the
>> documentation and hopefully I'll get it right. ;)
> Don't worry. You will learn it quickly.
Thanks for the reassurance. Which branch should I rebase against, 
linux-can or linux-can-next?
>>>>> be of any consequence to this community, except to show that this
>>>>> inconsistency does indeed result in wasted effort and frustration.
>>>> Feel free to post your patches.
>>>>
>>>>> Of course, I'll help out if I can, e.g. by testing on the platforms
>>>>> that I have available.
>>> That would be very useful. Currently I do not have any CAN hardware at
>>> hand.
>> One last thing: The canutils project assumes that the flexcan way of
>> error-state reporting is correct, but that might break if we apply these
>> changes, so I guess that we'll have to change that too.
> Yes, can-utils needs to be updated as well and maybe we even need some
> kind of version number or flag in the CAN error messages to act properly.
>
>
It seems that 'data' is fully occupied, although 'data[5..7]' isn't 
reserved for anything specific but rather "controller specific 
additional information". Could we maybe put the version into data 5, 6 or 7?

Another option is to place it into the 3 or 4 msb of the id; something 
like this:
#define CAN_ERR_VERSION_MASK 0x1E000000U  // Note: CAN_ERR_MASK is 
0x1FFFFFFFU

Are 15 possible versions enough (or even too much)?

Andri.

Re: Inconsistent error state transition error frames

[Wolfgang Grandegger]

On Tue, 9 Sep 2014 14:49:58 +0000, Andri Yngvason
<andri.yngvason@marel.com> wrote:
> On þri 9.sep 2014 13:53, Wolfgang Grandegger wrote:
>> On Tue, 9 Sep 2014 11:59:28 +0000, Andri Yngvason
>> <andri.yngvason@marel.com> wrote:
>>> On þri 9.sep 2014 10:48, Wolfgang Grandegger wrote:
...
>>> haven't posted patches to the kernel before, but I've read the
>>> documentation and hopefully I'll get it right. ;)
>> Don't worry. You will learn it quickly.
> Thanks for the reassurance. Which branch should I rebase against, 
> linux-can or linux-can-next?

linux-can-next please.

...
> It seems that 'data' is fully occupied, although 'data[5..7]' isn't 
> reserved for anything specific but rather "controller specific 
> additional information". Could we maybe put the version into data 5, 6
or
> 7?

data[6..7] is used for the tx and rx error counts.
 
> Another option is to place it into the 3 or 4 msb of the id; something 
> like this:
> #define CAN_ERR_VERSION_MASK 0x1E000000U  // Note: CAN_ERR_MASK is 
> 0x1FFFFFFFU
> 
> Are 15 possible versions enough (or even too much)?

I would prefer an unsued field otherwise old can-util tools will report
rubbish. data[5] sounds good. But we should add a version number only
if we really need it.

Wolfgang.

Re: Inconsistent error state transition error frames

[Oliver Hartkopp]

On 09.09.2014 17:41, Wolfgang Grandegger wrote:
> On Tue, 9 Sep 2014 14:49:58 +0000, Andri Yngvason
> <andri.yngvason@marel.com> wrote:


>> It seems that 'data' is fully occupied, although 'data[5..7]' isn't 
>> reserved for anything specific but rather "controller specific 
>> additional information". Could we maybe put the version into data 5, 6
> or
>> 7?
> 
> data[6..7] is used for the tx and rx error counts.
>  

Oh yes. This is currently not documented in error.h :-(

What about adding some defines for it, like this:

diff --git a/include/uapi/linux/can/error.h b/include/uapi/linux/can/error.h
index c247446..c290f30 100644
--- a/include/uapi/linux/can/error.h
+++ b/include/uapi/linux/can/error.h
@@ -58,10 +58,12 @@
 #define CAN_ERR_RESTARTED    0x00000100U /* controller restarted */
 
 /* arbitration lost in bit ... / data[0] */
+#define CAN_ERR_LOSTARB_BYTE 0
 #define CAN_ERR_LOSTARB_UNSPEC   0x00 /* unspecified */
 				      /* else bit number in bitstream */
 
 /* error status of CAN-controller / data[1] */
+#define CAN_ERR_CRTL_BYTE 1
 #define CAN_ERR_CRTL_UNSPEC      0x00 /* unspecified */
 #define CAN_ERR_CRTL_RX_OVERFLOW 0x01 /* RX buffer overflow */
 #define CAN_ERR_CRTL_TX_OVERFLOW 0x02 /* TX buffer overflow */
@@ -73,6 +75,7 @@
 				      /* the protocol-defined level of 127)  */
 
 /* error in CAN protocol (type) / data[2] */
+#define CAN_ERR_PROT_BYTE 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 */
@@ -84,6 +87,7 @@
 #define CAN_ERR_PROT_TX          0x80 /* error occurred on transmission */
 
 /* error in CAN protocol (location) / data[3] */
+#define CAN_ERR_PROT_LOC_BYTE 3
 #define CAN_ERR_PROT_LOC_UNSPEC  0x00 /* unspecified */
 #define CAN_ERR_PROT_LOC_SOF     0x03 /* start of frame */
 #define CAN_ERR_PROT_LOC_ID28_21 0x02 /* ID bits 28 - 21 (SFF: 10 - 3) */
@@ -106,6 +110,7 @@
 #define CAN_ERR_PROT_LOC_INTERM  0x12 /* intermission */
 
 /* error status of CAN-transceiver / data[4] */
+#define CAN_ERR_TRX_BYTE 4
 /*                                             CANH CANL */
 #define CAN_ERR_TRX_UNSPEC             0x00 /* 0000 0000 */
 #define CAN_ERR_TRX_CANH_NO_WIRE       0x04 /* 0000 0100 */
@@ -118,6 +123,10 @@
 #define CAN_ERR_TRX_CANL_SHORT_TO_GND  0x70 /* 0111 0000 */
 #define CAN_ERR_TRX_CANL_SHORT_TO_CANH 0x80 /* 1000 0000 */
 
-/* controller specific additional information / data[5..7] */
+/* controller specific additional information / data[5] */
+
+/* controller error counter values / data[6..7] */
+#define CAN_ERR_TXERR_BYTE 6
+#define CAN_ERR_RXERR_BYTE 7
 
 #endif /* _UAPI_CAN_ERROR_H */


>> Another option is to place it into the 3 or 4 msb of the id; something 
>> like this:
>> #define CAN_ERR_VERSION_MASK 0x1E000000U  // Note: CAN_ERR_MASK is 
>> 0x1FFFFFFFU
>>
>> Are 15 possible versions enough (or even too much)?
> 
> I would prefer an unsued field otherwise old can-util tools will report
> rubbish. data[5] sounds good. But we should add a version number only
> if we really need it.

Yes.

As the current situation needs a re-work and is somehow broken I wonder if
the current implementation is really used widely by that many people ?!?

As you introduce a new bit we should check if there is really that much
fallout that a version info becomes necessary.

Maybe be we can change the definition and only update the can-utils accordingly.

Or do you see any general problem for the suggested changes?

Regards,
Oliver

Re: Inconsistent error state transition error frames

[Andri Yngvason]

On þri 9.sep 2014 18:08, Oliver Hartkopp wrote:
>
> On 09.09.2014 17:41, Wolfgang Grandegger wrote:
>> On Tue, 9 Sep 2014 14:49:58 +0000, Andri Yngvason
>> <andri.yngvason@marel.com> wrote:
>
>>> It seems that 'data' is fully occupied, although 'data[5..7]' isn't
>>> reserved for anything specific but rather "controller specific
>>> additional information". Could we maybe put the version into data 5, 6
>> or
>>> 7?
>> data[6..7] is used for the tx and rx error counts.
>>   
> Oh yes. This is currently not documented in error.h :-(
>
> What about adding some defines for it, like this:
>
> diff --git a/include/uapi/linux/can/error.h b/include/uapi/linux/can/error.h
> index c247446..c290f30 100644
> --- a/include/uapi/linux/can/error.h
> +++ b/include/uapi/linux/can/error.h
> @@ -58,10 +58,12 @@
>   #define CAN_ERR_RESTARTED    0x00000100U /* controller restarted */
>   
>   /* arbitration lost in bit ... / data[0] */
> +#define CAN_ERR_LOSTARB_BYTE 0
>   #define CAN_ERR_LOSTARB_UNSPEC   0x00 /* unspecified */
>   				      /* else bit number in bitstream */
>   
>   /* error status of CAN-controller / data[1] */
> +#define CAN_ERR_CRTL_BYTE 1
>   #define CAN_ERR_CRTL_UNSPEC      0x00 /* unspecified */
>   #define CAN_ERR_CRTL_RX_OVERFLOW 0x01 /* RX buffer overflow */
>   #define CAN_ERR_CRTL_TX_OVERFLOW 0x02 /* TX buffer overflow */
> @@ -73,6 +75,7 @@
>   				      /* the protocol-defined level of 127)  */
>   
>   /* error in CAN protocol (type) / data[2] */
> +#define CAN_ERR_PROT_BYTE 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 */
> @@ -84,6 +87,7 @@
>   #define CAN_ERR_PROT_TX          0x80 /* error occurred on transmission */
>   
>   /* error in CAN protocol (location) / data[3] */
> +#define CAN_ERR_PROT_LOC_BYTE 3
>   #define CAN_ERR_PROT_LOC_UNSPEC  0x00 /* unspecified */
>   #define CAN_ERR_PROT_LOC_SOF     0x03 /* start of frame */
>   #define CAN_ERR_PROT_LOC_ID28_21 0x02 /* ID bits 28 - 21 (SFF: 10 - 3) */
> @@ -106,6 +110,7 @@
>   #define CAN_ERR_PROT_LOC_INTERM  0x12 /* intermission */
>   
>   /* error status of CAN-transceiver / data[4] */
> +#define CAN_ERR_TRX_BYTE 4
>   /*                                             CANH CANL */
>   #define CAN_ERR_TRX_UNSPEC             0x00 /* 0000 0000 */
>   #define CAN_ERR_TRX_CANH_NO_WIRE       0x04 /* 0000 0100 */
> @@ -118,6 +123,10 @@
>   #define CAN_ERR_TRX_CANL_SHORT_TO_GND  0x70 /* 0111 0000 */
>   #define CAN_ERR_TRX_CANL_SHORT_TO_CANH 0x80 /* 1000 0000 */
>   
> -/* controller specific additional information / data[5..7] */
> +/* controller specific additional information / data[5] */
> +
> +/* controller error counter values / data[6..7] */
> +#define CAN_ERR_TXERR_BYTE 6
> +#define CAN_ERR_RXERR_BYTE 7
>   
>   #endif /* _UAPI_CAN_ERROR_H */
>
I'm not sure what's considered good practice in kernel code but wouldn't 
it be better to use a macro like this instead?
     #define CAN_ERR_CRTL_DATA(frame) ((frame)->data[1])
which would be used like this:
     if(CAN_ERR_CRTL_DATA(frame) & CAN_ERR_CRTL_whatever) ...

This looks a little nicer and reads better than 
"frame->data[CAN_ERR_CRTL_BYTE]".
>>> Another option is to place it into the 3 or 4 msb of the id; something
>>> like this:
>>> #define CAN_ERR_VERSION_MASK 0x1E000000U  // Note: CAN_ERR_MASK is
>>> 0x1FFFFFFFU
>>>
>>> Are 15 possible versions enough (or even too much)?
>> I would prefer an unsued field otherwise old can-util tools will report
>> rubbish. data[5] sounds good. But we should add a version number only
>> if we really need it.
> Yes.
>
> As the current situation needs a re-work and is somehow broken I wonder if
> the current implementation is really used widely by that many people ?!?
>
> As you introduce a new bit we should check if there is really that much
> fallout that a version info becomes necessary.
>
> Maybe be we can change the definition and only update the can-utils accordingly.
>
> Or do you see any general problem for the suggested changes?
>
I don't actually see a problem. This doesn't even work the same way for 
all protocols, so existing code that monitors error state changes can be 
considered unreliable at best.

Re: Inconsistent error state transition error frames

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3193 bytes --]

On 09/10/2014 12:19 PM, Andri Yngvason wrote:
>> diff --git a/include/uapi/linux/can/error.h
>> b/include/uapi/linux/can/error.h
>> index c247446..c290f30 100644
>> --- a/include/uapi/linux/can/error.h
>> +++ b/include/uapi/linux/can/error.h
>> @@ -58,10 +58,12 @@
>>   #define CAN_ERR_RESTARTED    0x00000100U /* controller restarted */
>>     /* arbitration lost in bit ... / data[0] */
>> +#define CAN_ERR_LOSTARB_BYTE 0
>>   #define CAN_ERR_LOSTARB_UNSPEC   0x00 /* unspecified */
>>                         /* else bit number in bitstream */
>>     /* error status of CAN-controller / data[1] */
>> +#define CAN_ERR_CRTL_BYTE 1
>>   #define CAN_ERR_CRTL_UNSPEC      0x00 /* unspecified */
>>   #define CAN_ERR_CRTL_RX_OVERFLOW 0x01 /* RX buffer overflow */
>>   #define CAN_ERR_CRTL_TX_OVERFLOW 0x02 /* TX buffer overflow */
>> @@ -73,6 +75,7 @@
>>                         /* the protocol-defined level of 127)  */
>>     /* error in CAN protocol (type) / data[2] */
>> +#define CAN_ERR_PROT_BYTE 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 */
>> @@ -84,6 +87,7 @@
>>   #define CAN_ERR_PROT_TX          0x80 /* error occurred on
>> transmission */
>>     /* error in CAN protocol (location) / data[3] */
>> +#define CAN_ERR_PROT_LOC_BYTE 3
>>   #define CAN_ERR_PROT_LOC_UNSPEC  0x00 /* unspecified */
>>   #define CAN_ERR_PROT_LOC_SOF     0x03 /* start of frame */
>>   #define CAN_ERR_PROT_LOC_ID28_21 0x02 /* ID bits 28 - 21 (SFF: 10 -
>> 3) */
>> @@ -106,6 +110,7 @@
>>   #define CAN_ERR_PROT_LOC_INTERM  0x12 /* intermission */
>>     /* error status of CAN-transceiver / data[4] */
>> +#define CAN_ERR_TRX_BYTE 4
>>   /*                                             CANH CANL */
>>   #define CAN_ERR_TRX_UNSPEC             0x00 /* 0000 0000 */
>>   #define CAN_ERR_TRX_CANH_NO_WIRE       0x04 /* 0000 0100 */
>> @@ -118,6 +123,10 @@
>>   #define CAN_ERR_TRX_CANL_SHORT_TO_GND  0x70 /* 0111 0000 */
>>   #define CAN_ERR_TRX_CANL_SHORT_TO_CANH 0x80 /* 1000 0000 */
>>   -/* controller specific additional information / data[5..7] */
>> +/* controller specific additional information / data[5] */
>> +
>> +/* controller error counter values / data[6..7] */
>> +#define CAN_ERR_TXERR_BYTE 6
>> +#define CAN_ERR_RXERR_BYTE 7
>>     #endif /* _UAPI_CAN_ERROR_H */
>>
> I'm not sure what's considered good practice in kernel code but wouldn't
> it be better to use a macro like this instead?
>     #define CAN_ERR_CRTL_DATA(frame) ((frame)->data[1])
> which would be used like this:
>     if(CAN_ERR_CRTL_DATA(frame) & CAN_ERR_CRTL_whatever) ...

I'm not sure....but...

Make it a static inline function and give it a proper name, something
like can_err_frame_get_ctrl(). Maybe without err_....

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: Inconsistent error state transition error frames

[Wolfgang Grandegger]

On Wed, 10 Sep 2014 12:29:23 +0200, Marc Kleine-Budde <mkl@pengutronix.de>
wrote:
> On 09/10/2014 12:19 PM, Andri Yngvason wrote:
>>> diff --git a/include/uapi/linux/can/error.h
>>> b/include/uapi/linux/can/error.h
>>> index c247446..c290f30 100644
>>> --- a/include/uapi/linux/can/error.h
>>> +++ b/include/uapi/linux/can/error.h
>>> @@ -58,10 +58,12 @@
>>>   #define CAN_ERR_RESTARTED    0x00000100U /* controller restarted */
>>>     /* arbitration lost in bit ... / data[0] */
>>> +#define CAN_ERR_LOSTARB_BYTE 0
>>>   #define CAN_ERR_LOSTARB_UNSPEC   0x00 /* unspecified */
>>>                         /* else bit number in bitstream */
>>>     /* error status of CAN-controller / data[1] */
>>> +#define CAN_ERR_CRTL_BYTE 1
>>>   #define CAN_ERR_CRTL_UNSPEC      0x00 /* unspecified */
>>>   #define CAN_ERR_CRTL_RX_OVERFLOW 0x01 /* RX buffer overflow */
>>>   #define CAN_ERR_CRTL_TX_OVERFLOW 0x02 /* TX buffer overflow */
>>> @@ -73,6 +75,7 @@
>>>                         /* the protocol-defined level of 127)  */
>>>     /* error in CAN protocol (type) / data[2] */
>>> +#define CAN_ERR_PROT_BYTE 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 */
>>> @@ -84,6 +87,7 @@
>>>   #define CAN_ERR_PROT_TX          0x80 /* error occurred on
>>> transmission */
>>>     /* error in CAN protocol (location) / data[3] */
>>> +#define CAN_ERR_PROT_LOC_BYTE 3
>>>   #define CAN_ERR_PROT_LOC_UNSPEC  0x00 /* unspecified */
>>>   #define CAN_ERR_PROT_LOC_SOF     0x03 /* start of frame */
>>>   #define CAN_ERR_PROT_LOC_ID28_21 0x02 /* ID bits 28 - 21 (SFF: 10 -
>>> 3) */
>>> @@ -106,6 +110,7 @@
>>>   #define CAN_ERR_PROT_LOC_INTERM  0x12 /* intermission */
>>>     /* error status of CAN-transceiver / data[4] */
>>> +#define CAN_ERR_TRX_BYTE 4
>>>   /*                                             CANH CANL */
>>>   #define CAN_ERR_TRX_UNSPEC             0x00 /* 0000 0000 */
>>>   #define CAN_ERR_TRX_CANH_NO_WIRE       0x04 /* 0000 0100 */
>>> @@ -118,6 +123,10 @@
>>>   #define CAN_ERR_TRX_CANL_SHORT_TO_GND  0x70 /* 0111 0000 */
>>>   #define CAN_ERR_TRX_CANL_SHORT_TO_CANH 0x80 /* 1000 0000 */
>>>   -/* controller specific additional information / data[5..7] */
>>> +/* controller specific additional information / data[5] */
>>> +
>>> +/* controller error counter values / data[6..7] */
>>> +#define CAN_ERR_TXERR_BYTE 6
>>> +#define CAN_ERR_RXERR_BYTE 7
>>>     #endif /* _UAPI_CAN_ERROR_H */
>>>
>> I'm not sure what's considered good practice in kernel code but
wouldn't
>> it be better to use a macro like this instead?
>>     #define CAN_ERR_CRTL_DATA(frame) ((frame)->data[1])
>> which would be used like this:
>>     if(CAN_ERR_CRTL_DATA(frame) & CAN_ERR_CRTL_whatever) ...
> 
> I'm not sure....but...
> 
> Make it a static inline function and give it a proper name, something
> like can_err_frame_get_ctrl(). Maybe without err_....

I'm not sure... we also need to set the value. Therefore I would
vote for:

 frame->data[CAN_ERR_TXERR_BYTE];

It does make the code more readable.

Wolfgang.

Re: Inconsistent error state transition error frames

[Oliver Hartkopp]

On 10.09.2014 13:53, Wolfgang Grandegger wrote:

>>>> +/* controller error counter values / data[6..7] */
>>>> +#define CAN_ERR_TXERR_BYTE 6
>>>> +#define CAN_ERR_RXERR_BYTE 7
>>>>     #endif /* _UAPI_CAN_ERROR_H */
>>>>
>>> I'm not sure what's considered good practice in kernel code but
> wouldn't
>>> it be better to use a macro like this instead?
>>>     #define CAN_ERR_CRTL_DATA(frame) ((frame)->data[1])
>>> which would be used like this:
>>>     if(CAN_ERR_CRTL_DATA(frame) & CAN_ERR_CRTL_whatever) ...
>>
>> I'm not sure....but...
>>
>> Make it a static inline function and give it a proper name, something
>> like can_err_frame_get_ctrl(). Maybe without err_....

Maybe without _frame ? :-)

Think of 

	frame->data[CAN_ERR_CTRL_BYTE] |= CAN_ERR_CRTL_RX_OVERFLOW;

How would this look like?

	can_err_frame_set_ctrl(frame) = can_err_frame_get_ctrl(frame) | CAN_ERR_CRTL_RX_OVERFLOW;

Hm. Not nice.

	CAN_ERR_CRTL_DATA(frame) |= CAN_ERR_CRTL_RX_OVERFLOW;

Is hard to understand at first sight.

> 
> I'm not sure... we also need to set the value. Therefore I would
> vote for:
> 
>  frame->data[CAN_ERR_TXERR_BYTE];
> 
> It does make the code more readable.

Unfortunately CAN_ERR_TXERR_BYTE is pretty long and I have no good idea
how to make it shorter. CAN_ERR_TXERR_IDX is probably more handy.

E.g.

	frame->data[CAN_ERR_CTRL_IDX] |= CAN_ERR_CRTL_RX_OVERFLOW;

I would vote for something like this too.

Should I send a patch for this approach?

Regards,
Oliver

Re: Inconsistent error state transition error frames

[Andri Yngvason]

On mið 10.sep 2014 19:08, Oliver Hartkopp wrote:
>
>> I'm not sure... we also need to set the value. Therefore I would
>> vote for:
>>
>>   frame->data[CAN_ERR_TXERR_BYTE];
>>
>> It does make the code more readable.
> Unfortunately CAN_ERR_TXERR_BYTE is pretty long and I have no good idea
> how to make it shorter. CAN_ERR_TXERR_IDX is probably more handy.
>
> E.g.
>
> 	frame->data[CAN_ERR_CTRL_IDX] |= CAN_ERR_CRTL_RX_OVERFLOW;
>
> I would vote for something like this too.
>
> Should I send a patch for this approach?
Don't most people have auto-completion in their editors nowadays? 
Anyway, I'd been thinking about suggesting 'INDEX' rather than 'BYTE' 
because it really is a more descriptive name, so I'm happy with 'IDX'. 
'BYTE' can mean other things while 'IDX' is pretty clear.

Andri