Hi Lars,

see my comments below
>>> - Here is a high level view of the current and suggested new API
>>>
>>> Current driver has 5 public functions:
>>> * CAN_HANDLE open(s_BOARD* (name, baudrate)
>>> * close(CAN_HANDLE)
>>> * setbaudrate(CAN_HANDLE, baudrate)
>>> * send(canmsg)
>>> * canmsg recive
>>>
>>> The new API has only one public function
>>> * driverStruct openEx(const char name).
>>>
>>> The open function does fill the driverStructure with function pointers
>>> and variable, do some basic init stuff procedure.
>>> The function does not set baud rate or start the device.
>>>
>>> driverStruct
>>> {
>>>     void*    pointer to driver internal struct. Like current CAN_HANDLE
>>>     long int supported CtrlMode. bit-field indicating what function mode
>>> the driver supports (fd, fd non-is, listen only etc.)
>>>     setBaudrate (function pointer) to a function that set main and
>>> databaudrate individually..... maybe better to set them at the same
>>> time?
>>>     send        (function pointer) same function as current API
>>>     recive      (function pointer) same function as current API
>>>     setState    (function pointer) start/stop the driver
>>>
>>>     //The functions pointer below are optional and may be set to NULL
>>>     getCanState //active, warning passive etc.
>>>     getErrorCounters //get tx/rx counter
>>>     setMode (turns on and off multiple modes at the same time)
>>> }
>>>
>>> Using the new API to init the driver one must call multiple functions
>>> to replace the current open function
>>>
>>> 1. openEx       to get the driver structure
>>> 2. setBaudrate  to set baud rate
>>> 3. setState     to start the device
>>>
>>> - Why use struck and more functions:
>>>
>>> The reason for splitting up open into multiple stages is to enable
>>> function like autobaudrate.
>>> There are multiple reason for using.
>>> 1. One can add optional functions and test for support in runtime by
>>> checking if the function pointer is NULL
>>> 2. One can create a layered structure by replacing send/receive
>>> function with one that does something with the message before it is
>>> sent or receive.
>>>      To make a software filter:
>>>      1. Get the struct from the driver
>>>      2. Make a copy of the struct and change the receive function to
>>> filter function that when call calles the driver receive function and
>>> checks the message and then optional passes it on.
>>>
>>>      Implementing CiA 302 (redundancy) can also be a lot simpler to
>>> create a layer between the driver and the normal caneopen stack
>> Iooks nice, this driverStruct may be seen like the big co_data struct of
>> canfestival, an object containing a complete instance of the driver.
>> I like the similarity of those two structs.
>> In the same spirit, i would prefer to see the getCanState and
>> getErrorCounter as callback functions.
>> Something like canStateChange() and errorCounterChange() called by the
>> driver when update occurs.
>> If the driver implement this, it provides default callbacks that does
>> nothing, if it does not implement this functions pointers are set to NULL.
>> Then the user is free to register his own callbacks if needed.
> That is a god idea, i can add a "checkForChange" function that checks
> for changes and calls the callback on changes.

I was thinking of avoiding polling, providing an asynchronous API.
May be it could work like this, at least for socketcan:

1- configure socketcan to allow the reception of error frames

2- in SocketFD_receive() when a frame is received check canid to see if 
its an error frame or a data frame

3- if data frame return like you already do

4- If it is an error frame then decode the error or status change (see 
/usr/include/linux/can/error.h)
     Then call the callback and after don't return from the function but 
loop to can receive again

May be we can also provide a polling API by updating a status and error 
variable that can be read via access functions like getState()
This way user can choose between asynchronous or polling API.

>
>> May be we need a more general callback for errors, i think of all errors
>> reported by socketcan, each time the driver receives an error frame it
>> calls the callback with the error code as parameter.
> I see that QT uses something similar to socketCan error frame.
> http://doc.qt.io/qt-5/qcanbusframe.html
>
> Can we just do the same?
> Add a variable to the Message that says it is a error frame and return
> if from canReceive_driver like a normal can message.
This is what already exists in socketcan, bit29 in canid indicates it is 
not a data frame but an error frame.
The type of error /  event is coded in the datas with DLC=8
Can we take this as a general can errors abstraction and use it also for 
other drivers ?

Instead of doing what i suggested for asynchronous call of error 
callbacks, we could return the error frame to user and let him filter 
and decide what to do.
But this will require a change to canfestival to be able to provide this 
error information to CF user.

Regards

Fran├žois Beaulier

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Canfestival-devel mailing list
Canfestival-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/canfestival-devel

Reply via email to