Re: [libdc] Big patch committed

[Damien D. <ddo...@is...>]

Hi David,

Before I start, thanks for the hard work! ;)

On Mon, 2007-11-26 at 02:13 -0500, David Moore wrote:
> I have committed my "big patch" that migrates the linux, juju, and Mac
> OS X platforms to the new API for camera enumeration.  The
> dc1394_find_cameras() function is now gone.  All the examples have been
> updated to properly use the new enumeration method.
> 
> The net effect of this patch is actually that the library is now 300
> lines of code shorter.  I guess that's a good omen that this new
> approach should make everything leaner and meaner.
> 
> The API that defines platform-specific interaction is now defined in
> dc1394/platform.h.  Of course, this is private to the library and is not
> exposed to clients.
> 
> Platforms now simply provide a list of all firewire devices to the
> library with a config ROM, and the library will sort out which are
> cameras and which are not.  This simplifies most of the
> platform-specific code and should make it easier to maintain.
> 
> The common enumeration routines have been split out from
> dc1394/control.c and are now in dc1394/enumeration.c.
> 
> There are still some cleanups I intend to do, but they are smaller in
> scope, so please feel free to commit new patches to SVN now.
> 
> Some of the things I still need to do:
>  - Remove volatile quantities from the dc1394camera_t struct.
>  - Make sure all public function names start with dc1394_
>  - Clean up some naming inconsistencies
> 
> Some of the things I need to work with Damien and others to do:
>  - Change the error handling mechanism of the library.
> 
> If you are using the SVN version of libdc1394, please give these new
> changes a whirl and let me know if I've horribly broken anything.

I'm going through coriander to update what needs to be updated, and this
brings a number of (small) problems.

1) the member "unit_directory" of dc1394_camera_t is gone. Any reasons
for that? It's not used a lot (just a register read/write function). But
since the unit directory contains important information it may be useful
to keep this in the camera struct for direct-access-debugging.

2) register.c/h: some functions need to be renamed as dc1394_xx (I'll do
that soon)

3) note to self: the flags member of dc1394_camera_t should be
initialised to zero when a new camera instance is created. (I'll do that
too)

4) still regarding dc1394_camera_t, the member "memory_channel_num" is
gone too. Is there another way to get that information? IOW, do we need
to write a function that gets this info? If we don't have the channel
number we can't use memory channels at all. BTW this channel number is
not volatile; it remains the same.

5) yet some other members of dc1394_camera_t removed: port and node.
This was planned for hotplug etc... But right now the result is that the
broadcast function can't be used. Shall we write some access functions
to port and node?

Two other random thoughts:

I'm also wondering if we should keep the dc1394_camera_id_t since it's
not used much anymore. The function to compare cameras based on that
struct could be removed too. OR: We put the struct back in
dc1394_camera_t, use dc1394_camera_id_t as input for
dc1394_camera_new_unit and still allow people to create a camera with
only the GUID (dc1394_camera_new).

Another thing to do will be to move the new API higher in control.h and
stop referring it as new since it's the only one now ;) 

Cheers,

-- 
Damien 高原 Douxchamps
Assistant Professor, Image Processing Laboratory, NAIST
http://damien.douxchamps.net/