lv_conf_internal: fix some widget dependencies when using Kconfig

[aabadie]

Description of the feature or fix

This PR is making the widget selection consistent in lv_conf_internal.h when using Kconfig. Some widgets (slider I believe dropdown, roller, textarea, img have the same problem) are always selected even if unchecked in Kconfig, this results in a build failure for slider because bar is not selected.
The missing dependency checks are also added to some widgets headers.

Some context: I'm porting latest lvgl version to RIOT in RIOT-OS/RIOT#17681 and got this issue when building the RIOT sample applications, that don't use the slider widget.

Checkpoints

• Follow the styling guide
• Run code-format.py from the scripts folder. astyle needs to be installed.
• Update the documentation

[kisvegabor]

Hi,

Thanks for the PR!

lv_conf_internal.h is auto generated by lv_conf_internal_gen.py so we shouldn't edit lv_conf_internal.h manually.

Isn't it rather a dependency issue in the Kconfig file?

cc @C47D

[aabadie]

Isn't it rather a dependency issue in the Kconfig file?

The Kconfig file is correct but I think the bug comes from the generator script (I missed it initially oups).

[aabadie]

The problem comes from this line:

lvgl/scripts/lv_conf_internal_gen.py

Line 102 in efb9c5c

is_one = re.search(r'[\s]*#[\s]*define[\s]*[A-Z0-9_]+[\s]+1[\s]*$', line)

In the lv_conf template, a line containing /*Requires: lv_label*/ doesn't match the regex, so the generated internal conf will enable the corresponding widget even if not selected in Kconfig.

[aabadie]

I adapted the generator script.

[kisvegabor]

Ahh, great catch!

[kisvegabor]

I think something like this would be more resilient as there can other comments there too:

Suggested change

	is_one_with_deps = re.search(r'[\s]*#[\s]*define[\s]*[A-Z0-9_]+[\s]+1[\s]*/\*Requires:.*$', line)
	is_one_with_deps = rre.search(r'[\s]*#[\s]*define[\s]*[A-Z0-9_]+[\s]+1[\s\r\n]+', line)

However [\s\r\n]+ is not working... Do you know what can be the problem with that?

[aabadie]

If the regex is too lazy with the end of line, it will catch maybe unrelated lines. With the following diff in the script:

diff --git a/scripts/lv_conf_internal_gen.py b/scripts/lv_conf_internal_gen.py
index e3bcb221..34e937e9 100755
--- a/scripts/lv_conf_internal_gen.py
+++ b/scripts/lv_conf_internal_gen.py
@@ -104,9 +104,8 @@ for line in fin.read().splitlines():
 
     #If the value should be 1 (enabled) by default use a more complex structure for Kconfig checks because
     #if a not defined CONFIG_... value should be interpreted as 0 and not the LVGL default
-    is_one = re.search(r'[\s]*#[\s]*define[\s]*[A-Z0-9_]+[\s]+1[\s]*$', line)
-    is_one_with_deps = re.search(r'[\s]*#[\s]*define[\s]*[A-Z0-9_]+[\s]+1[\s]*/\*Requires:.*$', line)
-    if is_one or is_one_with_deps:
+    is_one = re.search(r'[\s]*#[\s]*define[\s]*[A-Z0-9_]+[\s]+1([\s]*$|[\s]+)', line)
+    if is_one:
       #1. Use the value if already set from lv_conf.h or anything else (i.e. do nothing)
       #2. In Kconfig environment use the CONFIG_... value if set, else use 0
       #3. In not Kconfig environment use the LVGL's default value

I get the following change (compared to this PR) in lv_conf_internal.h:

diff --git a/src/lv_conf_internal.h b/src/lv_conf_internal.h
index 942cb2ba..a07b51bb 100644
--- a/src/lv_conf_internal.h
+++ b/src/lv_conf_internal.h
@@ -597,15 +597,23 @@
 /*Enable asserts if an operation is failed or an invalid data is found.
  *If LV_USE_LOG is enabled an error message will be printed on failure*/
 #ifndef LV_USE_ASSERT_NULL
