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

[Oliver Hartkopp <socketcan@hartkopp.net>]

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