Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Settings
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by MikeS - 2006-08-05

FS#5772 - BL Hold Switch Option (for all) and LCD Sleep (for x5)

There's a fair amount here in this one.

* Adds a Backlight (On Hold Switch) option for all platforms where HAS_BUTTON_HOLD is defined (Adapted from  FS#5735  by Bernych). (H100, H300, 3G, 4G, iFP-790, X5)
* Adds LCD Sleep (After Backlight Off) for those defining HAVE_LCD_SLEEP in config (RaeNye's idea). (x5)
* Adds HAVE_LCD_ENABLE (x5, 3G, H300) and BL_X5.
* Turns off visible display op. with backlight for x5.
* Makes nescessary LCD driver changes to implement sleep and disp. shutdown plus a few tweaks that save IRAM and RAM without perf. sacrifice.
* Adds a hack to make sure bl flash doesn't happen if bl is off…namely make sure shutting down screen is always displayed (TODO: skip that if low batt shutdown), bl thread is stopped, and no yields occur once the system is flushed (x5).
* Starts some things to add the hold setting to the remote. No functionality is lost; just moved some code.
* Contains a few miscellaneous changes that my text editor made, namely tabs→spaces, strip trailing whitespace. Not too much but a big pain in the behind to remove. :)
* Other things I can't remember that I did.

   new-hold-switch-and-lcd-sleep... (52.8 KiB)
Closed by  RaeNye
2006-08-08 22:06
Reason for closing:  Accepted
Additional comments about closing:  

w/o the \"halt\" part.

darkkone commented on 2006-08-05 15:10

A few notes:
1) You should try to split different 'features' into different tasks. Often you'll be asked to split them anyway, because one is worth committing and another isn't, or whatnot.
2) With the changes your text editor made, does this patch adhere to the posted guidelines for Rockbox code or not? Again, if it doesn't you'll be the one who needs to change it.

MikeS commented on 2006-08-05 19:14

1) I was asked to put this together as a unit.
2) The text editor changes actually make it more compliant since I set my options according to guidelines: Tabs→four Spaces, End file in linefeed (for gcc), UNIX linefeeds. No rule about trailing whitespace but it shouldn't really be there (maybe helps with <80 char lines).

MikeS commented on 2006-08-06 05:58

Update:
* Had to resync to CVS in misc.c.
* Changed the thread handling for the x5 backlight shutdown code 'cause I hated it. Still hate that hack but it's a bit better.
* Cleaned it up a lot (Things accidentally left in not untimately needed and such).

Additional lesson: Turn off "Remove trailing whitespace".

   new-hold-switch-and-lcd-sleep... (45.4 KiB)
RaeNye commented on 2006-08-06 05:59

I tested this patch and it works well.
two comments:

- the while(1) yield –> halt change is actually unrelated and should be discussed separately (I'll commit the patch without this change).
- the setting description that appears is 'Sleep (after BL is off)' and I'll change it to 'LCD sleep (after BL is off)'.

RaeNye commented on 2006-08-06 06:17

Of course I meant the first patch… I'll test the new version later (I was just about to commit it…)

MikeS commented on 2006-08-06 06:22

Actually I hate that hack just because it's dirty to have to kill the BL thread/ticks like that and it adds code just for one player in a few spots but no real choice unless you like to use your X5 as a strobe light.

MikeS commented on 2006-08-06 08:02

Got some comments in while I wasn't watching huh? :) Some important notes follow:

The use of halt was meant to prevent loss of control from the main thread. With the BL thread killed, I don't think it will matter though yes, I will definitely discuss that if you must remove it.

No real difference in the new version except instead of creating a new thread.c function get_thread_id(), I just save the backlight thread id returned by create_thread and use that to kill it.
I thought about using "LCD Sleep (After Backlight Off)" but felt the LCD part to be as redundant as saying "LCD Contrast" or "LCD Brightness" would be from the LCD menu and is also why I placed it next to those.

BTW: I added a second instance of the word "Normal" apart from the one used by the LCD Flip option since I'm not sure the sense and hence the word would be the same in all translations (hence the use of LANG_NORMAL_LCD_FLIP instead of LANG_FLIP by the CVS code). That of course can be changed at any time if they should be the same.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing