On 24.04.25 09:44, Vincent Mailhol wrote:
> On Tue. 22 Apr. 2025 at 21:03, Felix Maurer <[email protected]> wrote:
[...]
>> diff --git a/tools/testing/selftests/net/can/test_raw_filter.c
>> b/tools/testing/selftests/net/can/test_raw_filter.c
>> index 7fe11e020a1c..8d43053824d2 100644
>> --- a/tools/testing/selftests/net/can/test_raw_filter.c
>> +++ b/tools/testing/selftests/net/can/test_raw_filter.c
>> @@ -101,94 +101,113 @@ FIXTURE_VARIANT(can_filters) {
>> int exp_num_rx;
>> int exp_rxbits;
>> };
>> +#define T_EFF (CAN_EFF_FLAG >> 28)
>> +#define T_RTR (CAN_RTR_FLAG >> 28)
>
> I do not like this
>
> >> 28
>
> shift. I understand that it is part of the original design, but for
> me, this is just obfuscation.
>
> Why just not using CAN_EFF_FLAG and CAN_RTR_FLAG as-is for the
> expected values? What benefit does this shift add?
I agree, that looks like magic numbers and the original design is not
very nice here. The main reason for the >>28 is that later on values are
shifted by T_EFF and/or T_RTR, so they shouldn't be too large (with the
>>28, the shift value later is in the range 0-14). See below for a
slightly different idea.
>> +/* Ignore EFF flag in filter ID if not covered by filter mask */
>> FIXTURE_VARIANT_ADD(can_filters, base_eff) {
>> .testcase = 2,
>> .id = ID | CAN_EFF_FLAG,
>> .mask = CAN_SFF_MASK,
>> .exp_num_rx = 4,
>> - .exp_rxbits = 4369,
>> + .exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF |
>> T_RTR)),
> ^
> What is the meaning of this 1?
The 1 means that a packet will be received with no flags set.
The whole rxbit thing took me a while to understand and the result now
is not straightforward either. Let's see if we can come up with
something better.
The exp_rxbits is basically a bitfield that describes which flags should
be set on the received frames. Maybe this could be made more explicit
with something like this:
.exp_rxbits = FRAME_NOFLAGS | FRAME_EFF | FRAME_RTR | FRAME_EFFRTR,
And in the receive loop something like this:
rxbits |= FRAME_RCVD(frame.can_id);
Of course, the definitions of these macros would still have the >>28,
but at a central point, with better explanation. Do you think that's
more understandable? Or do you have a different idea?
Thanks,
Felix
