[PATCH RFC] can: add optional DLC element to Classical CAN frame structure

[PATCH RFC] can: add optional DLC element to Classical CAN frame structure

[Oliver Hartkopp]

ISO 11898-1 Chapter 8.4.2.3 defines a 4 bit data length code (DLC) table which
maps the DLC to the payload length of the CAN frame in bytes:

    DLC      ->  payload length
    0 .. 8   ->  0 .. 8
    9 .. 15  ->  8

Although the DLC values 8 .. 15 in Classical CAN always result in a payload
length of 8 bytes these DLC values are transparently transmitted on the CAN
bus. As the struct can_frame only provides a 'len' element (formerly 'can_dlc')
which contains the plain payload length ( 0 .. 8 ) of the CAN frame, the raw
DLC is not visible to the application programmer, e.g. for testing use-cases.

To access the raw DLC values 9 .. 15 the len8_dlc element is introduced, which
is only valid when the payload length 'len' is 8 and the DLC is greater than 8.

The len8_dlc element is filled by the CAN interface driver and used for CAN
frame creation by the CAN driver when the CAN_CTRLMODE_CC_LEN8_DLC flag is
supported by the driver and enabled via netlink configuration interface.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 include/uapi/linux/can.h         | 36 ++++++++++++++++++++------------
 include/uapi/linux/can/netlink.h |  1 +
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
index 6a6d2c7655ff..bcf28ae7f14c 100644
--- a/include/uapi/linux/can.h
+++ b/include/uapi/linux/can.h
@@ -82,34 +82,44 @@ typedef __u32 canid_t;
  */
 typedef __u32 can_err_mask_t;
 
 /* CAN payload length and DLC definitions according to ISO 11898-1 */
 #define CAN_MAX_DLC 8
+#define CAN_MAX_RAW_DLC 15
 #define CAN_MAX_DLEN 8
 
 /* CAN FD payload length and DLC definitions according to ISO 11898-7 */
 #define CANFD_MAX_DLC 15
 #define CANFD_MAX_DLEN 64
 
 /**
  * struct can_frame - basic CAN frame structure
- * @can_id:  CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
- * @can_dlc: frame payload length in byte (0 .. 8) aka data length code
- *           N.B. the DLC field from ISO 11898-1 Chapter 8.4.2.3 has a 1:1
- *           mapping of the 'data length code' to the real payload length
- * @__pad:   padding
- * @__res0:  reserved / padding
- * @__res1:  reserved / padding
- * @data:    CAN frame payload (up to 8 byte)
+ * @can_id:   CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
+ * @len:      CAN frame payload length in byte (0 .. 8)
+ * @can_dlc:  deprecated name for CAN frame payload length in byte (0 .. 8)
+ * @__pad:    padding
+ * @__res0:   reserved / padding
+ * @len8_dlc: optional DLC value (9 .. 15) at 8 byte payload length 
+ *            len8_dlc contains values from 9 .. 15 when the payload length is
+ *            8 bytes but the DLC value (see ISO 11898-1) is greater then 8.
+ *            CAN_CTRLMODE_CC_LEN8_DLC flag has to be enabled in CAN driver.
+ * @data:     CAN frame payload (up to 8 byte)
  */
 struct can_frame {
 	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
-	__u8    can_dlc; /* frame payload length in byte (0 .. CAN_MAX_DLEN) */
-	__u8    __pad;   /* padding */
-	__u8    __res0;  /* reserved / padding */
-	__u8    __res1;  /* reserved / padding */
-	__u8    data[CAN_MAX_DLEN] __attribute__((aligned(8)));
+	union {
+		/* CAN frame payload length in byte (0 .. CAN_MAX_DLEN)
+		 * was previously named can_dlc so we need to carry that
+		 * name for legacy support
+		 */
+		__u8 len;
+		__u8 can_dlc; /* deprecated */
+	};
+	__u8 __pad; /* padding */
+	__u8 __res0; /* reserved / padding */
+	__u8 len8_dlc; /* optional DLC for 8 byte payload length (9 .. 15) */
+	__u8 data[CAN_MAX_DLEN] __attribute__((aligned(8)));
 };
 
 /*
  * defined bits for canfd_frame.flags
  *
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index 6f598b73839e..f730d443b918 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -98,10 +98,11 @@ struct can_ctrlmode {
 #define CAN_CTRLMODE_ONE_SHOT		0x08	/* One-Shot mode */
 #define CAN_CTRLMODE_BERR_REPORTING	0x10	/* Bus-error reporting */
 #define CAN_CTRLMODE_FD			0x20	/* CAN FD mode */
 #define CAN_CTRLMODE_PRESUME_ACK	0x40	/* Ignore missing CAN ACKs */
 #define CAN_CTRLMODE_FD_NON_ISO		0x80	/* CAN FD in non-ISO mode */
+#define CAN_CTRLMODE_CC_LEN8_DLC	0x100	/* Classic CAN DLC option */
 
 /*
  * CAN device statistics
  */
 struct can_device_stats {
-- 
2.28.0

Re: [PATCH RFC] can: add optional DLC element to Classical CAN frame structure

[Vincent MAILHOL]

I did some tests:

  * Applied the patch

  * Modified my driver

  * Modified drivers/net/can/dev.c:can_changelink() to force the
    CAN_CTRLMODE_CC_LEN8_DLC on (this is just me being lazy to do it
    from userland).

  * Modified cansend and candump from can-utils to be able to
    respectively send and print DLCs greater than 8.

  * Set up two channels can0 and can1 and connected those to the same
    bus.

  * Ran 'cangen can0' and 'candump any' simultaneously.


Results of candump:
  can0  539   [8]  05 21 8C 5C F7 B0 7C 69
  can1  539   [8]  05 21 8C 5C F7 B0 7C 69
  can0  47E   [1]  53
  can1  47E   [1]  53
  can0  317   [6]  E5 00 B5 73 66 CF
  can1  317   [6]  E5 00 B5 73 66 CF
  can0  524   [E]  64 C3 B0 6E 55 7A D7 2E
  can1  524   [E]  64 C3 B0 6E 55 7A D7 2E
  can0  39C   [D]  63 18 96 69 F6 7A AB 36
  can1  39C   [D]  63 18 96 69 F6 7A AB 36
  can0  60D   [F]  F2 C6 B6 1D 80 FB BC 7E
  can1  60D   [F]  F2 C6 B6 1D 80 FB BC 7E
  can0  5DD   [9]  7E 55 56 5B C0 23 B0 53
  can1  5DD   [9]  7E 55 56 5B C0 23 B0 53
  can0  573   [E]  20 8E A3 31 1B 54 B2 16
  can1  573   [E]  20 8E A3 31 1B 54 B2 16
  can0  470   [3]  31 9C 06
  can1  470   [3]  31 9C 06
  can0  465   [4]  93 75 A2 08
  can1  465   [4]  93 75 A2 08


Works fine :-)
Thanks Oliver!


