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

FDCAN peripheral refactor #3380

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

hansihe
Copy link
Contributor

@hansihe hansihe commented Sep 27, 2024

Contains a (mostly) full rewrite refactor of the FDCAN peripheral driver.

STM32 controllers contain two variants of this peripheral as far as I can tell:

  • The H7 series contains the full M_CAN peripheral IP licensed from Bosch. This driver targets this, and therefore the H7 as its primary target.
  • Other series (like G4) contain a much simplified version of the M_CAN, but the base capabilities are the same. Some simplifications include non configurable message ram, small FIFOs/Queues, etc. These things are mainly just #[cfg(...)]ed out.

The goal of this rewrite is to:

  • Expose more capabilities of the underlying hardware
  • Make the driver more usable in a production CAN system
  • Improve error handling
  • Improve configurability and flexibility

Feel free to ask follow up questions if I should go more into depth on anything.

TODOs:

  • Mostly written against H7 but with the G4 kept in mind. Likely doesn't compile against G4 yet.
  • Update test suite.

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:

  • A high level interface which the library users would interact with. This deals with maintaining internal state in the driver.
  • A low level, stateless interface. This is a fairly thin abstraction over the registers, and implements a lot of procedures which are described in the datasheets.

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.

@Dirbaio
Copy link
Member

Dirbaio commented Sep 27, 2024

  • You mention your rewrite exposes more hardware capabilities, and is more usable/configurable/flexible. Can you show examples? i.e. "with the previous API doing X was annoying/impossible, with the new API it's easy thanks to Y"
  • Why does fixing the above require a ground-up rewrite? Why can't it be done with incremental fixes or additions to the existing driver?

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.

@hansihe hansihe changed the title FDCAN peripheral rewrite FDCAN peripheral refactor Sep 27, 2024
@hansihe
Copy link
Contributor Author

hansihe commented Sep 27, 2024

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:

  • From what I can tell this was written to target the FDCAN in the G4. This is really a simplified and cut down version of the M_CAN peripheral IP from Bosch.
  • This driver was later extended to work with the full M_CAN, but kept the same limitations of the cut down version.
  • In order to properly expose a lot of the features of the full version, some pretty deep changes were required (the Message RAM changes)
  • This snowballed into exposing those capabilities through the higher levels of the driver.

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 🙂

@hansihe
Copy link
Contributor Author

hansihe commented Sep 27, 2024

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 errors

Previously 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 buffers

The 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 independently

The 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 tasks

As is today, you are required to have a mutable reference to the Can struct to be able to transmit and receive. This is not really all that bad in itself, but that limitation really begins to show once support for the multiple Tx/Rx mechanisms the hardware supports are exposed (including the dedicated Rx buffers mentioned above).

Tracking finished transmissions

Before there was a flush method exposed that could be used to wait for a certain Tx slot to finish. This had some limitations:

  • You were required to provide the slot index, but there was no way to know which slot your message got queued into with the API.
  • If the struct is made sharable, this API is racy.

A TxRef type is added as the return value from the tx_* functions, which enables you to cancel the transmission and wait for it to be finished.

Message RAM configuration

The 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:

  • 2x Rx FIFOs with 3 slots
  • 0 Dedicated Rx buffers
  • Tx FIFO/Priority queue with 3 slots
  • 0 Dedicated Tx buffers
  • 28 Standard ID filters
  • 8 Extended ID filters

In the full (H7/M_CAN) variant, everything is configurable (limiting factor usually being FDCANRAM size), with up to:

  • 2x Rx FIFOs with 64 slots
  • 64 Dedicated Rx buffers
  • 32 Tx slots (configurable as Tx/FIFO/Dedicated buffers)
  • 128 Standard ID filters
  • 64 Extended ID filters

As you can see this is quite a massive difference, which these changes enable usage of where the HW supports it.

@cschuhen
Copy link

@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.

@hansihe
Copy link
Contributor Author

hansihe commented Sep 28, 2024

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 tx_* methods do not require a mutable reference to send messages. Do you think this would cover the use cases you were thinking about for Tx?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants