Skip to content

Conversation

@H-Grobben
Copy link
Contributor

@H-Grobben H-Grobben commented Dec 1, 2021

FDCAN peripheral was not performing as expected, so I debugged and tested it.

Original code where this was based on used a independent state with regards to the interrupt. During heavy bus error conditions the internal state could become out-of-sync with the interrupts.

Testing on NucleoG474 and using PR #6911 (which is not yet merged..) but checked the HAL function to work with H7 family

More explanation:
During the development of an application using the CAN communication, we found a interrupt-run-away in some situations. We found that the error interrupt triggered (al be it Warning, Passive or Bus-Off, all triggered it) the run-away. We could not get out of the non-responsive state without a reset.

I checked the code, and found two things: the error interrupt is enabled but not cleared in the interrupt routine, and an internal variable 'State' that was used to track the message received state (empty, new, full, overflow) that was not directly related to interrupt that indicated the state.

In this pull request I fixed these issue by adding more values for the interrupt reason (warning, passive, bus off) and clearing the error interrupts, and making the internal state directly dependent on the interrupt state for received messages.

While testing the fixes I also found that introducing the FIFO1 in the CAN receive stage, another issue existed. Even if the messages are received into the FIFO1 (by selecting message filtering for FIFO0 and FIFO1), the interrupt firing was indicating FIFO0 Rx. The configuration of the interrupts for this is now also fixed. The CAN peripheral has 2 interrupt lines going into the NVIC controller. The assignment of the interrupt reasons to these 2 interrupt lines was missing. Now the reception of FIFO1 messages, triggers the 2nd interrupt line. Other interrupts (Rx FIFO0 and Bus Error) are assigned to the 1st interrupt line.

#if MICROPY_HW_ENABLE_FDCAN
// TODO implement for FDCAN
return mp_const_none;
// Done: check implementation for FDCAN
Copy link
Member

Choose a reason for hiding this comment

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

I guess this comment can be removed now, because it works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, it can. I will put it on my todo list. Next to the G4 PR...

@iabdalkader
Copy link
Contributor

Hi, does this PR fix this issue #8169 ?

@H-Grobben
Copy link
Contributor Author

H-Grobben commented Jan 13, 2022

Hi, does this PR fix this issue #8169 ?

No, this is an other problem, the problem you are facing is located in the pyb_can.c file, in function pyb_can_send. There the data size is limited to to original CAN spec of 8 bytes. Not the FDCAN updated values.

Fir this PR I focused on getting the peripheral working.

@iabdalkader
Copy link
Contributor

the problem you are facing is located in the pyb_can.c file, in function pyb_can_send

Yes but FDCAN is initialized in fdcan.c in classical CAN mode, it will not support transmission/reception of FD frames. This FD/Classic mode needs to be configurable in fdcan.c:can_init and detected in pyb_can_send() and fdcan.c:can_receive(). Moreover, if understand correctly from the datasheet, it seems possible to configure CAN in FD mode, and control the frame format via FDF bit in TX buffer.

@iabdalkader
Copy link
Contributor

FYI I sent this #8185, it enables FD frames + BRS support, and fixes the Message RAM usage. I also documented how the message RAM is partitioned and what each section is used for. You should probably rebase on that after it's merged. There's minimal changes to fdcan.c

@JohnieBraaf
Copy link
Contributor

I was not aware of this PR and also made a PR #8206 for enabling the rxcallbacks on FD CAN interfaces.

This PR seems to be more complete in its implementation.

@JohnieBraaf
Copy link
Contributor

I have tested this PR, it works very well. Also nice to see the info() message has been impemented.

@dpgeorge
Copy link
Member

Thanks for the effort on this @H-Grobben and thanks @JohnieBraaf for testing.

Squashed and merged in 160e4d9 and 517e82e with some minor clean up (removed duplicate definition of some FIFO macros, and removed duplicated code in info()).

@dpgeorge dpgeorge closed this Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants