can: length: add definitions for frame lengths in bits

  When created in [1], frames length definitions were added to implement
byte queue limits (bql). Because bql expects lengths in bytes, bit
length definitions were not considered back then.

Recently, a need to refer to the exact frame length in bits, with CAN
bit stuffing, appeared in [2].

Add 9 frames length definitions:

 - CAN_FRAME_OVERHEAD_SFF_BITS:
 - CAN_FRAME_OVERHEAD_EFF_BITS
 - CANFD_FRAME_OVERHEAD_SFF_BITS
 - CANFD_FRAME_OVERHEAD_EFF_BITS
 - CAN_BIT_STUFFING_OVERHEAD
 - CAN_FRAME_LEN_MAX_BITS_NO_STUFFING
 - CAN_FRAME_LEN_MAX_BITS_STUFFING
 - CANFD_FRAME_LEN_MAX_BITS_NO_STUFFING
 - CANFD_FRAME_LEN_MAX_BITS_STUFFING

CAN_FRAME_LEN_MAX_BITS_STUFFING and CANFD_FRAME_LEN_MAX_BITS_STUFFING
define respectively the maximum number of bits in a classical CAN and
CAN-FD frame including bit stuffing. The other definitions are
intermediate values.

In addition to the above:

 - Include linux/bits.h and then replace the value 8 by BITS_PER_BYTE
   whenever relevant.
 - Include linux/math.h because of DIV_ROUND_UP(). N.B: the use of
   DIV_ROUND_UP() is not new to this patch, but the include was
   previously omitted.
 - Update the existing length definitions to use the newly defined values.
 - Add myself as copyright owner for 2020 (as coauthor of the initial
   version, c.f. [1]) and for 2023 (this patch).

[1] commit 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce
    function to get data length of frame in data link layer")
Link: https://git.kernel.org/torvalds/c/85d99c3e2a13

[2] RE: [PATCH] can: mcp251xfd: Increase poll timeout
Link: https://lore.kernel.org/linux-can/BL3PR11MB64846C83ACD04E9330B0FE66FB729@BL3PR11MB6484.namprd11.prod.outlook.com/

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
As always, let me know if you have better inspiration than me for the
naming.
---
 include/linux/can/length.h | 84 ++++++++++++++++++++++++++++++++------
 1 file changed, 72 insertions(+), 12 deletions(-)
  

> When created in [1], frames length definitions were added to implement
> byte queue limits (bql). Because bql expects lengths in bytes, bit
> length definitions were not considered back then.
> 
> Recently, a need to refer to the exact frame length in bits, with CAN
> bit stuffing, appeared in [2].
> 
> Add 9 frames length definitions:
> 
>  - CAN_FRAME_OVERHEAD_SFF_BITS:
>  - CAN_FRAME_OVERHEAD_EFF_BITS
>  - CANFD_FRAME_OVERHEAD_SFF_BITS
>  - CANFD_FRAME_OVERHEAD_EFF_BITS
>  - CAN_BIT_STUFFING_OVERHEAD
>  - CAN_FRAME_LEN_MAX_BITS_NO_STUFFING
>  - CAN_FRAME_LEN_MAX_BITS_STUFFING
>  - CANFD_FRAME_LEN_MAX_BITS_NO_STUFFING
>  - CANFD_FRAME_LEN_MAX_BITS_STUFFING
> 
> CAN_FRAME_LEN_MAX_BITS_STUFFING and
> CANFD_FRAME_LEN_MAX_BITS_STUFFING
> define respectively the maximum number of bits in a classical CAN and
> CAN-FD frame including bit stuffing. The other definitions are
> intermediate values.
> 
> In addition to the above:
> 
>  - Include linux/bits.h and then replace the value 8 by BITS_PER_BYTE
>    whenever relevant.
>  - Include linux/math.h because of DIV_ROUND_UP(). N.B: the use of
>    DIV_ROUND_UP() is not new to this patch, but the include was
>    previously omitted.
>  - Update the existing length definitions to use the newly defined values.
>  - Add myself as copyright owner for 2020 (as coauthor of the initial
>    version, c.f. [1]) and for 2023 (this patch).
> 
> [1] commit 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce
>     function to get data length of frame in data link layer")
> Link: https://git.kernel.org/torvalds/c/85d99c3e2a13
> 
> [2] RE: [PATCH] can: mcp251xfd: Increase poll timeout
> Link: https://lore.kernel.org/linux-
> can/BL3PR11MB64846C83ACD04E9330B0FE66FB729@BL3PR11MB6484.n
> amprd11.prod.outlook.com/
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
> As always, let me know if you have better inspiration than me for the
> naming.
> ---
>  include/linux/can/length.h | 84 ++++++++++++++++++++++++++++++++----
> --
>  1 file changed, 72 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/can/length.h b/include/linux/can/length.h
> index 6995092b774e..60492fcbe34d 100644
> --- a/include/linux/can/length.h
> +++ b/include/linux/can/length.h
> @@ -1,13 +1,17 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
>   * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
> + * Copyright (C) 2020, 2023 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>   */
> 
>  #ifndef _CAN_LENGTH_H
>  #define _CAN_LENGTH_H
> 
> +#include <linux/bits.h>
> +#include <linux/math.h>
> +
>  /*
> - * Size of a Classical CAN Standard Frame
> + * Size of a Classical CAN Standard Frame in bits
>   *
>   * Name of Field                       Bits
>   * ---------------------------------------------------------
> @@ -25,12 +29,19 @@
>   * End-of-frame (EOF)                  7
>   * Inter frame spacing                 3
>   *
> - * rounded up and ignoring bitstuffing
> + * ignoring bitstuffing
>   */
> -#define CAN_FRAME_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
> +#define CAN_FRAME_OVERHEAD_SFF_BITS 47
> 
>  /*
> - * Size of a Classical CAN Extended Frame
> + * Size of a Classical CAN Standard Frame
> + * (rounded up and ignoring bitstuffing)
> + */
> +#define CAN_FRAME_OVERHEAD_SFF \
> +       DIV_ROUND_UP(CAN_FRAME_OVERHEAD_SFF_BITS, BITS_PER_BYTE)
> +
> +/*
> + * Size of a Classical CAN Extended Frame in bits
>   *
>   * Name of Field                       Bits
>   * ---------------------------------------------------------
> @@ -50,12 +61,19 @@
>   * End-of-frame (EOF)                  7
>   * Inter frame spacing                 3
>   *
> - * rounded up and ignoring bitstuffing
> + * ignoring bitstuffing
>   */
> -#define CAN_FRAME_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
> +#define CAN_FRAME_OVERHEAD_EFF_BITS 67
> 
>  /*
> - * Size of a CAN-FD Standard Frame
> + * Size of a Classical CAN Extended Frame
> + * (rounded up and ignoring bitstuffing)
> + */
> +#define CAN_FRAME_OVERHEAD_EFF \
> +       DIV_ROUND_UP(CAN_FRAME_OVERHEAD_EFF_BITS, BITS_PER_BYTE)
> +
> +/*
> + * Size of a CAN-FD Standard Frame in bits
>   *
>   * Name of Field                       Bits
>   * ---------------------------------------------------------
> @@ -77,12 +95,19 @@
>   * End-of-frame (EOF)                  7
>   * Inter frame spacing                 3
>   *
> - * assuming CRC21, rounded up and ignoring bitstuffing
> + * assuming CRC21 and ignoring bitstuffing
>   */
> -#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(61, 8)
> +#define CANFD_FRAME_OVERHEAD_SFF_BITS 61
> 
>  /*
> - * Size of a CAN-FD Extended Frame
> + * Size of a CAN-FD Standard Frame
> + * (assuming CRC21, rounded up and ignoring bitstuffing)
> + */
> +#define CANFD_FRAME_OVERHEAD_SFF \
> +       DIV_ROUND_UP(CANFD_FRAME_OVERHEAD_SFF_BITS, BITS_PER_BYTE)
> +
> +/*
> + * Size of a CAN-FD Extended Frame in bits
>   *
>   * Name of Field                       Bits
>   * ---------------------------------------------------------
> @@ -106,9 +131,32 @@
>   * End-of-frame (EOF)                  7
>   * Inter frame spacing                 3
>   *
> - * assuming CRC21, rounded up and ignoring bitstuffing
> + * assuming CRC21 and ignoring bitstuffing
> + */
> +#define CANFD_FRAME_OVERHEAD_EFF_BITS 80
> +
> +/*
> + * Size of a CAN-FD Extended Frame
> + * (assuming CRC21, rounded up and ignoring bitstuffing)
> + */
> +#define CANFD_FRAME_OVERHEAD_EFF \
> +       DIV_ROUND_UP(CANFD_FRAME_OVERHEAD_EFF_BITS, BITS_PER_BYTE)
> +
> +/* CAN bit stuffing overhead multiplication factor */
> +#define CAN_BIT_STUFFING_OVERHEAD 1.2
> +
> +/*
> + * Maximum size of a Classical CAN frame in bits, ignoring bitstuffing
>   */
> -#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(80, 8)
> +#define CAN_FRAME_LEN_MAX_BITS_NO_STUFFING \
> +       (CAN_FRAME_OVERHEAD_EFF_BITS + CAN_MAX_DLEN *
> BITS_PER_BYTE)
> +
> +/*
> + * Maximum size of a Classical CAN frame in bits, including bitstuffing
> + */
> +#define CAN_FRAME_LEN_MAX_BITS_STUFFING                                \
> +       ((unsigned int)(CAN_FRAME_LEN_MAX_BITS_NO_STUFFING *    \
> +                       CAN_BIT_STUFFING_OVERHEAD))
> 
>  /*
>   * Maximum size of a Classical CAN frame
> @@ -116,6 +164,18 @@
>   */
>  #define CAN_FRAME_LEN_MAX (CAN_FRAME_OVERHEAD_EFF +
> CAN_MAX_DLEN)
> 
> +/*
> + * Maximum size of a CAN-FD frame in bits, ignoring bitstuffing
> + */
> +#define CANFD_FRAME_LEN_MAX_BITS_NO_STUFFING \
> +       (CANFD_FRAME_OVERHEAD_EFF_BITS + CANFD_MAX_DLEN *
> BITS_PER_BYTE)
> +
> +/*
> + * Maximum size of a CAN-FD frame in bits, ignoring bitstuffing
> + */
> +#define CANFD_FRAME_LEN_MAX_BITS_STUFFING                      \
> +       ((unsigned int)(CANFD_FRAME_LEN_MAX_BITS_NO_STUFFING *  \
> +                       CAN_BIT_STUFFING_OVERHEAD))
>  /*
>   * Maximum size of a CAN-FD frame
>   * (rounded up and ignoring bitstuffing)
> --
I was working on the same thing on Friday but didn't get around to sending it off, so here are a couple thoughts I had when working on the defines in length.h

The definitions for IFS in here are called intermission in the standard and I'd argue they shouldn't be part of the frame at all. To quote the ISO: "DFs and RFs shall be separated from preceding frames, whatever frame type they are (DF, RF, EF, OF),  by a time period called inter-frame space."
So, my suggestion would be to pull out the 3 bit IFS definition that's currently in and introduce 11 bit Bus idle and if necessary 3 bit Intermission separately.

index 6995092b774ec..62e92c1553376 100644
--- a/include/linux/can/length.h
+++ b/include/linux/can/length.h
@@ -6,6 +6,26 @@
 #ifndef _CAN_LENGTH_H
 #define _CAN_LENGTH_H

+/*
+ * First part of the Inter Frame Space
+ */
+#define CAN_INTERMISSION_BITS 3
+
+/*
+ * Number of consecutive recessive bits on the bus for integration etc.
+ */
+#define CAN_IDLE_CONDITION_BITS 11
+

The field currently called Stuff bit count (SBC) is also not correct I'd say. I'm not sure about the history but given that this is dependent on the DLC I think what's meant is the number of Fixed Stuff bits (FSB) . The ISO does not define a term for the Stuff bit Count but the CiA did define/document it this way. What's meant though is not the number of fixed stuff bits (FSB) which the comment implies here but the modulo 8 3 bit gray-code followed by the parity bit. So for the FD frame definitions I'd propose something like this: Renaming the current SBC to FSB and adding the SBC.
/*
  * Size of a CAN-FD Standard Frame
@@ -69,17 +87,17 @@
  * Error Status Indicator (ESI)                1
  * Data length code (DLC)              4
  * Data field                          0...512
- * Stuff Bit Count (SBC)               0...16: 4 20...64:5
+ * Stuff Bit Count (SBC)               4
  * CRC                                 0...16: 17 20...64:21
  * CRC delimiter (CD)                  1
+ * Fixed Stuff bits (FSB)              0...16: 6 20...64:7
  * ACK slot (AS)                       1
  * ACK delimiter (AD)                  1
  * End-of-frame (EOF)                  7
- * Inter frame spacing                 3
  *
- * assuming CRC21, rounded up and ignoring bitstuffing
+ * assuming CRC21, rounded up and ignoring dynamic bitstuffing
  */

Best Regards,
Thomas
  

On 08.05.2023 08:54:18, Thomas.Kopp@microchip.com wrote:
> I was working on the same thing on Friday but didn't get around to
> sending it off, so here are a couple thoughts I had when working on
> the defines in length.h
> 
> The definitions for IFS in here are called intermission in the
> standard

ACK, and IMF seems to be a common abbreviation.

> and I'd argue they shouldn't be part of the frame at all.

The diagram in https://www.can-cia.org/can-knowledge/can/can-fd/
suggests that IMF is part of the frame.

