[Xenomai-core] Re: Magics of [CAN] message filtering

[Xenomai-core] Re: Magics of [CAN] message filtering

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2024 bytes --]

Hi all,

I just bluntly jump in and capture this posting for the original issue
behind it. :->

Oliver Hartkopp wrote:
> Hello Wolfgang,
> 
> indeed you are right with all your examples.
> 
> To get more practice i would suggest to check out the possibilities by 
> using the tools 'cangen', 'cansend' and 'candump'.
> 
> E.g. these are /your/ examples on the virtual CAN interface vcan0:
> 
>> The following filter will select all messages, extended and standard 
>> messages:
>>
>>   rfilter[0].can_id   = 0x0;
>>   rfilter[0].can_mask = 0x0;
>>   
> candump vcan0
> or
> candump -m 0 -v 0 vcan0
> /* mask = value = 0 is the default */
> 
>> The following filter will select only extended messages:
>>
>>   rfilter[0].can_id   = CAN_EFF_FLAG;
>>   rfilter[0].can_mask = CAN_EFF_FLAG;
>>
>>   
> candump -m 0x80000000 -v 0x80000000 vcan0
>> The following filter will select only standard messages:
>>
>>   rfilter[0].can_id   = CAN_EFF_FLAG | CAN_INV_FILTER;
>>   rfilter[0].can_mask = CAN_EFF_FLAG;
>>
>>   
> candump -m 0x80000000 -v 0x80000000 -i 1 vcan0

...and I guess this would work as well:

candump -m 0x80000000 -v 0x00000000 vcan0

>> And RTR-Messages could be filtered in a similar way. Is this correct?
>>
>>   
> Yes. E.g.
> 
> candump -m 0x40000000 -v 0x40000000 vcan0
> 

Makes perfect sense, it's considering EFF and RTR as yet some more bits
of the CAN ID. That's what I actually tried to apply yesterday under
Xenomai and wondered why it didn't work. :o)

So, to align RT-Socket-CAN with plain Socket-CAN again, we (Xenomai) should

a) include EFF and RTS transparently into mask building and ID
   comparison
b) add support for inverse masks

Wolfgang, as we currently have some "exception" specified in the CAN
profile, I would just fix the code in this regard (for 2.3.1). The two
changes above should be queued for Xenomai 2.4 together with a profile
and revision number update - to avoid breakage inside the stable series.

Jan


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

[Xenomai-core] Re: Magics of [CAN] message filtering

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 747 bytes --]

Oliver Hartkopp wrote:
> When you're touching anything inside your API, have you ever thought to add
> 
> __attribute__ ((aligned(8)))
> 
> to the data[8] element of the struct can_frame?
> 
> This would enable you to make 64 bit compares directly in the data
> section of the can_frame ...
> 
> typedef __u32 canid_t;
> 
> struct can_frame {
>     canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
>     __u8    can_dlc; /* data length code: 0 .. 8 */
>     __u8    data[8] __attribute__ ((aligned(8)));
> };

[Swallowing down my well-known opinion on typeof(can_dlc) :)]

Yes, this should be done, already for the more urging sake of
unambiguous layout of the structure across the kernel/user space border.

Jan


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

[Xenomai-core] Re: Magics of [CAN] message filtering

[Wolfgang Grandegger]

Jan Kiszka wrote:
> Oliver Hartkopp wrote:
>> When you're touching anything inside your API, have you ever thought to add
>>
>> __attribute__ ((aligned(8)))
>>
>> to the data[8] element of the struct can_frame?
>>
>> This would enable you to make 64 bit compares directly in the data
>> section of the can_frame ...
>>
>> typedef __u32 canid_t;
>>
>> struct can_frame {
>>     canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
>>     __u8    can_dlc; /* data length code: 0 .. 8 */
>>     __u8    data[8] __attribute__ ((aligned(8)));
>> };
> 
> [Swallowing down my well-known opinion on typeof(can_dlc) :)]
> 
> Yes, this should be done, already for the more urging sake of
> unambiguous layout of the structure across the kernel/user space border.

Is this not already the case? At least the size of struct can_frame is 
16 bytes.

Wolfgang.

[Xenomai-core] Re: Magics of [CAN] message filtering

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

