NUCLEO_F446RE: CAN RX interrupt ends up in infinite loop

[pavels]

Description

• Type: Bug
• Priority: Major

---

Bug

Target
NUCLEO_F446RE

Toolchain:
GCC_ARM

Toolchain version:
arm-none-eabi-gcc (Arch Repository) 7.1.0

mbed-cli version:
1.2.0

mbed-os sha:
cc7556a (HEAD, tag: mbed_lib_rev152, tag: mbed-os-5.6.1, tag: latest, origin/mbed-os-5.6)

---

Something is wrong with the CAN RX interrupt handling.
I am simply attaching my interrupt handler using attach method in instance of CAN class.
First - it is not possible to call read() on the can object in the interrupt routine as there is a mutex lock and this will fail in the interrupt context. On the other end if you do not read from the can interface. The interrupt routine is called again and again in infinite loop so it is not possible to pass the actual read to for example EventQueue as the interrupt handler will be called again and again and the program never leaves the interrupt context.

If i comment out the locking in CAN.cpp and execute read() in the interrupt handler, all is OK and the interrupt handler is called only when there is some messages left in the mailbox.

[0xc0170]

cc @bcostm @adustm @LMESTM @jeromecoutant
[Mirrored to Jira]

[LMESTM]

@0xc0170 can you help describe how this is supposed to work ?

Isn't application supposed to read() from interrupt handler when an Can Rx interrupt is received ?
I understand that this is how this works with serial driver for instance and probably why there is no mutex lock is the serial driver

void SerialBase::lock() {
    // Stub
}
void SerialBase:: unlock() {
    // Stub
}

@pavels : maybe an alternative is to disable the interrupt from the interrupt handler before using the EventQueue, then read from your CAN thread and enable interrupt again ...

[Mirrored to Jira]

[pavels]

@LMESTM if you mean to disable by calling attach with null as first param (handler function) than that is not possible either as there is mutex too. Only option is go directly to registers - but that sounds like really bad idea (i am changing registers when there is multiple layers of API which expects that nobody changes them directly).

To me it looks like there should be two sets of functions like read and read_unsecure or mabye read_interrupt one locks and throws error, the second one doesn't but you need to know what you are doing - it is up to you to take care of possible parallel reads from multiple threads.

btw in Serial.cpp there is this:

void Serial::lock() {
    _mutex.lock();
}

void Serial::unlock() {
    _mutex.unlock();
}

[Mirrored to Jira]

[LMESTM]

@0xc0170 - can you have a look at the discussion and provide guidance ? I don't see a good way frowared as of now.

@pavels
Ok - serial driver and classes is more complex. What I want to say that lock management is clearly different and it is allowed to read serial from the interrupt.
[Mirrored to Jira]

[0xc0170]

I am looking at this now, I believe that using mutex is an issue here. I'll have a closer look now and send a patch.
[Mirrored to Jira]

[LMESTM]

@0xc0170 bump ...
[Mirrored to Jira]

[0xc0170]

Back to this one.

CAN is thread safe by default - It provides virtual locking methods that thus a parent can override these to handle thread safety on its own.

@c1728p9 @SenRamakri suggestions to resolve this one? Should we make CAN interrupt safe?
[Mirrored to Jira]

[omdathetkan]

Bump! We are seeing the same issue... This also applies to LPC1768.
The only easy workaround I can think of is getting rid of the lock in CAN::read (can be done without modifying the mbed-os files by subclassing it in the application and adding a CAN::readUnsafe())

int CAN::read(CANMessage &msg, int handle) {
    //lock();
    int ret = can_read(&_can, &msg, handle);
    //unlock();
    return ret;
}

Would the long term solution to this maybe using mailboxes that are handled in the CAN api?

CAN can0(p9,p10);
Mail<CANMssage_t, 16> mail_box_tx;
Mail<CANMssage_t, 16> mail_box_rx;

int main (void) {
    int i = 0;

    can0.attachRxMailbox(&mail_box_rx);
    can0.attachTxMailbox(&mail_box_tx);

    mail_t *mail = mail_box_tx.alloc();
    mail->id = 10;
    mail->data[0] = i++;
    mail_box.put(mail);

    mail_t *mail = mail_box_tx.alloc();
    mail->id = 10;
    mail->data[0] = i++;
    mail_box.put(mail);

    mail_t *mail = mail_box_tx.alloc();
    mail->id = 10;
    mail->data[0] = i++;
    mail_box.put(mail);

    while (true) {
        osEvent evt = mail_box_rx.get(0); // Non Blocking
        if (evt.status == osEventMail) {
            CANMssage_t *mail = (CANMssage_t*)evt.value.p;
            printf("[%0x]: [%02x]\n\r", mail->id, mail->data);
            mail_box.free(mail);
        }
        // Do stuff
    }
}

OR

CAN can0(p9,p10);
Mail<CANMssage_t, 16> mail_box_rx;
Thread messageHandlerThread(osPriorityRealtime);

void can_handler(void)
{
    while (true) {
        osEvent evt = mail_box_rx.get(); // Blocking, Thread suspended until new data available
        if (evt.status == osEventMail) {
            CANMssage_t *mail = (CANMssage_t*)evt.value.p;
            printf("[%0x]: [%02x]\n\r", mail->id, mail->data);
            mail_box.free(mail);
        }
    }
}

int main (void) {
    messageHandlerThread.start(callback(can_handler));
    can0.attachRxMailbox(&mail_box_rx);
    while(1) {
        // Do Stuff
    }
}

[Mirrored to Jira]

[SenRamakri]

We should not get rid of locking in CAN driver, its good to have the locking if multiple threads are to operate on the same object. A good pattern would be to have a event/mailbox trigger a reader thread from the callback and have that thread read the CAN frame/data.
[Mirrored to Jira]

[omdathetkan]

@SenRamakri yes but the problem here is that the interrupt only clears after the data has been read. If you do not read the data in the IRQ context, the interrupt will keep firing. Currently it is not possible to read it from the irq context, so there is no way to use the rx interrupt.
[Mirrored to Jira]

[pavels]

The Serial ended up in the exact same situation so this is relevant #5446
[Mirrored to Jira]

[LMESTM]

@0xc0170 shouldn't label be updated ? This does not seem to be st specific and maybe the wrong labeling is a possible cause of slow progress here ?
[Mirrored to Jira]