> To
> quote the ISO: "DFs and RFs shall be separated from preceding frames,
> whatever frame type they are (DF, RF, EF, OF), by a time period called
> inter-frame space."
> 
> So, my suggestion would be to pull out the 3 bit IFS definition that's
> currently in and introduce 11 bit Bus idle and if necessary 3 bit
> Intermission separately.
> 
> index 6995092b774ec..62e92c1553376 100644
> --- a/include/linux/can/length.h
> +++ b/include/linux/can/length.h
> @@ -6,6 +6,26 @@
>  #ifndef _CAN_LENGTH_H
>  #define _CAN_LENGTH_H
> 
> +/*
> + * First part of the Inter Frame Space
> + */
> +#define CAN_INTERMISSION_BITS 3
> +
> +/*
> + * Number of consecutive recessive bits on the bus for integration etc.
> + */
> +#define CAN_IDLE_CONDITION_BITS 11
> +
> 
> The field currently called Stuff bit count (SBC) is also not correct
> I'd say. I'm not sure about the history but given that this is
> dependent on the DLC I think what's meant is the number of Fixed Stuff
> bits (FSB) . The ISO does not define a term for the Stuff bit Count
> but the CiA did define/document it this way. What's meant though is
> not the number of fixed stuff bits (FSB) which the comment implies
> here but the modulo 8 3 bit gray-code followed by the parity bit. So
> for the FD frame definitions I'd propose something like this: Renaming
> the current SBC to FSB and adding the SBC.

> /*
>   * Size of a CAN-FD Standard Frame
> @@ -69,17 +87,17 @@
>   * Error Status Indicator (ESI)                1
>   * Data length code (DLC)              4
>   * Data field                          0...512
> - * Stuff Bit Count (SBC)               0...16: 4 20...64:5
> + * Stuff Bit Count (SBC)               4

ACK

>   * CRC                                 0...16: 17 20...64:21
>   * CRC delimiter (CD)                  1
> + * Fixed Stuff bits (FSB)              0...16: 6 20...64:7

As far as I understand
https://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=8338047 the FSB
is 5 or 6.

>   * ACK slot (AS)                       1
>   * ACK delimiter (AD)                  1
>   * End-of-frame (EOF)                  7
> - * Inter frame spacing                 3
>   *
> - * assuming CRC21, rounded up and ignoring bitstuffing
> + * assuming CRC21, rounded up and ignoring dynamic bitstuffing
>   */
> 
> Best Regards,
> Thomas
> 

Marc
  

On 08.05.2023 00:55:06, Vincent Mailhol wrote:
> When created in [1], frames length definitions were added to implement
> byte queue limits (bql). Because bql expects lengths in bytes, bit
> length definitions were not considered back then.
> 
> Recently, a need to refer to the exact frame length in bits, with CAN
> bit stuffing, appeared in [2].
> 
> Add 9 frames length definitions:
> 
>  - CAN_FRAME_OVERHEAD_SFF_BITS:
>  - CAN_FRAME_OVERHEAD_EFF_BITS
>  - CANFD_FRAME_OVERHEAD_SFF_BITS
>  - CANFD_FRAME_OVERHEAD_EFF_BITS
>  - CAN_BIT_STUFFING_OVERHEAD
>  - CAN_FRAME_LEN_MAX_BITS_NO_STUFFING
>  - CAN_FRAME_LEN_MAX_BITS_STUFFING
>  - CANFD_FRAME_LEN_MAX_BITS_NO_STUFFING
>  - CANFD_FRAME_LEN_MAX_BITS_STUFFING
> 
> CAN_FRAME_LEN_MAX_BITS_STUFFING and CANFD_FRAME_LEN_MAX_BITS_STUFFING
> define respectively the maximum number of bits in a classical CAN and
> CAN-FD frame including bit stuffing. The other definitions are
> intermediate values.
> 
> In addition to the above:
> 
>  - Include linux/bits.h and then replace the value 8 by BITS_PER_BYTE
>    whenever relevant.
>  - Include linux/math.h because of DIV_ROUND_UP(). N.B: the use of
>    DIV_ROUND_UP() is not new to this patch, but the include was
>    previously omitted.
>  - Update the existing length definitions to use the newly defined values.
>  - Add myself as copyright owner for 2020 (as coauthor of the initial
>    version, c.f. [1]) and for 2023 (this patch).
> 
> [1] commit 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce
>     function to get data length of frame in data link layer")
> Link: https://git.kernel.org/torvalds/c/85d99c3e2a13
> 
> [2] RE: [PATCH] can: mcp251xfd: Increase poll timeout
> Link: https://lore.kernel.org/linux-can/BL3PR11MB64846C83ACD04E9330B0FE66FB729@BL3PR11MB6484.namprd11.prod.outlook.com/
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
> As always, let me know if you have better inspiration than me for the
> naming.
> ---
>  include/linux/can/length.h | 84 ++++++++++++++++++++++++++++++++------
>  1 file changed, 72 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/can/length.h b/include/linux/can/length.h
> index 6995092b774e..60492fcbe34d 100644
> --- a/include/linux/can/length.h
> +++ b/include/linux/can/length.h
> @@ -1,13 +1,17 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
>   * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
> + * Copyright (C) 2020, 2023 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>   */
>  
>  #ifndef _CAN_LENGTH_H
>  #define _CAN_LENGTH_H
>  
> +#include <linux/bits.h>
> +#include <linux/math.h>
> +
>  /*
> - * Size of a Classical CAN Standard Frame
> + * Size of a Classical CAN Standard Frame in bits
>   *
>   * Name of Field			Bits
>   * ---------------------------------------------------------
> @@ -25,12 +29,19 @@
>   * End-of-frame (EOF)			7
>   * Inter frame spacing			3
>   *
> - * rounded up and ignoring bitstuffing
> + * ignoring bitstuffing
>   */
> -#define CAN_FRAME_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
> +#define CAN_FRAME_OVERHEAD_SFF_BITS 47
>  
>  /*
> - * Size of a Classical CAN Extended Frame
> + * Size of a Classical CAN Standard Frame
> + * (rounded up and ignoring bitstuffing)
> + */
> +#define CAN_FRAME_OVERHEAD_SFF \
> +	DIV_ROUND_UP(CAN_FRAME_OVERHEAD_SFF_BITS, BITS_PER_BYTE)
> +
> +/*
> + * Size of a Classical CAN Extended Frame in bits
>   *
>   * Name of Field			Bits
>   * ---------------------------------------------------------
> @@ -50,12 +61,19 @@
>   * End-of-frame (EOF)			7
>   * Inter frame spacing			3
>   *
> - * rounded up and ignoring bitstuffing
> + * ignoring bitstuffing
>   */
> -#define CAN_FRAME_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
> +#define CAN_FRAME_OVERHEAD_EFF_BITS 67
>  
>  /*
> - * Size of a CAN-FD Standard Frame
> + * Size of a Classical CAN Extended Frame
> + * (rounded up and ignoring bitstuffing)
> + */
> +#define CAN_FRAME_OVERHEAD_EFF \
> +	DIV_ROUND_UP(CAN_FRAME_OVERHEAD_EFF_BITS, BITS_PER_BYTE)
> +
> +/*
> + * Size of a CAN-FD Standard Frame in bits
>   *
>   * Name of Field			Bits
>   * ---------------------------------------------------------
> @@ -77,12 +95,19 @@
>   * End-of-frame (EOF)			7
>   * Inter frame spacing			3
>   *
> - * assuming CRC21, rounded up and ignoring bitstuffing
> + * assuming CRC21 and ignoring bitstuffing
>   */
> -#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(61, 8)
> +#define CANFD_FRAME_OVERHEAD_SFF_BITS 61
>  
>  /*
> - * Size of a CAN-FD Extended Frame
> + * Size of a CAN-FD Standard Frame
> + * (assuming CRC21, rounded up and ignoring bitstuffing)
> + */
> +#define CANFD_FRAME_OVERHEAD_SFF \
> +	DIV_ROUND_UP(CANFD_FRAME_OVERHEAD_SFF_BITS, BITS_PER_BYTE)
> +
> +/*
> + * Size of a CAN-FD Extended Frame in bits
>   *
>   * Name of Field			Bits
>   * ---------------------------------------------------------
> @@ -106,9 +131,32 @@
>   * End-of-frame (EOF)			7
>   * Inter frame spacing			3
>   *
> - * assuming CRC21, rounded up and ignoring bitstuffing
> + * assuming CRC21 and ignoring bitstuffing
> + */
> +#define CANFD_FRAME_OVERHEAD_EFF_BITS 80
> +
> +/*
> + * Size of a CAN-FD Extended Frame
> + * (assuming CRC21, rounded up and ignoring bitstuffing)
> + */
> +#define CANFD_FRAME_OVERHEAD_EFF \
> +	DIV_ROUND_UP(CANFD_FRAME_OVERHEAD_EFF_BITS, BITS_PER_BYTE)
> +
> +/* CAN bit stuffing overhead multiplication factor */
> +#define CAN_BIT_STUFFING_OVERHEAD 1.2
> +
> +/*
> + * Maximum size of a Classical CAN frame in bits, ignoring bitstuffing
>   */
> -#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(80, 8)
> +#define CAN_FRAME_LEN_MAX_BITS_NO_STUFFING \
> +	(CAN_FRAME_OVERHEAD_EFF_BITS + CAN_MAX_DLEN * BITS_PER_BYTE)
> +
> +/*
> + * Maximum size of a Classical CAN frame in bits, including bitstuffing
> + */
> +#define CAN_FRAME_LEN_MAX_BITS_STUFFING				\
> +	((unsigned int)(CAN_FRAME_LEN_MAX_BITS_NO_STUFFING *	\
> +			CAN_BIT_STUFFING_OVERHEAD))

