drivers/mcp2515: enable filtering #18062

firas-hamdi · 2022-05-05T16:07:16Z

Contribution description

The MCP2515 driver initialization sets the acceptance mode to receiving all packets without checking the filters set for each mailbox. This leads to the situation where the node is receiving all messages on the CAN bus.
This PR goal is to enable filtering the CAN messages that are having a CAN ID that matches the CAN filters.

Testing procedure

This was tested using a SAME54-xpro and a MCP2515 breakout board following these steps:

Enable filtering macro MCP2515_RECV_FILTER_EN
Set the CAN filters for both mailboxes to reject totally the CAN message. If not, the message will be accepted on the second mailbox (according to Figure4.3 in the MCP2515 datasheet)
Send CAN messages with matching and not matching CAN IDs

wosym · 2022-05-15T14:58:08Z

drivers/mcp2515/candev_mcp2515.c

@@ -421,7 +421,7 @@ static int _get(candev_t *candev, canopt_t opt, void *value, size_t max_len)
 static int _set_filter(candev_t *dev, const struct can_filter *filter)
 {
    DEBUG("inside _set_filter of MCP2515\n");
-    bool filter_added = true;
+    bool filter_added = false;


Looks like before this change, filter_added was always true. However, that doesn't mean this function always returned true, because on failures -1 was returned.

Currently there's three types of returns in this function: bool filter_added, int res and regular -1 indicating failures. Maybe it might be a good opportunity to streamline this?
0 means success (filter added), any negative number indicates an error.

wosym · 2022-05-15T15:10:57Z

tests/candev/main.c

-
-    /* do not care, receive all message id */
+}
+    /* Add filters here when needed. Currently do not care, receive all message id */


Maybe add an example on how to use it? The reason for writing this candev application was because the CAN system was extremely complex before. This showed a very basic and easy way to use CAN in RIOT. Now that you've enabled another feature, maybe this feature can also be demonstrated in the candev test?

wosym

Looks good to me.
Only checked code, did not test it out on hardware.

wosym

Actually, after checking out the other PR, it dawned on me... now that we changed the return type... do we still need this filter_added variable? It's being set, but no longer returned.

The only place where it's still being used for something useful, is to break out of this while loop. But maybe we can replace that with a simply break statement?

(Github didn't link my comment to the relevant line. I must have done something wrong here. This comment relates to line 456 in file drivers/mcp2515/candev_mcp2515.c)

wosym · 2022-05-27T08:27:37Z

Another question: Now that you've set the filters in the test-application, shouldn't the RX_callback also be adapted? It currently does not take into account mailboxes.

firas-hamdi · 2022-06-07T13:40:29Z

Another question: Now that you've set the filters in the test-application, shouldn't the RX_callback also be adapted? It currently does not take into account mailboxes.

IMHO I think it shouldn't be adapted because the driver itself considers the mailboxes when triggering the reception interrupts (INT_RX0 or INT_RX1). So the RX callback will keep receiving only the data matching the filters and to see on which mailbox the data is received, the debug should be enabled in candev_mcp2515.c

wosym · 2022-06-10T13:25:03Z

Did you forget to push? I don't think I see the changes?

firas-hamdi · 2022-06-15T08:35:09Z

Did you forget to push? I don't think I see the changes?

Sorry you're right! Fixed now

wosym

Looks good.

firas-hamdi · 2022-06-21T12:22:35Z

Rebased and squashed commits

benpicco

Proxy-ACK for @wosym

wosym

Sorry for late reply. Since it's only a rebase&squash, still all good for me.

There's just one question I would raise: do we really need to remove all those (unused) defines? What's the RIOT way of doing things in this case? For my personal projects I sometimes leave these defines in, because they can still be useful for when someone starts adapting this driver for his own personal use case. The current driver doesn't use all capabilities of this chip, but some user might need a specific function. For that user, it is useful that the registers are already defined and readily available.

Just my two cents. Doesn't need to block merging and can even be ignored.

benpicco · 2022-06-26T14:11:30Z

drivers/mcp2515/mcp2515_defines.h

-#define MCP2515_RXB1CTRL_RXM0                   0x20
-#define MCP2515_RXB1CTRL_RXM1                   0x40
-#define MCP2515_RXB1CTRL_MODE_RECV_STD_OR_EXT   0x00
-#define MCP2515_RXB1CTRL_MODE_RECV_STD          MCP2515_RXB1CTRL_RXM0
-#define MCP2515_RXB1CTRL_MODE_RECV_EXT          MCP2515_RXB1CTRL_RXM1
-#define MCP2515_RXB1CTRL_MODE_RECV_ALL          (MCP2515_RXB1CTRL_RXM1 | \
-                                                 MCP2515_RXB1CTRL_RXM0)
+#define MCP2515_RXB1CTRL_MODE_RECV_FILTER       0x00
+#define MCP2515_RXB1CTRL_MODE_RECV_ALL          0x60