Yours sincerely,
Vincent Mailhol

Re: [PATCH RFC] can: add optional DLC element to Classical CAN frame structure

[Oliver Hartkopp]

On 24.10.20 14:00, Vincent MAILHOL wrote:
> I did some tests:
> 
>    * Applied the patch
> 
>    * Modified my driver
> 
>    * Modified drivers/net/can/dev.c:can_changelink() to force the
>      CAN_CTRLMODE_CC_LEN8_DLC on (this is just me being lazy to do it
>      from userland).

Good choice for a test. Modifying the ip tool is not that tricky - but 
you need to clone that stuff, etc.

>    * Modified cansend and candump from can-utils to be able to
>      respectively send and print DLCs greater than 8.

Yes, there will be even more effort on can-utils as it also has an 
impact on logfile formats, etc.

E.g. the value between [] in candump was 'len' so far - we could probaly 
make them () when we might show the DLC values.

And the logfile format gets the length implicit from the number of 
printed data bytes, which is always the 'len'.

So we would need to discuss, where we would need command line options to 
enable the CC len8_dlc feature.

>    * Set up two channels can0 and can1 and connected those to the same
>      bus.
> 
>    * Ran 'cangen can0' and 'candump any' simultaneously.
> 
> 
> Results of candump:
>    can0  539   [8]  05 21 8C 5C F7 B0 7C 69
>    can1  539   [8]  05 21 8C 5C F7 B0 7C 69
>    can0  47E   [1]  53
>    can1  47E   [1]  53
>    can0  317   [6]  E5 00 B5 73 66 CF
>    can1  317   [6]  E5 00 B5 73 66 CF
>    can0  524   [E]  64 C3 B0 6E 55 7A D7 2E
>    can1  524   [E]  64 C3 B0 6E 55 7A D7 2E
>    can0  39C   [D]  63 18 96 69 F6 7A AB 36
>    can1  39C   [D]  63 18 96 69 F6 7A AB 36
>    can0  60D   [F]  F2 C6 B6 1D 80 FB BC 7E
>    can1  60D   [F]  F2 C6 B6 1D 80 FB BC 7E
>    can0  5DD   [9]  7E 55 56 5B C0 23 B0 53
>    can1  5DD   [9]  7E 55 56 5B C0 23 B0 53
>    can0  573   [E]  20 8E A3 31 1B 54 B2 16
>    can1  573   [E]  20 8E A3 31 1B 54 B2 16
>    can0  470   [3]  31 9C 06
>    can1  470   [3]  31 9C 06
>    can0  465   [4]  93 75 A2 08
>    can1  465   [4]  93 75 A2 08
> 
> 
> Works fine :-)

Excellent job!!

Thanks Vincent!

Best,
Oliver

Re: [PATCH RFC] can: add optional DLC element to Classical CAN frame structure

[Oliver Hartkopp]

@Marc, all,

are you fine with that patch and the naming too?

If so, I would go further and provide a patch with the complete netlink 
code in can-dev - and maybe a first driver adaption to support len8_dlc 
for Classical CAN.

Best regards,
Oliver

On 24.10.20 15:00, Oliver Hartkopp wrote:
> 
> 
> On 24.10.20 14:00, Vincent MAILHOL wrote:
>> I did some tests:
>>
>>    * Applied the patch
>>
>>    * Modified my driver
>>
>>    * Modified drivers/net/can/dev.c:can_changelink() to force the
>>      CAN_CTRLMODE_CC_LEN8_DLC on (this is just me being lazy to do it
>>      from userland).
> 
> Good choice for a test. Modifying the ip tool is not that tricky - but 
> you need to clone that stuff, etc.
> 
>>    * Modified cansend and candump from can-utils to be able to
>>      respectively send and print DLCs greater than 8.
> 
> Yes, there will be even more effort on can-utils as it also has an 
> impact on logfile formats, etc.
> 
> E.g. the value between [] in candump was 'len' so far - we could probaly 
> make them () when we might show the DLC values.
> 
> And the logfile format gets the length implicit from the number of 
> printed data bytes, which is always the 'len'.
> 
> So we would need to discuss, where we would need command line options to 
> enable the CC len8_dlc feature.
> 
>>    * Set up two channels can0 and can1 and connected those to the same
>>      bus.
>>
>>    * Ran 'cangen can0' and 'candump any' simultaneously.
>>
>>
>> Results of candump:
>>    can0  539   [8]  05 21 8C 5C F7 B0 7C 69
>>    can1  539   [8]  05 21 8C 5C F7 B0 7C 69
>>    can0  47E   [1]  53
>>    can1  47E   [1]  53
>>    can0  317   [6]  E5 00 B5 73 66 CF
>>    can1  317   [6]  E5 00 B5 73 66 CF
>>    can0  524   [E]  64 C3 B0 6E 55 7A D7 2E
>>    can1  524   [E]  64 C3 B0 6E 55 7A D7 2E
>>    can0  39C   [D]  63 18 96 69 F6 7A AB 36
>>    can1  39C   [D]  63 18 96 69 F6 7A AB 36
>>    can0  60D   [F]  F2 C6 B6 1D 80 FB BC 7E
>>    can1  60D   [F]  F2 C6 B6 1D 80 FB BC 7E
>>    can0  5DD   [9]  7E 55 56 5B C0 23 B0 53
>>    can1  5DD   [9]  7E 55 56 5B C0 23 B0 53
>>    can0  573   [E]  20 8E A3 31 1B 54 B2 16
>>    can1  573   [E]  20 8E A3 31 1B 54 B2 16
>>    can0  470   [3]  31 9C 06
>>    can1  470   [3]  31 9C 06
>>    can0  465   [4]  93 75 A2 08
>>    can1  465   [4]  93 75 A2 08
>>
>>
>> Works fine :-)
> 
> Excellent job!!
> 
> Thanks Vincent!
> 
> Best,
> Oliver

