Rockbox

Tasklist

FS#9724 - Backlight fading: code police

Attached to Project: Rockbox
Opened by Thomas Martitz (kugel.) - Monday, 29 December 2008, 02:22 GMT
Last edited by Thomas Martitz (kugel.) - Tuesday, 27 January 2009, 00:29 GMT
Task Type Patches
Category Operating System/Drivers
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Version 3.1
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

Here's some code police, suggested by Zagor, and going slightly farer.

-rename USE_BACKLIGHT_SW_FADING and USE_BACKLIGHT_CUSTOM_FADING_BOOL to HAVE_* to be more consistent with the other config #defines
-add a BACKLIGHT_ON_TYPE config, so that the #if <target> in backlight.c are not so target-specific
-reword/rewrite comments and add some to clear things up

Note1: should be tested on affected targets for safety (grep didn't show a missing rename), mainly on the BACKLIGHT_ON_HARDWARE targets (I tested on my BACKLIGHT_ON_SETTING e200)
Note2: I'd also like to have backlight-thread-fading.[c/h] renamed to backlight-sw-fading.[c/h], I didn't do that since svn diff doesn't do well with svn mv'd files, those should be renamed at committing or in a seperate commit

PS: I'd really like to fuse the backlight drivers for e200,c200,fuze,e200v2,sa9200 (probably c200v2), since I noticed that they're all entirely the same, except for buttonlight (that should be kept seperate). Opinions on that?
This task depends upon

Closed by  Thomas Martitz (kugel.)
Tuesday, 27 January 2009, 00:29 GMT
Reason for closing:  Accepted
Additional comments about closing:  Committed in r19860, thanks.
Comment by Björn Stenberg (zagor) - Monday, 29 December 2008, 09:28 GMT
Where is HAVE_BACKLIGHT_CUSTOM_FADING_BOOL used in the code? I only see it or:ed with USE_BACKLIGHT_SW_FADING.

I would like BACKLIGHT_ON_TYPE to be renamed HAVE_BRIGHTNESS_IN_HW and be used as a bool-type #ifdef define. This serves two purposes: We don't have to create new value defines in config.h, and it makes it easier to see in the code where the define is used that there can only be two alternatives: have, and have not.
Comment by Thomas Martitz (kugel.) - Monday, 29 December 2008, 12:13 GMT
USE_BACKLIGHT_CUSTOM_FADING_BOOL is the Gigabeat S's hardware fading. It cannot be configured like PWM fading, that's why it usese the same options as USE_BACKLIGHT_SW_FADING. That was introduced shortly after the commit of the sw fading patch.

Regarding your the BACKLIGHT_ON_TYPE: I can imagine that there're other kinds of backlight on (e.g. brightness is automatically at the lowest level after backlight on by hardware, etc..) so I'd rather let this possibility open, since changing that later might require lots of error-prone changes.
I think it's easier to follow the code if the defines are in config.h, as it's the only shared config file which affacts all targets, and you don't have to look up several config-<target>.h (often more than one since some defines are not in every file (not even commented out)).
"We don't have to create new value defines in config.h" - what's exactly bad about that?
Comment by Björn Stenberg (zagor) - Monday, 29 December 2008, 12:39 GMT
So USE_BACKLIGHT_CUSTOM_FADING_BOOL is simply hardware fading, instead of sw? Then I suggest it is simply called HAVE_BACKLIGHT_HW_FADING.

About the second point: We explicitly do not write code that "might be good to have". All code in Rockbox should have a defined purpose in the current version. Otherwise we'd quickly end up with little "nice to have" frameworks everywhere. It is better to refactor the code later if we need it to work differently than today.

The bad thing about defining new symbols in config.h is that it hurts code readability. Every new symbol the reader has to look up makes the code harder to follow and understand. We want the code to be simple and "obvious", without having to "look under the hood" to understand what is going on.
Comment by Thomas Martitz (kugel.) - Monday, 29 December 2008, 13:28 GMT
USE_BACKLIGHT_CUSTOM_FADING_BOOL is not mine, you might ask jhMikes why he called it like that.

About your last point: I totally agree, and I think you can achieve that by putting some stuff into the commonly shared files (especially the comments I added), not all of course.
"look under the hood" is exactly what you get when you have it in the config-<target>.h files which you need to browse (1 isn't enough, you need to lookup in several ones) in that case.
About the "nice to have", I also agree with you, but in this case it's only about preprocessor symbols which don't occupy binsize or something. So I think in this case it might be useful to have it like that, instead of risking error prone changes later (and preprocessor changes are always a bit special, especially regarding meaningless gcc errors).

Anyway, I oriented myself after the CONFIG_CHARGING types, as they all mean charging is available, but you need to take special attention in the different types in the implementation. That's similar to our BACKLIGHT_ON_TYPE, so I made it like that.
Comment by Thomas Martitz (kugel.) - Monday, 05 January 2009, 23:09 GMT
Do it all a bit better (hopefully).

Now there's CONFIG_BACKLIGHT_FADING (which consists of 5 possible values), each target may define its own it its config-target.h (similar to CONFIG_CHARGING). See config.h for comments.

config.h handles the transition to HAVE_BACKLIGHT_PWM_FADING and HAVE_BACKLIGHT_SW_FADING which is still used in SOURCES and backlight.c (i.e. in firmware/ code), and the deactivation of all fading for bootloader and simulator (so other code doesn't need to do that anymore).

backlight.h handles the transition to HAVE_BACKLIGHT_FADING_INT_SETTING and _BOOL_SETTING. Now apps/ code only sees these two, nothing else.

Updated features.txt, which requires backlight.h to be included, and manual (including slight rewording).

Comment and tests are welcome.
Comment by Thomas Martitz (kugel.) - Thursday, 22 January 2009, 14:59 GMT
So, I'm going to commit this on the weekend, giving it some time to be tested and thought about.

Loading...