I-class CAN driver APIs

[Giovanni]

Hi,

News about this? I am evaluating its integration in its current state but there are some points that I wish to discuss first.

Callbacks have parameters that are STM32-specific, this is not acceptable being the callback signature part of the high level API.

The function canAbortTransmission() uses a parameter "mbox" that is STM32-specific so it should not be in the high level driver, is this function really required anyway?

There is a new state CAN_ERROR but it is not used anywhere, should it be removed?

Giovanni

[tinito]

Hi Giovanni,

I've added some "easy to use" addFilter functions but I've some work to do before committing them. I'm intensively using the CAN driver in these days so I should also stress-test it.
I'm going to commit the new addition before the end of this week.

About your questions:

Callbacks have parameters that are STM32-specific, this is not acceptable being the callback signature part of the high level API.

I know. The problem here is that you really need to know which mailbox or fifo (speaking about STM32) triggered the callback. On other devices they are called Message Object ID (NXP/ATMEL). Can we generically use a "id" field to recognize the source of the event that called the callback function, and then the low level driver translate this to the device-specific mailbox/fifo/message object?

The function canAbortTransmission() uses a parameter "mbox" that is STM32-specific so it should not be in the high level driver, is this function really required anyway?

In my experience I need to abort messages as in real-time applications I have deadlines to match, and if a deadline is lost I've to remove the message from the transmit queue. But it could be a not-so-common use case. If the "generic message ID" approach suggested before is OK, we can use this abstraction here too.

There is a new state CAN_ERROR but it is not used anywhere, should it be removed?

The can driver should enter this state when an error interrupt is raised, before the callback is called and the event is sent. But I forgot to do that

[tinito]

I've committed the update version with Filter support functions:
- void canSetFilters(CANDriver *canp, CANFilter *filters, uint32_t num)
- void canClearFilters(CANDriver *canp)
- bool_t canAddFilter(CANDriver *canp, uint32_t id, uint32_t mask, uint32_t *fip)

the low level implementation of canAddFilter is a mess, needs a total rewrite but I'm not sure you want to keep this in the official ChibiOS driver, so I still didn't update it.

I was also wondering about canTransmitTimeout and canReceiveTimeout functions, using semaphores as in I/O queues, and then exploiting these functions also for the blocking tx/rx functions instead of current implementation that directly reschedule the waiting thread.
What would you prefer?

And about the "generalized IDs" for mailboxes/fifo/filters as in the previous post, do you like them or do you prefer to avoid that part in the _offical_ release?

[Giovanni]

Hi,

I would avoid anything STM32-specific in the API, FIFO and MBs are part of the STM32 implementation and should not appear anywhere in the API, you should not even assume there are identifiable endpoints in the underlying HW.

Timeouts were already in the old API, is there any specific reason to remove them?

In general, it will be fine if you go for KISS solutions. For each thing ask yourself if it is really required, if there is a use case.

Giovanni

[tinito]

What I've implemented was because i needed it, that is "my" use case. And I really need to get identifiable endpoints from the HW.
I'll move them to the tx/rx frame structures that are hardware-specific so they will not affect the APIs.

About the timeouts, I needed the -I APIs, then I really don't know why I went for the current sleep code instead of the previous queue-based code. Now I realize that the previous code with timeouts was much more flexible, I'll revert to it..

[Giovanni]

Hi,

Moving IDs to frames is a good idea. Let's move the discussion after your next changes. In the meanwhile I will just test the current implementation on the F4 with no changes, there is at least one user needing it.

Giovanni

[tinito]

Ok, I've committed the updated driver here: https://github.com/openrobots-dev/ChibiOS/

Changes:
- back to semaphores, but keeping helper macros for low level drivers
- canTransmit() is now a macro to canTransmitTimeout(TIME_INFINITE)
- moved the mailbox ID in the frame descriptor
- removed platform-dependent fields from callbacks
- the error ISR updates the driver state to CAN_ERROR

It seems to work with my test app (the same I used to test it again, it is attached to some post in this thread).
Throw away the canAddFilter() functions: I need them but they still are platform-dependent.

[tinito]

Sorry for reviving this old thread, finally I'm using a custom CAN driver for my projects but still looking to the official one (I would like to use "standard" chibi without any patches).

Currently you have TX semaphore and event, but the TX event doesn't specify if the transmission was successful or not (e.g., arbitration lost or tx error). I guess this may be a problem not only in my specific application, how would you improve it respecting ChibiOS standards?

[Giovanni]

I imagine through an error event and by adding a specific flag to the pending flags.

Note that events are going to be enhanced so the drivers using them will be modified, the topic is here: viewtopic.php?f=3&t=606

Feel free to propose changes.

Giovanni

[tinito]

About the CAN driver, it's not easy to handle TX status due to the asynchronous flow of a can frame through the controller and the bus (you cannot know if a message will be really transmitted when you enqueue it), and due to the differences in CAN controllers (e.g., how many tx mailboxes they have).
Actually, the canTryTransmit() function tells you if a CAN frame was successfully _queued_ for transmission, that means if a free mailbox was available or not. Then, the frame can be succesfully transmitted (it won the arbitration and no bus errors occurred), can loose the arbitration (a higher priority message took the bus), or isn't transmitted due to transmission errors (e.g., it won the arbitration but something went wrong). The current implementation does not let you to know if a frame was successfully transmitted, that looks as a major problem to me.

To distinguish which event occurred (TXOK, ARBITRATION_LOST, TXERR) CAN controllers have TX status flags associated to each TX mailbox (which number may vary from 1 to, as far I've seen, 16 on Atmel micros).
As a consequence, the only way to know if a message was succesfully transmitted or not, is to keep trace of the mailbox it was queued into, and then get the TX status flags when the TX interrupt triggers. To keep trace of the mailbox, in my CAN driver implementation I simply added a "mbox" field to the CANTxFrame structure, which is filled by can_lld_transmit().

The problem comes when you have to handle the TX interrupt: using event flags looks like the best solution to me, but you need 3 flags (TXOK, ARBITRATION_LOST, TXERR) for each mailbox. Thus, the current flag mask, which is abstracted from the specific architecture, can be too small.

In my CAN driver version, what I did is:
- canTxFrame structure has an additional "mbox" field which is filled by can_lld_transmit() when the frame in queued for transmission
- the TX ISR saves the tx status flags in a "txstatus" field I added to the CANDriver structure
- I've helper macros to get the flags for a specific mailbox (OR and shift)
- in my application code, I keep trace of the mbox i've put my data and on TX event I get the TX status for each mailbox and I enqueue again the frames which loose the arbitration

In this way, if a controller has many mailboxes (which means more than 32 bit txstatus mask), it should be possible to handle this through the helper macro.