Skip to content

Conversation

@congnguyenhuu
Copy link
Contributor

@congnguyenhuu congnguyenhuu commented Nov 23, 2022

Add support FD frame filter to configure type frame for each Rx msg to receive corresponding frames (classic, FD frames).

I have a PR #52450 which introduces support for NXP S32 CANEXCEL, it need to implement this.

Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still evaluating the idea of this API change, but if we are to add it we need the existing in-tree CAN-FD driver (yes, there's only one: Bosch M_CAN) adapted to support this and a test case added to prove it.

@congnguyenhuu
Copy link
Contributor Author

congnguyenhuu commented Nov 29, 2022

Member

Still evaluating the idea of this API change, but if we are to add it we need the existing in-tree CAN-FD driver (yes, there's only one: Bosch M_CAN) adapted to support this and a test case added to prove it.

I think we should only add the filter type frame to can drivers which require eg. NXP S32 CANEXCEL. I see Bosch M_CAN doesn't require this.
So I added a Kconfig to enable filter type frame. We can use if driver require. And I modified the test canfd to support this.
But now I don't have any board (run CAN-FD over Bosch M_CAN) to check it. Anyone have board, pls help me run it?

@henrikbrixandersen
Copy link
Member

I think we should only add the filter type frame to can drivers which require eg. NXP S32 CANEXCEL.

No. That will lead to different APIs for different CAN controllers, making it difficult to write portable application code. This is not an acceptable solution.

I see Bosch M_CAN doesn't require this.

It doesn't require it, but it could support it.

The above is exactly why I still hesitate in accepting this API change. We need to think carefully about user applications, portability, and support from more than just one driver/hardware peripheral.

@congnguyenhuu congnguyenhuu changed the title include: driver: can: add new filter to match CAN-FD frames driver: can: add new filter to match CAN-FD frames Nov 29, 2022
@congnguyenhuu
Copy link
Contributor Author

No. That will lead to different APIs for different CAN controllers, making it difficult to write portable application code. This is not an acceptable solution.

But in Bosch M_CAN driver, mcan filter no has field to configure filter type frame.

struct can_mcan_std_filter {
	volatile uint32_t id2  : 11; /* ID2 for dual or range, mask otherwise */
	volatile uint32_t res  :  5;
	volatile uint32_t id1  : 11;
	volatile uint32_t sfce :  3; /* Filter config */
	volatile uint32_t sft  :  2; /* Filter type */
} __packed __aligned(4);

@henrikbrixandersen
Copy link
Member

But in Bosch M_CAN driver, mcan filter no has field to configure filter type frame.

It doesn't have a field for RTR either, but that is handled in software. Something similar can be done for FDF.

@congnguyenhuu
Copy link
Contributor Author

It doesn't have a field for RTR either, but that is handled in software. Something similar can be done for FDF.

Great ! I handled in software as you said.

@zephyrbot zephyrbot added the area: native port Host native arch port (native_sim) label Nov 29, 2022
@zephyrbot zephyrbot requested a review from aescolar November 29, 2022 15:55
Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to update can_utils_filter_match() in drivers/can/can_utils.h to handle the new flag as well.

@congnguyenhuu
Copy link
Contributor Author

You need to update can_utils_filter_match() in drivers/can/can_utils.h to handle the new flag as well.

I updated for handling it

Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good. Apologies for the review delay.

@congnguyenhuu
Copy link
Contributor Author

Thanks! Looks good. Apologies for the review delay.

Thank you.

Add support FD frame filter to configure type frame for
each Rx msg to receive corresponding frames (classic, FD frames).

The Bosch M_CAN driver does not support FD frame filter,
so inmplement driver to handle it in software.

Signed-off-by: Cong Nguyen Huu <[email protected]>
Add new filter to configure correctly FD frame.

Signed-off-by: Cong Nguyen Huu <[email protected]>
Copy link
Member

@martinjaeger martinjaeger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for the fix.

@dleach02 dleach02 merged commit dd468fc into zephyrproject-rtos:main Jan 4, 2023
@dleach02 dleach02 deleted the can-add-filter-match-FD-frames branch January 4, 2023 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: CAN area: native port Host native arch port (native_sim) area: Networking

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants