m5stack / M5Core2

M5Core2 Arduino Library
MIT License
262 stars 113 forks source link

activating all warnings prevents compilation when you `#include <M5Core2.h>` #133

Closed JM-FRANCE closed 1 year ago

JM-FRANCE commented 1 year ago

Describe the bug

The ESP32 developers decided to add the -Werror=all flag to the compilation command when you set the "Compiler warnings" preference to the "More" or "All" levels.

This flag causes compilation to fail when the sketch code produces a compilation warning.

the challenge is that there are many places where such warnings occur, mainly related to fonts

To reproduce

set the compiler warning level to All in Arduino's IDE preference panel

open a sketch with the following simple code

#include <M5Core2.h>

void setup() {
  M5.begin(); //Init M5Core2. Initialize M5Core2
  M5.Lcd.print("Hello World");
}

void loop() {}

hit compile.

➜ compilation will fail with mention that

some warnings being treated as errors

Expected behavior

of course we would expect the code to have no warning at all.

Screenshots

No response

Environment

happens on all Linux, Mac OS and Windows latest version of IDE 1.8.x or 2.x

Additional context

Most of the warnings come from the FONTS

this is the definition of the GFXFont structure

typedef struct { // Data stored for FONT AS A WHOLE:
    uint8_t  *bitmap;      // Glyph bitmaps, concatenated
    GFXglyph *glyph;       // Glyph array
    uint16_t  first, last; // ASCII extents
    uint8_t   yAdvance;    // Newline distance (y axis)
#ifdef USE_M5_FONT_CREATOR
    uint16_t range_num;
    EncodeRange *range;
    uint8_t smooth_bpp;
#endif
} GFXfont;

if you go see the end of a custom font definition file, for example https://github.com/m5stack/M5Core2/blob/fb820a59fd899adebf10cbba23d0af299a16f720/src/Fonts/Custom/Orbitron_Light_24.h#L388-L398 you'll see

const GFXfont Orbitron_Light_24 PROGMEM = {
    (uint8_t *)Orbitron_Light_24Bitmaps,
    (GFXglyph *)Orbitron_Light_24Glyphs,
    0x20,
    0x7D,
    24,
#ifdef USE_M5_FONT_CREATOR
    0,
    0,
#endif
};

You'll notice that the struct definition requires 3 member variables in the #ifdef USE_M5_FONT_CREATOR and only 2 are provided

==> all the font files (custom and standard) need to be modified to include 3 attributes in the #ifdef (some have only two attributes, other don't have the #ifdef altogether

#ifdef USE_M5_FONT_CREATOR
  0, nullptr, 0,
#endif

another such warning is in glcdfont.c

https://github.com/m5stack/M5Core2/blob/fb820a59fd899adebf10cbba23d0af299a16f720/src/Fonts/glcdfont.c#L10

if the font is unused the compiler will complain that the variable is unused

~/Documents/Arduino/libraries/M5Core2/src/Fonts/glcdfont.c:10:28: error: 'font' defined but not used [-Werror=unused-const-variable=]
 static const unsigned char font[] PROGMEM = {
                            ^~~~

causing again the compilation to fail

to fix this change the definition to include __attribute__((unused))

static const unsigned char font[] __attribute__((unused)) PROGMEM = {


then there are the RTC issues already reported in #122 (https://github.com/m5stack/M5Core2/issues/122))

/Documents/Arduino/libraries/M5Core2/src/RTC.cpp:221:30: warning: comparison is always true due to limited range of data type [-Wtype-limits]
   if (RTC_TimeStruct.Minutes >= 0) {
       ~~~~~~~~~~~~~~~~~~~~~~~^~~~

where you seem to assume that attributes could be negative but that can't happen since they are unsigned...

Fixing all those would let us keep the warning at the highest level, which is helpful to produce quality code.

Issue checklist

Tinyu-Zhao commented 1 year ago

Thanks for the feedback, I'll take care of this as soon as I can.

Tinyu-Zhao commented 1 year ago

Patch to fix tons of warnings has been pushed to Github,https://github.com/m5stack/M5Core2/commit/a54b35f1eed260b6462a4572c4679d98b379297e

Do you have any good suggestions for #122?

Tinyu-Zhao commented 1 year ago

Fixed in https://github.com/m5stack/M5Core2/commit/24d557c8bd8394ba8b304bdbf9abff47e23aa307

JM-FRANCE commented 10 months ago

Do you have any good suggestions for #122?

the test

RTC_TimeStruct.Minutes >= 0

is useless, so just removing it won't change anything.