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

Solution for mutex problem in CAN read ISR #14688

Merged
merged 12 commits into from
Jul 12, 2021
Merged

Solution for mutex problem in CAN read ISR #14688

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
033b08d
Implementation for can_read to be deferred to thread context when rx
MubeenHCLite May 21, 2021
1a2d624
Added Raw CAN (unlocked can read api) and updated the code for the
MubeenHCLite Jun 29, 2021
5d861f8
Merge branch 'master' into CAN_rxinterrupt_fix
MubeenHCLite Jun 29, 2021
3565ae3
Removed the new line at the end of file.
MubeenHCLite Jun 30, 2021
7a4a4ea
Added a newline at the end of RawCAN.cpp
MubeenHCLite Jun 30, 2021
debfda6
Updated the Licenses for RawCAN and aligned indentation
MubeenHCLite Jun 30, 2021
d4eba5c
Resolving the astyle error
MubeenHCLite Jun 30, 2021
8622f66
Resolved review comments
MubeenHCLite Jul 3, 2021
ff01046
Spell check corrected
MubeenHCLite Jul 3, 2021
65a72c2
Added documentation for the implementation
MubeenHCLite Jul 6, 2021
27df8c7
Indentation set for the comments
MubeenHCLite Jul 7, 2021
d477efe
Corrected the conditional compilation of RawCAN.
MubeenHCLite Jul 9, 2021
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
18 changes: 18 additions & 0 deletions targets/TARGET_STM/can_api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,8 +646,12 @@ void can_irq_set(can_t *obj, CanIrqType type, uint32_t enable)
#include <string.h>
#include <inttypes.h>

#define ENABLED true
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than inventing your own values, just name the variable so booleans work:

static bool rx_irq_enabled = false;

#define DISABLED false

static uint32_t can_irq_ids[CAN_NUM] = {0};
static can_irq_handler irq_handler;
static uint32_t rx_irq_status = DISABLED;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this not need to be either an array [CAN_NUM] or a flag inside the can_t object? Probably the former, but I'm not quite sure how this is handling multiple instances.


static void can_registers_init(can_t *obj)
{
Expand Down Expand Up @@ -767,6 +771,7 @@ void can_irq_free(can_t *obj)
can->IER &= ~(CAN_IT_FMP0 | CAN_IT_FMP1 | CAN_IT_TME | \
CAN_IT_ERR | CAN_IT_EPV | CAN_IT_BOF);
can_irq_ids[obj->index] = 0;
rx_irq_status = DISABLED;
}

void can_free(can_t *obj)
Expand Down Expand Up @@ -998,6 +1003,10 @@ int can_read(can_t *obj, CAN_Message *msg, int handle)
can->RF1R |= CAN_RF1R_RFOM1;
}

if(rx_irq_status == ENABLED) {
__HAL_CAN_ENABLE_IT(&obj->CanHandle, CAN_IT_FMP0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we believe this is race clear? If new data has already arrived before this line, will this generate an interrupt?

Hopefully this is solid, but it's possible you may have to do

enter critical section
enable FMP0 interrupt
release FIFO
disable critical section

to make sure the thing is ready to generate interrupts as the FIFO is released. I don't know if it's worth doing that just in case.

}

return 1;
}

Expand All @@ -1011,6 +1020,7 @@ void can_reset(can_t *obj)

/* restore registers state as saved in obj context */
can_registers_init(obj);
rx_irq_status = DISABLED;
}

unsigned char can_rderror(can_t *obj)
Expand Down Expand Up @@ -1162,6 +1172,11 @@ static void can_irq(CANName name, int id)
tmp1 = __HAL_CAN_MSG_PENDING(&CanHandle, CAN_FIFO0);
tmp2 = __HAL_CAN_GET_IT_SOURCE(&CanHandle, CAN_IT_FMP0);

// In legacy can, reading is the only way to clear rx interrupt. But can_read has mutex locks
MubeenHCLite marked this conversation as resolved.
Show resolved Hide resolved
// since mutexes cannot be used in ISR context, rx interrupt is masked here to temporary disable it
// rx interrupts will be unamsked in read operation. reads must be deffered to thread context.
__HAL_CAN_DISABLE_IT(&CanHandle, CAN_IT_FMP0);

if ((tmp1 != 0) && tmp2) {
irq_handler(can_irq_ids[id], IRQ_RX);
}
Expand Down Expand Up @@ -1261,6 +1276,7 @@ void can_irq_set(can_t *obj, CanIrqType type, uint32_t enable)
ier = CAN_IT_FMP0;
irq_n = CAN1_IRQ_RX_IRQN;
vector = (uint32_t)&CAN1_IRQ_RX_VECT;
rx_irq_status = ENABLED;
break;
case IRQ_TX:
ier = CAN_IT_TME;
Expand Down Expand Up @@ -1293,6 +1309,7 @@ void can_irq_set(can_t *obj, CanIrqType type, uint32_t enable)
ier = CAN_IT_FMP0;
irq_n = CAN2_IRQ_RX_IRQN;
vector = (uint32_t)&CAN2_IRQ_RX_VECT;
rx_irq_status = ENABLED;
break;
case IRQ_TX:
ier = CAN_IT_TME;
Expand Down Expand Up @@ -1326,6 +1343,7 @@ void can_irq_set(can_t *obj, CanIrqType type, uint32_t enable)
ier = CAN_IT_FMP0;
irq_n = CAN3_IRQ_RX_IRQN;
vector = (uint32_t)&CAN3_IRQ_RX_VECT;
rx_irq_status = ENABLED;
break;
case IRQ_TX:
ier = CAN_IT_TME;
Expand Down