The 1.2 overhead doesn't apply to the whole frame, according to
https://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=8338047.

Marc
  

Hi Thomas,

Thank you for your review. You had me reopen ISO 11898-1 (haven't done
so for a long time).

I mostly agree with you. Many of your points are not related to this
patch but to the already existing definition. So I will handle these
in a separate patch.

I will prepare a v2 within the next few days.

On Mon. 8 May 2023 à 21:29, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 08.05.2023 08:54:18, Thomas.Kopp@microchip.com wrote:
> > I was working on the same thing on Friday but didn't get around to
> > sending it off, so here are a couple thoughts I had when working on
> > the defines in length.h
> >
> > The definitions for IFS in here are called intermission in the
> > standard
>
> ACK, and IMF seems to be a common abbreviation.

ACK. I think I will put a reference to both naming (intermission and
IMF) and use the IMF forward for conciseness.

> > and I'd argue they shouldn't be part of the frame at all.
>
> The diagram in https://www.can-cia.org/can-knowledge/can/can-fd/
> suggests that IMF is part of the frame.

ISO 11898-1:2015 section 10.4.6 "Specification of inter-frame space"
makes it clear that the intermission is not part of the frame but part
of the "Inter-frame space".

To close the discussion, I would finally argue that the intermission
occurs after the EOF bit. Something after an "End of Frame" is not
part of the frame, right?

I agree with and will follow Thomas's suggestion.

> > To
> > quote the ISO: "DFs and RFs shall be separated from preceding frames,
> > whatever frame type they are (DF, RF, EF, OF), by a time period called
> > inter-frame space."
> >
> > So, my suggestion would be to pull out the 3 bit IFS definition that's
> > currently in and introduce 11 bit Bus idle and if necessary 3 bit
> > Intermission separately.

ACK.

> > index 6995092b774ec..62e92c1553376 100644
> > --- a/include/linux/can/length.h
> > +++ b/include/linux/can/length.h
> > @@ -6,6 +6,26 @@
> >  #ifndef _CAN_LENGTH_H
> >  #define _CAN_LENGTH_H
> >
> > +/*
> > + * First part of the Inter Frame Space
> > + */
> > +#define CAN_INTERMISSION_BITS 3
> > +
> > +/*
> > + * Number of consecutive recessive bits on the bus for integration etc.
> > + */
> > +#define CAN_IDLE_CONDITION_BITS 11
> > +
> >
> > The field currently called Stuff bit count (SBC) is also not correct
> > I'd say. I'm not sure about the history but given that this is
> > dependent on the DLC I think what's meant is the number of Fixed Stuff
> > bits (FSB) . The ISO does not define a term for the Stuff bit Count
> > but the CiA did define/document it this way. What's meant though is
> > not the number of fixed stuff bits (FSB) which the comment implies
> > here but the modulo 8 3 bit gray-code followed by the parity bit. So
> > for the FD frame definitions I'd propose something like this: Renaming
> > the current SBC to FSB and adding the SBC.

ACK. I double checked the standard, you are right.

> > /*
> >   * Size of a CAN-FD Standard Frame
> > @@ -69,17 +87,17 @@
> >   * Error Status Indicator (ESI)                1
> >   * Data length code (DLC)              4
> >   * Data field                          0...512
> > - * Stuff Bit Count (SBC)               0...16: 4 20...64:5
> > + * Stuff Bit Count (SBC)               4
>
> ACK
>
> >   * CRC                                 0...16: 17 20...64:21
> >   * CRC delimiter (CD)                  1
> > + * Fixed Stuff bits (FSB)              0...16: 6 20...64:7
>
> As far as I understand
> https://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=8338047 the FSB
> is 5 or 6.

Reading the ISO, the fixed bit stuff applies to the CRC field (which,
in the standard nomenclature, includes both the stuff count and the
CRC sequence).
The CRC field starts with a fixed stuff bit and then has another fixed
stuff bit after each fourth bit.

So the formula would be:

  FSB count = 1 + round_down(len(CRC field)/4)
            = 1 + round_down((len(stuff count) + len(CRC sequence))/4)

In case of CRC_17:

  FSB count = 1 + round_down((4 + 17)/4)
            = 6

This is coherent with the last figure of
https://www.can-cia.org/can-knowledge/can/can-fd/ which shows 6 FSB
for CRC_17.

In case of CRC_21:

  FSB count = 1 + round_down((4 + 21)/4)
            = 7

So, ACK for Thomas, NACK for Marc (sorry :))

> >   * ACK slot (AS)                       1
> >   * ACK delimiter (AD)                  1
> >   * End-of-frame (EOF)                  7
> > - * Inter frame spacing                 3
> >   *
> > - * assuming CRC21, rounded up and ignoring bitstuffing
> > + * assuming CRC21, rounded up and ignoring dynamic bitstuffing
> >   */

Yours sincerely,
Vincent Mailhol
  

