Fwd: can-utils at http://gitorious.org/+linux-can-developers/linux-can/can-utils

Fwd: can-utils at http://gitorious.org/+linux-can-developers/linux-can/can-utils

[Marc Kleine-Budde]

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

Hey Håkan,

let's wait for Oliver to say something to the length. As far as I know
the canutils already support can-fd, which can send up to 64 bytes. I
don't know the standard for sure, but maybe they got rid of the RTR in
canfd.

Regarding the mailinglist, it's linux-can@vger.kernel.org (Cc'ed), but
that only accepts text (i.e. non HTML) emails.

regards, Marc

-------- Original Message --------
Subject: 	can-utils at
http://gitorious.org/+linux-can-developers/linux-can/can-utils
Date: 	Fri, 15 Feb 2013 11:07:29 +0100
From: 	Håkan Engblom <H.Engblom@tele-radio.com>
To: 	socketcan@hartkopp.net <socketcan@hartkopp.net>, wg@grandegger.com
<wg@grandegger.com>, mkl@pengutronix.de <mkl@pengutronix.de>,
urs@isnogud.escape.de <urs@isnogud.escape.de>



Hi,



Don’t know if it is OK to write to you directly, but I could not find
any mailing-list or anything else.



Anyway, I think I found a small bug in cansend, when sedning Remote
Frames. The syntax is (an example):



$ cansend can0 181#R



But according to the standard, the dlc field should be set to how much
data that is expected in the expected tpdo. So I did the change in the
attached patch, and the syntax now is



$ cansend can0 181#R8



to indicate that 8 bytes of process data is expected.



If you agree with the fix it would be nice to have it merged. The patch
is done with commit id 50775159276d896d8b3102b6dbc658a91a2a1d53 as base,
but I’ve checked in the latest lib.c in the master branch, and the code
looks the same there.



Br Håkan Engblom, Software designer, Tele-Radio AB



diff -ur a/lib.c b/lib.c

--- a/lib.c     2013-02-15 11:01:27.308944607 +0100

+++ b/lib.c     2013-02-15 11:04:25.104942729 +0100

@@ -141,6 +141,9 @@

        if((cs[idx] == 'R') || (cs[idx] == 'r')){ /* RTR frame */

                cf->can_id |= CAN_RTR_FLAG;

+               if ((tmp = asc2nibble(cs[++idx])) > 0x08) /* 8 is max dlc */

+                       return 1;

+               cf->can_dlc = tmp;

                return 0;

        }


-- 
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: 263 bytes --]

Re: can-utils at http://gitorious.org/+linux-can-developers/linux-can/can-utils

[Yegor Yefremov]

On Fri, Feb 15, 2013 at 11:44 AM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> Hey Håkan,
>
> let's wait for Oliver to say something to the length. As far as I know
> the canutils already support can-fd, which can send up to 64 bytes. I
> don't know the standard for sure, but maybe they got rid of the RTR in
> canfd.
>
> Regarding the mailinglist, it's linux-can@vger.kernel.org (Cc'ed), but
> that only accepts text (i.e. non HTML) emails.
>
> regards, Marc
>
> -------- Original Message --------
> Subject:        can-utils at
> http://gitorious.org/+linux-can-developers/linux-can/can-utils
> Date:   Fri, 15 Feb 2013 11:07:29 +0100
> From:   Håkan Engblom <H.Engblom@tele-radio.com>
> To:     socketcan@hartkopp.net <socketcan@hartkopp.net>, wg@grandegger.com
> <wg@grandegger.com>, mkl@pengutronix.de <mkl@pengutronix.de>,
> urs@isnogud.escape.de <urs@isnogud.escape.de>
>
>
>
> Hi,
>
>
>
> Don’t know if it is OK to write to you directly, but I could not find
> any mailing-list or anything else.

