Compiler warnings - Githubissues

thijstriemstra commented 6 months ago

Using platformio:

[env:pico]
platform = raspberrypi
board = pico
framework = arduino

lib_deps =
  robtillaart/I2C_EEPROM@^1.8.2

seeing some warnings while compiling my project:


.pio\libdeps\pico\I2C_EEPROM\I2C_eeprom.cpp: In member function 'bool I2C_eeprom::writeBlockVerify(uint16_t, const uint8_t*, uint16_t)':
.pio\libdeps\pico\I2C_EEPROM\I2C_eeprom.cpp:206:11: warning: variable length array 'data' is used [-Wvla]
  206 |   uint8_t data[length];
      |           ^~~~
.pio\libdeps\pico\I2C_EEPROM\I2C_eeprom.cpp: In member function 'bool I2C_eeprom::setBlockVerify(uint16_t, uint8_t, uint16_t)':
.pio\libdeps\pico\I2C_EEPROM\I2C_eeprom.cpp:216:11: warning: variable length array 'data' is used [-Wvla]
  216 |   uint8_t data[length];
      |           ^~~~
.pio\libdeps\pico\I2C_EEPROM\I2C_eeprom.cpp: In member function 'bool I2C_eeprom::updateBlockVerify(uint16_t, const uint8_t*, uint16_t)':
.pio\libdeps\pico\I2C_EEPROM\I2C_eeprom.cpp:239:11: warning: variable length array 'data' is used [-Wvla]
  239 |   uint8_t data[length];
      |           ^~~~

RobTillaart commented 6 months ago

Analysis

data[length] is used as a buffer to verify / compare.

Possible solution

Maybe a _verifyBlock(addr, data, length) that verifies while reading. Could work per byte, uses less memory and fails faster?

RobTillaart commented 6 months ago

@thijstriemstra Created a develop branch for testing

added verifyBlock() for readBlockVerify() and writeBlockVerify
that did not fit the setBlockVerify so I solver that with dynamic memory for now.

Can you check if the warnings still exist (or are replaced with others)?

thijstriemstra commented 6 months ago

Can you check if the warnings still exist (or are replaced with others)?

I'll give it a try. Might be a good idea to setup a platformio builder and make it fail on warnings like this.

RobTillaart commented 6 months ago

That is a good idea, Do you know if there is a GitHub workflow that I could use? Then I could roll it out quite easily for all libraries.

thijstriemstra commented 6 months ago

See https://docs.platformio.org/en/latest/integration/ci/github-actions.html

so something like:

name: PlatformIO CI

  on: [push]

  jobs:
    build:
      runs-on: ubuntu-latest
      strategy:
        matrix:
          example: [path/to/test/file.c, examples/file.ino, path/to/test/directory]

      steps:
        - uses: actions/checkout@v4
        - uses: actions/cache@v3
          with:
            path: |
              ~/.cache/pip
              ~/.platformio/.cache
            key: ${{ runner.os }}-pio
        - uses: actions/setup-python@v5
          with:
            python-version: '3.11'
        - name: Install PlatformIO Core
          run: pip install --upgrade platformio

        - name: Build PlatformIO examples
          run: pio ci --board=<ID_1> --board=<ID_2> --board=<ID_N>
          env:
            PLATFORMIO_CI_SRC: ${{ matrix.example }}

thijstriemstra commented 6 months ago