On Mon. 8 May 2023 at 21:29, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 08.05.2023 00:55:06, Vincent Mailhol wrote:
> > When created in [1], frames length definitions were added to implement
> > byte queue limits (bql). Because bql expects lengths in bytes, bit
> > length definitions were not considered back then.
> >
> > Recently, a need to refer to the exact frame length in bits, with CAN
> > bit stuffing, appeared in [2].
> >
> > Add 9 frames length definitions:
> >
> >  - CAN_FRAME_OVERHEAD_SFF_BITS:
> >  - CAN_FRAME_OVERHEAD_EFF_BITS
> >  - CANFD_FRAME_OVERHEAD_SFF_BITS
> >  - CANFD_FRAME_OVERHEAD_EFF_BITS
> >  - CAN_BIT_STUFFING_OVERHEAD
> >  - CAN_FRAME_LEN_MAX_BITS_NO_STUFFING
> >  - CAN_FRAME_LEN_MAX_BITS_STUFFING
> >  - CANFD_FRAME_LEN_MAX_BITS_NO_STUFFING
> >  - CANFD_FRAME_LEN_MAX_BITS_STUFFING
> >
> > CAN_FRAME_LEN_MAX_BITS_STUFFING and CANFD_FRAME_LEN_MAX_BITS_STUFFING
> > define respectively the maximum number of bits in a classical CAN and
> > CAN-FD frame including bit stuffing. The other definitions are
> > intermediate values.
> >
> > In addition to the above:
> >
> >  - Include linux/bits.h and then replace the value 8 by BITS_PER_BYTE
> >    whenever relevant.
> >  - Include linux/math.h because of DIV_ROUND_UP(). N.B: the use of
> >    DIV_ROUND_UP() is not new to this patch, but the include was
> >    previously omitted.
> >  - Update the existing length definitions to use the newly defined values.
> >  - Add myself as copyright owner for 2020 (as coauthor of the initial
> >    version, c.f. [1]) and for 2023 (this patch).
> >
> > [1] commit 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce
> >     function to get data length of frame in data link layer")
> > Link: https://git.kernel.org/torvalds/c/85d99c3e2a13
> >
> > [2] RE: [PATCH] can: mcp251xfd: Increase poll timeout
> > Link: https://lore.kernel.org/linux-can/BL3PR11MB64846C83ACD04E9330B0FE66FB729@BL3PR11MB6484.namprd11.prod.outlook.com/
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > ---
> > As always, let me know if you have better inspiration than me for the
> > naming.
> > ---
> >  include/linux/can/length.h | 84 ++++++++++++++++++++++++++++++++------
> >  1 file changed, 72 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/linux/can/length.h b/include/linux/can/length.h
> > index 6995092b774e..60492fcbe34d 100644
> > --- a/include/linux/can/length.h
> > +++ b/include/linux/can/length.h
> > @@ -1,13 +1,17 @@
> >  /* SPDX-License-Identifier: GPL-2.0 */
> >  /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
> >   * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
> > + * Copyright (C) 2020, 2023 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >   */
> >
> >  #ifndef _CAN_LENGTH_H
> >  #define _CAN_LENGTH_H
> >
> > +#include <linux/bits.h>
> > +#include <linux/math.h>
> > +
> >  /*
> > - * Size of a Classical CAN Standard Frame
> > + * Size of a Classical CAN Standard Frame in bits
> >   *
> >   * Name of Field                     Bits
> >   * ---------------------------------------------------------
> > @@ -25,12 +29,19 @@
> >   * End-of-frame (EOF)                        7
> >   * Inter frame spacing                       3
> >   *
> > - * rounded up and ignoring bitstuffing
> > + * ignoring bitstuffing
> >   */
> > -#define CAN_FRAME_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
> > +#define CAN_FRAME_OVERHEAD_SFF_BITS 47
> >
> >  /*
> > - * Size of a Classical CAN Extended Frame
> > + * Size of a Classical CAN Standard Frame
> > + * (rounded up and ignoring bitstuffing)
> > + */
> > +#define CAN_FRAME_OVERHEAD_SFF \
> > +     DIV_ROUND_UP(CAN_FRAME_OVERHEAD_SFF_BITS, BITS_PER_BYTE)
> > +
> > +/*
> > + * Size of a Classical CAN Extended Frame in bits
> >   *
> >   * Name of Field                     Bits
> >   * ---------------------------------------------------------
> > @@ -50,12 +61,19 @@
> >   * End-of-frame (EOF)                        7
> >   * Inter frame spacing                       3
> >   *
> > - * rounded up and ignoring bitstuffing
> > + * ignoring bitstuffing
> >   */
> > -#define CAN_FRAME_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
> > +#define CAN_FRAME_OVERHEAD_EFF_BITS 67
> >
> >  /*
> > - * Size of a CAN-FD Standard Frame
> > + * Size of a Classical CAN Extended Frame
> > + * (rounded up and ignoring bitstuffing)
> > + */
> > +#define CAN_FRAME_OVERHEAD_EFF \
> > +     DIV_ROUND_UP(CAN_FRAME_OVERHEAD_EFF_BITS, BITS_PER_BYTE)
> > +
> > +/*
> > + * Size of a CAN-FD Standard Frame in bits
> >   *
> >   * Name of Field                     Bits
> >   * ---------------------------------------------------------
> > @@ -77,12 +95,19 @@
> >   * End-of-frame (EOF)                        7
> >   * Inter frame spacing                       3
> >   *
> > - * assuming CRC21, rounded up and ignoring bitstuffing
> > + * assuming CRC21 and ignoring bitstuffing
> >   */
> > -#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(61, 8)
> > +#define CANFD_FRAME_OVERHEAD_SFF_BITS 61
> >
> >  /*
> > - * Size of a CAN-FD Extended Frame
> > + * Size of a CAN-FD Standard Frame
> > + * (assuming CRC21, rounded up and ignoring bitstuffing)
> > + */
> > +#define CANFD_FRAME_OVERHEAD_SFF \
> > +     DIV_ROUND_UP(CANFD_FRAME_OVERHEAD_SFF_BITS, BITS_PER_BYTE)
> > +
> > +/*
> > + * Size of a CAN-FD Extended Frame in bits
> >   *
> >   * Name of Field                     Bits
> >   * ---------------------------------------------------------
> > @@ -106,9 +131,32 @@
> >   * End-of-frame (EOF)                        7
> >   * Inter frame spacing                       3
> >   *
> > - * assuming CRC21, rounded up and ignoring bitstuffing
> > + * assuming CRC21 and ignoring bitstuffing
> > + */
> > +#define CANFD_FRAME_OVERHEAD_EFF_BITS 80
> > +
> > +/*
> > + * Size of a CAN-FD Extended Frame
> > + * (assuming CRC21, rounded up and ignoring bitstuffing)
> > + */
> > +#define CANFD_FRAME_OVERHEAD_EFF \
> > +     DIV_ROUND_UP(CANFD_FRAME_OVERHEAD_EFF_BITS, BITS_PER_BYTE)
> > +
> > +/* CAN bit stuffing overhead multiplication factor */
> > +#define CAN_BIT_STUFFING_OVERHEAD 1.2
> > +
> > +/*
> > + * Maximum size of a Classical CAN frame in bits, ignoring bitstuffing
> >   */
> > -#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(80, 8)
> > +#define CAN_FRAME_LEN_MAX_BITS_NO_STUFFING \
> > +     (CAN_FRAME_OVERHEAD_EFF_BITS + CAN_MAX_DLEN * BITS_PER_BYTE)
> > +
> > +/*
> > + * Maximum size of a Classical CAN frame in bits, including bitstuffing
> > + */
> > +#define CAN_FRAME_LEN_MAX_BITS_STUFFING                              \
> > +     ((unsigned int)(CAN_FRAME_LEN_MAX_BITS_NO_STUFFING *    \
> > +                     CAN_BIT_STUFFING_OVERHEAD))
>
> The 1.2 overhead doesn't apply to the whole frame, according to
> https://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=8338047.

