I-class CAN driver APIs

[tinito]

Ops, as usual i misunderstand const pointers, pointer to const etc etc etc. The current implementation reflects what I meant. Sorry, my fault!

[tinito]

I'm implementing the waiting synchronous APIs, that reflect what done in other drivers, like

Code: Select all

  if (!can_lld_can_transmit(canp))
    _can_wait_s(canp);
  msg = chThdSelf()->p_u.rdymsg;
  if (msg == RDY_OK)
    can_lld_transmit(canp, ctfp);

where _can_wait_s(canp) is a macro like in the SPI driver:

Code: Select all

#define _can_wait_s(canp) {                                                 \
  chDbgAssert((canp)->thread == NULL,                                       \
              "_can_wait(), #1", "already waiting");                        \
  (canp)->thread = chThdSelf();                                             \
  chSchGoSleepS(THD_STATE_SUSPENDED);                                       \
}

This is done in both canTransmit() and canReceive(), but in this way one thread cannot wait for transmission if another thread is waiting for receive. What do you suggest?
We can use two "thread" fields, like canp->txthread and canp->rxthread, for example.

[Giovanni]

Yes, you need two thread fields in that case.

Giovanni

[tinito]

Hi,

I've wrote most of the "new" CAN driver implementation, it still need review and some feature are not working / temporary, but I've made some tests and it seems to work!
The APIs has:
- canTryTransmit()/canTryReceive(): asynchronous tx/rx functions that rely on I-class functions (as in other drivers), they return TRUE/FALSE if the operation was completed or not. A callback is called on TX complete and RX interrupts (there is an error callback too)
- canAbortTransmit() to abort an enqueued transmission (still to finish)
- canTransmit()/canReceive() (if enabled with CAN_USE_WAIT): synchronous calls that put the thread in SUSPEND state while waiting for completition
- events triggering (if enabled with CAN_USE_EVENTS) on tx/rx/error/sleep/wakeup events
- low level helper macros like in ADC/SPI drivers

most of the low level stuff are unchanged (init, tx, rx are the same, the IRQ functions are slightly changed).

I've tested the driver with the simple program attached, that uses async calls, sync calls, events and sleep/wakeup functions. It works, but again I need to review everything and complete some parts, but the architecture should be defined. I've created a fork on my github account as it is easier for me, you can see it here (only can.h can.c STM32/can_lld.h STM32/can_lld.c are update of course) https://github.com/openrobots-dev/ChibiOS

I hope there are not too much mistakes, any comment?

[Giovanni]

Hi,

It looks good so far, one of the conventions is that the verb used for asynchronous functions is "Start" so it should always be <drv>Start<Something>().

Edit: Never mind, "try" is OK in this case since is it trying to enqueue or dequeue a message, the operation is not necessarily started by the function it can be already ongoing.

Giovanni

[tinito]

Hi,

back working on the CAN driver. I'm now on the filtering part, I would like to have some canAddFilter() and canDelFilter() functions to dynamically register and unregister filters, like

Code: Select all

int8 canAddFilter(canp, id, mask)

which returns the number of the filter that has been added (or -1 if there are no more filters available), so it is easy to set up an array of callbacks to handle the filtered messages.

Trying to be not STM32-ish as usual, I've looked into NXP and Atmel CAN peripheral reference manuals discovering that, like for the I2C, the STM32 implementation is pretty different from others.

Basically, NXP and Atmel have "Message Objects" (up to 32 MOBs) and each can be configured as TX or RX mailbox. If a MOB is configured as RX, a id/mask can be specified and that will be the filter, so each matching CAN frame is stored in that MOB. You can also configure them as FIFO, there is a lot of flexibility while the filtering scheme remains absolutely simple.

