Rockbox

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.

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?

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.

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.

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.

   backlight-fade-code-police-v2... (27.3 KiB)

So, I’m going to commit this on the weekend, giving it some time to be tested and thought about.

   backlight-fading-preprocessor... (33.9 KiB)