You are right. In fact, I realized this mistake before reading your
message (while I was studying the standard to answer Thomas).

ISO 11898-1:2015 section 10.5 "Frame coding" says:

  the frame segment as SOF, arbitration field, control field,
  data field, and CRC sequence shall be coded by the method of
  bit stuffing.

and:

  The remaining bit fields of the DF or RF (CRC delimiter, ACK
  field and EOF) shall be of fixed form and not stuffed.

So, indeed, the bit stuffing does not apply to the last 10 bits (1 + 1 + 1 + 7).
Furthermore, for FD frames, the CRC field already contains the fixed
stuff bits. So the overhead shall not be applied again or else, stuff
bits would be counted twice. In conclusion, for FD frames, the
otherhead for dynamic bit stuffing overhead should apply to the SOF,
arbitration field, control field and data field segments.

After reading the standard, I thought again about the overhead ratio
and it is more complicated than we all thought.

Let's use below nomenclature in the following examples (borrowed from ISO):

  - "0": dominant bit
  - "o": dominant stuff bit
  - "1": recessive bit
  - "i": recessive stuff bit

We probably all though below example to be the worst case:

  Destuffed: 11111  11111  11111  11111
  Stuffed:   11111o 11111o 11111o 11111o

Here, indeed, one stuff bit is added every five bits giving us an
overhead of 6/5 = 1.2.

However, ISO 11898-1:2015 section 10.5 "Frame coding" also says:

  Whenever a transmitter detects five consecutive bits (*including
  stuff bits*) of identical value in the bit stream to be
  transmitted, it shall automatically insert a complementary
  bit (called stuff bit) ...

Pay attention to the *including stuff bits* part. The worst case is
actually a sequence in which dominant and recessive alternate every
four bits:

  Destuffed: 1 1111  0000  1111  0000  1111
  Stuffed:   1 1111o 0000i 1111o 0000i 1111o

Ignoring the first bit, one stuff bit is added every four bits giving
us an overhead of 5/4 = 1.25.

The exact formula taking in account the first bit we previously ignored then:

  Number of dynamic stuff bit = 1 + round_down((len(stream) - 5) / 4)
                              = round_down((len(stream) - 1) / 4)


Yours sincerely,
Vincent Mailhol
  

> On 08.05.2023 08:54:18, Thomas.Kopp@microchip.com wrote:
> > I was working on the same thing on Friday but didn't get around to
> > sending it off, so here are a couple thoughts I had when working on
> > the defines in length.h
> >
> > The definitions for IFS in here are called intermission in the
> > standard
> 
> ACK, and IMF seems to be a common abbreviation.
> 
> > and I'd argue they shouldn't be part of the frame at all.
> 
> The diagram in https://www.can-cia.org/can-knowledge/can/can-fd/
> suggests that IMF is part of the frame.

I'd disagree as the ISO specifically says it's not part of the frame. The diagram on page PDF page 21 of the 2.0 spec: http://esd.cs.ucr.edu/webres/can20.pdf is also in the ISO and shows the Intermission outside the frame. Also the word INTERframe space suggests it shouldn't be part of the frame and lastly the definition is used for a) determining how many bits/bytes are needed to store frames which doesn't need the intermission bits and b) timing, but for those purpose the frame has ended already and if the timing of several frames is needed, complete  interframe spaces need to be added.

> > To
> > quote the ISO: "DFs and RFs shall be separated from preceding frames,
> > whatever frame type they are (DF, RF, EF, OF), by a time period called
> > inter-frame space."
> >
> > So, my suggestion would be to pull out the 3 bit IFS definition that's
> > currently in and introduce 11 bit Bus idle and if necessary 3 bit
> > Intermission separately.
> >
> > index 6995092b774ec..62e92c1553376 100644
> > --- a/include/linux/can/length.h
> > +++ b/include/linux/can/length.h
> > @@ -6,6 +6,26 @@
> >  #ifndef _CAN_LENGTH_H
> >  #define _CAN_LENGTH_H
> >
> > +/*
> > + * First part of the Inter Frame Space
> > + */
> > +#define CAN_INTERMISSION_BITS 3
> > +
> > +/*
> > + * Number of consecutive recessive bits on the bus for integration etc.
> > + */
> > +#define CAN_IDLE_CONDITION_BITS 11
> > +
> >
> > The field currently called Stuff bit count (SBC) is also not correct
> > I'd say. I'm not sure about the history but given that this is
> > dependent on the DLC I think what's meant is the number of Fixed Stuff
> > bits (FSB) . The ISO does not define a term for the Stuff bit Count
> > but the CiA did define/document it this way. What's meant though is
> > not the number of fixed stuff bits (FSB) which the comment implies
> > here but the modulo 8 3 bit gray-code followed by the parity bit. So
> > for the FD frame definitions I'd propose something like this: Renaming
> > the current SBC to FSB and adding the SBC.
> 
> >   * CRC                                 0...16: 17 20...64:21
> >   * CRC delimiter (CD)                  1
> > + * Fixed Stuff bits (FSB)              0...16: 6 20...64:7
> 
> As far as I understand
> https://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=8338047 the
> FSB
> is 5 or 6.
I don't know where the paper got its numbers from but it also seems to be missing the SBC field completely? The ISO says: " There shall be a fixed stuff bit before the first bit of the stuff count[...]" " A further fixed stuff bit shall be inserted after each fourth bit of the CRC field" Not that the CRC field in FD frames also contains the SBC so that adds fixed stuff bits.
A good visual representation of the FSBs is on the first page you provided as a source: https://www.can-cia.org/can-knowledge/can/can-fd/ all the way on the bottom. 


Best Regards,
Thomas
  

On 09.05.2023 13:16:08, Vincent MAILHOL wrote:
> > The diagram in https://www.can-cia.org/can-knowledge/can/can-fd/
> > suggests that IMF is part of the frame.
> 
> ISO 11898-1:2015 section 10.4.6 "Specification of inter-frame space"
> makes it clear that the intermission is not part of the frame but part
> of the "Inter-frame space".

For this reason, it is good to have open standards...oh wait!

> To close the discussion, I would finally argue that the intermission
> occurs after the EOF bit. Something after an "End of Frame" is not
> part of the frame, right?
> 
> I agree with and will follow Thomas's suggestion.

[...]