And to make it fail on warnings add -Werror (see https://docs.platformio.org/en/latest/projectconf/sections/env/options/build/build_flags.html):

Make all warnings into hard errors. With this option, if any source code triggers warnings, the compilation will be aborted.

So:

- name: Run PlatformIO
  run: pio ci path/to/test/file.c --board=<ID_1> --board=<ID_2> --board=<ID_N>
  env:
    PLATFORMIO_BUILD_FLAGS: -D -Werror

(untested)

RobTillaart commented 6 months ago

Created an issue for this in a smaller library to investigate and get it to work

https://github.com/RobTillaart/A1301/issues/7

thijstriemstra commented 6 months ago

Cool! The only disadvantage i can see is compiler warnings in third party libraries/frameworks, that'll fail the build as well (I think). Narrowing warnings down to lib only code might also be possible but not sure/tested.

RobTillaart commented 6 months ago

OK, I will try to find out coming days/weeks (quite busy with other projects) how to get the platformIO build running.

RobTillaart commented 6 months ago

Got something building with green results 😎

https://github.com/RobTillaart/A1301/actions/runs/8403081050/job/23013196189

yaml file is not portable or engineered (yet) but there is a starting point.

RobTillaart commented 6 months ago

The -Werror fails upon the Arduino core libs like HWSerial, so that switch will not be used...

Building in release mode
Compiling .pio/build/uno/src/A1301_autoMidPoint.ino.cpp.o
Compiling .pio/build/uno/lib954/A1301/A1301.cpp.o
Archiving .pio/build/uno/libFrameworkArduinoVariant.a
<command-line>:0:1: error: macro names must be identifiers
Compiling .pio/build/uno/FrameworkArduino/CDC.cpp.o
<command-line>:0:1: error: macro names must be identifiers
Indexing .pio/build/uno/libFrameworkArduinoVariant.a
<command-line>:0:1: error: macro names must be identifiers
Compiling .pio/build/uno/FrameworkArduino/HardwareSerial.cpp.o
<command-line>:0:1: error: macro names must be identifiers
*** [.pio/build/uno/src/A1301_autoMidPoint.ino.cpp.o] Error 1
*** [.pio/build/uno/lib954/A1301/A1301.cpp.o] Error 1
*** [.pio/build/uno/FrameworkArduino/CDC.cpp.o] Error 1
*** [.pio/build/uno/FrameworkArduino/HardwareSerial.cpp.o] Error 1
========================== [FAILED] Took 0.[65](https://github.com/RobTillaart/A1301/actions/runs/8403259599/job/23013574092#step:7:66) seconds ==========================

RobTillaart commented 6 months ago

compiling all examples by coding a hard list works

https://github.com/RobTillaart/A1301/actions/runs/8403378082

to be continued later.

RobTillaart commented 6 months ago

Notes to me: current test code A1301 repo


name: PlatformIO CI

on: [push, pull_request]

jobs:
  build:
    runs-on: ${{ matrix.os }}
    #  added timeout
    timeout-minutes: 20
    strategy:
      matrix:
        os: [ubuntu-latest]
        example: [
                   examples/A1301_autoMidPoint/A1301_autoMidPoint.ino,
                   examples/A1301_demo/A1301_demo.ino,
                   examples/A1301_determineNoise/A1301_determineNoise.ino,
                   examples/A1301_performance/A1301_performance.ino,
                   examples/A1301_plotter/A1301_plotter.ino,
                   examples/A1301_test/A1301_test.ino,
                   examples/A1301_test_saturation/A1301_test_saturation.ino
                  ]

    steps:
      - uses: actions/checkout@v4
      - uses: actions/cache@v3
        with:
          path: |
            ~/.cache/pip
            ~/.platformio/.cache
          key: ${{ runner.os }}-pio
      - uses: actions/setup-python@v5
        with:
          python-version: '3.11'
      - name: Install PlatformIO Core
        run: pip install --upgrade platformio
      - name: Download library
        run: |
          wget https://github.com/RobTillaart/A1301/archive/master.zip -O /tmp/master.zip
          unzip /tmp/master.zip -d /tmp
      - name: Build PlatformIO examples
        run: pio ci --lib "." --lib="/tmp/A1301-master" --board=uno
        env:
          PLATFORMIO_CI_SRC: ${{ matrix.example }}
          #  PLATFORMIO_BUILD_FLAGS: -D -Wall  # too much errors in core Arduino.

hardcoded are

examples to compile
repository to checkout
branch to checkout

These should ideally be created / derived from the branch pushed. Some options to get it done:

maintain manually
python script generating the yml file
python script that runs on VM and do the matrix
do magic in yml file

RobTillaart commented 6 months ago

After reading

https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions

I could fix the repository and correct branch (first it was hardcoded master while it had to be a develop branch).

      - name: Download library
        run: |
          wget https://github.com/${{github.repository}}/archive/${{github.ref_name}}.zip -O /tmp/${{github.ref_name}}.zip
          unzip /tmp/${{github.ref_name}}.zip -d /tmp/${{github.actor}}
      - name: Build PlatformIO examples
        run: pio ci --lib "." --lib="/tmp/${{github.repository}}-${{github.ref_name}}" --board=uno
        env:
          PLATFORMIO_CI_SRC: ${{ matrix.example }}
          #  PLATFORMIO_BUILD_FLAGS: -D -Wall  # too much

Only the list of examples files need to be generated,. Maintaining that list manually is doable as the list does not change often.

Note: strategy of matrix job is set up before the download and build starts. (need to read more)

RobTillaart commented 6 months ago

Did a test with ESP32DEV instead of UNO, also warnings in the ESP32 specific files. So I doubt if the Wall option will work

Compiling .pio/build/esp32dev/FrameworkArduino/libb64/cdecode.c.o
/home/runner/.platformio/packages/framework-arduinoespressif32/cores/esp32/esp32-hal-uart.c: In function 'uartSetPins':
/home/runner/.platformio/packages/framework-arduinoespressif32/cores/esp32/esp32-hal-uart.c:153:9: warning: 'return' with no value, in function returning non-void
         return;
         ^~~~~~

This bug is already reported at - https://github.com/espressif/arduino-esp32/issues/8641

RobTillaart commented 6 months ago

@thijstriemstra

I'll give it a try. Might be a good idea to setup a platformio builder and make it fail on warnings like this.

Did you have time to try the develop branch?

thijstriemstra commented 6 months ago

After switching to:

[env:pico]
platform = https://github.com/maxgerhardt/platform-raspberrypi.git
board = rpipico
framework = arduino
monitor_speed = 115200
; board can use both Arduino cores -- we select Arduino-Pico here
board_build.core = earlephilhower
; Flash Size: 2MB (Sketch: 1.5MB, FS: 0.5MB)
board_build.filesystem_size = 0.5m

the compile errors seem to be gone, and since I'll be using earlephilhower (since platformio raspberrypi is not supported by platformio labs and basically abandoned) I'll leave it up to you to implement your suggested fix or not.

Verbose mode can be enabled via `-v, --verbose` option
CONFIGURATION: https://docs.platformio.org/page/boards/raspberrypi/rpipico.html
PLATFORM: Raspberry Pi RP2040 (1.11.0+sha.60d6ae8) > Pico
HARDWARE: RP2040 133MHz, 264KB RAM, 2MB Flash
DEBUG: Current (blackmagic) External (blackmagic, cmsis-dap, jlink, pico-debug, picoprobe, raspberrypi-swd)
PACKAGES:
 - framework-arduinopico @ 1.30700.0+sha.88ccf0c
 - tool-rp2040tools @ 1.0.2
 - toolchain-rp2040-earlephilhower @ 5.120300.240125 (12.3.0)
Flash size: 2.00MB
Sketch size: 1.50MB
Filesystem size: 0.50MB
Maximium Sketch size: 1568768 EEPROM start: 0x101ff000 Filesystem start: 0x1017f000 Filesystem end: 0x101ff000       
LDF: Library Dependency Finder -> https://bit.ly/configure-pio-ldf
LDF Modes: Finder ~ chain, Compatibility ~ soft
Found 56 compatible libraries
Scanning dependencies...
Dependency Graph
|-- I2C_LCD @ 0.2.1
|-- I2C_EEPROM @ 1.8.2
|-- DHT sensor library @ 1.4.6
|-- Adafruit Unified Sensor @ 1.1.14
|-- SPI @ 1.0
|-- Wire @ 1.0
Building in release mode
Compiling .pio\build\pico\FrameworkArduinoBootloader\boot2_w25q080_2_padded_checksum.S.o
Compiling .pio\build\pico\src\DryBoxDisplay.cpp.o
Compiling .pio\build\pico\src\WRKeyState.cpp.o
Compiling .pio\build\pico\src\main.cpp.o
Generating linkerscript C:\Users\Thijs\projects\drybox\.pio\build\pico/memmap_default.ld
Compiling .pio\build\pico\lib785\Wire\Wire.cpp.o
Compiling .pio\build\pico\libf9e\I2C_LCD\I2C_LCD.cpp.o
Compiling .pio\build\pico\lib002\I2C_EEPROM\I2C_eeprom.cpp.o
Compiling .pio\build\pico\lib0c1\Adafruit Unified Sensor\Adafruit_Sensor.cpp.o
Compiling .pio\build\pico\libcb5\DHT sensor library\DHT.cpp.o
Compiling .pio\build\pico\libcb5\DHT sensor library\DHT_U.cpp.o
Archiving .pio\build\pico\lib785\libWire.a
Archiving .pio\build\pico\lib002\libI2C_EEPROM.a
Compiling .pio\build\pico\lib91d\SPI\SPI.cpp.o
Archiving .pio\build\pico\lib0c1\libAdafruit Unified Sensor.a
Compiling .pio\build\pico\FrameworkArduino\BluetoothDebug.cpp.o
Compiling .pio\build\pico\FrameworkArduino\Bootsel.cpp.o
Compiling .pio\build\pico\FrameworkArduino\CoreMutex.cpp.o
Compiling .pio\build\pico\FrameworkArduino\FS.cpp.o
Archiving .pio\build\pico\libf9e\libI2C_LCD.a
Compiling .pio\build\pico\FrameworkArduino\PIOProgram.cpp.o
Compiling .pio\build\pico\FrameworkArduino\RP2040Support.cpp.o
Compiling .pio\build\pico\FrameworkArduino\RP2040USB.cpp.o
Archiving .pio\build\pico\libcb5\libDHT sensor library.a
Compiling .pio\build\pico\FrameworkArduino\SerialPIO.cpp.o
Compiling .pio\build\pico\FrameworkArduino\SerialUART.cpp.o
Compiling .pio\build\pico\FrameworkArduino\SerialUSB.cpp.o
Compiling .pio\build\pico\FrameworkArduino\StackThunk.cpp.o
Compiling .pio\build\pico\FrameworkArduino\Tone.cpp.o
Compiling .pio\build\pico\FrameworkArduino\WMath.cpp.o
Archiving .pio\build\pico\lib91d\libSPI.a
Compiling .pio\build\pico\FrameworkArduino\_freertos.cpp.o
Compiling .pio\build\pico\FrameworkArduino\api\Common.cpp.o
Compiling .pio\build\pico\FrameworkArduino\api\IPAddress.cpp.o
Compiling .pio\build\pico\FrameworkArduino\api\PluggableUSB.cpp.o
Compiling .pio\build\pico\FrameworkArduino\api\Print.cpp.o
Compiling .pio\build\pico\FrameworkArduino\api\Stream.cpp.o
Compiling .pio\build\pico\FrameworkArduino\api\String.cpp.o
Compiling .pio\build\pico\FrameworkArduino\cyw43_wrappers.cpp.o
Compiling .pio\build\pico\FrameworkArduino\delay.cpp.o
Compiling .pio\build\pico\FrameworkArduino\libb64\cdecode.cpp.o
Compiling .pio\build\pico\FrameworkArduino\libb64\cencode.cpp.o
Compiling .pio\build\pico\FrameworkArduino\lock.cpp.o
Compiling .pio\build\pico\FrameworkArduino\lwip_wrap.cpp.o
Compiling .pio\build\pico\FrameworkArduino\main.cpp.o
Compiling .pio\build\pico\FrameworkArduino\malloc-lock.cpp.o
Compiling .pio\build\pico\FrameworkArduino\posix.cpp.o
Compiling .pio\build\pico\FrameworkArduino\sdkoverride\att_db.c.o
Compiling .pio\build\pico\FrameworkArduino\sdkoverride\btstack_flash_bank.cpp.o
Compiling .pio\build\pico\FrameworkArduino\sdkoverride\hids_device.c.o
Compiling .pio\build\pico\FrameworkArduino\sdkoverride\pico_bootsel_via_double_reset.c.o
Compiling .pio\build\pico\FrameworkArduino\stdlib_noniso.cpp.o
Compiling .pio\build\pico\FrameworkArduino\wiring_analog.cpp.o
Compiling .pio\build\pico\FrameworkArduino\wiring_digital.cpp.o
Compiling .pio\build\pico\FrameworkArduino\wiring_private.cpp.o
Compiling .pio\build\pico\FrameworkArduino\wiring_pulse.cpp.o
Compiling .pio\build\pico\FrameworkArduino\wiring_shift.cpp.o
Archiving .pio\build\pico\libFrameworkArduino.a
Linking .pio\build\pico\firmware.elf
Generating UF2 image
elf2uf2 ".pio\build\pico\firmware.elf" ".pio\build\pico\firmware.uf2"
Retrieving maximum program size .pio\build\pico\firmware.elf
Flash size: 2.00MB
Sketch size: 1.50MB
Filesystem size: 0.50MB
Maximium Sketch size: 1568768 EEPROM start: 0x101ff000 Filesystem start: 0x1017f000 Filesystem end: 0x101ff000       
Checking size .pio\build\pico\firmware.elf
Advanced Memory Usage is available via "PlatformIO Home > Project Inspect"
RAM:   [=         ]   5.5% (used 14480 bytes from 262144 bytes)
Flash: [          ]   5.0% (used 78288 bytes from 1568768 bytes)
Building .pio\build\pico\firmware.bin
Building .pio\build\pico\firmware.bin.signed

RobTillaart commented 5 months ago

OK, I will merge the develop branch today

adds verifyBlock(memoryAddress, buffer, length)
updates build-CI scripts

RobTillaart / I2C_EEPROM

Compiler warnings #64

Analysis

Possible solution