-
Notifications
You must be signed in to change notification settings - Fork 807
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
FDCAN peripheral refactor #3380
base: main
Are you sure you want to change the base?
Conversation
It'd have been nice if you reached out first before dropping a 3000-line "rewrite everything" PR. It'd have allowed you to discuss your issues and maybe find solutions with the existing API, and get some confirmation whether maintainers agree these issues are worth rewriting everything or not. Same about the buffered CAN. @cschuhen added it, i'm sure they had a reason to need it. It's nice to ask first instead of removing it because you don't know that reason. |
Thanks for the quick feedback! I definitely see what you are saying, I realize this was bad form of me. I got a bit too wrapped up in the technical aspects, and in retrospect could have communicated ahead of time instead of dropping this massive PR out of nowhere. When I started the work on this, it was never meant to be as big of a thing as it turned into, and it grew more out of my own requirements than anything else. As for why I think this warrants a larger set of changes:
Will provide a more in depth message where I try to expand on some of the limitations I found in the current version of the driver 🙂 |
Let me try to write out some limitations I found with the original driver, and how they could be handled with these changes: Handling bus errorsPreviously the Bus_Off recovery procedure was automatically restarted any time. There was no easy way to detect this and react to it. If you want to write a device that exists on the bus in say a car, you need some way to detect when your device goes into Bus_Off, and react appropriately. In some devices this might include resetting itself, in some disabling itself permanently, and in others rejoining the bus immediately. With these changes you have the ability to choose an error handling mode. You can have the device rejoin the bus automatically as today, or you can choose to bubble up the error to all senders/receivers or handle the errors centrally in a task. Using dedicated Rx buffersThe peripheral has the capability to allocate dedicated buffers, and receive messages with certain IDs into those buffers. This functionality is really useful for CAN because you often have devices transmitting status updates at regular intervals on the bus, and using these dedicated slots could enable tasks to only wait for the messages they need without having to drain the FIFO. With these changes, the dedicated Rx buffers are supported and usable. Using the two FIFOs independentlyThe peripheral has two Rx FIFOs. The hardware allows configuring these differently, and routing different message IDs to different ones of them. With these changes, these two are exposed independently and can be used by, for instance, different tasks. Sharability between tasksAs is today, you are required to have a mutable reference to the Tracking finished transmissionsBefore there was a
A Message RAM configurationThe H7 variant of the peripheral is really configurable in terms of how its different buffers and queues are configured. You can allocate different slices of the memory to different things in a really flexible way. Today the configuration is fully static, and limited to the lowest common denominator (the G4 variant), which has a static memory layout and really limited buffer sizes. The G4 variant is limited to:
In the full (H7/M_CAN) variant, everything is configurable (limiting factor usually being FDCANRAM size), with up to:
As you can see this is quite a massive difference, which these changes enable usage of where the HW supports it. |
@Dirbaio, thanks for the heads up. I'm on vacation (in Europe actually) at the moment and I'm not really in a position to even look at these patches for at least a couple of weeks. I certainly would prefer not to see the buffered RX to go. Even I think buffered TX can have some benefit in some use cases, especially as it allows multiple tasks to easily send. For example, with the Mutex approach, it seems more complicated to first have to (a)wait/unlock the mutex and then await again to send. Though, there are probably other solutions to this also. |
Thanks for chiming in @cschuhen! Hope you are enjoying visiting Europe For sure, if there is a want to have buffered Rx, I can pull that back in 🙂 I would also love to hear more about what you are using Buffered Rx for if you are willing to share, that might make it easier for me to understand the use case you have for it. With the changes in this PR, the driver is designed to be shared across tasks, the |
Contains a (mostly) full
rewriterefactor of the FDCAN peripheral driver.STM32 controllers contain two variants of this peripheral as far as I can tell:
#[cfg(...)]
ed out.The goal of this rewrite is to:
Feel free to ask follow up questions if I should go more into depth on anything.
TODOs:
Some major points:
Message RAM
Configuration structs
At the base of the changes is a new abstraction over the Message RAM. In the full M_CAN peripheral, this memory region is fully configuable, and this driver exposes all of that to the user. This includes full configurability of the different Tx and Rx buffers and queues.
In the G4, message ram is not configurable to the same degree. The same Message RAM abstraction is kept, but is instantiated with the sizing parameters that are hardwired in the hardware.
Code structure
The driver is more or less split in two layers:
Error handling
The different options available for error handling are described in the config.
Buffered interface removed
I have removed the buffered interface, as I am unsure how useful it actually is in the real world.
For the full H7 interface, the different queues can be configured to be fairly long (up to 32 entries in a lot of cases), so this should in many cases be preferred.
I am not fully opposed to adding a buffered interface for Rx for use in the G4 variant of the peripheral, but I would like to hear from someone who actually needs it before investing in it.
I don't believe a buffered interface for Tx is a good idea at all.