Re: [PATCH RFC] can: add optional DLC element to Classical CAN frame structure

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 5440 bytes --]

On 10/23/20 10:30 PM, Oliver Hartkopp wrote:
> ISO 11898-1 Chapter 8.4.2.3 defines a 4 bit data length code (DLC) table which
> maps the DLC to the payload length of the CAN frame in bytes:
> 
>     DLC      ->  payload length
>     0 .. 8   ->  0 .. 8
>     9 .. 15  ->  8
> 
> Although the DLC values 8 .. 15 in Classical CAN always result in a payload
> length of 8 bytes these DLC values are transparently transmitted on the CAN
> bus. As the struct can_frame only provides a 'len' element (formerly 'can_dlc')
> which contains the plain payload length ( 0 .. 8 ) of the CAN frame, the raw
> DLC is not visible to the application programmer, e.g. for testing use-cases.
> 
> To access the raw DLC values 9 .. 15 the len8_dlc element is introduced, which
> is only valid when the payload length 'len' is 8 and the DLC is greater than 8.
> 
> The len8_dlc element is filled by the CAN interface driver and used for CAN
> frame creation by the CAN driver when the CAN_CTRLMODE_CC_LEN8_DLC flag is

CC stands for Classic CAN? Is this the "official" name?

For example there was a press release to harmonize the CAN transceiver nameing
recently:

https://can-cia.org/news/press-releases/view/harmonized-transceiver-naming/2020/7/16/