I agree that dropping the unused defines is unnecessary since they just encode information about the hardware and could become useful in the future.

drivers/mcp2515: enable filtering The current driver implementation initializes the driver in a way to receive all the CAN messages without matching the filters. This commit changes that by adding a macro definition that will be enabling or disabling the filtering and accordingly set the appropriate mcp2515 acceptance mode

github-actions bot added Area: drivers Area: Device drivers Area: tests Area: tests and testing framework labels May 5, 2022

benpicco requested review from wosym and fjmolinas May 5, 2022 22:03

firas-hamdi force-pushed the mcp2515-enable-filtering branch from 3dfdad6 to c9bcbfd Compare May 9, 2022 14:36

benpicco requested review from vincent-d and toonst May 10, 2022 09:12

firas-hamdi force-pushed the mcp2515-enable-filtering branch 4 times, most recently from 22646f0 to 9fa1535 Compare May 11, 2022 12:17

github-actions bot added the Area: tools Area: Supplementary tools label May 11, 2022

firas-hamdi force-pushed the mcp2515-enable-filtering branch from 9fa1535 to fcfbf78 Compare May 11, 2022 13:58

wosym reviewed May 15, 2022

View reviewed changes

firas-hamdi force-pushed the mcp2515-enable-filtering branch from 686120d to 58f8bfe Compare May 18, 2022 13:26

firas-hamdi requested a review from wosym May 18, 2022 13:30

firas-hamdi mentioned this pull request May 24, 2022

drivers/mcp2515: apply filters to target mailbox #18131

Merged

wosym approved these changes May 25, 2022

View reviewed changes

wosym suggested changes May 25, 2022

View reviewed changes

firas-hamdi requested a review from wosym June 7, 2022 13:46

wosym approved these changes Jun 17, 2022

View reviewed changes

firas-hamdi force-pushed the mcp2515-enable-filtering branch from f1f7626 to 0bf4a54 Compare June 17, 2022 14:58

firas-hamdi requested a review from wosym June 21, 2022 12:22

benpicco approved these changes Jun 24, 2022

View reviewed changes

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 24, 2022

gschorcht changed the title ~~MCP2515: enable filtering~~ drivers/mcp2515: enable filtering Jun 25, 2022

wosym approved these changes Jun 25, 2022

View reviewed changes

benpicco reviewed Jun 26, 2022

View reviewed changes

firas-hamdi added 2 commits June 26, 2022 17:38

tests/candev: adapt test application

285ba27

benpicco force-pushed the mcp2515-enable-filtering branch from 0bf4a54 to 285ba27 Compare June 26, 2022 15:38

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 26, 2022

benpicco merged commit d9fc082 into RIOT-OS:master Jun 26, 2022

chrysn added this to the Release 2022.07 milestone Aug 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/mcp2515: enable filtering #18062

drivers/mcp2515: enable filtering #18062

firas-hamdi commented May 5, 2022

wosym May 15, 2022 •

edited

Loading

wosym May 15, 2022

wosym left a comment

wosym left a comment •

edited

Loading

wosym commented May 27, 2022

firas-hamdi commented Jun 7, 2022 •

edited

Loading

wosym commented Jun 10, 2022

firas-hamdi commented Jun 15, 2022

wosym left a comment

firas-hamdi commented Jun 21, 2022

benpicco left a comment

wosym left a comment

benpicco Jun 26, 2022

drivers/mcp2515: enable filtering #18062

drivers/mcp2515: enable filtering #18062

Conversation

firas-hamdi commented May 5, 2022

Contribution description

Testing procedure

wosym May 15, 2022 • edited Loading

Choose a reason for hiding this comment

wosym May 15, 2022

Choose a reason for hiding this comment

wosym left a comment

Choose a reason for hiding this comment

wosym left a comment • edited Loading

Choose a reason for hiding this comment

wosym commented May 27, 2022

firas-hamdi commented Jun 7, 2022 • edited Loading

wosym commented Jun 10, 2022

firas-hamdi commented Jun 15, 2022

wosym left a comment

Choose a reason for hiding this comment

firas-hamdi commented Jun 21, 2022

benpicco left a comment

Choose a reason for hiding this comment

wosym left a comment

Choose a reason for hiding this comment

benpicco Jun 26, 2022

Choose a reason for hiding this comment

wosym May 15, 2022 •

edited

Loading

wosym left a comment •

edited

Loading

firas-hamdi commented Jun 7, 2022 •

edited

Loading