questions vanity lights

[Michael E Brown]

On Tue, Mar 27, 2007 at 12:55:10AM -0500, Michael E Brown wrote:
> On Mon, Mar 26, 2007 at 09:31:54AM +0200, Oliver Eichler wrote:
> > 
> > Hi Michael
> > 
> > > Too cool. I'll have a look at it when I get back into the office.
> > 
> > I enhanced the code a little bit over the weekend.
> 
> First, I dont have any experience with dbus, so I cannot give any
> dbus-related feedback. Might be useful to get somebody with qt/dbus
> experience for more feedback.
> 
> Next, I had some slight difficulty compiling on my system. I have an FC6
> system. I had to install qt4-devel to compile and run qmake-qt4. Not
> sure if this is intended or not (vs qt3, etc) 
> 
> This looks interesting, my problem is currently what exactly to do with
> it. It has dependencies outside of what I currently require (qt), so I
> cannot put it directly into libsmbios tarball. It would have to go into
> its own project. That means its own RPM and such. The problem is that I
> have no easy way to test it. I would be willing to write a spec file and
> make the RPM, host it for download, etc. I would probably even maintain
> it for fedora and possibly opensuse (once I figure out the opensuse
> contributor process). But, I would need contributors (you, possibly
> others) to maintain it.
> 
> Let me know what you would like to do.
> 
> Here are a few things I saw:
> 
> -- code quality looks good. Code is readable and formatted nicely.
> Builds and runs after some initial problems getting everything
> installed. Gives a nice "LED not supported" message on my system (which
> is accurate).
> 
> -- Versioning? Probably need to have the server export an API version
> and check in the client to avoid incompatibilities.
> 
> -- your init script should probably modprobe dcdbas 
>   -> if dcdbas is not loaded, daemon terminates with:
>   terminate called after throwing an instance of
>   'smbios::InternalErrorImpl'
>     what():  Could not open file /sys/devices/platform/dcdbas/smi_data.
>     Check that dcdbas driver is properly loaded.
>   Aborted
> 
> -- probably want to make a x-platform init script that will work on
> fedora/rh/suse. lsb maybe?  
> 
> -- init script needs fedora-style chkconfig lines as well as the suse
> lines.
> 
> -- The service should probably check if lights are supported and exit
> gracefully so it doesnt take up resources on systems without lights.
> Any way to just have dbus return a nice error in this case without
> keeping the daemon running?
> 
> -- Seems like you would want the service to daemonize itself, with an
> optional arg to tell it not to.
> 
> src/dellxpsd/CXPSAdaptor.cpp:
> -- probably want to use enums in the switch() instead of hardcoded
> values in setLEDColor()

Also:
-- name should probably be dellxpsledd, QXpsled, or have some indication
that it is for the LED control.

-- fully-qualified paths from your build system in some files. Need to
convert to relative. 
    -- doxyfile
    -- kdevelop project files

-- remove backup files from the tarball: *~

-- Warnings: CXPSAdaptor.cpp:38: warning: ‘dell_led_color’
defined but not used

--
Michael