> supported by the driver and enabled via netlink configuration interface.
> 
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
>  include/uapi/linux/can.h         | 36 ++++++++++++++++++++------------
>  include/uapi/linux/can/netlink.h |  1 +
>  2 files changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
> index 6a6d2c7655ff..bcf28ae7f14c 100644
> --- a/include/uapi/linux/can.h
> +++ b/include/uapi/linux/can.h
> @@ -82,34 +82,44 @@ typedef __u32 canid_t;
>   */
>  typedef __u32 can_err_mask_t;
>  
>  /* CAN payload length and DLC definitions according to ISO 11898-1 */
>  #define CAN_MAX_DLC 8
> +#define CAN_MAX_RAW_DLC 15
>  #define CAN_MAX_DLEN 8
>  
>  /* CAN FD payload length and DLC definitions according to ISO 11898-7 */
>  #define CANFD_MAX_DLC 15
>  #define CANFD_MAX_DLEN 64
>  
>  /**
>   * struct can_frame - basic CAN frame structure
> - * @can_id:  CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
> - * @can_dlc: frame payload length in byte (0 .. 8) aka data length code
> - *           N.B. the DLC field from ISO 11898-1 Chapter 8.4.2.3 has a 1:1
> - *           mapping of the 'data length code' to the real payload length
> - * @__pad:   padding
> - * @__res0:  reserved / padding
> - * @__res1:  reserved / padding
> - * @data:    CAN frame payload (up to 8 byte)
> + * @can_id:   CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
> + * @len:      CAN frame payload length in byte (0 .. 8)
> + * @can_dlc:  deprecated name for CAN frame payload length in byte (0 .. 8)
> + * @__pad:    padding
> + * @__res0:   reserved / padding
> + * @len8_dlc: optional DLC value (9 .. 15) at 8 byte payload length 
> + *            len8_dlc contains values from 9 .. 15 when the payload length is
> + *            8 bytes but the DLC value (see ISO 11898-1) is greater then 8.
> + *            CAN_CTRLMODE_CC_LEN8_DLC flag has to be enabled in CAN driver.
> + * @data:     CAN frame payload (up to 8 byte)
>   */
>  struct can_frame {
>  	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
> -	__u8    can_dlc; /* frame payload length in byte (0 .. CAN_MAX_DLEN) */
> -	__u8    __pad;   /* padding */
> -	__u8    __res0;  /* reserved / padding */
> -	__u8    __res1;  /* reserved / padding */
> -	__u8    data[CAN_MAX_DLEN] __attribute__((aligned(8)));
> +	union {
> +		/* CAN frame payload length in byte (0 .. CAN_MAX_DLEN)
> +		 * was previously named can_dlc so we need to carry that
> +		 * name for legacy support
> +		 */
> +		__u8 len;
> +		__u8 can_dlc; /* deprecated */
> +	};

There was an old compiler version, which struggled with C99 initialized unions
within structs.....So this change would break existing source, but I think that
old compilers are long gone (for good).

> +	__u8 __pad; /* padding */
> +	__u8 __res0; /* reserved / padding */
> +	__u8 len8_dlc; /* optional DLC for 8 byte payload length (9 .. 15) */
> +	__u8 data[CAN_MAX_DLEN] __attribute__((aligned(8)));
>  };
>  
>  /*
>   * defined bits for canfd_frame.flags
>   *
> diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
> index 6f598b73839e..f730d443b918 100644
> --- a/include/uapi/linux/can/netlink.h
> +++ b/include/uapi/linux/can/netlink.h
> @@ -98,10 +98,11 @@ struct can_ctrlmode {
>  #define CAN_CTRLMODE_ONE_SHOT		0x08	/* One-Shot mode */
>  #define CAN_CTRLMODE_BERR_REPORTING	0x10	/* Bus-error reporting */
>  #define CAN_CTRLMODE_FD			0x20	/* CAN FD mode */
>  #define CAN_CTRLMODE_PRESUME_ACK	0x40	/* Ignore missing CAN ACKs */
>  #define CAN_CTRLMODE_FD_NON_ISO		0x80	/* CAN FD in non-ISO mode */
> +#define CAN_CTRLMODE_CC_LEN8_DLC	0x100	/* Classic CAN DLC option */
>  
>  /*
>   * CAN device statistics
>   */
>  struct can_device_stats {
> 

Marc

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


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

Re: [PATCH RFC] can: add optional DLC element to Classical CAN frame structure

[Oliver Hartkopp]

On 27.10.20 14:06, Marc Kleine-Budde wrote:
> On 10/23/20 10:30 PM, Oliver Hartkopp wrote:
>> ISO 11898-1 Chapter 8.4.2.3 defines a 4 bit data length code (DLC) table which
>> maps the DLC to the payload length of the CAN frame in bytes:
>>
>>      DLC      ->  payload length
>>      0 .. 8   ->  0 .. 8
>>      9 .. 15  ->  8
>>
>> Although the DLC values 8 .. 15 in Classical CAN always result in a payload
>> length of 8 bytes these DLC values are transparently transmitted on the CAN
>> bus. As the struct can_frame only provides a 'len' element (formerly 'can_dlc')
>> which contains the plain payload length ( 0 .. 8 ) of the CAN frame, the raw
>> DLC is not visible to the application programmer, e.g. for testing use-cases.
>>
>> To access the raw DLC values 9 .. 15 the len8_dlc element is introduced, which
>> is only valid when the payload length 'len' is 8 and the DLC is greater than 8.
>>
>> The len8_dlc element is filled by the CAN interface driver and used for CAN
>> frame creation by the CAN driver when the CAN_CTRLMODE_CC_LEN8_DLC flag is
> 
> CC stands for Classic CAN? Is this the "official" name?

No. It is 'Classical CAN'. I'm not very happy with that naming as there 
was already a 'CAN2.0B' specification to separate from the first version 
which only had 11 Bit identifiers. This could be Ancient CAN now :-D

> For example there was a press release to harmonize the CAN transceiver nameing
> recently:
> 
> https://can-cia.org/news/press-releases/view/harmonized-transceiver-naming/2020/7/16/

Yes, there you can find:

"CAN high-speed transceivers might be used in Classical CAN, CAN FD, or 
CAN XL networks"

Maybe we could update some comments and documentation for CAN2.0 -> 
Classical CAN later.

> 
>> supported by the driver and enabled via netlink configuration interface.
>>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>> ---
>>   include/uapi/linux/can.h         | 36 ++++++++++++++++++++------------
>>   include/uapi/linux/can/netlink.h |  1 +
>>   2 files changed, 24 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
>> index 6a6d2c7655ff..bcf28ae7f14c 100644
>> --- a/include/uapi/linux/can.h
>> +++ b/include/uapi/linux/can.h
>> @@ -82,34 +82,44 @@ typedef __u32 canid_t;
>>    */
>>   typedef __u32 can_err_mask_t;
>>   
>>   /* CAN payload length and DLC definitions according to ISO 11898-1 */
>>   #define CAN_MAX_DLC 8
>> +#define CAN_MAX_RAW_DLC 15
>>   #define CAN_MAX_DLEN 8
>>   
>>   /* CAN FD payload length and DLC definitions according to ISO 11898-7 */
>>   #define CANFD_MAX_DLC 15
>>   #define CANFD_MAX_DLEN 64
>>   
>>   /**
>>    * struct can_frame - basic CAN frame structure
>> - * @can_id:  CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
>> - * @can_dlc: frame payload length in byte (0 .. 8) aka data length code
>> - *           N.B. the DLC field from ISO 11898-1 Chapter 8.4.2.3 has a 1:1
>> - *           mapping of the 'data length code' to the real payload length
>> - * @__pad:   padding
>> - * @__res0:  reserved / padding
>> - * @__res1:  reserved / padding
>> - * @data:    CAN frame payload (up to 8 byte)
>> + * @can_id:   CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
>> + * @len:      CAN frame payload length in byte (0 .. 8)
>> + * @can_dlc:  deprecated name for CAN frame payload length in byte (0 .. 8)
>> + * @__pad:    padding
>> + * @__res0:   reserved / padding
>> + * @len8_dlc: optional DLC value (9 .. 15) at 8 byte payload length
>> + *            len8_dlc contains values from 9 .. 15 when the payload length is
>> + *            8 bytes but the DLC value (see ISO 11898-1) is greater then 8.
>> + *            CAN_CTRLMODE_CC_LEN8_DLC flag has to be enabled in CAN driver.
>> + * @data:     CAN frame payload (up to 8 byte)
>>    */
>>   struct can_frame {
>>   	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
>> -	__u8    can_dlc; /* frame payload length in byte (0 .. CAN_MAX_DLEN) */
>> -	__u8    __pad;   /* padding */
>> -	__u8    __res0;  /* reserved / padding */
>> -	__u8    __res1;  /* reserved / padding */
>> -	__u8    data[CAN_MAX_DLEN] __attribute__((aligned(8)));
>> +	union {
>> +		/* CAN frame payload length in byte (0 .. CAN_MAX_DLEN)
>> +		 * was previously named can_dlc so we need to carry that
>> +		 * name for legacy support
>> +		 */
>> +		__u8 len;
>> +		__u8 can_dlc; /* deprecated */
>> +	};
> 
> There was an old compiler version, which struggled with C99 initialized unions
> within structs.....So this change would break existing source, but I think that
> old compilers are long gone (for good).

Good to know. Do you know 'version numbers', so that we may place a 
warning somewhere?

I still have a 2.6.28 system here (gcc 2.95) - but I would not know why 
updating the can-utils there today. Maybe for the cansniffer ;-)

Btw. when it is only about the initialization of static struct 
can_frame's, I can also check for these cases in can-utils.

Best,
Oliver

Re: [PATCH RFC] can: add optional DLC element to Classical CAN frame structure

[Oliver Hartkopp]

On 27.10.20 14:23, Oliver Hartkopp wrote:
> On 27.10.20 14:06, Marc Kleine-Budde wrote:

>>> +    union {
>>> +        /* CAN frame payload length in byte (0 .. CAN_MAX_DLEN)
>>> +         * was previously named can_dlc so we need to carry that
>>> +         * name for legacy support
>>> +         */
>>> +        __u8 len;
>>> +        __u8 can_dlc; /* deprecated */
>>> +    };
>>
>> There was an old compiler version, which struggled with C99 
>> initialized unions
>> within structs.....So this change would break existing source, but I 
>> think that
>> old compilers are long gone (for good).
> 

It looks like we only run into a problem when the elements of the union 
have a different size:

https://stackoverflow.com/questions/11812555/how-to-zero-initialize-an-union

Which is not the case with len and can_dlc (both __u8).

> Good to know. Do you know 'version numbers', so that we may place a 
> warning somewhere?
> 
> I still have a 2.6.28 system here (gcc 2.95) - but I would not know why 
> updating the can-utils there today. Maybe for the cansniffer ;-)
> 
> Btw. when it is only about the initialization of static struct 
> can_frame's, I can also check for these cases in can-utils.
> 
> Best,
> Oliver