Wolfgang Grandegger wrote:
> Jan Kiszka wrote:
>> Oliver Hartkopp wrote:
>>> When you're touching anything inside your API, have you ever thought to add
>>>
>>> __attribute__ ((aligned(8)))
>>>
>>> to the data[8] element of the struct can_frame?
>>>
>>> This would enable you to make 64 bit compares directly in the data
>>> section of the can_frame ...
>>>
>>> typedef __u32 canid_t;
>>>
>>> struct can_frame {
>>>     canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
>>>     __u8    can_dlc; /* data length code: 0 .. 8 */
>>>     __u8    data[8] __attribute__ ((aligned(8)));
>>> };
>> [Swallowing down my well-known opinion on typeof(can_dlc) :)]
>>
>> Yes, this should be done, already for the more urging sake of
>> unambiguous layout of the structure across the kernel/user space border.
> 
> Is this not already the case? At least the size of struct can_frame is 
> 16 bytes.

On all target archs? With all supported compilers? Better make it explicit.

Jan


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

[Xenomai-core] Re: Magics of [CAN] message filtering

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1304 bytes --]

Jan Kiszka wrote:
> Wolfgang Grandegger wrote:
>> Jan Kiszka wrote:
>>> Oliver Hartkopp wrote:
>>>> When you're touching anything inside your API, have you ever thought to add
>>>>
>>>> __attribute__ ((aligned(8)))
>>>>
>>>> to the data[8] element of the struct can_frame?
>>>>
>>>> This would enable you to make 64 bit compares directly in the data
>>>> section of the can_frame ...
>>>>
>>>> typedef __u32 canid_t;
>>>>
>>>> struct can_frame {
>>>>     canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
>>>>     __u8    can_dlc; /* data length code: 0 .. 8 */
>>>>     __u8    data[8] __attribute__ ((aligned(8)));
>>>> };
>>> [Swallowing down my well-known opinion on typeof(can_dlc) :)]
>>>
>>> Yes, this should be done, already for the more urging sake of
>>> unambiguous layout of the structure across the kernel/user space border.
>> Is this not already the case? At least the size of struct can_frame is 
>> 16 bytes.
> 
> On all target archs? With all supported compilers? Better make it explicit.

gcc -Os is such an example, toasting the assumed can_frame layout by
moving that 3-byte hole *after* data.

So this was an urging ABI bug, and I applied the alignment to both
stable and trunk Xenomai SVNs. Thanks to Oliver for insisting on this!

Jan


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

[Xenomai-core] Re: Magics of [CAN] message filtering

[Wolfgang Grandegger]

Jan Kiszka wrote:
> Jan Kiszka wrote:
>> Wolfgang Grandegger wrote:
>>> Jan Kiszka wrote:
>>>> Oliver Hartkopp wrote:
>>>>> When you're touching anything inside your API, have you ever thought to add
>>>>>
>>>>> __attribute__ ((aligned(8)))
>>>>>
>>>>> to the data[8] element of the struct can_frame?
>>>>>
>>>>> This would enable you to make 64 bit compares directly in the data
>>>>> section of the can_frame ...
>>>>>
>>>>> typedef __u32 canid_t;
>>>>>
>>>>> struct can_frame {
>>>>>     canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
>>>>>     __u8    can_dlc; /* data length code: 0 .. 8 */
>>>>>     __u8    data[8] __attribute__ ((aligned(8)));
>>>>> };
>>>> [Swallowing down my well-known opinion on typeof(can_dlc) :)]
>>>>
>>>> Yes, this should be done, already for the more urging sake of
>>>> unambiguous layout of the structure across the kernel/user space border.
>>> Is this not already the case? At least the size of struct can_frame is 
>>> 16 bytes.
>> On all target archs? With all supported compilers? Better make it explicit.
> 
> gcc -Os is such an example, toasting the assumed can_frame layout by
> moving that 3-byte hole *after* data.

Ah, oh, this is always the case, not only with "-0s", as my tests show. 
But the size of the structure is always 16.

> 
> So this was an urging ABI bug, and I applied the alignment to both
> stable and trunk Xenomai SVNs. Thanks to Oliver for insisting on this!

Thanks.

Wolfgang.
x