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.

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
94 changes: 94 additions & 0 deletions drivers/include/drivers/RawCAN.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/*
* Copyright (C) 2021, STMicroelectronics, All Rights Reserved
* SPDX-License-Identifier: Apache-2.0
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#ifndef RAWCAN_H
#define RAWCAN_H

#include "platform/platform.h"
#include "drivers/CAN.h"

#if DEVICE_CAN || defined(DOXYGEN_ONLY)

#include "interfaces/InterfaceCAN.h"
#include "hal/can_api.h"
#include "platform/Callback.h"
#include "platform/PlatformMutex.h"

namespace mbed {
#ifndef FEATURE_EXPERIMENTAL_API
class RawCAN: public CAN {
public:
/** Creates an unlocked CAN interface connected to specific pins.
*
* @param rd read from transmitter
* @param td transmit to transmitter
*
* Example:
* @code
* #include "mbed.h"
*
*
* Ticker ticker;
* DigitalOut led1(LED1);
* DigitalOut led2(LED2);
* //The constructor takes in RX, and TX pin respectively.
* //These pins, for this example, are defined in mbed_app.json
* RawCAN can1(MBED_CONF_APP_CAN1_RD, MBED_CONF_APP_CAN1_TD);
* RawCAN can2(MBED_CONF_APP_CAN2_RD, MBED_CONF_APP_CAN2_TD);
*
* unsigned char counter = 0;
*
* void send() {
* if(can1.write(CANMessage(1337U, &counter, 1))) {
* printf("Message sent: %d\n", counter);
* counter++;
* }
* led1 = !led1;
* }
*
* int main() {
* ticker.attach(&send, 1);
* CANMessage msg;
* while(1) {
* if(can2.read(msg)) {
* printf("Message received: %d\n\n", msg.data[0]);
* led2 = !led2;
* }
* ThisThread::sleep_for(200);
* }
* }
*
* @endcode
*/

/* Note: The can apis are unlocked hence using this when multiple
* threads are accessing a single instance of CAN will lead to
* race conditions, can be used in single threaded CAN.
*/
using CAN::CAN;


// override lock apis to create unlocked CAN
void lock() override {};
void unlock() override {};

};
#endif //FEATURE_EXPERIMENTAL_API
}

#endif
#endif //RAWCAN_H
1 change: 1 addition & 0 deletions mbed.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
#include "drivers/I2C.h"
#include "drivers/I2CSlave.h"
#include "drivers/CAN.h"
#include "drivers/RawCAN.h"
#include "drivers/UnbufferedSerial.h"
#include "drivers/BufferedSerial.h"
#include "drivers/FlashIAP.h"
Expand Down
57 changes: 57 additions & 0 deletions targets/TARGET_STM/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,63 @@ For application that require optimized maximum performance, the recommendation i
The SPI DMA transfer support shall be implemented on a case-by-case based on below example
https://github.com/ABOSTM/mbed-os/tree/I2C_SPI_DMA_IMPLEMENTATION_FOR_STM32L4

### CAN receive interrupt problem due to mutex and resolution

In bxCAN and earlier versions the receive interrupt flags can be cleared only on performing a read operation in ST MCUs
But can_read() cannot be used in interrupt context as it is gaurded by lock operation and mbed does not allow locks in
interrupt context. Hence the Rx interrupt is disabled for a while and read is deferred to thread context, the interrupt is
enabled on a successful read.

As an other option RawCAN (with unlocked CAN apis) is also available and can be used directly, if only one thread is accessing
the CAN interface.

While using RxInterrupt with the CAN object the receive ISR callback registered should defer read to thread context.
A simple example is as shown below:

#include "mbed.h"

Ticker ticker;
Thread canReadThread;

DigitalOut led1(LED1);
DigitalOut led2(LED2);
DigitalOut led3(LED3);

CAN can1(PD_0 ,PD_1);

EventQueue queue(32 * EVENTS_EVENT_SIZE);

int counter = 0xABCD;
CANMessage msg;

void canRead(){
if(can1.read(msg)) {
if(msg.id==1100)
led2 = !led2;
if(msg.id==1102){
led3 = !led3;
}
}
}

void canISR(){
queue.call(canRead);
led3 = !led3;
}