Re: [PATCH RFC] can: add optional DLC element to Classical CAN frame structure

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 3236 bytes --]

On 10/27/20 2:23 PM, Oliver Hartkopp wrote:
> On 27.10.20 14:06, Marc Kleine-Budde wrote:
>> On 10/23/20 10:30 PM, Oliver Hartkopp wrote:
>>> ISO 11898-1 Chapter 8.4.2.3 defines a 4 bit data length code (DLC) table which
>>> maps the DLC to the payload length of the CAN frame in bytes:
>>>
>>>      DLC      ->  payload length
>>>      0 .. 8   ->  0 .. 8
>>>      9 .. 15  ->  8
>>>
>>> Although the DLC values 8 .. 15 in Classical CAN always result in a payload
>>> length of 8 bytes these DLC values are transparently transmitted on the CAN
>>> bus. As the struct can_frame only provides a 'len' element (formerly 'can_dlc')
>>> which contains the plain payload length ( 0 .. 8 ) of the CAN frame, the raw
>>> DLC is not visible to the application programmer, e.g. for testing use-cases.
>>>
>>> To access the raw DLC values 9 .. 15 the len8_dlc element is introduced, which
>>> is only valid when the payload length 'len' is 8 and the DLC is greater than 8.
>>>
>>> The len8_dlc element is filled by the CAN interface driver and used for CAN
>>> frame creation by the CAN driver when the CAN_CTRLMODE_CC_LEN8_DLC flag is
>>
>> CC stands for Classic CAN? Is this the "official" name?
> 
> No. It is 'Classical CAN'. I'm not very happy with that naming as there 
> was already a 'CAN2.0B' specification to separate from the first version 
> which only had 11 Bit identifiers. This could be Ancient CAN now :-D

So Classical CAN is CAN2.0B?

>> For example there was a press release to harmonize the CAN transceiver nameing
>> recently:
>>
>> https://can-cia.org/news/press-releases/view/harmonized-transceiver-naming/2020/7/16/
> 
> Yes, there you can find:
> 
> "CAN high-speed transceivers might be used in Classical CAN, CAN FD, or 
> CAN XL networks"

Doh! I should have re-read that article :) We found it, when we were searching
for consistent naming for the different CAN transceivers.

> Maybe we could update some comments and documentation for CAN2.0 -> 
> Classical CAN later.

Good point.

[...]

>>> +	union {
>>> +		/* CAN frame payload length in byte (0 .. CAN_MAX_DLEN)
>>> +		 * was previously named can_dlc so we need to carry that
>>> +		 * name for legacy support
>>> +		 */
>>> +		__u8 len;
>>> +		__u8 can_dlc; /* deprecated */
>>> +	};
>>
>> There was an old compiler version, which struggled with C99 initialized unions
>> within structs.....So this change would break existing source, but I think that
>> old compilers are long gone (for good).
> 
> Good to know. Do you know 'version numbers', so that we may place a 
> warning somewhere?

No, I don't remember. Some old gcc-3.x

> I still have a 2.6.28 system here (gcc 2.95) - but I would not know why 
> updating the can-utils there today. Maybe for the cansniffer ;-)
> 
> Btw. when it is only about the initialization of static struct 
> can_frame's, I can also check for these cases in can-utils.

Marc

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


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

Re: [PATCH RFC] can: add optional DLC element to Classical CAN frame structure

[Kurt Van Dijck]

On Tue, 27 Oct 2020 14:48:30 +0100, Marc Kleine-Budde wrote:
> On 10/27/20 2:23 PM, Oliver Hartkopp wrote:
> > On 27.10.20 14:06, Marc Kleine-Budde wrote:
> >> On 10/23/20 10:30 PM, Oliver Hartkopp wrote:

> > 
> > No. It is 'Classical CAN'. I'm not very happy with that naming as there 
> > was already a 'CAN2.0B' specification to separate from the first version 
> > which only had 11 Bit identifiers. This could be Ancient CAN now :-D
> 
> So Classical CAN is CAN2.0B?
> 
> >> For example there was a press release to harmonize the CAN transceiver nameing
> >> recently:
> >>
> >> https://can-cia.org/news/press-releases/view/harmonized-transceiver-naming/2020/7/16/
> > 
> > Yes, there you can find:
> > 
> > "CAN high-speed transceivers might be used in Classical CAN, CAN FD, or 
> > CAN XL networks"

What happened to 'Standard CAN' (<= CAN2.0A) and 'Extended CAN' (CAN2.0B)?
Did those names became fossils now?

Kurt

Re: [PATCH RFC] can: add optional DLC element to Classical CAN frame structure

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1329 bytes --]

On 10/27/20 3:49 PM, Kurt Van Dijck wrote:
> On Tue, 27 Oct 2020 14:48:30 +0100, Marc Kleine-Budde wrote:
>> On 10/27/20 2:23 PM, Oliver Hartkopp wrote:
>>> On 27.10.20 14:06, Marc Kleine-Budde wrote:
>>>> On 10/23/20 10:30 PM, Oliver Hartkopp wrote:
> 
>>>
>>> No. It is 'Classical CAN'. I'm not very happy with that naming as there 
>>> was already a 'CAN2.0B' specification to separate from the first version 
>>> which only had 11 Bit identifiers. This could be Ancient CAN now :-D
>>
>> So Classical CAN is CAN2.0B?
>>
>>>> For example there was a press release to harmonize the CAN transceiver nameing
>>>> recently:
>>>>
>>>> https://can-cia.org/news/press-releases/view/harmonized-transceiver-naming/2020/7/16/
>>>
>>> Yes, there you can find:
>>>
>>> "CAN high-speed transceivers might be used in Classical CAN, CAN FD, or 
>>> CAN XL networks"
> 
> What happened to 'Standard CAN' (<= CAN2.0A) and 'Extended CAN' (CAN2.0B)?
> Did those names became fossils now?

As far as I understand Classical CAN is CAN2.0B

Marc

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


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

Re: [PATCH RFC] can: add optional DLC element to Classical CAN frame structure

[Oliver Hartkopp]