I've updated the CAN related page of the eLinux wiki:
http://elinux.org/CAN_Bus. Let me know what else can added here and
whether we could use this page as a central SocketCAN related
resources with HOWTO's etc.?

Yegor

Re: Fwd: can-utils at http://gitorious.org/+linux-can-developers/linux-can/can-utils

[Oliver Hartkopp]

Hello Hakan,

On 15.02.2013 11:44, Marc Kleine-Budde wrote:

> let's wait for Oliver to say something to the length. As far as I know
> the canutils already support can-fd, which can send up to 64 bytes. I
> don't know the standard for sure, but maybe they got rid of the RTR in
> canfd.


Yes. CAN FD does not support RTR.


> Anyway, I think I found a small bug in cansend, when sedning Remote
> Frames. The syntax is (an example):
> 
> 
> 
> $ cansend can0 181#R
> 
> 
> 
> But according to the standard, the dlc field should be set to how much
> data that is expected in the expected tpdo. So I did the change in the
> attached patch, and the syntax now is
> 
> 
> 
> $ cansend can0 181#R8
> 
> 
> 
> to indicate that 8 bytes of process data is expected.
> 


In general RTR are declared to be bad by several CAN related papers.

The CAN Spec states for RTR frames:

	There is no DATA FIELD, independent of the values of the DATA LENGTH
	CODE which may be signed any value within the admissible range 0...8.
	The value is the DATA LENGTH CODE of the corresponding DATA FRAME.

Which probably contains the problem WHY it is declared to be bad, as you would
need a DLC check in the CAN controller for RTR frames, etc.

Btw. you are right, that RTR frames can contain a DLC value: 0 .. 8 - as CAN
FD is not supported anyway.

Looking into the CAN drivers in linux/drivers/net/can it looks like that no
data is copied from the registers in the case of a received RTR frame. But it
also looks like that the can_dlc is set from the controller registers value in
a correct way. I need to doublecheck, if really all drivers handle the can_dlc
correctly in the case of RTR frames.

E.g. the CC770 handles the RTR frames with some local intelligence, and does
not even provide the CAN ID ... and the can_dlc is set to zero in any case:

http://lxr.linux.no/#linux+v3.7.8/drivers/net/can/cc770/cc770.c#L477

In general you idea is ok to support the can_dlc values in CAN 2.0B frames:

$ cansend can0 181#R8

BUT if we add this can_dlc option for RTR frames, it should be *optional* to
be able to ...

- indicate that the can_dlc was not valid
- be backwards compatible to older log files

The RTR stuff in conjunction with can_dlc is not always consistent.

Would this *optional* can_dlc value approach be fine to you?

> @@ -141,6 +141,9 @@
>         if((cs[idx] == 'R') || (cs[idx] == 'r')){ /* RTR frame */
>                 cf->can_id |= CAN_RTR_FLAG;
> +               if ((tmp = asc2nibble(cs[++idx])) > 0x08) /* 8 is max dlc */
> +                       return 1;


This error condition exit would be too hard for an optional value then.

> +               cf->can_dlc = tmp;
>                 return 0;
>         }



Regards,
Oliver

SV: Fwd: can-utils at http://gitorious.org/+linux-can-developers/linux-can/can-utils

[Håkan Engblom]

Hi,

Yes, the RTR is not recommended, you're right.

In any case it would be nice to have it there as I have a slave that discards remote frames unless the DLC is the same as in the requested data. 

To take away the error-check would be fine for me. Is something like this what you mean, just making sure I did not missunderstand you:

	if((cs[idx] == 'R') || (cs[idx] == 'r')){ /* RTR frame */
		cf->can_id |= CAN_RTR_FLAG;
		cf->can_dlc = 0;
		if(cs[++idx] && (tmp = asc2nibble(cs[idx])) <= 0x08) /* 8 is max dlc */
			cf->can_dlc = tmp;
		return 0;
	}


