Bugzilla – Bug 8237
AutoDisplay plugin broken by new SqueezeCenter v7.0.1
Last modified: 2009-09-08 09:21:58 UTC
The AutoDisplay plugin has been broken by the new SqueezeCenter v7.0.1. The revised version worked fine with v7.0. As reported in the forum: http://forums.slimdevices.com/showthread.php?t=47857 It seems to have been working as late as 7.01-19352 but the release version "fights" with the plugin and turns the display back up to its off level. Basically there now seems to be some sort of brightness check hidden away in the new core that checks everytime the brightness is changed to make sure it is at the "correct" off level. The autodimming functionality should probably just be rolled into the core anyhow as noted way back in: https://bugs-archive.lyrion.org/show_bug.cgi?id=1850
the plugin should be storing its own pluginParam and setting the pref accordingly. SC is doing what it should be doing, and actively updating based on pref (should anyone be changing the pref on the fly, etc)
sorry, make that $client->pluginData. This is designed for holding plugin state information see also bug 7922 (unfortunately, this bug fix was missed in the changelog) Given those details, this is probably best merged with bug 1850 as this is either a fix for the plugin author to make, or the existing enhancement request.
It appears that what is happening is roughly thus: The AutoDisplay plugin comes in, and correctly executes a Client->brightness() call. This causes the brightness to be set for a short period of time (a couple seconds max), then the brightness seems to reset to the value stored in the main settings file for that. This is only being executed when the player is "off", so it's some sort of an inactive screensaver issues. The plugin does not try to change the value set in the client's main preferences value, only the dynamic call to Client->brightness().
Eric, the change is due to the fix in bug 7922, which applies whenever brightness is set to automatically adjust. The server was previously not taking into account pref changes, which is why it updates based on the pref. The plugin can set a pluginData value to store the old pref, then change the pref and reset later. This would have the added benefit of no longer requiring the player to be off (which I would guess was required because in other modes, the server would correctly update the brightness to the pref) Current workaround would be to set to manual brightness only, but this is set to be deprecated around the 7.2 timeline.
Unfortunately, this solution, at least as I understand it, makes preference setting very confusing. If the plugin is manipulating the value for brightness, that will show up in the interface, and it would be dynamic. You'd set a value into the preferences, and a while later it would change magically when the plugin went around mangling things. Making sure that all the values made sense would be a bit of a programming nightmare (potential bug minefield with all the corner cases). This also pretty much makes the Client->brightness(value) call worthless, as it no longer does anything. It sets a value, but that value's going to get changed in a second. Definitely not what the API implies. Rather than attempting to set the brightness every second (I hate polling), I'd think it would make more sense to change the brightness when it needs to be changed, such as power on/off, going idle, and when the value is changed in the preferences. Requires either tracking the previous values to detect changes, or having a listener/callback interface to watch for preference changes, but in the long run, I think it's cleaner code.
I'm not sure that the fix for https://bugs-archive.lyrion.org/show_bug.cgi?id=7922 was really the correct thing to do; sort of echoing Eric's comment of the now apparent uselessness of Client->brightness(value) call and the expense of constant polling. My admittedly brief skim of 7922 suggests that it was more of a firmware bug that has now been fixed with what seems to be a bit of a kludge in SqueezeCenter. Also without actually going though any of the scenarios some of the suggested workarounds might potentially lead to the display blanking during the "dim" time even if the player is on (*not* what you want happening).
Andy: Can you please look this over and comment accordingly?
KDF, Andy asks your opinion about the right thing to do.
I'd just like to empahsize what I said in my initial comment: The autodimming functionality should probably just be rolled into the core anyhow as noted way back in: https://bugs-archive.lyrion.org/show_bug.cgi?id=1850 I don't know how anyone who has a squeezebox in their bedroom can live without this functionality.
I've already given my personal opinion. It's your software so it's up to you to either revert and re-open the other bug in order to fix this. Then you'll need to mark the other as wont fix and redefine the auto-brightness as something that WONT change when the pref is changed, only when the modes change. If you leave the autodim working as it should, then the way to manipulate brightness is through the pref, or you end up doing yet another special case to handle a plugin that may not be in use by every user who downloads and installs SC I've said my peace so I will leave the debate to everyone else
After talking to Felix and the other developers, it seems like bug 1850 is the "right" way to fix this issue if we want to do that.
*** Bug 1850 has been marked as a duplicate of this bug. ***
So is this the correct summary of the current situation?: -The "fix" for bug 7922 has rendered the Client->brightness() call basically useless unless you are in 'Manual' brightness mode. This "fix" has broken the AutoDisplay, showBriefly, and possibly other plugins. It also makes the control of brightness by future plugins difficult. -Bug 1850, suggesting rolling AutoDisplay (time based dimming) into the core, has been closed. This was contrary to the consensus above that it was the "right" way to prevent contention over the control of brightness. Of course this would not help showBriefly or any other present of future plugins that might want to control brightness. -The kludge of 'Manual' brightness mode is going away very shortly and those of us with squeezeboxes in our bedrooms will have to unplug them, or throw dirty underwear over top to sleep... :( Isn't there someway to rethink what 7922 does? For example only adjust the brightness when the preference changes rather than constantly poll or check for changes. This is arguably wasteful of cycles as well. As an aside I have found 7.x more sluggish than older slimserver versions. Now I don't really blame 7922, but "fixes" like this could add up. I'll cross post some of the above to 7922.
Eric, Andy suggests that the plugin be edited to change all three of the SC preference in addition to the current brightness.
So basically it's going to have to store the active off preference as another plugin preference and set the SC off preference as appropriate during it's active and inactive periods? I guess that would work, with the user realizing that the "real" SC preference will always be ignored and over ridden by the plugin. Could be a bit confusing for those who don't think about it.