[ag-automation] Questions/Annotations about implementation of fddi

[Thomas Rothfuss]

Hello,


I had a closer look at the fddi implementation in the svn repository.
There are some questions/annotations about it.

1. Code documentation. There are tools like doxygen which generates
    documentation from the source code. Therefore the functions and
    structures have to be commented.
    Are there intentions to use such a tool?

2. Tests. There are some tests in the "tests" folder. They exit with
    0 or in case of an error with another value.
    There are test frameworks like CppUnit for automatic testing.
    Does it make sense to use such a framework?

3. Interface vs. implementation. In fddi_device.h there is following
    struct:

        struct fddi_device {

        	/* public */
        	fddi_id_t id;
        	fddi_state_enum_t state;

        	/* private */
        	struct fddi_device *next;
        	struct fddi_device *previous;

        	fddi_param_t *param_list;

        	void *priv;

        };

    fddi_types.h:

        typedef struct fddi_device fddi_device_t;


    I assume that "public" means the following members are used for the
    interface. "private" is part of the implementation.

    Since this should be a C interface not a C++ one fddi_device is not
    an abstract class to derive from. But it is possible to separate
    the interface from the implementation by using the pimpl-Pattern
    (see http://www.gotw.ca/gotw/024.htm). Using "void *priv;" is the
    a similar mechanism.

    In this case the implementation fddi_device_impl needs a pointer
    to its interface class for iteration reason:

        struct fddi_device_impl;  /* forward declaration or use void * */

        struct fddi_device {

        	fddi_id_t id;
        	fddi_state_enum_t state;

        	struct fddi_device_impl * impl;
        	/* or maybe
         void * impl;
         */

        };

        struct fddi_device_impl {

         fddi_device_t* i_fddi_device;

        	struct fddi_device *next;
        	struct fddi_device *previous;

        	fddi_param_t *param_list;

        	void *priv;

        };



4. Why does the function "fddi_dev_getstate" exist?
    ("extern int fddi_dev_getstate(fddi_device_t *dev, fddi_state_enum_t 
*state);")

    The member variable in the fddi_device struct is public. So it is
    possible to access it directly.

    Otherwise use the pimpl-Pattern in point 3. So the fddi_device
    struct is reduced to

        struct fddi_device {
        	struct fddi_device_impl * impl;
        };


5. const parameters. To retrieve the state from the device the function

    extern int fddi_dev_getstate(fddi_device_t *dev, fddi_state_enum_t 
*state);

    should have the device as a const parameter:

    extern int fddi_dev_getstate(const fddi_device_t *dev, 
fddi_state_enum_t *state);


6. XML namespaces. For OSADL XML files and schemas we should use
    namespaces, e.g.

        xmlns:fddi="http://www.osadl.org/fddi"

    I am not sure if the xml-files are just examples. But if it should
    become an OSADL standard the namespaces are necessary.

7. Styleguide. Is there a styleguide for XML?
    Upper-/lowercase, underline yes/no,...

    We at Homag only use attributes. Instead of

        <FddiParam Id="1" Name="ParamAddress" Type="int">14</FddiParam>

    we write

        <FddiParam Id="1" Name="ParamAddress" Type="int" Value="14"/>

    First it is more clear what each value means. Then the parsing is
    a bit easier.


Regards,
Thomas