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

[Robert Schwebel]

On Thu, Sep 27, 2007 at 10:50:30AM +0200, Thomas Rothfuss wrote:
> I had a closer look at the fddi implementation in the svn repository.
> There are some questions/annotations about it.

Great!

> 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?

Yes, we'll use doxygen for API documentation. But I think at this stage
it doesn't make any sense to start writing documentation, because the
whole APIs are under heavy progress. Once they have stabilized, you're
welcome to help in this area.

> 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?

Well, tests/ is the autotool method of doing tests. Automake generates
makefiles which let you do

	make check

and all the tests are running; the usual release procedure is to perform

	make distcheck

which does the following:

- build the distribution tarball(s)
- extract it again into a new directory
- make this source directory read only
- create a separate build directory
- perform an out-of-tree build
- run the testsuite in there

This way you can easily find out if you've forgotten to add something to
the makefiles which should be part of the distribution.

Are there any features of CppUnit you are missing here?

> 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.

That's the idea.

>     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;
>
>         };

Sounds reasonable. I'll make some prototypes to check if it feels clean
enough.

> 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.

It would probably be more OO to use accessor functions, but taken we
migrate to the scheme you outlined above, it would be clean as well (the
current implementation would only ensure correct accesses by policy, not
by design, which is bad).

>     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);

Thanks, taken. I'll add it to the repository.

> 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.

Yes, seconded. We currently implement a library for this xml format
(OpenIO); it should be almost ready, so we can play with it soon and
check if it fullfills our needs or what is missing.

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

I don't think that xml itself imposes such a thing. We tried to follow
the "c is a minimum language" pattern the kernel is using, and designed
the xml after this philosophy. So no CamelCase and underscores as
separators.

But I'm aware that this is a topic which can be discussed for the next 5
years, and everyone will have arguments :-)

>     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.

Well, my experience is that it depends on what you wanna do. Example:

	<parameter key="bla" value="42"/>

Here you're right. value is not that different from key.

	<description>This switch is for doing crazy things.</description>

Here it is better to use cdata.

However, thanks for your input. I'll do the necessary modifications and
give you more feedback then.

Robert
-- 
Pengutronix - Linux Solutions for Science and Industry
Entwicklungszentrum Nord     http://www.pengutronix.de