This would be perfictly fine with me, I've also done a quick test of this, and it seems to work. And it does not give any fault when node additional length argument is given.

Br Håkan Engblom

-----Ursprungligt meddelande-----
Från: Oliver Hartkopp [mailto:socketcan@hartkopp.net] 
Skickat: den 15 februari 2013 17:13
Till: Marc Kleine-Budde; Håkan Engblom
Kopia: linux-can@vger.kernel.org
Ämne: Re: Fwd: can-utils at http://gitorious.org/+linux-can-developers/linux-can/can-utils

Hello Hakan,

On 15.02.2013 11:44, Marc Kleine-Budde wrote:

> let's wait for Oliver to say something to the length. As far as I know 
> the canutils already support can-fd, which can send up to 64 bytes. I 
> don't know the standard for sure, but maybe they got rid of the RTR in 
> canfd.


Yes. CAN FD does not support RTR.


> Anyway, I think I found a small bug in cansend, when sedning Remote 
> Frames. The syntax is (an example):
> 
> 
> 
> $ cansend can0 181#R
> 
> 
> 
> But according to the standard, the dlc field should be set to how much 
> data that is expected in the expected tpdo. So I did the change in the 
> attached patch, and the syntax now is
> 
> 
> 
> $ cansend can0 181#R8
> 
> 
> 
> to indicate that 8 bytes of process data is expected.
> 


In general RTR are declared to be bad by several CAN related papers.

The CAN Spec states for RTR frames:

	There is no DATA FIELD, independent of the values of the DATA LENGTH
	CODE which may be signed any value within the admissible range 0...8.
	The value is the DATA LENGTH CODE of the corresponding DATA FRAME.

Which probably contains the problem WHY it is declared to be bad, as you would need a DLC check in the CAN controller for RTR frames, etc.

Btw. you are right, that RTR frames can contain a DLC value: 0 .. 8 - as CAN FD is not supported anyway.

Looking into the CAN drivers in linux/drivers/net/can it looks like that no data is copied from the registers in the case of a received RTR frame. But it also looks like that the can_dlc is set from the controller registers value in a correct way. I need to doublecheck, if really all drivers handle the can_dlc correctly in the case of RTR frames.

E.g. the CC770 handles the RTR frames with some local intelligence, and does not even provide the CAN ID ... and the can_dlc is set to zero in any case:

http://lxr.linux.no/#linux+v3.7.8/drivers/net/can/cc770/cc770.c#L477

In general you idea is ok to support the can_dlc values in CAN 2.0B frames:

$ cansend can0 181#R8

BUT if we add this can_dlc option for RTR frames, it should be *optional* to be able to ...

- indicate that the can_dlc was not valid
- be backwards compatible to older log files

The RTR stuff in conjunction with can_dlc is not always consistent.

Would this *optional* can_dlc value approach be fine to you?

> @@ -141,6 +141,9 @@
>         if((cs[idx] == 'R') || (cs[idx] == 'r')){ /* RTR frame */
>                 cf->can_id |= CAN_RTR_FLAG;
> +               if ((tmp = asc2nibble(cs[++idx])) > 0x08) /* 8 is max dlc */
> +                       return 1;


This error condition exit would be too hard for an optional value then.

> +               cf->can_dlc = tmp;
>                 return 0;
>         }



Regards,
Oliver

Re: SV: Fwd: can-utils at http://gitorious.org/+linux-can-developers/linux-can/can-utils

[Oliver Hartkopp]

On 15.02.2013 17:49, Håkan Engblom wrote:

> Yes, the RTR is not recommended, you're right.
> 
> In any case it would be nice to have it there as I have a slave that discards remote frames unless the DLC is the same as in the requested data. 
> 
> To take away the error-check would be fine for me. Is something like this what you mean, just making sure I did not missunderstand you:


Yes this is what i also thought of.