On the STM32, you have 3 TX mailboxes and 2 RX FIFO, and a set of "filter banks" (14 in normal line, 28 in connectivity line). Each "bank" can be configured as single 32-bit id/mask pair, or two 32-bit id list, or 2 16-bit id/mask pair, or 4 16-bit id list. Then, each filter is identified by a number that depends also on the previous filters configuration (reference manual RM0008 Fig. 201 pag 555 has an example of filter numbering). Each filter can be assigned to FIFO0 or FIFO1, and this again changes the numbering scheme. One more annoying point is that seems to me that there is no way to understand if a filter is free or not: you can activate a whole filter bank, so up to 4 16-bit id-list filters, but then you cannot guess which of the 4 filters are used and which are free.
As a consequence i think we have to trace which filters are used and which not. But if the filter bank configuration is modified, the filtering numbering scheme changes and to handle this a lot of complexity (and overhead) is needed.

So, the first proposal is to define at compile time if the driver uses id/mask pairs or id-list, and 32-bit or 16-bit long filters. This defines the maximum number of filters available and a constant numbering scheme that reflects what we have in other platforms. We loose some flexibility on STM32 platform but everything becomes much easier.

Then we have to keep trace of used and unused filters. On NXP/Atmel CAN peripherals you can determine the status of a filter looking into some register bits, so the "tracing" part should stay in the STM32 low level driver.

Finally, we can also avoid the storage of the filters in an array (each filters is at least 9 bytes long, and we can have up to 54 filters) to save RAM, as we can register/unregister/get the filter status and configuration from the registers. I was thinking about a canFilterInit() function that accepts an array of filters (like the one we have now) for static CAN filter configuration, and the "new" functions for dynamic use..

Any comment/suggestion?
Sorry for being that verbose, it is useful aslo to clarify everything to myself

[Giovanni]

A note about return values, use bool_t for errors, the convention in ChibiOS is:
False = OK
True = problem.
Direct use of integer types is discouraged in API functions.

About filters, in general I agree to remove them from the configuration and add a canAddFilter() function, a filter would be platform dependent so this structure CanFilter should be defined in the LLD. The allocation would be simple, a index variable points to the next filter to be added, initially it is zero and it is incremented after each add.
A function canClearFilters() could be added in order to remove all filters at once (set to zero the index variable and set filter registers to their reset value).

A default value CAN_FILTER_ALL should be predefined for the CanFilter variable (or just passing NULL to the canAddFilter() function).

Note that the canAddFilter() would add a whole bank in the STM32 regardless the bank mode, you can also have mixed banks.

In architectures with MOBs the allocator would enable the MOBs.

How does this look?

Giovanni

[tinito]

Thansk Giovanni.

Ok for the return vaule, I see that no APIs return anything different from void, bool_t or msg_t.

Having a pointer to the next free filter is ok unless you want to clear _one_ (not all) of the configured filters (e.g., i'm not interestend in that frame ID anymore), but if you think this is an uncommon situation and the overhead is not justified we can simply keep the pointer to the next free filter.

About using the CanFilter structure, having it defined in the LLD like now (that is a sort of wrapper to the registers) doesn't limit the portability of the driver? I was thinking about canAddFilter(GENERIC_AND_VALID_CAN_ID), not canAddFilter(canFilter), and the lld takes care of mapping the ID to the platform-dependent structure (or directly to the registers).
I see that most (all?) drivers have platform-dependent configurations, as the CAN driver would have, but I see message filtering at the application level, not at configuration level, so I was thinking about a generic function that we can call regardless the platform we are running.
A solution would be to have a canAddFilter(struct canFilter) function and helper macros like canAddFilterID() that are implemented in the LLD to map the GENERIC_AND_VALID_CAN_ID to the platform-dependent canFilter structure, but I'm not sure that this reflect ChibiOS drivers style..

[Giovanni]

The problem is that filtering is something you should not expect equal on all platforms in terms of capabilities this is why I see it as a configuration structure. On some platforms filtering could not be present at all.

Giovanni

[tinito]

Ok, so I'll leave canAddFilter(canFilter), with the platform-dependent canFilter structure, while writing some macros for STM32 to translate a standard CAN ID to the canFilter structure.
What I don't like is having to configure a whole bank at time, so if you want to configure only one filter you are wasting up to 3 filter slots, but this is a limit due to STM32 CAN peripheral implementation that is pretty weird.