On Thu. 14 Jul. 2022 at 15:11, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> On 14.07.22 03:23, Vincent Mailhol wrote:
> > On Thu. 14 Jul. 2022 at 05:02, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> > If we follow your idea, use __u8 xlsec in order to avoid undefined behaviours.
> >
> >> Where we define
> >>
> >> #define CANXL_TAG 0x7F
> >
> > Here, you "burn" all the flags for future usage.
> > FYI, this doesn't have to be 0x7F. It could be any of the length
> > values not allowed by CAN-FD, namely (in decimal): 9-11, 13-15, 17-19,
> > 21-23, 25-31, 33-47, 49-63
>
> Yes, I detected this issue too when waking up this morning ...
>
> >> #define CANXL_SEC 0x80
> >
> > I did not get why CANXL_XLF isn't needed anymore.
> >
> >> which has to be set in the xlsec element then.
> >>
> >> With this move we get rid of any flags problems (we only need the SEC
> >> bit anyway) and define a clear 'escape value' in the former length element.
> >
> > If I try to bounce on your idea, I will propose:
> >
> > /*********** begin **********/
> > struct canxl_frame {
> > canid_t prio; /* 11 bit priority for arbitration (canid_t) */
> > __u8 flags; /* additional flags for CAN XL. MSB must be set */
> > __u8 sdt; /* SDU (service data unit) type */
> > __u16 len; /* frame payload length in bytes */
> > __u32 af; /* acceptance field */
> > __u8 data[];
> > };
>
> ACK.
>
> > #define CANXL_XLF 0x01 /* mark CAN XL for dual use of struct canfd_frame */
>
> IMO the dual use stuff between CAN FD & CAN XL is not working anymore
> and became obsolete here.
>
> CAN_XLF needs to be a hard switch for CAN XL - no optional thing.
>
> > #define CANXL_SEC 0x02 /* Simple Extended Content (security/segmentation) */
> > #define CANXL_DISCRIMINATOR 0x80; /* Mandatory to distinguish between
> > CAN(-FD) and XL frames */
> > /*********** end ************/
> >
> > This has no undefined behaviour and we still have five flags (0x04 to
> > 0x40) for future use.
> >
>
> I would suggest the following:
>
> #define CANXL_XLF 0x80 /* mark CAN XL frame (must be set) */
> #define CANXL_SEC 0x40 /* Simple Extended Content (security/segmentation) */
OK. Having CANXL_SEC on the most significant bit (0x40) or instead of
the least significant bit (0x01) works for me.
> And the rest of the bits are reserved (shall be set to zero).
ACK.
> This way the CAN_XLF value would make the former 'len' element 128 -
> which is a proper distinction for CAN XL frames as such length value
> surely bounces on CAN/CANFD frames.
The purpose of the CAN_XLF is still unclear to me. In your initial
patch, you wrote that CAN_XLF was to "mark CAN XL for dual use of
struct canfd_frame". So are we really OK to specify that CAN_XLF is
always set? How are we going to tag dual use?
But my main point was to always set 0x80 to differentiate between
CAN(-FD) and CANXL, and we are aligned on this. :D
The last topic remaining is the data[] vs. data[CANXL_MAX_DLEN]. I
thought about it but can not find any killer arguments for either
solution.
The biggest difference is that for data[] we can do sizeof(struct
canxl_frame) to get the header file whereas for data[CANXL_MAX_DLEN],
we need to introduce the CANXL_HEADER_SIZE macro.
My preference still goes toward the data[] because I see flexible
member arrays as being more idiomatic C. But it is more for personal
taste than anything else…
Yours sincerely,
Vincent Mailhol
