On Fri. 22 Jul. 2022 at 05:26, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> On 21.07.22 05:13, Vincent Mailhol wrote:
> > On Thu. 21 juil. 2022 at 11:37, Vincent Mailhol
> > <vincent.mailhol@xxxxxxxxx> wrote:
> >> On Wed. 21 Jul. 2022 at 01:43, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
>
>
> >>> And even the user space side to handle a mix of CAN frame types is
> >>> pretty simple IMO:
> >>>
> >>> union {
> >>> struct can_frame cc;
> >>> struct canfd_frame fd;
> >>> struct canxl_frame xl;
> >>> } can;
> >>
> >> Do you want to add this union in the kernel uapi or is it just a
> >> userland example?
> >
> > More brainstorming. If we want to introduce a generic can structure in
> > linux/can.h, we could do:
> >
> > struct canxl_frame {
> > canid_t prio; /* 11 bit priority for arbitration (canid_t) */
> > __u8 xl_flags; /* additional flags for CAN XL */
> > __u8 fd_flags; /* CAN(-FD) flags */
> > __u16 len; /* frame payload length in byte */
> > __u32 af; /* acceptance field */
> > __u8 sdt; /* SDU (service data unit) type */
> > __u8 __res0; /* reserved / padding */
> > __u8 __res1; /* reserved / padding */
> > __u8 __res2; /* reserved / padding */
> > __u8 data[CANXL_MAX_DLEN] __attribute__((aligned(8)));
> > };
> >
> > union can_generic_frame {
> > struct {
> > union {
> > canid_t can_id;
> > canid_t prio;
> > };
> > union {
> > __u16 type;
> > struct {
> > __u8 xl_flags;
> > __u8 fd_flags;
> > } __attribute__((packed));
> > } __attribute__((packed));
> > };
> > struct can_frame cc;
> > struct canfd_frame fd;
> > struct canxl_frame xl;
> > };
> >
> > #define CANXL_XLF 0x80 /* apply to canxl_frame::xl_flags */
> >
> > #define CAN_TYPE_CC 0
> > #ifdef __LITTLE_ENDIAN
> > #define CAN_TYPE_FD (CANFD_FDF << 8)
> > #define CAN_TYPE_XL (CANXL_XLF)
> > #else /* __BIG_ENDIAN */
> > #define CAN_TYPE_FD (CANFD_FDF)
> > #define CAN_TYPE_XL (CANXL_XLF << 8)
> > #endif
> >
> > #define CAN_TYPE_MASK (CAN_TYPE_FD | CAN_TYPE_XL)
> >
> > Because can_generic_frame::type overlaps with the can(fd)_frame::len,
> > it will contain garbage and thus it is necessary to mask it with
> > CAN_TYPE_MASK.
> > The CANFD_FDF is only set in the rx path. In the tx path it is simply
> > ignored. This done, we can use it as below when *receiving* can
> > frames:
>
> No problem to set CANFD_FDF in raw_sendmsg() when we process a CAN FD
> frame in the tx path ...
ACK.
> >
> > int foo()
> > {
> > union can_generic_frame can;
> >
> > /* receive a frame */
> >
> > switch (can.type & CAN_TYPE_MASK) {
> > case CAN_TYPE_CC:
> > printf("Received classical CAN Frame\n");
> > break;
> >
> > case CAN_TYPE_FD:
> > printf("Received CAN FD Frame\n");
> > break;
> >
> > case CAN_TYPE_XL:
> > printf("Received CAN XL Frame\n");
> > break;
> >
> > default:
> > fprintf(stderr, "Unknown type: %x\n", can.type & CAN_TYPE_MASK);
> > }
> >
> > return EXIT_SUCCESS;
> > }
> >
>
> If you just want to get rid of the nbytes checking and we make sure
> CANFD_FDF is properly set in the future we are not far from such an easy
> check - even without moving the sdt element or endian magic:
ACK. I did not like the mix between the CANXL_XLF and the CAN*_MTU
checks. Would be great to have a consistent method to check the type.
>
> if (can.xl_flags & CANXL_XLF) {
> printf("Received CAN XL Frame\n");
> } else if (can.fd_flags & CANFD_FDF) {
> printf("Received CAN FD Frame\n");
> } else {
> printf("Received classical CAN Frame\n");
> }
ACK. I like this approach :)