> > > /*
> > >   * Size of a CAN-FD Standard Frame
> > > @@ -69,17 +87,17 @@
> > >   * Error Status Indicator (ESI)                1
> > >   * Data length code (DLC)              4
> > >   * Data field                          0...512
> > > - * Stuff Bit Count (SBC)               0...16: 4 20...64:5
> > > + * Stuff Bit Count (SBC)               4
> >
> > ACK
> >
> > >   * CRC                                 0...16: 17 20...64:21
> > >   * CRC delimiter (CD)                  1
> > > + * Fixed Stuff bits (FSB)              0...16: 6 20...64:7
> >
> > As far as I understand
> > https://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=8338047 the FSB
> > is 5 or 6.
> 
> Reading the ISO, the fixed bit stuff applies to the CRC field (which,
> in the standard nomenclature, includes both the stuff count and the
> CRC sequence).
> The CRC field starts with a fixed stuff bit and then has another fixed
> stuff bit after each fourth bit.
> 
> So the formula would be:
> 
>   FSB count = 1 + round_down(len(CRC field)/4)
>             = 1 + round_down((len(stuff count) + len(CRC sequence))/4)
> 
> In case of CRC_17:
> 
>   FSB count = 1 + round_down((4 + 17)/4)
>             = 6
> 
> This is coherent with the last figure of
> https://www.can-cia.org/can-knowledge/can/can-fd/ which shows 6 FSB
> for CRC_17.
> 
> In case of CRC_21:
> 
>   FSB count = 1 + round_down((4 + 21)/4)
>             = 7
> 
> So, ACK for Thomas, NACK for Marc (sorry :))

It seems that the reviewers of this paper missed some parts :)

Marc
  

Hi Vincent,

> On Mon. 8 May 2023 at 21:29, Marc Kleine-Budde <mkl@pengutronix.de>
> wrote:
> > On 08.05.2023 00:55:06, Vincent Mailhol wrote:
> > > When created in [1], frames length definitions were added to implement
> > > byte queue limits (bql). Because bql expects lengths in bytes, bit
> > > length definitions were not considered back then.
> > >
> > > Recently, a need to refer to the exact frame length in bits, with CAN
> > > bit stuffing, appeared in [2].
> > >
> > > Add 9 frames length definitions:
> > >
> > >  - CAN_FRAME_OVERHEAD_SFF_BITS:
> > >  - CAN_FRAME_OVERHEAD_EFF_BITS
> > >  - CANFD_FRAME_OVERHEAD_SFF_BITS
> > >  - CANFD_FRAME_OVERHEAD_EFF_BITS
> > >  - CAN_BIT_STUFFING_OVERHEAD
> > >  - CAN_FRAME_LEN_MAX_BITS_NO_STUFFING
> > >  - CAN_FRAME_LEN_MAX_BITS_STUFFING
> > >  - CANFD_FRAME_LEN_MAX_BITS_NO_STUFFING
> > >  - CANFD_FRAME_LEN_MAX_BITS_STUFFING
> > >
> > > CAN_FRAME_LEN_MAX_BITS_STUFFING and
> CANFD_FRAME_LEN_MAX_BITS_STUFFING
> > > define respectively the maximum number of bits in a classical CAN and
> > > CAN-FD frame including bit stuffing. The other definitions are
> > > intermediate values.
> > >
> > > In addition to the above:
> > >
> > >  - Include linux/bits.h and then replace the value 8 by BITS_PER_BYTE
> > >    whenever relevant.
> > >  - Include linux/math.h because of DIV_ROUND_UP(). N.B: the use of
> > >    DIV_ROUND_UP() is not new to this patch, but the include was
> > >    previously omitted.
> > >  - Update the existing length definitions to use the newly defined values.
> > >  - Add myself as copyright owner for 2020 (as coauthor of the initial
> > >    version, c.f. [1]) and for 2023 (this patch).
> > >
> > > [1] commit 85d99c3e2a13 ("can: length: can_skb_get_frame_len():
> introduce
> > >     function to get data length of frame in data link layer")
> > > Link: https://git.kernel.org/torvalds/c/85d99c3e2a13
> > >
> > > [2] RE: [PATCH] can: mcp251xfd: Increase poll timeout
> > > Link: https://lore.kernel.org/linux-
> can/BL3PR11MB64846C83ACD04E9330B0FE66FB729@BL3PR11MB6484.n
> amprd11.prod.outlook.com/
> > >
> > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > ---
> > > As always, let me know if you have better inspiration than me for the
> > > naming.
> > > ---
> > >  include/linux/can/length.h | 84
> ++++++++++++++++++++++++++++++++------
> > >  1 file changed, 72 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/include/linux/can/length.h b/include/linux/can/length.h
> > > index 6995092b774e..60492fcbe34d 100644
> > > --- a/include/linux/can/length.h
> > > +++ b/include/linux/can/length.h
> > > @@ -1,13 +1,17 @@
> > >  /* SPDX-License-Identifier: GPL-2.0 */
> > >  /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
> > >   * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
> > > + * Copyright (C) 2020, 2023 Vincent Mailhol
> <mailhol.vincent@wanadoo.fr>
> > >   */
> > >
> > >  #ifndef _CAN_LENGTH_H
> > >  #define _CAN_LENGTH_H
> > >
> > > +#include <linux/bits.h>
> > > +#include <linux/math.h>
> > > +
> > >  /*
> > > - * Size of a Classical CAN Standard Frame
> > > + * Size of a Classical CAN Standard Frame in bits
> > >   *
> > >   * Name of Field                     Bits
> > >   * ---------------------------------------------------------
> > > @@ -25,12 +29,19 @@
> > >   * End-of-frame (EOF)                        7
> > >   * Inter frame spacing                       3
> > >   *
> > > - * rounded up and ignoring bitstuffing
> > > + * ignoring bitstuffing
> > >   */
> > > -#define CAN_FRAME_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
> > > +#define CAN_FRAME_OVERHEAD_SFF_BITS 47
> > >
> > >  /*
> > > - * Size of a Classical CAN Extended Frame
> > > + * Size of a Classical CAN Standard Frame
> > > + * (rounded up and ignoring bitstuffing)
> > > + */
> > > +#define CAN_FRAME_OVERHEAD_SFF \
> > > +     DIV_ROUND_UP(CAN_FRAME_OVERHEAD_SFF_BITS, BITS_PER_BYTE)
> > > +
> > > +/*
> > > + * Size of a Classical CAN Extended Frame in bits
> > >   *
> > >   * Name of Field                     Bits
> > >   * ---------------------------------------------------------
> > > @@ -50,12 +61,19 @@
> > >   * End-of-frame (EOF)                        7
> > >   * Inter frame spacing                       3
> > >   *
> > > - * rounded up and ignoring bitstuffing
> > > + * ignoring bitstuffing
> > >   */
> > > -#define CAN_FRAME_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
> > > +#define CAN_FRAME_OVERHEAD_EFF_BITS 67
> > >
> > >  /*
> > > - * Size of a CAN-FD Standard Frame
> > > + * Size of a Classical CAN Extended Frame
> > > + * (rounded up and ignoring bitstuffing)
> > > + */
> > > +#define CAN_FRAME_OVERHEAD_EFF \
> > > +     DIV_ROUND_UP(CAN_FRAME_OVERHEAD_EFF_BITS, BITS_PER_BYTE)
> > > +
> > > +/*
> > > + * Size of a CAN-FD Standard Frame in bits
> > >   *
> > >   * Name of Field                     Bits
> > >   * ---------------------------------------------------------
> > > @@ -77,12 +95,19 @@
> > >   * End-of-frame (EOF)                        7
> > >   * Inter frame spacing                       3
> > >   *
> > > - * assuming CRC21, rounded up and ignoring bitstuffing
> > > + * assuming CRC21 and ignoring bitstuffing
> > >   */
> > > -#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(61, 8)
> > > +#define CANFD_FRAME_OVERHEAD_SFF_BITS 61
> > >
> > >  /*
> > > - * Size of a CAN-FD Extended Frame
> > > + * Size of a CAN-FD Standard Frame
> > > + * (assuming CRC21, rounded up and ignoring bitstuffing)
> > > + */
> > > +#define CANFD_FRAME_OVERHEAD_SFF \
> > > +     DIV_ROUND_UP(CANFD_FRAME_OVERHEAD_SFF_BITS,
> BITS_PER_BYTE)
> > > +
> > > +/*
> > > + * Size of a CAN-FD Extended Frame in bits
> > >   *
> > >   * Name of Field                     Bits
> > >   * ---------------------------------------------------------
> > > @@ -106,9 +131,32 @@
> > >   * End-of-frame (EOF)                        7
> > >   * Inter frame spacing                       3
> > >   *
> > > - * assuming CRC21, rounded up and ignoring bitstuffing
> > > + * assuming CRC21 and ignoring bitstuffing
> > > + */
> > > +#define CANFD_FRAME_OVERHEAD_EFF_BITS 80
> > > +
> > > +/*
> > > + * Size of a CAN-FD Extended Frame
> > > + * (assuming CRC21, rounded up and ignoring bitstuffing)
> > > + */
> > > +#define CANFD_FRAME_OVERHEAD_EFF \
> > > +     DIV_ROUND_UP(CANFD_FRAME_OVERHEAD_EFF_BITS,
> BITS_PER_BYTE)
> > > +
> > > +/* CAN bit stuffing overhead multiplication factor */
> > > +#define CAN_BIT_STUFFING_OVERHEAD 1.2
> > > +
> > > +/*
> > > + * Maximum size of a Classical CAN frame in bits, ignoring bitstuffing
> > >   */
> > > -#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(80, 8)
> > > +#define CAN_FRAME_LEN_MAX_BITS_NO_STUFFING \
> > > +     (CAN_FRAME_OVERHEAD_EFF_BITS + CAN_MAX_DLEN *
> BITS_PER_BYTE)
> > > +
> > > +/*
> > > + * Maximum size of a Classical CAN frame in bits, including bitstuffing
> > > + */
> > > +#define CAN_FRAME_LEN_MAX_BITS_STUFFING                              \
> > > +     ((unsigned int)(CAN_FRAME_LEN_MAX_BITS_NO_STUFFING *    \
> > > +                     CAN_BIT_STUFFING_OVERHEAD))
> >
> > The 1.2 overhead doesn't apply to the whole frame, according to
> > https://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=8338047.
> 
> You are right. In fact, I realized this mistake before reading your
> message (while I was studying the standard to answer Thomas).
> 
> ISO 11898-1:2015 section 10.5 "Frame coding" says:
> 
>   the frame segment as SOF, arbitration field, control field,
>   data field, and CRC sequence shall be coded by the method of
>   bit stuffing.
> 
> and:
> 
>   The remaining bit fields of the DF or RF (CRC delimiter, ACK
>   field and EOF) shall be of fixed form and not stuffed.
> 
> So, indeed, the bit stuffing does not apply to the last 10 bits (1 + 1 + 1 + 7).
> Furthermore, for FD frames, the CRC field already contains the fixed
> stuff bits. So the overhead shall not be applied again or else, stuff
> bits would be counted twice. In conclusion, for FD frames, the
> otherhead for dynamic bit stuffing overhead should apply to the SOF,
> arbitration field, control field and data field segments.
> 
> After reading the standard, I thought again about the overhead ratio
> and it is more complicated than we all thought.
> 
> Let's use below nomenclature in the following examples (borrowed from ISO):
> 
>   - "0": dominant bit
>   - "o": dominant stuff bit
>   - "1": recessive bit
>   - "i": recessive stuff bit
> 
> We probably all though below example to be the worst case:
> 
>   Destuffed: 11111  11111  11111  11111
>   Stuffed:   11111o 11111o 11111o 11111o
> 
> Here, indeed, one stuff bit is added every five bits giving us an
> overhead of 6/5 = 1.2.
> 
> However, ISO 11898-1:2015 section 10.5 "Frame coding" also says:
> 
>   Whenever a transmitter detects five consecutive bits (*including
>   stuff bits*) of identical value in the bit stream to be
>   transmitted, it shall automatically insert a complementary
>   bit (called stuff bit) ...
> 
> Pay attention to the *including stuff bits* part. The worst case is
> actually a sequence in which dominant and recessive alternate every
> four bits:
> 
>   Destuffed: 1 1111  0000  1111  0000  1111
>   Stuffed:   1 1111o 0000i 1111o 0000i 1111o
>
> Ignoring the first bit, one stuff bit is added every four bits giving
> us an overhead of 5/4 = 1.25.

