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

[Oliver Hartkopp <socketcan@hartkopp.net>]

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