int main() {

can1.frequency(100000);
can1.mode(CAN::Normal);

can1.attach(canISR, CAN::RxIrq);

canReadThread.start(callback(&queue, &EventQueue::dispatch_forever));

while(1) {
}
}


## Mbed OS Wiki pages

Expand Down
1 change: 1 addition & 0 deletions targets/TARGET_STM/TARGET_STM32F0/objects.h
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ struct can_s {
CAN_HandleTypeDef CanHandle;
int index;
int hz;
int rxIrqEnabled;
};
#endif

Expand Down
1 change: 1 addition & 0 deletions targets/TARGET_STM/TARGET_STM32F1/objects.h
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ struct can_s {
CAN_HandleTypeDef CanHandle;
int index;
int hz;
int rxIrqEnabled;
};
#endif

Expand Down
1 change: 1 addition & 0 deletions targets/TARGET_STM/TARGET_STM32F2/objects.h
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ struct can_s {
CAN_HandleTypeDef CanHandle;
int index;
int hz;
int rxIrqEnabled;
};
#endif

Expand Down
1 change: 1 addition & 0 deletions targets/TARGET_STM/TARGET_STM32F3/objects.h
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ struct can_s {
CAN_HandleTypeDef CanHandle;
int index;
int hz;
int rxIrqEnabled;
};
#endif

Expand Down
1 change: 1 addition & 0 deletions targets/TARGET_STM/TARGET_STM32F4/objects.h
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ struct can_s {
CAN_HandleTypeDef CanHandle;
int index;
int hz;
int rxIrqEnabled;
};
#endif

Expand Down
1 change: 1 addition & 0 deletions targets/TARGET_STM/TARGET_STM32F7/objects.h
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ struct can_s {
CAN_HandleTypeDef CanHandle;
int index;
int hz;
int rxIrqEnabled;
};
#endif

Expand Down
1 change: 1 addition & 0 deletions targets/TARGET_STM/TARGET_STM32L4/objects.h
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ struct can_s {
CAN_HandleTypeDef CanHandle;
int index;
int hz;
int rxIrqEnabled;
};
#endif

Expand Down
1 change: 1 addition & 0 deletions targets/TARGET_STM/TARGET_STM32L5/objects.h
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ struct can_s {
CAN_HandleTypeDef CanHandle;
int index;
int hz;
int rxIrqEnabled;
};
#endif

Expand Down
16 changes: 16 additions & 0 deletions targets/TARGET_STM/can_api.c
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,7 @@ void can_irq_init(can_t *obj, can_irq_handler handler, uint32_t id)
{
irq_handler = handler;
can_irq_ids[obj->index] = id;
obj->rxIrqEnabled = false;
}

void can_irq_free(can_t *obj)
Expand All @@ -787,6 +788,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;
obj->rxIrqEnabled = false;
}

void can_free(can_t *obj)
Expand Down Expand Up @@ -1012,6 +1014,10 @@ int can_read(can_t *obj, CAN_Message *msg, int handle)
/* Release the FIFO */
can->RF0R |= CAN_RF0R_RFOM0;

if(obj->rxIrqEnabled == true) {
__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 @@ -1025,6 +1031,7 @@ void can_reset(can_t *obj)

/* restore registers state as saved in obj context */
can_registers_init(obj);
obj->rxIrqEnabled = false;
}

unsigned char can_rderror(can_t *obj)
Expand Down Expand Up @@ -1177,6 +1184,12 @@ 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 (bxCAN and earlier), reading is the only way to clear rx interrupt. But can_read has mutex locks
// 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.
// refer to the CAN receive interrupt problem due to mutex and resolution section of README doc.
__HAL_CAN_DISABLE_IT(&CanHandle, CAN_IT_FMP0);

if ((tmp1 != 0) && tmp2) {
irq_handler(can_irq_ids[id], IRQ_RX);
}
Expand Down Expand Up @@ -1276,6 +1289,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;
obj->rxIrqEnabled = true;
break;
case IRQ_TX:
ier = CAN_IT_TME;
Expand Down Expand Up @@ -1308,6 +1322,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;
obj->rxIrqEnabled = true;
break;
case IRQ_TX:
ier = CAN_IT_TME;
Expand Down Expand Up @@ -1341,6 +1356,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;
obj->rxIrqEnabled = true;
break;
case IRQ_TX:
ier = CAN_IT_TME;
Expand Down