> 
> 	if((cs[idx] == 'R') || (cs[idx] == 'r')){ /* RTR frame */
> 		cf->can_id |= CAN_RTR_FLAG;
> 		cf->can_dlc = 0;


This is superfluous, as the struct can_frame is always initialized to zero.
 

> 		if(cs[++idx] && (tmp = asc2nibble(cs[idx])) <= 0x08) /* 8 is max dlc */
> 			cf->can_dlc = tmp;


The latest can-utils use struct canfd_frames => 'can_dlc' moved to 'len'

> 		return 0;
> 	}
> 
> 
> This would be perfictly fine with me, I've also done a quick test of this, and it seems to work. And it does not give any fault when node additional length argument is given.


Yep.

I wonder if it makes sense to the length 'len' too, before accessing
cs[++idx] ... but when cs[++idx] is zero your code is correct too.


Here's my suggestion:

If it's ok for you, i can add your Signed-off-by ...

diff --git a/lib.c b/lib.c
index 4857e80..c2baa77 100644
--- a/lib.c
+++ b/lib.c
@@ -169,6 +169,11 @@ int parse_canframe(char *cs, struct canfd_frame *cf) {
 
 	if((cs[idx] == 'R') || (cs[idx] == 'r')){ /* RTR frame */
 		cf->can_id |= CAN_RTR_FLAG;
+
+		/* check for optional DLC value for CAN 2.0B frames */
+		if(cs[++idx] && (tmp = asc2nibble(cs[idx])) <= CAN_MAX_DLC)
+			cf->len = tmp;
+
 		return ret;
 	}
 
@@ -236,7 +241,11 @@ void sprint_canframe(char *buf , struct canfd_frame *cf, int sep, int maxdlen) {
 
 	/* standard CAN frames may have RTR enabled. There are no ERR frames with RTR */
 	if (maxdlen == CAN_MAX_DLEN && cf->can_id & CAN_RTR_FLAG) {
-		sprintf(buf+offset, "R");
+		if (cf->len <= CAN_MAX_DLC)
+			sprintf(buf+offset, "R%d", cf->len);
+		else
+			sprintf(buf+offset, "R");
+
 		return;
 	}
 


Regards,
Oliver

Re: SV: Fwd: can-utils at http://gitorious.org/+linux-can-developers/linux-can/can-utils

[Oliver Hartkopp]

Just a small improvement:

RTR frames with DLC == zero are printed as before:

cansend vcan0 123#R
cansend vcan0 123#R0
cansend vcan0 123#R4

leads to

(1360953150.494945) vcan0 123#R
(1360953152.004855) vcan0 123#R
(1360953153.538162) vcan0 123#R4


And here's the patch for that:

diff --git a/lib.c b/lib.c
index 4857e80..9fb1056 100644
--- a/lib.c
+++ b/lib.c
@@ -169,6 +169,11 @@ int parse_canframe(char *cs, struct canfd_frame *cf) {

 	if((cs[idx] == 'R') || (cs[idx] == 'r')){ /* RTR frame */
 		cf->can_id |= CAN_RTR_FLAG;
+
+		/* check for optional DLC value for CAN 2.0B frames */
+		if(cs[++idx] && (tmp = asc2nibble(cs[idx])) <= CAN_MAX_DLC)
+			cf->len = tmp;
+
 		return ret;
 	}

@@ -236,7 +241,13 @@ void sprint_canframe(char *buf , struct canfd_frame *cf,
int sep, int maxdlen) {

 	/* standard CAN frames may have RTR enabled. There are no ERR frames with RTR */
 	if (maxdlen == CAN_MAX_DLEN && cf->can_id & CAN_RTR_FLAG) {
-		sprintf(buf+offset, "R");
+
+		/* print a given CAN 2.0B DLC if it's not zero */
+		if (cf->len && cf->len <= CAN_MAX_DLC)
+			sprintf(buf+offset, "R%d", cf->len);
+		else
+			sprintf(buf+offset, "R");
+
 		return;
 	}





On 15.02.2013 18:20, Oliver Hartkopp wrote:

> On 15.02.2013 17:49, Håkan Engblom wrote:
> 
>> Yes, the RTR is not recommended, you're right.
>>
>> In any case it would be nice to have it there as I have a slave that discards remote frames unless the DLC is the same as in the requested data. 
>>
>> To take away the error-check would be fine for me. Is something like this what you mean, just making sure I did not missunderstand you:
> 
> 
> Yes this is what i also thought of.
> 
>>
>> 	if((cs[idx] == 'R') || (cs[idx] == 'r')){ /* RTR frame */
>> 		cf->can_id |= CAN_RTR_FLAG;
>> 		cf->can_dlc = 0;
> 
> 
> This is superfluous, as the struct can_frame is always initialized to zero.
>  
> 
>> 		if(cs[++idx] && (tmp = asc2nibble(cs[idx])) <= 0x08) /* 8 is max dlc */
>> 			cf->can_dlc = tmp;
> 
> 
> The latest can-utils use struct canfd_frames => 'can_dlc' moved to 'len'
> 
>> 		return 0;
>> 	}
>>
>>
>> This would be perfictly fine with me, I've also done a quick test of this, and it seems to work. And it does not give any fault when node additional length argument is given.
> 
> 
> Yep.
> 
> I wonder if it makes sense to the length 'len' too, before accessing
> cs[++idx] ... but when cs[++idx] is zero your code is correct too.
> 
> 
> Here's my suggestion:
> 
> If it's ok for you, i can add your Signed-off-by ...
> 
> diff --git a/lib.c b/lib.c
> index 4857e80..c2baa77 100644
> --- a/lib.c
> +++ b/lib.c
> @@ -169,6 +169,11 @@ int parse_canframe(char *cs, struct canfd_frame *cf) {
>  
>  	if((cs[idx] == 'R') || (cs[idx] == 'r')){ /* RTR frame */
>  		cf->can_id |= CAN_RTR_FLAG;
> +
> +		/* check for optional DLC value for CAN 2.0B frames */
> +		if(cs[++idx] && (tmp = asc2nibble(cs[idx])) <= CAN_MAX_DLC)
> +			cf->len = tmp;
> +
>  		return ret;
>  	}
>  
> @@ -236,7 +241,11 @@ void sprint_canframe(char *buf , struct canfd_frame *cf, int sep, int maxdlen) {
>  
>  	/* standard CAN frames may have RTR enabled. There are no ERR frames with RTR */
>  	if (maxdlen == CAN_MAX_DLEN && cf->can_id & CAN_RTR_FLAG) {
> -		sprintf(buf+offset, "R");
> +		if (cf->len <= CAN_MAX_DLC)
> +			sprintf(buf+offset, "R%d", cf->len);
> +		else
> +			sprintf(buf+offset, "R");
> +
>  		return;
>  	}
>  
> 
> 
> Regards,
> Oliver
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Fwd: can-utils at http://gitorious.org/+linux-can-developers/linux-can/can-utils

[Heinz-Jürgen Oertel]

Am Freitag 15 Februar 2013, 11:44:00 schrieb Marc Kleine-Budde:
> let's wait for Oliver to say something to the length. As far as I know
> the canutils already support can-fd, which can send up to 64 bytes. I
> don't know the standard for sure, but maybe they got rid of the RTR in
> canfd.

CAN FD is not supporting RTR frames anymore.
Besides this in classic CAN you can specify a dlc between 0 and 15
in data frames. Nevertheless, for dlc > 8 always max 8 data bytes are in the frame.

 Regards
    Heinz 

SV: SV: Fwd: can-utils at http://gitorious.org/+linux-can-developers/linux-can/can-utils

[Håkan Engblom]

Hi,

Seems good from my point of view. Also, see blow.

Br Håkan Engblom

> -----Ursprungligt meddelande-----
> Från: Oliver Hartkopp [mailto:socketcan@hartkopp.net]
> Skickat: den 15 februari 2013 18:20
> Till: Håkan Engblom
> Kopia: Marc Kleine-Budde; linux-can@vger.kernel.org
> Ämne: Re: SV: Fwd: can-utils at http://gitorious.org/+linux-can-
> developers/linux-can/can-utils
> 
> On 15.02.2013 17:49, Håkan Engblom wrote:
> 
> > Yes, the RTR is not recommended, you're right.
> >
> > In any case it would be nice to have it there as I have a slave that discards
> remote frames unless the DLC is the same as in the requested data.
> >
> > To take away the error-check would be fine for me. Is something like this
> what you mean, just making sure I did not missunderstand you:
> 
> 
> Yes this is what i also thought of.
> 
> >
> > 	if((cs[idx] == 'R') || (cs[idx] == 'r')){ /* RTR frame */
> > 		cf->can_id |= CAN_RTR_FLAG;
> > 		cf->can_dlc = 0;
> 
> 
> This is superfluous, as the struct can_frame is always initialized to zero.
> 
> 
> > 		if(cs[++idx] && (tmp = asc2nibble(cs[idx])) <=
> 0x08) /* 8 is max dlc */
> > 			cf->can_dlc = tmp;
> 
> 
> The latest can-utils use struct canfd_frames => 'can_dlc' moved to 'len'
> 
> > 		return 0;
> > 	}
> >
> >
> > This would be perfictly fine with me, I've also done a quick test of this, and
> it seems to work. And it does not give any fault when node additional length
> argument is given.
> 
> 
> Yep.
> 
> I wonder if it makes sense to the length 'len' too, before accessing cs[++idx]
> ... but when cs[++idx] is zero your code is correct too.
> 
> 
> Here's my suggestion:
> 
> If it's ok for you, i can add your Signed-off-by ...

OK for me.


> 
> diff --git a/lib.c b/lib.c
> index 4857e80..c2baa77 100644
> --- a/lib.c
> +++ b/lib.c
> @@ -169,6 +169,11 @@ int parse_canframe(char *cs, struct canfd_frame
> *cf) {
> 
>  	if((cs[idx] == 'R') || (cs[idx] == 'r')){ /* RTR frame */
>  		cf->can_id |= CAN_RTR_FLAG;
> +
> +		/* check for optional DLC value for CAN 2.0B
> frames */
> +		if(cs[++idx] && (tmp = asc2nibble(cs[idx])) <=
> CAN_MAX_DLC)
> +			cf->len = tmp;
> +
>  		return ret;
>  	}
> 
> @@ -236,7 +241,11 @@ void sprint_canframe(char *buf , struct canfd_frame
> *cf, int sep, int maxdlen) {
> 
>  	/* standard CAN frames may have RTR enabled. There are no
> ERR frames with RTR */
>  	if (maxdlen == CAN_MAX_DLEN && cf->can_id &
> CAN_RTR_FLAG) {
> -		sprintf(buf+offset, "R");
> +		if (cf->len <= CAN_MAX_DLC)
> +			sprintf(buf+offset, "R%d", cf-
> >len);
> +		else
> +			sprintf(buf+offset, "R");
> +
>  		return;
>  	}
> 
> 
> 
> Regards,
> Oliver

Re: SV: SV: Fwd: can-utils at http://gitorious.org/+linux-can-developers/linux-can/can-utils

[Oliver Hartkopp]

On 18.02.2013 10:21, Håkan Engblom wrote:

> Seems good from my point of view.


Great! I pushed the patch on Friday evening.

I added some more documentation in lib.h too, see:

https://gitorious.org/linux-can/can-utils/commit/eaf87a3bf69b92a15d0f51c7e3f08e8d900862cc

Tnx & best regards,
Oliver