fix(bhi, leds): fixed faulty error handling due to always-false unsigned comparisons

-Wtype-limits revealed three errors in the firmware:

../pycardium/modules/sys_leds.c: In function 'mp_leds_get_rocket':
../pycardium/modules/sys_leds.c:207:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
  207 |  if (ret == -EINVAL) {
      |          ^~


../pycardium/mphalport.c: In function 'mp_hal_set_interrupt_char':
../pycardium/mphalport.c:115:8: warning: comparison is always true due to limited range of data type [-Wtype-limits]
  115 |  if (c != -1) {
      |        ^~

../epicardium/modules/bhi.c: In function 'epic_bhi160_enable_sensor':
../epicardium/modules/bhi.c:134:12: warning: comparison is always false due to limited range of data type [-Wtype-limits]
  134 |  if (vs_id < 0) {
      |            ^
../epicardium/modules/bhi.c: In function 'epic_bhi160_disable_sensor':
../epicardium/modules/bhi.c:191:12: warning: comparison is always false due to limited range of data type [-Wtype-limits]
  191 |  if (vs_id < 0) {
      |            ^

note, that on the target platform char and enums are unsigned. The compiler has probably removed all of these ifs so far because the conditions were determined to always be false.

Looks good to me! Thanks for keeping our codebase clean.

However, I'm only a bot - so a human will still have to approve this request.

added 1-Bug 6 - To Be Merged labels

Ouch, that is bad ... Maybe we should enable -Wtype-limits in the compiler options in general? Or even better, -Wextra as a whole?

i'm a big fan of more warnings ;)

-Wextra -Wno-unused-parameter -Wno-old-style-declaration seems pretty doable and would mostly require to fix some signed-unsigned comparisons / conversions.

there is one more interesting warning in the ble stack that -Wextra shows and that i'm very unsure about

../epicardium/ble/ble_main.c:125:1: warning: missing initializer for field 'maxAttemptTimeout' of 'smpCfg_t' {aka 'const struct <anonymous>'} [-Wmissing-field-initializers]
  125 | };
      | ^

Ok, so meson of course has a separate mechanism for this. There is an option called warning_level which can be either

• 0 (what we have right now)
• 1, seems to correspond to -Wall -Winvalid-pch
• 2, seems to add -Wall -Winvalid-pch -Wextra
• 3, -Wall -Winvalid-pch -Wextra -Wpedantic

---

I suggest we fix the warnings given by level 2 (where applicable) and then afterwards enable it globally. Then, just for the CI build I'd also add -Werror so any MRs coming in will fail the CI test if they produce warnings.

Patch to enable the warnings:

diff --git a/meson.build b/meson.build
index f1586ec93aa9..cb846217e7be 100644
--- a/meson.build
+++ b/meson.build
@@ -7,6 +7,7 @@ project(
     'c_std=c99',
     'b_staticpic=false',
     'b_asneeded=false',
+    'warning_level=2',
   ],
 )

I have little experience with meson, but what I did to enable individual warnings is modify the list in add_global_arguments. It could also be used to disable some warnings when transitioning to warning_level=2.

For example:

diff --git a/meson.build b/meson.build
index f1586ec9..8a0ae60b 100644
--- a/meson.build
+++ b/meson.build
@@ -7,6 +7,7 @@ project(
     'c_std=c99',
     'b_staticpic=false',
     'b_asneeded=false',
+    'warning_level=2',
   ],
 )
 
@@ -17,6 +18,8 @@ assert(
 )
 
 add_global_arguments(
+  '-Wno-unused-parameter',
+  '-Wno-old-style-declaration',
   meson.get_cross_property('target_defs'),
   language: 'c',
 )

Sounds reasonable, let's do that!

merged

mentioned in commit cac0bcf3

mentioned in commit 87e9f152

mentioned in merge request !360 (merged)

mentioned in commit 993f3248

mentioned in commit d0ec2d14

mentioned in commit 30a890a6

mentioned in commit 942658a6