Duh, absolutely right.

> The exact formula taking in account the first bit we previously ignored then:
> 
>   Number of dynamic stuff bit = 1 + round_down((len(stream) - 5) / 4)
>                               = round_down((len(stream) - 1) / 4)
> 
Right, do you plan on separating this for Arbitration bitrate and databitrate? It would probably make sense to use a fixed number of worst case stuffbits for the arbitration phase and the formula for the data phase.
 
Best Regards,
Thomas
  

Hi Thomas,

On Mon. 9 May 2023 at 16:12, <Thomas.Kopp@microchip.com> wrote:

[...]

> Right, do you plan on separating this for Arbitration bitrate and databitrate? It would probably make sense to use a fixed number of worst case stuffbits for the arbitration phase and the formula for the data phase.

I have a few ideas how to implement it, but seeing how complex things
are going, I am thinking of creating an inline helper function for the
bitstuffing calculation (the compiler should be able to fold it into a
constant expression, so there should be no penalty).

For the exact details, I have not decided yet. I need to experiment.
This not being so trivial and not having so much free time now, please
wait a few days for the v2 ;)
  

On Tue. 9 May 2023 at 15:50, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 09.05.2023 13:16:08, Vincent MAILHOL wrote:
> > > The diagram in https://www.can-cia.org/can-knowledge/can/can-fd/
> > > suggests that IMF is part of the frame.
> >
> > ISO 11898-1:2015 section 10.4.6 "Specification of inter-frame space"
> > makes it clear that the intermission is not part of the frame but part
> > of the "Inter-frame space".
>
> For this reason, it is good to have open standards...oh wait!

It is open is you (or your company, wink, wink) pay CHF 187:

  https://www.iso.org/standard/63648.html