[ag-automation] FDDI Progress

[Robert Schwebel]

On Tue, May 29, 2007 at 10:04:23AM +0200, Dieter Hess wrote:
> The header contains all data types and all functions of the fddi.

See my comments below. Btw, it would be better to post these things
inline, because it makes quoting & review easier.

For the final header files we should make sure the lines are smaller
than 80 characters.

> The following things are planned to be delivered in the next few days:
> 
> - an implementation proposal for the fddi
> - a definition of configuration profile for a field bus protocol
>   (which should we use as a first example? We at 3S have definitions for
>   CANopen, Profibus, EtherCat and SERCOS III)

Some use cases would also be a good thing, just to get the picture how
these things are supposte to work.

> /* fieldbus device driver interface */

Header files should clearly state a license; I'd suggest LGPL or BSD, to
make proprietary and oss programs possible.

> /* #include "lldi.h" */
> typedef void *LL_INTERFACE;
> 
> /* result codes */

Please use the normal errno return values, which would be ...

#define RESOK 0					0 (no need to define something here)
#define RESERR_INTERFACE_ALREADY_REGISTERD 1	EBUSY
#define RESERR_LLINTERFACE_ALREADY_IN_USE 2	EBUSY
#define RESERR_LLINTERFACE_HAS_WRONG_TYPE 3	?
#define RESERR_OUT_OF_MEM 4			ENOMEM
#define RESERR_TIMEOUT 5			ETIMEDOUT
#define RESERR_NO_ACCESS 6			EACC
#define RESERR_BUFFER_TO_SMALL 7		ENOBUFS
#define RESERR_WRONGTPUTYPE 8			?

> /* io-interface */
> struct _io_interface

Why the underscore?

> {
> 	char* name;				/* name of the interface */
> 	unsigned long version;	/* version of the interface */
> 
> 	int (*_fptr_bind_io_interface)(char* name, LL_INTERFACE llitf, struct _io_interface_inst **result);
> 	/* function pointer of the bind_io_interface method (see bind_io_interface for further information) */
> 	int (*_fptr_configure_io_interface_inst)(struct _io_interface_inst *itfinst, struct _io_device *root);
> 	/* function pointer of the configure_io_interface_inst method (see configure_io_interface for further information) */
> 	int (*_fptr_scan_io_interface_inst)(struct _io_interface_inst *itfinst, struct _io_device **root);
> 	/* function pointer of the scan_io_interface_inst method (see scan_io_interface_inst for further information) */
> };

Uuuh, just taking the style into account, what about

struct _io_interface
{
	char* name;
	unsigned long version;

	int (*bind)(char *name, LL_INTERFACE llitf, struct io_interface_inst **result);
	int (*configure)(struct _io_interface_inst *itfinst, struct _io_device *root);
	int (*scan)(struct _io_interface_inst *itfinst, struct _io_device **root);
};

You get the picture. There is no need to prefix functions with type
hints; if we design this API in a typesafe way, the compiler is much
better in checking the right types than the programmer. And don't add
unnecessary comments: everyone sees that it's a function pointer, and
it's obvious where to look for it's description. It just makes the code
less readable.

I don't comment the functions here, because that's probably better when
we have some use cases / test programs.

> typedef struct _io_interface *IO_INTERFACE;

Only macros should be upper case.

> /* instance of io-interface */
> struct _io_interface_inst
> {
> 	char* name;			/* name of the io-interface-instance */
> 	IO_INTERFACE ioitf;	/* the type of the io-interface */
> 	LL_INTERFACE llitf;	/* the low level interface, to which the io-interface is bound to */
> 
> 	struct _io_device *root; /* root device, set during configuration */
> 
> 	int num_tpus;		/* number of io-transport units (set after configuration) */
> 	struct _io_transport_units *tpus; /* list of io-transport units (set after configuration) */
> 
> 	void* inst_data;	/* type specific management data */
> };
> 
> typedef struct _io_interface_inst *IO_INTERFACE_INST;
> 
> 
> /* io-device */
> enum _io_device_state
> {
> 	DEVSTATE_VOID,				/* the device is not configured */
> 	DEVSTATE_MISSING,			/* the configuration of the device failed, because it is missing */ 
> 	DEVSTATE_CONFIG_ERROR,		/* the configuration of the device failed, because of wrong config data */ 
> 	DEVSTATE_PREOPERATIONAL,	/* the device is configured, but not working */
> 	DEVSTATE_OPERATIONAL,		/* the device is working */
> 	DEVSTATE_OPERATION_ERROR	/* the device encountered an error, while it is working */
> };
> 
> typedef enum _io_device_state IO_DEVICE_STATE;
> 
> enum _io_device_command
> {
> 	DEVCMD_START,		/* start the device */
> 	DEVCMD_STOP,		/* stop the device */ 
> 	DEVCMD_RESET,		/* reset the device */ 
> 	DEVCMD_IDENTIFY,	/* let the device do something to identify itself (like blinking with a LED) */
> 	DEVCMD_QUITERR		/* quit an error of the device, to return to normal operation */
> };
> 
> typedef enum _io_device_command IO_DEVICE_COMMAND;
> /* use this datatype with the function cmd_iodevice */
> 
> struct _io_device_param
> {
> 	unsigned long key;
> 	unsigned long value;
> 	unsigned short flags;
> };
> 
> #define PARAMFLAG_PTR  0	 /* value contains a pointer to a struct (frist word of the struct is size) */
> #define PARAMFLAG_BYTE 1	 /* value contais a byte */
> #define PARAMFLAG_WORD 2	 /* value contains a word */
> #define PARAMFLAG_LONG 3	 /* value contains a double word */
> #define PARAMFLAG_TYPEMASK 3 /* value contains a double word */
> #define PARAMFLAG_DEFAULT 4  /* value contains a device default parameter. the parameter is used to set up the configuration,
>                               * but need not to be sent to the device

Why no enum here? It makes these constants compile time type safe.

> typedef struct _io_device_param IO_DEVICE_PARAM;
> 
> struct _io_device
> {
> 	IO_INTERFACE_INST itfinst;	/* reference to owning io-interface instance (set by fddi) */
> 
> 	unsigned long physical_id;	/* physical id of the device */
> 	unsigned long logical_id;	/* logical id of the device */
> 	unsigned long type_id;		/* type id of the device */
> 	char* name;					/* optional name of the device (from configuration) */
> 	
> 	int num_params;				/* number of parameters */
> 	IO_DEVICE_PARAM* params;	/* pointer to list of parameters */
> 
> 	struct _io_device* parent;  /* parent device */
> 	struct _io_device* first_child; /* first child device of this device */
> 	struct _io_device* next_child;/* next child of the same parent */
> 
> 	IO_DEVICE_STATE state;		/* state of the device */   
> };
> 
> typedef struct _io_device *IO_DEVICE;
> 
> 
> /* io-transport unit */
> struct _io_transport_unit
> {
> 	IO_INTERFACE_INST itfinst;	/* reference to owning io-interface instance (set by fddi) */
> 
> 	unsigned long  id;
> 	unsigned long  time_stamp;
> 	unsigned long  flags;
> 	unsigned long  size;
> 	unsigned char* buf_read;
> 	unsigned char* buf_write;
> };
> 
> #define TPUFLAG_IN		1	/* application reads from this transport unit */
> #define TPUFLAG_OUT		2	/* application writes to this transport unit */
> #define TPUFLAG_SYNC	4	/* this transport unit synchronizes a group of io-devices */
> #define TPUFLAG_EXTMEM	8	/* the memory of this transport unit is not maintained by the fddi */
> #define TPUFLAG_CONSIST	16	/* the memory of this transport unit is kept consistent by the fddi (cannot be used with TPUFLAG_EXTMEM) */

Same here, why no enum?

> typedef struct _io_transport_unit *IO_TRANSPORT_UNIT;
> 
> 
> /* io-variable */
> struct _io_variable
> {
> 	char* name;				/* optional name of the variable */
> 	unsigned long size;		/* size of the variable */
> 	unsigned long offset;	/* offset of the variable in io-transport unit */
> 
> 	IO_TRANSPORT_UNIT iotpu;/* reference to io-transport unit */
> 	IO_DEVICE iodev;		/* reference to io-device */
> };
> 
> typedef struct _io_variable *IO_VARIABLE;

[...]

> /* functions of the fddi */
> int register_io_interface(IO_INTERFACE ioitf);
> 	/* registers a new io-interface to the fddi layer (for example a CANopen stack)
> 	*  
> 	*  ioitf:	[in] the definition of the new interface
> 	*
> 	*  result:	RESOK, RESERR_INTERFACE_ALREADY_REGISTERD, RESERR_OUT_OF_MEM 
> 	*
> 	*  remarks:	this function is only called by the module which implements his own driver
> 	*			it need not to be called by users of an existiong driver
> 	*/

For API documentation, please use docbook style.

More comments when we've seen some test code.

Robert
-- 
 Dipl.-Ing. Robert Schwebel | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry
   Handelsregister:  Amtsgericht Hildesheim, HRA 2686
     Hannoversche Str. 2, 31134 Hildesheim, Germany
   Phone: +49-5121-206917-0 |  Fax: +49-5121-206917-9