-    #ifdef CONFIG_LV_USE_ASSERT_NULL
-        #define LV_USE_ASSERT_NULL CONFIG_LV_USE_ASSERT_NULL
+    #ifdef _LV_KCONFIG_PRESENT
+        #ifdef CONFIG_LV_USE_ASSERT_NULL
+            #define LV_USE_ASSERT_NULL CONFIG_LV_USE_ASSERT_NULL
+        #else
+            #define LV_USE_ASSERT_NULL 0
+        #endif
     #else
         #define LV_USE_ASSERT_NULL          1   /*Check if the parameter is NULL. (Very fast, recommended)*/
     #endif
 #endif
 #ifndef LV_USE_ASSERT_MALLOC
-    #ifdef CONFIG_LV_USE_ASSERT_MALLOC
-        #define LV_USE_ASSERT_MALLOC CONFIG_LV_USE_ASSERT_MALLOC
+    #ifdef _LV_KCONFIG_PRESENT
+        #ifdef CONFIG_LV_USE_ASSERT_MALLOC
+            #define LV_USE_ASSERT_MALLOC CONFIG_LV_USE_ASSERT_MALLOC
+        #else
+            #define LV_USE_ASSERT_MALLOC 0
+        #endif
     #else
         #define LV_USE_ASSERT_MALLOC        1   /*Checks is the memory is successfully allocated or no. (Very fast, recommended)*/
     #endif
@@ -1377,15 +1385,23 @@
 #endif
 #if LV_USE_LABEL
     #ifndef LV_LABEL_TEXT_SELECTION
-        #ifdef CONFIG_LV_LABEL_TEXT_SELECTION
-            #define LV_LABEL_TEXT_SELECTION CONFIG_LV_LABEL_TEXT_SELECTION
+        #ifdef _LV_KCONFIG_PRESENT
+            #ifdef CONFIG_LV_LABEL_TEXT_SELECTION
+                #define LV_LABEL_TEXT_SELECTION CONFIG_LV_LABEL_TEXT_SELECTION
+            #else
+                #define LV_LABEL_TEXT_SELECTION 0
+            #endif
         #else
             #define LV_LABEL_TEXT_SELECTION 1 /*Enable selecting text of the label*/
         #endif
     #endif
     #ifndef LV_LABEL_LONG_TXT_HINT
-        #ifdef CONFIG_LV_LABEL_LONG_TXT_HINT
-            #define LV_LABEL_LONG_TXT_HINT CONFIG_LV_LABEL_LONG_TXT_HINT
+        #ifdef _LV_KCONFIG_PRESENT
+            #ifdef CONFIG_LV_LABEL_LONG_TXT_HINT
+                #define LV_LABEL_LONG_TXT_HINT CONFIG_LV_LABEL_LONG_TXT_HINT
+            #else
+                #define LV_LABEL_LONG_TXT_HINT 0
+            #endif
         #else
             #define LV_LABEL_LONG_TXT_HINT 1  /*Store some extra info in labels to speed up drawing of very long texts*/
         #endif

I don't know if that is to be expected or not.

[kisvegabor]

I've just realized that I modified is_one = ... and not added is_one_with_deps = .... Having both (as in your script) work well here too.

I don't know if that is to be expected or not

Yes, these fall into the same category as the other by default enabled modules. So it's desirable to have these changes too.

[aabadie]

I edited the diff of the script above to a simpler version, which gives the same diff in lv_conf_internal.h

[aabadie]

these fall into the same category as the other by default enabled modules. So it's desirable to have these changes too.

Ok thanks! I updated the PR accordingly (force push)

[aabadie]

Also, as a side effect, this should reduce the code size when building an application configured via Kconfig and when LV_CONF_MINIMAL is enabled, since even less things are included. Probably fixing #2976

[kisvegabor]

Looks good, thank you! 🙂

I'll merge it when the CI is ready.

[aabadie]

I'll merge it when the CI is ready.

Thanks! Do you think you'll publish a patch release soon (8.2.1?) ?

[C47D]

Nice to know it wasn't in the Kconfig file hehe, there's no action needed from my side, right?

[kisvegabor]

Do you think you'll publish a patch release soon (8.2.1?) ?

To keep thing simple we make patch releases only if there is an explicit need for a bugfix. So would it be important for you the have this fix in a tagged/released version?

Nice to know it wasn't in the Kconfig file hehe, there's no action needed from my side, right?

Nope, all should be good now. 🙂

[aabadie]

So would it be important for you the have this fix in a tagged/released version?

Just a nice to have, we can deal without a release in RIOT. Thanks for merging!

[kisvegabor]

In this case please use an intermediate commit.