Fwd: Re: imonlcd: spinning disc and progess bars

--- ./server/drivers/imonlcd.orig.c	2011-03-29 00:00:21.000000000 -0700
+++ ./server/drivers/imonlcd.c	2011-04-22 21:11:27.000000000 -0700
@@ -10,17 +10,6 @@
  *  Added Protocol configuration parameter
  *  Converted and upgraded t6963 font for use with iMon Soundgraph LCD
  *
- * Modified March 2009 by Eric Pooch - lots of cleanup and streamlining
- *  Removed fixed "96" width listed everywhere
- *  Streamlined config file parsing
- *  Sped up font access
- *  Implemented backing store to avoid unnecessary refreshes
- *  Adjusted p->width and p->height to use character width and height properly
- *  Fixed long long defines
- *  Replaced send_data() and send_byte_data() with send_packet()
- *  Improved send_command_data()
- *  Reduced functions for basic character drawing - removed draw_char() and draw_string()
- *
  * Copyright (c) 2004, Venky Raju <dev (at) venky (dot) ws>
  *               2007, Dean Harding <dean (at) codeka dotcom>
  *               2007, Christian Leuschen <christian (dot) leuschen (at) gmx (dot) de>
@@ -127,6 +116,7 @@
 
 	/* save the last output state so we don't needlessly reset the icons */
 	int last_output_state;
+	int last_output_bar_state;
 } PrivateData;
 
 /*
@@ -291,6 +281,7 @@
 
 	p->last_cd_state = 0;
 	p->last_output_state = 0x0;	/* no icons turned on at startup */
+	p->last_output_bar_state = 0x0;	/* no bars turned on at startup */
 	p->discMode = 0;
 
 	/* Get settings from config file */
@@ -862,18 +853,42 @@
 
 	PrivateData *p = drvthis->private_data;
 	uint64_t icon = 0x0;
+	
+	/* bit 28 : Abuse this for progress bars. See above for usage. */	
+	if (state & IMON_OUTPUT_PBARS_MASK) {
+		if (state != p->last_output_bar_state) {
+			p->last_output_bar_state = state;
+
+			/* extract the bar-values for each bar separately */
+			int topProgress = (state & 63);
+			int topLine = (state & (63 << 6)) >> 6;
+			int botProgress = (state & (63 << 12)) >> 12;
+			int botLine = (state & (63 << 18)) >> 18;
+
+			botProgress = botProgress > 32 ? -(botProgress - 32) : botProgress;
+			topProgress = topProgress > 32 ? -(topProgress - 32) : topProgress;
+			botLine = botLine > 32 ? -(botLine - 32) : botLine;
+			topLine = topLine > 32 ? -(topLine - 32) : topLine;
 
-	if (state == p->last_output_state)
-		return;
+			setLineLength(topLine, botLine, topProgress, botProgress, p);
 
+		}
+		/* Update the icons also. */
+		state = p->last_output_state;
+	}
+	
+	/* Don't update if no icons need to be changed. */
+	if (state == p->last_output_state && !(state & IMON_OUTPUT_CD_MASK)) {
+		return;
+	}
 	p->last_output_state = state;
 
 	if (state == -1) {	/* the value for "on" in the lcdproc-protocol */
 		icon = (uint64_t) IMON_ICON_ALL;
 		send_command_data(COMMANDS_SET_ICONS | icon, p);
 		setLineLength(32, 32, 32, 32, p);
-
 		return;
+		
 	} else if (state == 0x0) {	/* the value for "off" in the
 					 * lcdproc-protocol */
 		icon = (uint64_t) 0x0;;
@@ -881,22 +896,7 @@
 		setLineLength(0, 0, 0, 0, p);
 		return;
 	}