On 27.10.20 15:49, Kurt Van Dijck wrote:
> On Tue, 27 Oct 2020 14:48:30 +0100, Marc Kleine-Budde wrote:
>> On 10/27/20 2:23 PM, Oliver Hartkopp wrote:
>>> On 27.10.20 14:06, Marc Kleine-Budde wrote:
>>>> On 10/23/20 10:30 PM, Oliver Hartkopp wrote:
> 
>>>
>>> No. It is 'Classical CAN'. I'm not very happy with that naming as there
>>> was already a 'CAN2.0B' specification to separate from the first version
>>> which only had 11 Bit identifiers. This could be Ancient CAN now :-D
>>
>> So Classical CAN is CAN2.0B?
>>
>>>> For example there was a press release to harmonize the CAN transceiver nameing
>>>> recently:
>>>>
>>>> https://can-cia.org/news/press-releases/view/harmonized-transceiver-naming/2020/7/16/
>>>
>>> Yes, there you can find:
>>>
>>> "CAN high-speed transceivers might be used in Classical CAN, CAN FD, or
>>> CAN XL networks"
> 
> What happened to 'Standard CAN' (<= CAN2.0A) and 'Extended CAN' (CAN2.0B)?
> Did those names became fossils now?

I'm currently working in a CiA Working group for CAN XL higher layer 
protocols - and all documents that refer to the "CAN with 8 bytes 
payload" seem to use "Classical CAN".

Btw. I would prefer your naming much more.

"Classical" is like "New" or "Enhanced" or "Next Generation" - just 
relative attributes.

Best,
Oliver

Re: [PATCH RFC] can: add optional DLC element to Classical CAN frame structure

[Oliver Hartkopp]

On 27.10.20 15:53, Marc Kleine-Budde wrote:
> On 10/27/20 3:49 PM, Kurt Van Dijck wrote:
>> On Tue, 27 Oct 2020 14:48:30 +0100, Marc Kleine-Budde wrote:
>>> On 10/27/20 2:23 PM, Oliver Hartkopp wrote:
>>>> On 27.10.20 14:06, Marc Kleine-Budde wrote:
>>>>> On 10/23/20 10:30 PM, Oliver Hartkopp wrote:
>>
>>>>
>>>> No. It is 'Classical CAN'. I'm not very happy with that naming as there
>>>> was already a 'CAN2.0B' specification to separate from the first version
>>>> which only had 11 Bit identifiers. This could be Ancient CAN now :-D
>>>
>>> So Classical CAN is CAN2.0B?
>>>
>>>>> For example there was a press release to harmonize the CAN transceiver nameing
>>>>> recently:
>>>>>
>>>>> https://can-cia.org/news/press-releases/view/harmonized-transceiver-naming/2020/7/16/
>>>>
>>>> Yes, there you can find:
>>>>
>>>> "CAN high-speed transceivers might be used in Classical CAN, CAN FD, or
>>>> CAN XL networks"
>>
>> What happened to 'Standard CAN' (<= CAN2.0A) and 'Extended CAN' (CAN2.0B)?
>> Did those names became fossils now?
> 
> As far as I understand Classical CAN is CAN2.0B

http://www.microcontrol.net/wissen/bus-systeme/can-fd/

As CAN2.0B covers CAN2.0A you are somehow right.

BR,
Oliver

Re: [PATCH RFC] can: add optional DLC element to Classical CAN frame structure

[Oliver Hartkopp]

Hi Vincent,

On 24.10.20 15:00, Oliver Hartkopp wrote:
> On 24.10.20 14:00, Vincent MAILHOL wrote:

>>    * Set up two channels can0 and can1 and connected those to the same
>>      bus.
>>
>>    * Ran 'cangen can0' and 'candump any' simultaneously.
>>
>>
>> Results of candump:
>>    can0  539   [8]  05 21 8C 5C F7 B0 7C 69
>>    can1  539   [8]  05 21 8C 5C F7 B0 7C 69
>>    can0  47E   [1]  53
>>    can1  47E   [1]  53
>>    can0  317   [6]  E5 00 B5 73 66 CF
>>    can1  317   [6]  E5 00 B5 73 66 CF
>>    can0  524   [E]  64 C3 B0 6E 55 7A D7 2E
>>    can1  524   [E]  64 C3 B0 6E 55 7A D7 2E
>>    can0  39C   [D]  63 18 96 69 F6 7A AB 36
>>    can1  39C   [D]  63 18 96 69 F6 7A AB 36
>>    can0  60D   [F]  F2 C6 B6 1D 80 FB BC 7E
>>    can1  60D   [F]  F2 C6 B6 1D 80 FB BC 7E
>>    can0  5DD   [9]  7E 55 56 5B C0 23 B0 53
>>    can1  5DD   [9]  7E 55 56 5B C0 23 B0 53
>>    can0  573   [E]  20 8E A3 31 1B 54 B2 16
>>    can1  573   [E]  20 8E A3 31 1B 54 B2 16
>>    can0  470   [3]  31 9C 06
>>    can1  470   [3]  31 9C 06
>>    can0  465   [4]  93 75 A2 08
>>    can1  465   [4]  93 75 A2 08
>>
>>
>> Works fine :-)

I'm not really sure about the view of Classical CAN DLCs in this 'long' 
candump representation.

