feat: automatic CAN filter configuration #20

rureverek · 2023-05-03T17:42:36Z

Changes

Add automatic CAN filter configuration using ID_List mode
- The configuration is placed at the beginning of rtcan_start() function as the subscriber list is complete at that point.
- Prior filter configuration, the can_ids are obtained and the total number of can_ids is calculated. This step might be improved by adding first_hashmap_element pointer and the subscriber_counter to rtcan_handle_t struct. Instead, we have to search linearly to be sure we don't miss any CAN_ID.
- Finally, the number of filter banks is calculated and HAL_CAN_ConfigFilter() is called for each bank.

Related Issue(s)

Closes #4
Closes #5

t-bre

Nice work, looks really good. Some small tweaks but I think this will work well. I'd like to test this in hardware before we merge to main, since it's pretty critical to the function!

t-bre · 2023-05-03T17:58:56Z

src/rtcan.c

@@ -209,6 +189,54 @@ rtcan_status_t rtcan_init(rtcan_handle_t* rtcan_h,
 */
 rtcan_status_t rtcan_start(rtcan_handle_t* rtcan_h)
 {
+ /* Since the subscriber ID list is known, we can configure the CAN ID Filter now */
+
+ uint32_t can_id_list[RTCAN_HASHMAP_SIZE] = {0};


Perhaps counterintuitively there can be more subscribers than hashmap entries, so I think the size of this array should be replaced with something like NUM_IDS_PER_FILTER * NUM_FILTERS.

The reason there can be more subscribers than hashmap entries is that when two IDs result in a hash collision (same hash for different CAN ID), they are combined in that hashmap entry as a singly linked list using "separate chaining" (there are some notes in the docs folder about this implementation). The actual subscribers are allocated from a memory pool, which is ultimately what determines the maximum number of subscriptions.

src/rtcan.c

t-bre · 2023-05-03T18:01:43Z

src/rtcan.c

+ {
+ CAN_FilterTypeDef filter;
+ filter.FilterActivation = ENABLE;
+ filter.FilterFIFOAssignment = CAN_FILTER_FIFO0; 


Do we know what this FIFO assignment does? I don't know if we ever want to use anything other than FIFO0, but maybe its important?

According to datasheet: "If there is a match, the message is stored in the associated FIFO". Basically, when there's filter match, the data can be saved either in FIFO0 or FIFO1. Actually, this one may be useful for prioritizing certain IDs over another IDs which are in the same CAN bus.

From slides on the F7 bxCAN peripheral:

"Two receive FIFOs are used by hardware to store incoming messages. Each FIFO can store three complete messages."

So I don't think there's any inherent prioritisation of one FIFO over another, unless we did this as a feature of RTCAN where it will read and distribute the messages from one FIFO first, but I don't think we need that level of prioritisation.

I think we might want to consider using both FIFOs to share the load (so we use all 6 available message slots rather than just the 3 in FIFO0). Maybe having it alternate between FIFOs each loop?

t-bre

One small thing with one of the loops, otherwise looks great. Good spot on adding the iterations through hashmap entries with chained nodes, missed that in the last one.

Some thoughts about the FIFOs, up to you if you want to do that now now or in future.

t-bre · 2023-05-04T00:29:48Z

src/rtcan.c

+ {
+ CAN_FilterTypeDef filter;
+ filter.FilterActivation = ENABLE;
+ filter.FilterFIFOAssignment = CAN_FILTER_FIFO0; 


From slides on the F7 bxCAN peripheral:

"Two receive FIFOs are used by hardware to store incoming messages. Each FIFO can store three complete messages."

So I don't think there's any inherent prioritisation of one FIFO over another, unless we did this as a feature of RTCAN where it will read and distribute the messages from one FIFO first, but I don't think we need that level of prioritisation.

I think we might want to consider using both FIFOs to share the load (so we use all 6 available message slots rather than just the 3 in FIFO0). Maybe having it alternate between FIFOs each loop?

t-bre · 2023-05-04T00:33:34Z

src/rtcan.c

+ int number_ids = 0;
+
+ /* Save and count all CAN IDs stored in hashmap */
+ for (uint32_t i = 0; i < RTCAN_HASHMAP_SIZE; i++)


Loop iteration limit should be sizeof(can_id_list) / sizeof(uint32_t) or NUM_FILTERS * NUM_IDS_PER_FILTER. If you want to go for the second, I might make a new constant TOTAL_FILTER_CAN_IDS or something to use in both the can_id_list_size and the loop.

Are you sure this should be NUM_FILTERS * NUM_IDS_PER_FILTER? This would be 28*4=112 which is bigger than size of hashmap. The i is used only for hashmap entries, not counting can_ids. Instead, I can put break statement in case number ids will be bigger than NUM_FILTERS * NUM_IDS_PER_FILTER.

My mistake, you're absolutely right. Honestly no idea what I was thinking sorry!

rureverek · 2023-05-04T18:09:57Z

Loop iteration limit should be sizeof(can_id_list) / sizeof(uint32_t) or NUM_FILTERS * NUM_IDS_PER_FILTER. If you want to go for the second, I might make a new constant TOTAL_FILTER_CAN_IDS or something to use in both the can_id_list_size and the loop.

I am not sure what you mean by this. Is that want you meant? I can't assign NUM_FILTERS * NUM_IDS_PER_FILTER instead of RTCAN_HASHMAP_SIZE because its bigger than hashmap size (28*4 = 112 >100), and we iterate through hashmap entries here.

t-bre

Looks perfect. I'd like to do some testing on the actual car's CAN bus with this before we merge to main, we should be able to do that soon.

t-bre · 2023-05-06T12:19:47Z

src/rtcan.c

+ int number_ids = 0;
+
+ /* Save and count all CAN IDs stored in hashmap */
+ for (uint32_t i = 0; i < RTCAN_HASHMAP_SIZE; i++)


My mistake, you're absolutely right. Honestly no idea what I was thinking sorry!

rureverek · 2023-05-10T22:30:56Z

This should work now. We can test it next time.

rureverek added 2 commits May 3, 2023 18:24

Add ID List CAN ID Filter in rtcan_start()

6a99af9

Fix comments

f46f187

rureverek added the feature New feature or request label May 3, 2023

rureverek self-assigned this May 3, 2023

rureverek changed the title ~~Feat/id filter~~ feat: Automatic CAN filter configuration May 3, 2023

t-bre requested changes May 3, 2023

View reviewed changes

Add constants, for each hash entry look for chained nodes

b555b35

rureverek requested a review from t-bre May 3, 2023 19:09

t-bre changed the title ~~feat: Automatic CAN filter configuration~~ feat: automatic CAN filter configuration May 4, 2023

t-bre requested changes May 4, 2023

View reviewed changes

rureverek added 2 commits May 4, 2023 19:02

Alternate filter assignment between both FIFOs

9e4126c

Add limit (NUM_FILTERS * NUM_IDS_PER_FILTER) to the can id finder loop

3008cd3

rureverek requested a review from t-bre May 4, 2023 18:07

t-bre approved these changes May 6, 2023

View reviewed changes

rureverek added 2 commits May 10, 2023 22:25

Move the filter configuration to rtcan_subscribe()

ae80c5e

Fix can_ids

aaf21fd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: automatic CAN filter configuration #20

feat: automatic CAN filter configuration #20

rureverek commented May 3, 2023 •

edited by t-bre

t-bre left a comment

t-bre May 3, 2023

t-bre May 3, 2023

rureverek May 3, 2023

t-bre May 4, 2023

t-bre left a comment

t-bre May 4, 2023

t-bre May 4, 2023

rureverek May 4, 2023 •

edited

t-bre May 6, 2023

rureverek commented May 4, 2023

t-bre left a comment

t-bre May 6, 2023

rureverek commented May 10, 2023

feat: automatic CAN filter configuration #20

Are you sure you want to change the base?

feat: automatic CAN filter configuration #20

Conversation

rureverek commented May 3, 2023 • edited by t-bre

Changes

Related Issue(s)

t-bre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-bre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rureverek May 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rureverek commented May 4, 2023

t-bre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rureverek commented May 10, 2023

rureverek commented May 3, 2023 •

edited by t-bre

rureverek May 4, 2023 •

edited