-	/* bit 28 : Abuse this for progress bars. See above for usage. */
-	else if ((state & IMON_OUTPUT_PBARS_MASK) != 0 && state > 0) {
-		/* extract the bar-values for each bar separately */
-		int topProgress = (state & 63);
-		int topLine = (state & (63 << 6)) >> 6;
-		int botProgress = (state & (63 << 12)) >> 12;
-		int botLine = (state & (63 << 18)) >> 18;
-
-		botProgress = botProgress > 32 ? -(botProgress - 32) : botProgress;
-		topProgress = topProgress > 32 ? -(topProgress - 32) : topProgress;
-		botLine = botLine > 32 ? -(botLine - 32) : botLine;
-		topLine = topLine > 32 ? -(topLine - 32) : topLine;
-
-		setLineLength(topLine, botLine, topProgress, botProgress, p);
-		return;
-	}
+	
 	/* bit 0 : disc icon (0=off, 1='spin') */
 	if (state & IMON_OUTPUT_CD_MASK) {
 		/* Each icon bit represents a section of the cd,
@@ -917,19 +917,12 @@
 			tmp_cd_bitmap = ~tmp_cd_bitmap;
 
 		icon |= ((uint64_t)tmp_cd_bitmap) << 40;
-
-		/* Change the cached output state so that we will continue to
-		 * update / spin the cd. The set value makes a missed refresh
-		 * or clear very unusual.  The server core makes sure we get
-		 * the correct value next time around.
-		 */
-		p->last_output_state = (~IMON_OUTPUT_PBARS_MASK & ~IMON_OUTPUT_CD_MASK);
 	}
 	/*
-	 * bit 1,2,3 : top row (0=none, 1=music, 2=movie, 3=photo, 4=CD/DVD,
-	 * 5=TV, 6=Web, 7=News/Weather)
+	 * bit 1,2,3 : top row
+	 * (0=off, 1=MUSIC, 2=MOVIE, 3=PHOTO, 4=CD/DVD, 5=TV, 6=WEB, 7=NEWS/WEATHER)
 	 */