There are people that use this format as base for further processing - 
instead of the logfile format m(
So it may be a bad decision to put the DLC value in the length position 
here.

I changed the logfile format in a backward compatible way and therefore 
the cansend command line format too:

(1604004658.444168) vcan0 2C0##02643
(1604004658.494492) vcan0 615#R8_9
(1604004658.645395) vcan0 6FF#
(1604004658.695682) vcan0 623##318F88F7043C0
(1604004658.746046) vcan0 63A##28D1F37
(1604004658.796214) vcan0 1117DEA5##0BC281D711098
(1604004658.846397) vcan0 1490B893#R8_F
(1604004658.896585) vcan0 1F494EC8##39C47E2
(1604004658.947142) vcan0 1D1EC448#3DC1C81345D7ED7E_D
(1604004658.997689) vcan0 1C8661BB##14DEA7568D459160D
(1604004659.047851) vcan0 2D8#AFE0546E6522130D_E
(1604004659.098216) vcan0 1596E833##3E6789514EF10B466E6789514EF10B466

My can-utils len8_dlc test setup is here:
https://github.com/hartkopp/can-utils

Feel free to do some tests, e.g. with cangen, candump, canplayer. 
Feedback is appreciated.

Best,
Oliver

Re: [PATCH RFC] can: add optional DLC element to Classical CAN frame structure

[Vincent MAILHOL]

On 30.10.20 06:33, Oliver Hartkopp wrote:
> Hi Vincent,
>
> On 24.10.20 15:00, Oliver Hartkopp wrote:
>> On 24.10.20 14:00, Vincent MAILHOL wrote:
>
>>>    * Set up two channels can0 and can1 and connected those to the same
>>>      bus.
>>>
>>>    * Ran 'cangen can0' and 'candump any' simultaneously.
>>>
>>>
>>> Results of candump:
>>>    can0  539   [8]  05 21 8C 5C F7 B0 7C 69
>>>    can1  539   [8]  05 21 8C 5C F7 B0 7C 69
>>>    can0  47E   [1]  53
>>>    can1  47E   [1]  53
>>>    can0  317   [6]  E5 00 B5 73 66 CF
>>>    can1  317   [6]  E5 00 B5 73 66 CF
>>>    can0  524   [E]  64 C3 B0 6E 55 7A D7 2E
>>>    can1  524   [E]  64 C3 B0 6E 55 7A D7 2E
>>>    can0  39C   [D]  63 18 96 69 F6 7A AB 36
>>>    can1  39C   [D]  63 18 96 69 F6 7A AB 36
>>>    can0  60D   [F]  F2 C6 B6 1D 80 FB BC 7E
>>>    can1  60D   [F]  F2 C6 B6 1D 80 FB BC 7E
>>>    can0  5DD   [9]  7E 55 56 5B C0 23 B0 53
>>>    can1  5DD   [9]  7E 55 56 5B C0 23 B0 53
>>>    can0  573   [E]  20 8E A3 31 1B 54 B2 16
>>>    can1  573   [E]  20 8E A3 31 1B 54 B2 16
>>>    can0  470   [3]  31 9C 06
>>>    can1  470   [3]  31 9C 06
>>>    can0  465   [4]  93 75 A2 08
>>>    can1  465   [4]  93 75 A2 08
>>>
>>>
>>> Works fine :-)
>
> I'm not really sure about the view of Classical CAN DLCs in this 'long'
> candump representation.
>
> There are people that use this format as base for further processing -
> instead of the logfile format m(
> So it may be a bad decision to put the DLC value in the length position
> here.

There was actually zero thought when I wrote this. I just wanted to
test the kernel land and this was the first hack which came to my
mind.

candump is not an easy one to modify. When putting the DLC in the
picture, I can see three option:

  1. Do not modify the format. Nothing will break but it would be
     impossible to know the actual DLC value of a Classical CAN frame
     of length 8.

  2. Ask people doing processing on the candump output to modify their
     tool if they want to use the cc-len8-dlc switch (if not using it,
     should not break).

  3. The in-between solution: use a command line switch (e.g. candump
     -8 any) select option 1 or 2 at run time.

None of the solution is perfect.

I personally do not like option 3. So far, the command "candump any"
would capture all the information. For example, their is no switch to
enable/disable the capture of CAN-FD frames so it would seems odd to
me to have it only for the DLC.

Option 1 would be inconvenient for people who have needs similar to
mine. Option 2 would be inconvenient for people who want to use
cc-len8-dlc but do not want to touch their tools built on top of
candump.

At the end of the day, I can not think of a perfect solution but I
actually liked your previous idea of using parenthesis. The rule would
be that [x] would designate a DLC of len2dlc(x) and a length of x; and
(x) would designate a DLC of x and length of 8. On a security point of
view, I think that the impact would be minimal. If we change the
format form [x] to (x), parsing tools build on top should directly
crash and the user should notice directly.

> I changed the logfile format in a backward compatible way and therefore
> the cansend command line format too:
>
> (1604004658.444168) vcan0 2C0##02643
> (1604004658.494492) vcan0 615#R8_9
> (1604004658.645395) vcan0 6FF#
> (1604004658.695682) vcan0 623##318F88F7043C0
> (1604004658.746046) vcan0 63A##28D1F37
> (1604004658.796214) vcan0 1117DEA5##0BC281D711098
> (1604004658.846397) vcan0 1490B893#R8_F
> (1604004658.896585) vcan0 1F494EC8##39C47E2
> (1604004658.947142) vcan0 1D1EC448#3DC1C81345D7ED7E_D
> (1604004658.997689) vcan0 1C8661BB##14DEA7568D459160D
> (1604004659.047851) vcan0 2D8#AFE0546E6522130D_E
> (1604004659.098216) vcan0 1596E833##3E6789514EF10B466E6789514EF10B466

Not the most sexy solution but I have to admit that it does the job
and fits well with the other tools. Also, I am not able to think of a
better solutions.

Not sure if anyone also uses this log format as base for further
processing. Somewhere, someone might be impacted as well. But there is
probably nothing that we can do about that.

> My can-utils len8_dlc test setup is here:
> https://github.com/hartkopp/can-utils
>
> Feel free to do some tests, e.g. with cangen, candump, canplayer.
> Feedback is appreciated.

Sorry for the late feedback. I applied all the changes on the user
land and just started the tests.  First results are good :)

So far, I found a minor issue in cangen.c, will send you a patch in an
other email right after.

I will need more time to complete a full test and review.

Overall, I think that you did a good job in the naming convention and
the choice of the command line options.

Thanks a lot!


Yours sincerely,
Vincent Mailhol

Re: [PATCH RFC] can: add optional DLC element to Classical CAN frame structure

[Oliver Hartkopp]

