Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: automatic CAN filter configuration #20

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat: automatic CAN filter configuration #20

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6a99af9
Add ID List CAN ID Filter in rtcan_start()
rureverek May 3, 2023
f46f187
Fix comments
rureverek May 3, 2023
b555b35
Add constants, for each hash entry look for chained nodes
rureverek May 3, 2023
9e4126c
Alternate filter assignment between both FIFOs
rureverek May 4, 2023
3008cd3
Add limit (NUM_FILTERS * NUM_IDS_PER_FILTER) to the can id finder loop
rureverek May 4, 2023
ae80c5e
Move the filter configuration to rtcan_subscribe()
rureverek May 10, 2023
aaf21fd
Fix can_ids
rureverek May 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 48 additions & 20 deletions src/rtcan.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,26 +179,6 @@ rtcan_status_t rtcan_init(rtcan_handle_t* rtcan_h,
ADD_ERROR_IF(tx_status != TX_SUCCESS, RTCAN_ERROR_INTERNAL, rtcan_h);
}

// TODO: configure CAN filters
if (no_errors(rtcan_h))
{
CAN_FilterTypeDef filter;
filter.FilterActivation = ENABLE;
filter.FilterFIFOAssignment = CAN_FILTER_FIFO0;
filter.FilterIdHigh = 0x0000 << 5U;
filter.FilterIdLow = 0x0000 << 5U;
filter.FilterMaskIdHigh = 0x0000 << 5U;
filter.FilterMaskIdLow = 0x0000 << 5U;
filter.FilterMode = CAN_FILTERMODE_IDMASK;
filter.FilterScale = CAN_FILTERSCALE_16BIT;
filter.FilterBank = 0;

HAL_StatusTypeDef hal_status = HAL_CAN_ConfigFilter(rtcan_h->hcan,
&filter);

ADD_ERROR_IF(hal_status != HAL_OK, RTCAN_ERROR_INIT, rtcan_h);
}

return create_status(rtcan_h);
}

Expand All @@ -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};
Copy link
Member

Choose a reason for hiding this comment

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

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.

int number_ids = 0;

/* Save and count all CAN IDs stored in hashmap */
for (uint32_t i = 0; i < RTCAN_HASHMAP_SIZE; i++)
t-bre marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@rureverek rureverek May 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

{
if(rtcan_h->subscriber_map[i] != NULL)
{
/* Subscriber found! */
can_id_list[number_ids] = rtcan_h->subscriber_map[i]->can_id;
number_ids++;

}
}

int number_banks = (number_ids-1)/4 + 1;

/* Error if there's too many ids to apply filter */
ADD_ERROR_IF(number_banks > 28, RTCAN_ERROR_INIT, rtcan_h);
t-bre marked this conversation as resolved.
Show resolved Hide resolved

if (no_errors(rtcan_h))
{
CAN_FilterTypeDef filter;
filter.FilterActivation = ENABLE;
filter.FilterFIFOAssignment = CAN_FILTER_FIFO0;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

filter.FilterMode = CAN_FILTERMODE_IDLIST;
filter.FilterScale = CAN_FILTERSCALE_16BIT;

/* Configure Filter IDs for each filter bank */
for(int i = 0; i < number_banks; i++)
{
filter.FilterIdHigh = can_id_list[4 * i] << 5U;
filter.FilterIdLow = can_id_list[4 * i + 1] << 5U;
filter.FilterMaskIdHigh = can_id_list[4 * i + 2] << 5U;
filter.FilterMaskIdLow = can_id_list[4 * i + 3] << 5U;
filter.FilterBank = i;


HAL_StatusTypeDef hal_status = HAL_CAN_ConfigFilter(rtcan_h->hcan,
&filter);

ADD_ERROR_IF(hal_status != HAL_OK, RTCAN_ERROR_INIT, rtcan_h);
}
}


TX_THREAD* threads[2] = {&rtcan_h->tx_thread, &rtcan_h->rx_thread};

for (uint32_t i = 0; i < 2; i++)
Expand Down