-	if (((state & IMON_OUTPUT_TOPROW_MASK) != 0)) {
+	if (state & IMON_OUTPUT_TOPROW_MASK) {
 		switch (((state & IMON_OUTPUT_TOPROW_MASK) >> 1)) {
 		case 1:
 			icon |= IMON_ICON_MUSIC;
@@ -970,24 +963,24 @@
 		}
 	}
 	/* bit 6 : S/PDIF icon */
-	icon = ((state & IMON_OUTPUT_SPDIF_MASK) != 0) ? (icon | IMON_SPKR_SPDIF) : (icon & ~IMON_SPKR_SPDIF);
+	if (state & IMON_OUTPUT_SPDIF_MASK)	icon |= IMON_SPKR_SPDIF;
 	/* bit 7 : 'SRC' */
-	icon = ((state & IMON_OUTPUT_SRC_MASK) != 0) ? (icon | IMON_ICON_SRC) : (icon & ~IMON_ICON_SRC);
+	if (state & IMON_OUTPUT_SRC_MASK)	icon |= IMON_ICON_SRC;
 	/* bit 8 : 'FIT' */
-	icon = ((state & IMON_OUTPUT_FIT_MASK) != 0) ? (icon | IMON_ICON_FIT) : (icon & ~IMON_ICON_FIT);
+	if (state & IMON_OUTPUT_FIT_MASK)	icon |= IMON_ICON_FIT;
 	/* bit 9 : 'TV' */
-	icon = ((state & IMON_OUTPUT_TV_MASK) != 0) ? (icon | IMON_ICON_TV_2) : (icon & ~IMON_ICON_TV_2);
+	if (state & IMON_OUTPUT_TV_MASK)	icon |= IMON_ICON_TV_2;
 	/* bit 10 : 'HDTV' */
-	icon = ((state & IMON_OUTPUT_HDTV_MASK) != 0) ? (icon | IMON_ICON_HDTV) : (icon & ~IMON_ICON_HDTV);
+	if (state & IMON_OUTPUT_HDTV_MASK)	icon |= IMON_ICON_HDTV;
 	/* bit 11 : 'SRC1' */
-	icon = ((state & IMON_OUTPUT_SCR1_MASK) != 0) ? (icon | IMON_ICON_SCR1) : (icon & ~IMON_ICON_SCR1);
+	if (state & IMON_OUTPUT_SCR1_MASK)	icon |= IMON_ICON_SCR1;
 	/* bit 12 : 'SRC2' */
-	icon = ((state & IMON_OUTPUT_SCR2_MASK) != 0) ? (icon | IMON_ICON_SCR2) : (icon & ~IMON_ICON_SCR2);
-	/*
-	 * bit 13,14,15: bottom-right icons (0=off, 1=MP3, 2=OGG, 3=WMA,
-	 * 4=WAV)
+	if (state & IMON_OUTPUT_SCR2_MASK)	icon |= IMON_ICON_SCR2;
+	/* 
+	 * bit 13,14,15: bottom-right icons
+	 * (0=off, 1=MP3, 2=OGG, 3=WMA, 4=WAV)
 	 */
-	if (((state & IMON_OUTPUT_BRICONS_MASK) != 0)) {
+	if (state & IMON_OUTPUT_BRICONS_MASK) {
 		switch (((state & IMON_OUTPUT_BRICONS_MASK) >> 13)) {
 		case 1:
 			icon |= IMON_ICON_MP3;
@@ -1006,10 +999,10 @@
 		}
 	}
 	/*
-	 * bit 16,17,18: bottom-middle icons (0=off, 1=MPG, 2=AC3, 3=DTS,
-	 * 4=WMA)
+	 * bit 16,17,18: bottom-middle icons
+	 * (0=off, 1=MPG, 2=AC3, 3=DTS, 4=WMA)
 	 */
-	if (((state & IMON_OUTPUT_BMICONS_MASK) != 0)) {
+	if (state & IMON_OUTPUT_BMICONS_MASK) {
 		switch (((state & IMON_OUTPUT_BMICONS_MASK) >> 16)) {
 		case 1:
 			icon |= IMON_ICON_MPG2;
@@ -1028,10 +1021,10 @@
 		}
 	}
 	/*
-	 * bit 19,20,21: bottom-left icons (0=off, 1=MPG, 2=DIVX, 3=XVID,
-	 * 4=WMV)
+	 * bit 19,20,21: bottom-left icons
+	 * (0=off, 1=MPG, 2=DIVX, 3=XVID, 4=WMV)
 	 */
-	if (((state & IMON_OUTPUT_BLICONS_MASK) != 0)) {
+	if (state & IMON_OUTPUT_BLICONS_MASK) {
 		switch (((state & IMON_OUTPUT_BLICONS_MASK) >> 19)) {
 		case 1:
 			icon |= IMON_ICON_MPG;
@@ -1050,19 +1043,19 @@
 		}
 	}
 	/* bit 22 : 'VOL' (volume) */
-	icon = ((state & IMON_OUTPUT_VOL_MASK) != 0) ? (icon | IMON_ICON_VOL) : (icon & ~IMON_ICON_VOL);
+	if (state & IMON_OUTPUT_VOL_MASK)	icon |= IMON_ICON_VOL;
 	/* bit 23 : 'TIME' */
-	icon = ((state & IMON_OUTPUT_TIME_MASK) != 0) ? (icon | IMON_ICON_TIME) : (icon & ~IMON_ICON_TIME);
+	if (state & IMON_OUTPUT_TIME_MASK)	icon |= IMON_ICON_TIME;
 	/* bit 24 : 'ALARM' */
-	icon = ((state & IMON_OUTPUT_ALARM_MASK) != 0) ? (icon | IMON_ICON_ALARM) : (icon & ~IMON_ICON_ALARM);
+	if (state & IMON_OUTPUT_ALARM_MASK)	icon |= IMON_ICON_ALARM;
 	/* bit 25 : 'REC' (recording) */
-	icon = ((state & IMON_OUTPUT_REC_MASK) != 0) ? (icon | IMON_ICON_REC) : (icon & ~IMON_ICON_REC);
+	if (state & IMON_OUTPUT_REC_MASK)	icon |= IMON_ICON_REC;
 	/* bit 26 : 'REP' (repeat) */
-	icon = ((state & IMON_OUTPUT_REP_MASK) != 0) ? (icon | IMON_ICON_REP) : (icon & ~IMON_ICON_REP);
+	if (state & IMON_OUTPUT_REP_MASK)	icon |= IMON_ICON_REP;
 	/* bit 27 : 'SFL' (shuffle) */
-	icon = ((state & IMON_OUTPUT_SFL_MASK) != 0) ? (icon | IMON_ICON_SFL) : (icon & ~IMON_ICON_SFL);
+	if (state & IMON_OUTPUT_SFL_MASK)	icon |= IMON_ICON_SFL;
 	/* bit 29 : 'disc-in' */
-	icon = ((state & IMON_OUTPUT_DISK_IN_MASK) != 0) ? (icon | IMON_ICON_DISK_IN) : (icon & ~IMON_ICON_DISK_IN);
+	if (state & IMON_OUTPUT_DISK_IN_MASK)	icon |= IMON_ICON_DISK_IN;
 
 	send_command_data(COMMANDS_SET_ICONS | icon, p);
 }

--Eric
On Apr 21, 2011, at 11:47 AM, Karsten A. M. Günther wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hello Eric,
>
> Thanks for your fast reply. I will try out your patch as soon as
> possible and give you feedback. Tommorow I am going on vacation and  
> will
> not return home before Tuesday.
>
> Regarding the 1 second delay: I think you misunderstood me. In the  
> perl
> script I attached I set the spinning disc followed by a "sleep 1",
> followed by a progress bar update, followed by a "sleep 1" followed by
> spanning disc setting. In that case the progress bar update stops the
> disc from spinning, and after one second it starts spinning again
> because I set it on again.
>
> In my XBMC the display is updated 4 times a second, the disc stops  
> for a
> quarter second in that case.
>
> Karsten
>
> On 21.04.2011 02:03, Eric Pooch wrote:
>> I needed to make another small change.  try this patch instead:
>>
>>
>> patch on 0.5.4
>>  again, it complies but I did not test it.
>>
>> --Eric
>>
>> On Apr 20, 2011, at 4:16 PM, Eric Pooch wrote:
>>
>>> Try this patch on 0.5.4:<imonlcd_output_bars_spin.patch>
>>>
>>> It will update the icons (including the spinning cd) even when the
>>> client output request is to only update the bars.  However, it  
>>> retains
>>> the original 0.5.4 bug fix.
>>> It also cleans up some old unnecessary code in the output function.
>>>
>>>
>>> it compiles, but I did not test it.
>>>
>>> --Eric
>>>
>>> On Apr 20, 2011, at 3:25 PM, Eric Pooch wrote:
>>>
>>>> The change between 0.5.3 and 0.5.4 fixed a serious bug where the
>>>> driver would continuously send commands to update the display  
>>>> icons,
>>>> even when there were no changes needed.  As a result of this bug, I
>>>> experienced display instability.  Your patch re-introduces the bug.
>>>> I have not noticed the 1 second delay you mention. However, I  
>>>> can see
>>>> in the code where it only updates the bars when there is an output
>>>> command made by the client.  As a result, it skips an  
>>>> opportunity to
>>>> update the spinning disc.  This change should be relatively  
>>>> simple to
>>>> implement.  I will make a patch.
>>>> In the mean time, you might want to check what is going on with  
>>>> your
>>>> configuration that is causing 1 second delay to update the icons.
>>>> what kind of computer are you using?  Have you made configuration
>>>> changes to slow down LCDproc?
>>>>
>>>> --Eric
>>>>
>>>> On Apr 20, 2011, at 2:29 PM, Karsten A. M. Günther wrote:
>>>>
>>>>> -----BEGIN PGP SIGNED MESSAGE-----
>>>>> Hash: SHA1
>>>>>
>>>>> Hello lcdproc'ers,
>>>>>
>>>>> I would like to ask you a question regarding lcdproc 0.5.4 and the
>>>>> imonlcd driver. I am working on a patch for XBMC media center  
>>>>> for full
>>>>> icon support in combination with Soundgraph's IMON LCD display and
>>>>> lcdproc (http://trac.xbmc.org/ticket/8981). In fact, it is working
>>>>> quite
>>>>> fine, but since lcdproc 0.5.4 there is an issue regarding the  
>>>>> spinning
>>>>> disc animation. Everytime, the progress bars are updated, the  
>>>>> animation
>>>>> stops for a short moment until the icons are updated again. I  
>>>>> wrote the
>>>>> attached perl script to verify this. This script executed with  
>>>>> lcdproc
>>>>> 0.5.3 shows the spinning disc and a second later all progress  
>>>>> bars and
>>>>> lines. The disc keeps spinning as expected. With lcdproc 0.5.4  
>>>>> the disc
>>>>> starts spinning, after a seconds it stops and the progress bars  
>>>>> come up
>>>>> and after another second it starts spinning again.
>>>>>
>>>>> For my needs I reverted some changes in imonlcd.c, see attached
>>>>> patch file.
>>>>>
>>>>> My question: am I doing or understanding something wrong or is  
>>>>> this
>>>>> just
>>>>> a bug in the code cleanup and rework you did in imonlcd.c  
>>>>> between 0.5.3
>>>>> and 0.5.4?
>>>>>
>>>>> Best regards,
>>>>> Karsten Guenther
>>>>>
>>>>> -----BEGIN PGP SIGNATURE-----
>>>>> Version: GnuPG v1.4.10 (GNU/Linux)
>>>>> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>>>>>
>>>>> iQEcBAEBAgAGBQJNr1A1AAoJEGeYm4AaNYHY1i4IAIDnk2KjUrIrq0363R3Kmez0
>>>>> nIJKlGbZNepvISGK1ATpnPd14HCDfbDo0UJi1PPc4PMZsDJ0XkJoOd4sVTrKLyxr
>>>>> unFQGBOxJFUBFXUUTvjjQG9mTZsrdAZagHmskt9C0oPBGZGJeCeu4JQUiegr3OvK
>>>>> zBAgQi0ms3Fx3kcSjQQmMZ9/GjXOSMGl4mJ0gB9g+Us5GWg3eC3w8+mq0FeXWdYU
>>>>> YFGD8a45kHxUDRM3PLv3vbWXB81D3hwyYaTzpEwSoGjmxJqvCLXIxtuwr1nHLfFw
>>>>> fEsprXN75kvOc9Jt9Z6odx+XWzZEGrlHRI2wF593BR3vpb2lWoyppP5agFPWeX8=
>>>>> =hzqM
>>>>> -----END PGP
>>>>> SIGNATURE----- 
>>>>> <imon_disc_and_progress.pl><imonlcd.c.diff>_______________________ 
>>>>> ________________________
>>>>>
>>>>> LCDproc mailing list
>>>>> [email protected]
>>>>> http://lists.omnipotent.net/mailman/listinfo/lcdproc
>>>>
>>>
>>> _______________________________________________
>>> LCDproc mailing list
>>> [email protected]
>>> http://lists.omnipotent.net/mailman/listinfo/lcdproc
>>
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>
> iQEcBAEBAgAGBQJNsHvFAAoJEGeYm4AaNYHYTaEH/2T9TrHynneQHjlhO+ijFijT
> YSmzagVgCzUvKyEww8LCCdTAZOMyXF3+79QTez0k1YwSg0Fnptgq/QTSiytZmcHz
> 8APge/8mjFUM9+lzvUEys2dSlkRxdlQta/RhXWEd5G9GTjfQxjxTik/pI9Qkac0w
> pbWirTk00js+el4vWlpF2hVlgbTTGIJKkukpIHGOOpjX1tVy10I6u6em4/c+FnmE
> c8AJ6GOpTP+6VpdiAa5OPFEpsWK4UejAUjHGKydchtS6rzOOGDnFJzKmDj94hMVq
> bv30nT9hnzvpqbqZ4tOWagamiIDmwBZGcy3ps8KJhapkshN8nubJ9iM4Es/rIv8=
> =Oa67
> -----END PGP SIGNATURE-----
> _______________________________________________
> LCDproc mailing list
> [email protected]
> http://lists.omnipotent.net/mailman/listinfo/lcdproc