Bug 442341

Created attachment 302343 [details]
lirc-more-args.patch

Hm, I'm afraid I don't quite understand what you mean by "having to edit the
file by hand on startup"; apart from automatically adding --device=... to the
command line if /dev/lirc0 is present, how is the end result of this patch
different from editing /etc/sysconfig/lirc and adding the desired options
directly to LIRCD_OPTIONS?  In all cases, you'd need to edit the sysconfig
snippet and add at least REMOTE_DRIVER there too, no?  Adding more variables of
our own comes with a documentation maintenance burden and possible upgrade
problems whereas by just using the simple LIRCD_OPTIONS we can (and already do)
just point to upstream docs, and people can add whatever in it.

The current commentary in /etc/sysconfig/lirc could be improved somewhat; it
just mentions -H (== --driver) and not -d/--device at all, because it's from the
times there were no lirc kernel modules in Fedora.

Regarding REMOTE_MODULES, I'd prefer pointing to the /etc/sysconfig/modules
functionality of /etc/rc.sysinit rather than reimplementing module loading in
lirc scripts.

Regarding the actual current implementation in the patch, build_remote_args
should probably be invoked when needed, ie. inside start() only.  And it
mentions TRANSMITTER_DEVICE which doesn't seem to be used anywhere.

It's possible that I missed something, providing the patch+docs to the sysconfig
snippet would help me understand better.

The point is making /etc/sysconfig/lirc computer-editable. Having separate
variables for the driver and device would mean that a computer program (such as
gnome-lirc-properties) would be able to read from the file, and write back to it
without hard parsing, or hand-editing.

For the REMOTE_MODULES part, I agree with you, and I'll try to make the
necessary changes to gnome-lirc-properties for that purpose.

> Regarding the actual current implementation in the patch, build_remote_args
> should probably be invoked when needed, ie. inside start() only.  And it
> mentions TRANSMITTER_DEVICE which doesn't seem to be used anywhere.

That's my mistake. I'll try to clean this up tomorrow.

Created attachment 305130 [details]
lirc-more-args-2.patch

Remove unused variables in the init script
Add documentation to the sysconfig file

Jarod, Ville, any comments on this patch?

Ah, I knew there was another patch I was forgetting... :)

Another fairly innocuous patch, the only real issue I have with it is the naming
of the vars. REMOTE_DEVICE really seems more like it should be IR_DEVICE,
IR_DEVICE_NODE or some such thing. REMOTE_DEVICE leads me to believe it should
be specifying the type of remote control I'm using, rather than the lirc device
node lircd should be talking to. Similarly, REMOTE_DRIVER might better be
IR_DRIVER, since this driver is actually independent of the actual remote, so
long as the remote talks the right IR protocol, its dependent on the IR receiver. 

I'd rather stick to something close to what Debian uses as far as variable names
are concerned. If you don't feel that way, feel free to change them to something
closer to what you believe is right, and I'll modify gnome-lirc-properties to
handle those new names.

I think I'd personally prefer LIRC_DRIVER and LIRC_DEVICE, those should be
pretty self explanatory.  Despite its name, LIRC is not limited to infrared
devices so IR_* doesn't feel quite right.

I think I'm sold on LIRC_DRIVER and LIRC_DEVICE_NODE. Just LIRC_DEVICE is a
touch ambiguous, since you don't know for sure if we're talking about the device
type of lirc receiver or the device node.

(In reply to comment #9)
> I think I'm sold on LIRC_DRIVER and LIRC_DEVICE_NODE. Just LIRC_DEVICE is a
> touch ambiguous, since you don't know for sure if we're talking about the device
> type of lirc receiver or the device node.

It might be ambiguous, but it's pretty well documented in the script itself. At
the same time, I would discourage people from editing the file by hand, and use
gnome-lirc-properties for new setups. Existing setups won't need any changes.

I'll get this committed shortly. Going with LIRC_DRIVER and LIRC_DEVICE, with
some minor changes here and there to the initscript and documentation in the
sysconfig file to match.