Re: [Xenomai-core] Re: Extended CAN frame filtering

[Jan Kiszka <jan.kiszka@domain.hid>]

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

Wolfgang Grandegger wrote:
...
> 
> Attached is a patch adding the CAN_INV_FILTER feature. It also updates
> the doc. Now the filter scheme should be compatible with Socket-CAN. If
> it's OK, I will check it in (or you can do it).
> 
> Wolfgang.
> 
> 
> ------------------------------------------------------------------------
> 
> Index: include/rtdm/rtcan.h
> ===================================================================
> --- include/rtdm/rtcan.h	(revision 2193)
> +++ include/rtdm/rtcan.h	(working copy)

Please increment the profile revision (2 spots: #define and
documentation text).

> @@ -441,28 +441,38 @@ typedef enum CAN_STATE can_state_t;
>  
>  #define CAN_STATE_OPERATING(state) ((state) < CAN_STATE_BUS_OFF)
>  
> +/** Invert CAN filter definition */
> +#define CAN_INV_FILTER      CAN_ERR_FLAG
> +

I would move this close to the other CAN ID flags and explain explicitly
which one is valid in which context. Both are sharing the same bit, so
confusion can arise.

>  /**
>   * Filter for reception of CAN messages.
>   *
>   * This filter works as follows:
>   * A received CAN ID is AND'ed bitwise with @c can_mask and then compared to
> - * @c can_id. If this comparison is true the message will be received by the
> - * socket.
> + * @c can_id. This also includes the @ref CAN_EFF_FLAG and @ref CAN_RTR_FLAG
> + * of @ref CAN_xxx_FLAG. If this comparison is true, the message will be
> + * received by the socket. The logic can be inverted with the @c can_id flag
> + * @ref CAN_INV_FILTER :
> + *
> + * @code
> + * if (can_id & CAN_INV_FILTER) {
> + *    if ((received_can_id & can_mask) != (can_id & ~CAN_INV_FILTER))
> + *       accept-message;
> + * } else {
> + *    if ((received_can_id & can_mask) == can_id)
> + *       accept-message;
> + * }
> + * @endcode

Nice and helpful!

>   *
> - * Multiple filters can be arranged in a filter list and set with 
> - * @ref Sockopts. If one of these filters matches a CAN ID upon reception 
> + * Multiple filters can be arranged in a filter list and set with
> + * @ref Sockopts. If one of these filters matches a CAN ID upon reception
>   * of a CAN frame, this frame is accepted.
>   *
> - * @note Only @ref CAN_EFF_FLAG of @ref CAN_xxx_FLAG "CAN ID flags" is
> - * valid for @c can_id and none for @c can_mask. This means that the RTR bit
> - * is not taken into account while filtering messages.
> - *
> - * Extended IDs are received only if @ref CAN_EFF_FLAG is set in
> - * @c can_id. If it is cleared only standard IDs are accepted.
>   */
>  typedef struct can_filter {
>  
> -    /** CAN ID which must match with incoming IDs after passing the mask */
> +    /** CAN ID which must match with incoming IDs after passing the mask.
> +     *  The filter logic can be inverted with the flag @ref CAN_INV_FILTER. */
>      uint32_t    can_id;
>  
>      /** Mask which is applied to incoming IDs. See @ref CAN_xxx_MASK
> @@ -470,12 +480,11 @@ typedef struct can_filter {
>      uint32_t    can_mask;
>  } can_filter_t;
>  
> -
>  /**
>   * Socket address structure for the CAN address family
>   */
>  struct sockaddr_can {
> -    /** CAN address family, must be @c AF_CAN */    
> +    /** CAN address family, must be @c AF_CAN */
>      sa_family_t  can_family;
>      /** Interface index of CAN controller. See @ref SIOCGIFINDEX. */
>      int          can_ifindex;
> Index: ksrc/drivers/can/rtcan_module.c
> ===================================================================
> --- ksrc/drivers/can/rtcan_module.c	(revision 2193)
> +++ ksrc/drivers/can/rtcan_module.c	(working copy)
> @@ -262,11 +262,11 @@ static int rtcan_read_proc_filter(char *
>      rtdm_lockctx_t lock_ctx;
>      RTCAN_PROC_PRINT_VARS(80);
>  
> -    /*  fd __CAN_ID__ _CAN_Mask_ MatchCount
> -     *   3 0x12345678 0x12345678 1234567890
> +    /*  fd __CAN_ID__ _CAN_Mask_ Inv MatchCount
> +     *   3 0x12345678 0x12345678  no 1234567890
>       */
> -    
> -    if (!RTCAN_PROC_PRINT("fd __CAN_ID__ _CAN_Mask_ MatchCount\n"))
> +
> +    if (!RTCAN_PROC_PRINT("fd __CAN_ID__ _CAN_Mask_ Inv MatchCount\n"))
>          goto done;
>  
>      rtdm_lock_get_irqsave(&rtcan_recv_list_lock, lock_ctx);
> @@ -275,10 +275,13 @@ static int rtcan_read_proc_filter(char *
>      while (recv_listener != NULL) {
>  	context = rtcan_socket_context(recv_listener->sock);
>  
> -	if (!RTCAN_PROC_PRINT("%2d 0x%08x 0x%08x %10d\n",
> -			      context->fd, 
> +	if (!RTCAN_PROC_PRINT("%2d 0x%08x 0x%08x %s %10d\n",
> +			      context->fd,
>  			      recv_listener->can_filter.can_id,
> -			      recv_listener->can_filter.can_mask,
> +			      recv_listener->can_filter.can_mask &
> +			      ~CAN_INV_FILTER,
> +			      (recv_listener->can_filter.can_mask &
> +			       CAN_INV_FILTER) ? "yes" : " no",
>  			      recv_listener->match_count))
>  	    break;
>  	recv_listener = recv_listener->next;
> Index: ksrc/drivers/can/rtcan_raw.c
> ===================================================================
> --- ksrc/drivers/can/rtcan_raw.c	(revision 2193)
> +++ ksrc/drivers/can/rtcan_raw.c	(working copy)
> @@ -67,7 +67,10 @@ static struct rtdm_device rtcan_proto_ra
>  
>  static inline int rtcan_accept_msg(uint32_t can_id, can_filter_t *filter)
>  {
> -    return ((can_id & filter->can_mask) == filter->can_id);
> +    if ((filter->can_mask & CAN_INV_FILTER))
> +	return ((can_id & filter->can_mask) != filter->can_id);
> +    else
> +	return ((can_id & filter->can_mask) == filter->can_id);
>  }
>  
>  
> Index: ksrc/drivers/can/rtcan_raw_filter.c
> ===================================================================
> --- ksrc/drivers/can/rtcan_raw_filter.c	(revision 2193)
> +++ ksrc/drivers/can/rtcan_raw_filter.c	(working copy)
> @@ -55,13 +55,13 @@ void rtcan_raw_print_filter(struct rtcan
>  static inline void rtcan_raw_mount_filter(can_filter_t *recv_filter,
>  					  can_filter_t *filter)
>  {
> -   if (filter->can_id & CAN_EFF_FLAG)
> -	recv_filter->can_mask = ((filter->can_mask & CAN_EFF_MASK) |
> -				 CAN_EFF_FLAG);
> -    else
> -	recv_filter->can_mask = (filter->can_mask & CAN_SFF_MASK);
> -
> -    recv_filter->can_id = filter->can_id & recv_filter->can_mask;
> +    if (filter->can_id & CAN_INV_FILTER) {
> +	recv_filter->can_id = filter->can_id & ~CAN_INV_FILTER;
> +	recv_filter->can_mask = filter->can_mask | CAN_INV_FILTER;
> +    } else {
> +	recv_filter->can_id = filter->can_id;
> +	recv_filter->can_mask = filter->can_mask & ~CAN_INV_FILTER;
> +    }

Why do you push CAN_INV_FILTER internally into the mask instead of
keeping it in the filter's ID - as the pseudo code above states?

Jan


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