On Tue. 22 Apr. 2025 at 21:03, Felix Maurer <[email protected]> wrote:
> The expected results did not explain very well what was really tested. Make
> the expectations more clear by writing out the flags that should be set in
> the received frames and add a short explanation for each test case. Also,
> document the overall test design.
>
> Signed-off-by: Felix Maurer <[email protected]>
> ---
> .../selftests/net/can/test_raw_filter.c | 65 ++++++++++++++-----
> 1 file changed, 49 insertions(+), 16 deletions(-)
>
> 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?
> +/* Receive all frames when filtering for the ID in standard frame format */
> FIXTURE_VARIANT_ADD(can_filters, base) {
> .testcase = 1,
> .id = ID,
> .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)),
^ ^
Nitpick: those outermost parentheses are not needed.
This took me time to process. Isn't your expression redundant? What about
.exp_rxbits = 1 | 1 << (T_EFF | T_RTR),
?
This gives me the same result:
https://godbolt.org/z/cr3q5vjMr
> };
> +/* 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?
> };
(...)
Yours sincerely,
Vincent Mailhol
