ThingPulse / esp8266-oled-ssd1306

Driver for the SSD1306 and SH1106 based 128x64, 128x32, 64x48 pixel OLED display running on ESP8266/ESP32
https://thingpulse.com
Other
2.01k stars 638 forks source link

Unused variable warning on ESP8266 #381

Open nyanpasu64 opened 1 year ago

nyanpasu64 commented 1 year ago

Describe the bug On platforms other than ESP32, I get an unused variable warning in the SSD1306Wire constructor, since only the ARDUINO_ARCH_ESP32 branch references the _i2cBus parameter.

To Reproduce Steps to reproduce the behavior:

  1. Enable warnings in Arduino. V1 instructions: Preferences > Compiler warnings > All.
  2. Build an ESP8266 (not ESP32) project including SSD1306Wire.h.

Sample code Provide a MCVE below.

#include "SSD1306Wire.h"

Expected behavior No compiler warning. Perhaps mark the parameter [[maybe_unused]] (since C++17), or add a (void)_i2cBus statement, either in the #if !defined(ARDUINO_ARCH_ESP32) branch or globally?

Screenshots If applicable, add screenshots to help explain your problem.

https://github.com/ThingPulse/esp8266-oled-ssd1306/blob/f96fd6a6df6acca6fae2e0b40e7642566aaa34e6/src/SSD1306Wire.h#L75-L85

In file included from /home/nyanpasu64/Documents (Shared)/gbs-c/gbs-control/gbs-control.ino:22:
/home/nyanpasu64/Arduino/libraries/esp8266-oled-ssd1306-master/src/SSD1306Wire.h: In constructor 'SSD1306Wire::SSD1306Wire(uint8_t, int, int, OLEDDISPLAY_GEOMETRY, HW_I2C, int)':
/home/nyanpasu64/Arduino/libraries/esp8266-oled-ssd1306-master/src/SSD1306Wire.h:70:114: warning: unused parameter '_i2cBus' [-Wunused-parameter]
   70 |     SSD1306Wire(uint8_t _address, int _sda = -1, int _scl = -1, OLEDDISPLAY_GEOMETRY g = GEOMETRY_128_64, HW_I2C _i2cBus = I2C_ONE, int _frequency = 700000) {
      |                                                                                                           ~~~~~~~^~~~~~~~~~~~~~~~~

Versions (please complete the following information):

Additional context Add any other context about the problem here.

marcelstoer commented 1 year ago

@benoitm974 what is your take on this? I ask for your opinion as you contributed the code in question back then.

nyanpasu64 commented 1 year ago

I actually feel the "unused arguments" warning is too noisy and turn it off in my personal projects, but it's difficult to set custom compiler flags on Arduino IDE v1, and reducing the warning level turns off other useful warnings as well. In this case I think (void)_i2cBus isn't too ugly of a change to make.

In any case, should the _i2cBus argument even exist outside of ESP32? Though removing the argument on some platforms can break code on a platform-dependent basis, which is even worse than leaving it in.

Should the HW_I2C type only define I2C_TWO on ESP32, and otherwise be a one-value enum to prevent users from passing in I2C_TWO on ESP8266 which is silently ignored? Or is it better to leave I2C_TWO in?

benoitm974 commented 1 year ago

@marcelstoer Thanks for the head up and kind attention on this contribution.

Since this code is executed at init only why don't we secure it by adding a test/assert in case someone is trying to use a second wire on another ESP32 platform with no 2nd Wire?

`

if !defined(ARDUINO_ARCH_ESP32)

   //assert on trying to use a second wire on non-ESP32 platform.
   assert(_i2cBus!=I2C_ONE)
   this->_wire = &Wire; 

else

   this->_wire = (_i2cBus==I2C_ONE) ? &Wire : &Wire1; 

endif

`

Benoit