On 01.11.20 15:50, Vincent MAILHOL wrote:
> On 30.10.20 06:33, Oliver Hartkopp wrote:
>>>>     can0  60D   [F]  F2 C6 B6 1D 80 FB BC 7E
>>>>     can1  60D   [F]  F2 C6 B6 1D 80 FB BC 7E
>>>>     can0  5DD   [9]  7E 55 56 5B C0 23 B0 53
>>>>     can1  5DD   [9]  7E 55 56 5B C0 23 B0 53
>>>>     can0  573   [E]  20 8E A3 31 1B 54 B2 16
>>>>     can1  573   [E]  20 8E A3 31 1B 54 B2 16
>>>>     can0  470   [3]  31 9C 06
>>>>     can1  470   [3]  31 9C 06
>>>>     can0  465   [4]  93 75 A2 08
>>>>     can1  465   [4]  93 75 A2 08
>>>>
>>>>
>>>> Works fine :-)
>>
>> I'm not really sure about the view of Classical CAN DLCs in this 'long'
>> candump representation.
>>
>> There are people that use this format as base for further processing -
>> instead of the logfile format m(
>> So it may be a bad decision to put the DLC value in the length position
>> here.
> 
> There was actually zero thought when I wrote this. I just wanted to
> test the kernel land and this was the first hack which came to my
> mind.

No problem. I would have chosen the same approach ...

> candump is not an easy one to modify. When putting the DLC in the
> picture, I can see three option:
> 
>    1. Do not modify the format. Nothing will break but it would be
>       impossible to know the actual DLC value of a Classical CAN frame
>       of length 8.
> 
>    2. Ask people doing processing on the candump output to modify their
>       tool if they want to use the cc-len8-dlc switch (if not using it,
>       should not break).
> 
>    3. The in-between solution: use a command line switch (e.g. candump
>       -8 any) select option 1 or 2 at run time.
> 
> None of the solution is perfect.

Right.

> I personally do not like option 3. So far, the command "candump any"
> would capture all the information. For example, their is no switch to
> enable/disable the capture of CAN-FD frames so it would seems odd to
> me to have it only for the DLC.

In fact candump tries to switch the socket into CAN FD mode and when it 
fails -> ¯\_(ツ)_/¯

But there is a difference in the standard output:
CAN FD frames have always two digits for the length, e.g. [32] or [04] 
while the standard Classical CAN frame has one digit, e.g. [5] or [0]

> Option 1 would be inconvenient for people who have needs similar to
> mine. Option 2 would be inconvenient for people who want to use
> cc-len8-dlc but do not want to touch their tools built on top of
> candump.
> 
> At the end of the day, I can not think of a perfect solution but I
> actually liked your previous idea of using parenthesis. The rule would
> be that [x] would designate a DLC of len2dlc(x) and a length of x; and
> (x) would designate a DLC of x and length of 8. On a security point of
> view, I think that the impact would be minimal. If we change the
> format form [x] to (x), parsing tools build on top should directly
> crash and the user should notice directly.

Yes. After you shared your thoughts I would indeed tend to provide an 
option '-8' and then always print the raw DLC in parenthesis e.g. (0) or 
(F).

IMO this len8_dlc stuff is an expert feature that normal users do not 
care about. Therefore it should be disabled by default.

>> I changed the logfile format in a backward compatible way and therefore
>> the cansend command line format too:
>>
>> (1604004658.444168) vcan0 2C0##02643
>> (1604004658.494492) vcan0 615#R8_9
>> (1604004658.645395) vcan0 6FF#
>> (1604004658.695682) vcan0 623##318F88F7043C0
>> (1604004658.746046) vcan0 63A##28D1F37
>> (1604004658.796214) vcan0 1117DEA5##0BC281D711098
>> (1604004658.846397) vcan0 1490B893#R8_F
>> (1604004658.896585) vcan0 1F494EC8##39C47E2
>> (1604004658.947142) vcan0 1D1EC448#3DC1C81345D7ED7E_D
>> (1604004658.997689) vcan0 1C8661BB##14DEA7568D459160D
>> (1604004659.047851) vcan0 2D8#AFE0546E6522130D_E
>> (1604004659.098216) vcan0 1596E833##3E6789514EF10B466E6789514EF10B466
> 
> Not the most sexy solution but I have to admit that it does the job
> and fits well with the other tools. Also, I am not able to think of a
> better solutions.

At least is is backwards compatible with older versions of canplayer and 
log2asc.

It doesn't win a design price but I asked my wife what to put as a 
len8_dlc delimiter. And the '_' won as it was still good to distinguish 
when looking at the log file format output.

> Not sure if anyone also uses this log format as base for further
> processing. Somewhere, someone might be impacted as well. But there is
> probably nothing that we can do about that.

Yes, I know people using the log file format - and lib.h and lib.c

> I will need more time to complete a full test and review.
> 
> Overall, I think that you did a good job in the naming convention and
> the choice of the command line options.

Thanks!

Best regards,
Oliver

Re: [PATCH RFC] can: add optional DLC element to Classical CAN frame structure

[Oliver Hartkopp]

Hi Vincent,

On 01.11.20 17:12, Oliver Hartkopp wrote:

> Yes. After you shared your thoughts I would indeed tend to provide an 
> option '-8' and then always print the raw DLC in parenthesis e.g. (0) or 
> (F).
> 
> IMO this len8_dlc stuff is an expert feature that normal users do not 
> care about. Therefore it should be disabled by default.

I implemented '-8' for your testing with curly braces {} which just 
looks 'more unusual' than ().

Please give it a try also from a visual standpoint if it fits for you.

Best,
Oliver

Re: [PATCH RFC] can: add optional DLC element to Classical CAN frame structure

[Vincent MAILHOL]

On 02.11.20 04:58, Oliver Hartkopp wrote:
> On 01.11.20 17:12, Oliver Hartkopp wrote:
>
>> Yes. After you shared your thoughts I would indeed tend to provide an
>> option '-8' and then always print the raw DLC in parenthesis e.g. (0) or
>> (F).
>>
>> IMO this len8_dlc stuff is an expert feature that normal users do not
>> care about. Therefore it should be disabled by default.

OK, it is a reasonable compromise.

> I implemented '-8' for your testing with curly braces {} which just
> looks 'more unusual' than ().
>
> Please give it a try also from a visual standpoint if it fits for you.

I did the test. As usual, it works fine.

Is the use of the hexadecimal notation a deliberate choice? I
initially did that because it is the easiest hack. I was expecting to
use a decimal representation in the finalized version but the
hexadecimal notation stands out even more and seems like a good
choice.

With both the curly braces and the hexadecimal representation, I do
not see how we could do something more visual.

Concerning the () vs. {}, honestly, that's a coin flip for me. No
preferences, both are fine.

Also, when receiving both Classical and FD frames, until now, the only
visual clue was the presence of the zero prefix: [x] for Classical
vs. [0x] for FD. I think that I might use the -8 option even when not
using CAN_CTRLMODE_CC_LEN8_DLC. Like that, the Classical frames would
show as {x} and the FD frames as [0x]: a way more visual clue than the
current one.

I will continue to think of other use cases for the other can-utils
programs.


Yours sincerely,
Vincent Mailhol