GNOME Bugzilla – Bug 524425
Improved support for ThinkPad (sound) volume buttons
Last modified: 2008-09-04 16:36:18 UTC
Add better support for Thinkpad speaker volume buttons. At the moment, binding the volume buttons to "Volume mute/down/up" causes the software volume (alsa, OSS or gstreamer) to change in addition to the hardware's volume. This makes volume changes very abrupt and uncorrelated to the displayed volume. Unbinding the keys works in so much that only the hardware's volume is changed, but then there is no display of any volume level when the buttons are pushed. The ideal behavior is to have the hardware volume read when the buttons are pushed and displayed on the OSD. Other information:
An originating discussion can be found at https://bugs.launchpad.net/ubuntu/+source/acpid/+bug/51537
Created attachment 108038 [details] [review] An implementation of the desired behavior This patch was originally intended for the Ubuntu Hardy package of gnome-settings-daemon, but I don't think it depends on any of the package's other patches. With the patch, adding the key /apps/gnome_settings_daemon/hwvol with the string value 'thinkpad' makes the OSD show the hardware volume and doesn't change the software volume. It gets the hardware's volume from /proc/acpi/ibm/volume
Thanks for your report and the patch. Something like this would absolutely be worthwhile including, but I do have a few issues with the implementation. * whatever you do you shouldn't need to modify gsd-media-keys-manager. The functionality should be properly encapsulated so that isn't necessary. * why use a GConf key? Figuring out if you're using the driver the makes this necessary should be possible without. * how portable is this? ie. does the IBM driver exist for other platforms than Linux, does it work in the same way (via /proc? Is /proc really the best/only way to get the info?). If not, we need infrastructure to make sure it's only built on supported platforms. * use glib functions for accessing files * use a consistent coding style + if(parse_acpi(&volume,&mute)) That looks pretty wrong since parse_acpi returns -1 on error.
(In reply to comment #3) Hi Jens, Thanks for the comments. Here's another attempt. The only thing not done was a scheme to not compile the non-portable parts (the ibm-acpi driver is Linux only. More info can be found at http://ibm-acpi.sourceforge.net/). I'm not very familiar with how to pull that off... particularly in Makefile.am.
Created attachment 108252 [details] [review] A better implementation of the desired behavior
Ok, some more detailed comments this time. First off, please add -p to your diff command. --- gnome-settings-daemon-2.22.0/plugins/media-keys/actions/acme-volume.c 2008-01-28 06:55:51.000000000 -0500 +++ gnome-settings-daemon-2.22.0.new/plugins/media-keys/actions/acme-volume.c 2008-03-29 20:12:36.000000000 -0400 @@ -32,6 +32,7 @@ #include "acme-volume-gstreamer.h" #endif #include "acme-volume-dummy.h" The dummy include isn't needed. Please remove while you're modifying the file. @@ -98,7 +99,16 @@ AcmeVolume *acme_volume_new (void) { - AcmeVolume *vol; + AcmeVolume *vol; The rest of the file uses tabs for indentation. I don't really care what you use for new files (well, actually, I do prefer spaces), but please don't mix tabs and spaces in a single file. This and (most of) the following coding style remarks of course also apply to the other files and other locations in the same file. + /* Check if we should be doing ThinkPad volume handling rather + than the software stuff*/ + + if(g_file_test(ACME_VOLUME_THINKPAD_ACPI_PATH, G_FILE_TEST_EXISTS)) + { Coding style nitpicking: Spaces at the end of the comment and between "if" and "(" and function name and "(". And between function parameters (correct in this case) but not between "*" and argument/variable name. Also, *please* don't use GNU-style indentation for "{...}". It's horrible and a maintainer's nightmare. I believe the file test condition should be G_FILE_TEST_IS_REGULAR. + Adds Thinkpad volume reading. Based on acme-volume-dummy.c Remove the dummy mention. That's what it's for. + Based on acme-volume-dummy.c by Bastien Nocera <hadess@hadess.net> Same here and add yourself as author so we know who's to blame. ;-) +static int acme_volume_thinkpad_get_volume (AcmeVolume *self); +static void acme_volume_thinkpad_set_volume (AcmeVolume *self, int val); + +static gboolean acme_volume_thinkpad_parse_acpi +(int * level, gboolean * mute_status); If you rearrange the functions a bit you don't need the prototypes here. +acme_volume_thinkpad_finalize (GObject *object) You don't need to implement finalize if you don't actually do anything in there. + //Convert to a value between 0 and 100 + return CLAMP (volume*7, 0, 100); Don't use C++-style comments. Some compilers don't like that. Also add a note on why * 7 (ie. driver uses volume level from 0 to 15). +GType acme_volume_thinkpad_get_type (void) +{ ... +} Use G_DEFINE_TYPE instead (look at how the other volume classes do it; I suppose we should update dummy as well). +/* A simple function which parses the ACPI data. This requires a acpi + kernel module supporting IBM thinkpads.*/ Ideally, a casual observer would be able to tell whether it's simple or not. Just descrive functionality, please ;-) +static gboolean +acme_volume_thinkpad_parse_acpi(int * level, gboolean * mute_status) ... +} Um, ok, looking at this, it's rather ridiculous. Feel free to switch back to fscanf... +#define ACME_VOLUME_THINKPAD_ACPI_PATH "/proc/acpi/ibm/volume" > The only thing not done was > a scheme to not compile the non-portable parts. I'm not > very familiar with how to pull that off... particularly in Makefile.am. If you guard your code just like the other modules do (I suggest using ENABLE_IBM_THINKPAD) I'll take care of the configure magic.
Created attachment 108388 [details] [review] Another revision of changes Here's another go at it. I tried to catch everything you mentioned.
Just a few small things left. +acme_volume_thinkpad_get_volume (AcmeVolume *vol) +{ ... + if (acme_volume_thinkpad_parse_acpi (&volume, &mute)) + { + /* Convert to a volume between 0 and 100. The ibm-acpi + driver output a value between 0 and 15 */ s/output/outputs/ ... + } + else + return -1; That looks wrong. A volume of -1 is pretty much undefined. Better return 0 here. +acme_volume_thinkpad_class_init (AcmeVolumeThinkpadClass *klass) +{ + AcmeVolumeClass *volume_class = ACME_VOLUME_CLASS (klass); + G_OBJECT_CLASS (klass)->finalize = NULL; Nope. You still want the parent's finalize, so don't touch it. +#define ACME_TYPE_VOLUME_THINKPAD (acme_volume_get_type ()) Should be acme_volume_thinkpad_get_type. +AcmeVolume* acme_volume_thinkpad_new (void); You don't seem to actually provide this function... + rather than the software stuff. Checking for the existance s/existance/existence/ AcmeVolume *acme_volume_new (void) { AcmeVolume *vol; - + You're adding empty lines with just a tab (on a few other lines, too). Don't do that, please.
Created attachment 108448 [details] [review] New revision
Created attachment 108449 [details] [review] Changes to dummy Some of the comments you made applied to dummy.{c,h} as well. If it's useful, I've applied them to dummy.
Thanks. I have committed the patch and it will be available in 2.23.1.
Can I ask why we are reintroducing crappy per-laptop-model specialcasing again here ? This seems to be quite contrary to the recent trend of making sure that the kernel reports sane input events.
The kernel does report input events in this case, but both the driver and g-s-d fiddle with the volume the result of which is suboptimal. I'd obviously prefer to have this case handled by the existing modules, but short of disabling hardware button handling in the driver, I don't see how that would work.
Oh well, I just reverted the patch. I wouldn't have problems with a device-specific driver per se, but it needs to be provably better than the generic ones we have. Basically, that means it needs to work out of the box. Unfortunately, this one has a pretty fundamental issue that makes it worse than the generic ones (and I really should have realised this earlier - sorry, Lorne): it breaks dynamic device selection and assumes you'll always want to use the built-in device when using the thinkpad driver. That's most likely not correct. To fix this, one could add some additional GConf magic like in Lorne's initial patch. However, the problem can already be solved with the generic drivers using some additional GConf magic: Set /apps/gnome_settings_daemon/volume_step to a suitably low value (ideally 0 but that is currently handled as an error and uses the default of 6; I'm going to fix that).
A clarification and some comments: (In reply to comment #13) > The kernel does report input events in this case, but both the driver and g-s-d > fiddle with the volume the result of which is suboptimal. I'd obviously prefer > to have this case handled by the existing modules, but short of disabling > hardware button handling in the driver, I don't see how that would work. The ibm-acpi (or any other kernel driver) does not fiddle with the volume. For example, even if the butons are pushed pre-GRUB, the volume is changed/muted. As far as I can tell, the buttons are hardwired to change the volume with no software interaction needed. All the ibm-acpi does is provide a means to read the level/mute state (though, with root access you can change it via proc). This level/mute status is very much distinct from the ALSA/OSS/sound card mixer volume. Changing one is completely disjoint from the other. Also, I don't think we should expect the ALSA driver to support interacting with this speaker volume. The buttons are like the volume knob on your desktop speakers, with the exception that the level of the knob is only available via ACPI. IMHO the g-s-d on-screen-display is the best option to provide the user with the hardware volume and is similar to what is done with the Windows Thinkpad drivers. Without access to speaker volume, things get tedious and annoying since the volume level shown in any GUI isn't what comes out of the speakers (for example, it may be mute without any explanation). (In reply to comment #14) > However, the problem can already be solved with the generic drivers > using some additional GConf magic: Set /apps/gnome_settings_daemon/volume_step > to a suitably low value (ideally 0 but that is currently handled as an error > and uses the default of 6; I'm going to fix that). This doesn't seem much different to unbinding the volume keys in gnome-keybinding-properties, which is a suggestion I make in the bug description. It solves things in that the buttons will only change the hardware volume and not the ALSA/OSS volume. But the hardware/speaker volume will still be absent from any GUI. If volume_step=0, pushing the thinkpad volume buttons would cause the OSD to pop up with an unchanging ALSA/OSS/etc level. But the sound level out of the speakers would be changing. (In reply to comment #14) > To fix this, one could add some additional GConf magic like in Lorne's initial > patch. Another reason this might be needed is that some ThinkPads don't have this speaker volume (though still have the buttons). (The R series I think). I'm not sure if the ibm-acpi kernel driver creates /proc/acpi/ibm/volume in the cases when its irrelevant. If it does, it would break things in the patch.
> But the hardware/speaker volume > will still be absent from any GUI. If volume_step=0, pushing the thinkpad > volume buttons would cause the OSD to pop up with an unchanging ALSA/OSS/etc > level. But the sound level out of the speakers would be changing. No. Assuming the hardware driver that ALSA/OSS/GStreamer rely on works properly. It should actually show the correct volume since it's always read from the device when updating (because someone else might have changed the volume by other means in the meantime). Try setting volume_step to 1 with any current release version. That should basically have the same effect as 0 for your purpose. If that indeed doesn't work as expected we can either make it work, or the driver needs fixing.
(In reply to comment #16) > Try setting volume_step to 1 with any current release version. That should > basically have the same effect as 0 for your purpose. If that indeed doesn't > work as expected we can either make it work, or the driver needs fixing. It doesn't work as expected. I'll attach a screenshot. > Assuming the hardware driver that ALSA/OSS/GStreamer rely on works > properly. It should actually show the correct volume since it's always read > from the device when updating (because someone else might have changed the > volume by other means in the meantime). The ALSA/OSS/GStreamer volume handlers correctly read the "Master Volume" shown in Volume Control. But the volume I'd like displayed is the 14/15 shown in the terminal. I'm not sure we can expect the ALSA/OSS/GStreamer driver to read this since it's not part of the soundcard/chipset (I'm guessing a bit here).
Created attachment 108818 [details] Example of annoying behavior
So the 14/15 is PCM, not Master? In that case you just need to select the PCM track in System -> Preferences -> Sound -> Default Mixer Tracks.
(In reply to comment #19) > So the 14/15 is PCM, not Master? In that case you just need to select the PCM > track in System -> Preferences -> Sound -> Default Mixer Tracks. > It is neither. It is completely separate from any mixer bar available (even if I display them all via Edit->Preferences in the Volume Control window. In my past comments, I've tried to distiguish the two types of volume by "software volume" and "hardware volume". Perhaps this was the wrong terminology. The closest I can figure is that my soundcard/chipset knows as much about the volume shown in the terminal as a desktop soundcard knows the physical volume knob level on desktop speakers.
That qualifies for "the driver needs fixing", then. A bit of googling turned up this mail from the thinkpad-acpi maintainer: http://kerneltrap.org/mailarchive/linux-kernel/2007/10/15/343793
I'm here to add: - thinkpads have a hardware volume control, it's no pcm, no master, nothing, it's a volume control like the one you would find in your external speakers - i honestly don't think alsa/kernel will ever support an interface for this - all the other laptops work now, as far as i can tell, because they use the pcm or master volume and not an actual hardware volume The last point is what makes me think that this is not exactly a bad thing to have in g-s-d, sadly (for other laptops) only thinkpads do hardware volume as far as I know, so special casing thinkpads shouldn't be evil at all. When the kernel makes an interface for this we can remove the special case, but for now I can't imagine g-s-d being cluttered from night to day with 100 different laptop 'drivers', I see no strong reason to not include this. Yes, I have a thinkpad, and it rocks. :-)
Reopening.
Created attachment 117980 [details] [review] Updated to trunk This patch is updated to trunk, I added get_threshold method. Everything else is the same.
(In reply to comment #22) > I'm here to add: > - thinkpads have a hardware volume control, it's no pcm, no master, nothing, > it's a volume control like the one you would find in your external speakers So? You can apparently expose the mixer interface via /proc. Then you can also expose it through ALSA. > - i honestly don't think alsa/kernel will ever support an interface for this The *thinkpad kernel maintainer* said so. Sue him. > I see no strong reason to not include this. It breaks sound device selection. That alone is pretty much strong enough.