olikraus / u8g2

U8glib library for monochrome displays, version 2
Other
5.17k stars 1.05k forks source link

Compile error with 2nd_hw_i2c #1890

Closed XilefTech closed 2 years ago

XilefTech commented 2 years ago

I am trying to compile a script for the Atmega328PB using PlatformIO, where I got two OLED displays on HW I2C 1. Currently, HW I2C 2 is not used.

The code (only excerpt of display declaration):

#include <Arduino.h>
#include <Wire.h>
#include <U8g2lib.h>

U8G2_SSD1306_128X64_NONAME_1_HW_I2C display1(U8G2_R0, /* reset=*/ U8X8_PIN_NONE);
U8G2_SSD1306_128X64_NONAME_1_HW_I2C display2(U8G2_R0, /* reset=*/ U8X8_PIN_NONE);

but when I try to compile, I get this error:

.pio\libdeps\Upload_UART\U8g2\src\U8x8lib.cpp: In function 'uint8_t u8x8_byte_arduino_2nd_hw_i2c(u8x8_t*, uint8_t, uint8_t, void*)':
.pio\libdeps\Upload_UART\U8g2\src\U8x8lib.cpp:1368:7: error: 'Wire1' was not declared in this scope

A google search has led to nothing about this issue. So how can I tell U8g2 not to worry about the second HW I2C bus?

olikraus commented 2 years ago

looks like a PlatformIO issue to me. Anyhow, you could disable 2nd HW I2C by commenting this line: https://github.com/olikraus/u8g2/blob/fd2a1edd27205668f92af3b71d7f515811405641/cppsrc/U8x8lib.h#L138

dewhisna commented 2 years ago

I too have this issue and I need to use this u8g2 library on the second I2C interface, so I can't just disable that define. The problem is NOT a PlatformIO issue. The problem is that U8x8lib.cpp needs to include Wire1.h if the U8X8_HAVE_2ND_HW_I2C is defined. In other words, you need to change this:

https://github.com/olikraus/u8g2/blob/d987608d40284f8efc75da202308442f953ad431/cppsrc/U8x8lib.cpp#L48-L54

to this:

#ifdef U8X8_HAVE_HW_I2C
#  ifdef U8X8_HAVE_HW_I2C_TEENSY3
#    include <i2c_t3.h>
#  else
#    include <Wire.h>
#    ifdef U8X8_HAVE_2ND_HW_I2C
#      include <Wire1.h>
#    endif
#  endif
#endif /* U8X8_HAVE_HW_I2C */

I have confirmed that this works and I can open a PR with this, if you wish.

olikraus commented 2 years ago

On which platform did you test this? I think this is not compatible on all platforms. It might apply to your platform only so your above mentioned change will not work for all platforms.

On ESP for example Wire1 is part of Wire.h: https://github.com/espressif/arduino-esp32/blob/9762b2392ad3928d8643911294e505e78fcc5b2f/libraries/Wire/src/Wire.h#L163

dewhisna commented 2 years ago

This is on the ATmega328PB AVR micro using the current framework ARDUINO=10808, as ARDUINO_AVR_ATmega328PB. It's using the MiniCore framework core files with variant as pb-variant.

I think it's a very recent change to the Arduino Framework, as it used to work just fine. It seems that they have now split Wire into Wire and Wire1 libraries. The new Wire1 includes Wire. So, if your code included Wire1, it would automatically get Wire, but not vice versa.

I noticed that this change was also around the time they renamed the framework from framework-arduinoavr (which had everything) to framework-arduino-avr and then they split that into subparts of framework-arduino-avr-megacore, framework-arduino-avr-minicore, framework-arduino-lpc176x, framework-arduino-sam, etc. The split seems to have happened sometime around the April - June timeframe of this year.

Part of that split also included their splitting up some of the libraries, and Wire and Wire1 is one that got split on the PB variant of this micro. Before that, when they only had the single framework package, this U8g2 library worked without any issues, as I've been using there in that same build environment since at least 2019.

As a temporary workaround in my build system, I created a .patch file and am having it patch in the above code into the local copy of this library so that it will compile for me and work on Wire1 as I need it. But it would be nice to get this fixed so I can remove the patch.

olikraus commented 2 years ago

Hmmm... If possible u8g2 should be backward compatible. I mean if Wire1 had been recently introduced, then the inclusion of that file would break many existing projects, right? We should instead check for the Arduino version.

dewhisna commented 2 years ago

After doing some digging, it looks like on the cases where there are two I2C ports, there's always been both a Wire.h and Wire1.h header. At least I couldn't find any cases where there weren't. What they split was the cores from having a single framework with all of them to having individual framework packages for each core. There was always both a Wire and Wire1 library for each of the cores that supported multiple ports.

I may be wrong, but the best I can tell, is that it automagically found the Wire1.h include before because of how the Arduino build system rewrites and modifies the includes for libraries it's pulling in. But when they split the cores apart, the library path and hierarchy changed and it stopped working.

You'll have to confirm the backward compatibility aspect, but I think it's still safe to just include Wire1.h when there's two I2C ports.

And if C++17 or higher can be counted on (which is the current default for this build framework, by the way), then something like this could be used:

#ifdef U8X8_HAVE_HW_I2C
#include <Wire.h>
#if defined(U8X8_HAVE_2ND_HW_I2C) && __has_include(<Wire1.h>)
#include <Wire1.h>
#endif
#endif

since C++17 added the new __has_include() expression. But that wouldn't be backward compatible with old build environments with older compilers and such. However, it should be OK to just include Wire1.h when there's a second I2C.

olikraus commented 2 years ago

After doing some digging, it looks like on the cases where there are two I2C ports, there's always been both a Wire.h and Wire1.h header.

The above mentioned link to ESP32 seems to be a counter example.

dewhisna commented 2 years ago

A counter example for other targets, as I was only looking at the AVR MiniCore based targets, and specifically the ATmega328PB. But yes -- good catch.

This is all part of the problem with Arduino stuff in general -- it's a mangled mess of random garbage and mishmash where everyone has forked everything and made random changes with no regard to source control or proper versioning or anything. And people expect to be able to pull in pieces from all of these random sources and it just work. I guess it's a miracle things works as well as it does.

At least tools like PlatformIO is starting to make it possible to nail down the build system and library dependencies to specific git hashes and repository URLs to make repeatable builds, rather than getting a build from whatever you randomly happen to have installed on your computer. It's not quite to the level of Conan packages, but it's the closest thing the Arduino community has.

In any case, this is broken still for some targets. But I guess the fix is going to have to consider every possible target and add the target name defines to the #if check.

The simplest fix would be to use the C++17 header check, but it unfortunately probably can't because some aren't using C++17 mode, unless you want to start forcing people to have to switch to C++17 mode (or higher) for your library (which I don't think would be unreasonable).

And the code can't just prototype that Wire1 object itself even since the name of the underlying class is sometimes different, and not even with using decltype to make it the same type as Wire, since on some platforms and library arrangements, Wire and Wire1 have completely different backing objects and entirely different classes even (like TwoWire and TwoWire1, for example). What a mess.

In any case, I have a patch for this checked into my version control for my project, along with a python script that automates the patching process when building my code, so it's working for me. But I suspect many others to eventually stumble on this same problem, especially as more of the low-end micros get multiple I/O ports.

olikraus commented 2 years ago

it's a mangled mess of random garbage and mishmash

I like this and it is very much true. It is a nightmare for all lib developers. I simply can't test all the different environments.

A PR which might look like this:

#ifdef U8X8_HAVE_HW_I2C
#  ifdef U8X8_HAVE_HW_I2C_TEENSY3
#    include <i2c_t3.h>
#  else
#    include <Wire.h>
#    ifdef U8X8_HAVE_2ND_HW_I2C
#      if defined(MiniCore) || defined(ATmega328PB)
#        include <Wire1.h>
#      endif
#    endif
#  endif
#endif /* U8X8_HAVE_HW_I2C */

would be ok for me. Of course "MiniCore" and "ATmega328PB" must be replaced with some proper defines of the corresponding environment.

dewhisna commented 2 years ago

Looks like the correct defines are MINICORE (all caps) or MCUDUDE_MINICORE based on their source code https://github.com/MCUdude/MiniCore/blob/95565897dbc3cd82f331b04b60f5b138ad517f43/avr/variants/pb-variant/pins_arduino.h#L29-L30 and __AVR_ATmega328PB__ which comes from the AVR Toolchain for that micro.

It might should be an 'AND' instead of an 'OR', since there may be other cores out there for that micro that doesn't have a Wire1.h file. So perhaps something like this?:

#ifdef U8X8_HAVE_HW_I2C
#  ifdef U8X8_HAVE_HW_I2C_TEENSY3
#    include <i2c_t3.h>
#  else
#    include <Wire.h>
#    ifdef U8X8_HAVE_2ND_HW_I2C
#      if defined(MINICORE) && defined(__AVR_ATmega328PB__)
#        include <Wire1.h>
#      endif
#    endif
#  endif
#endif /* U8X8_HAVE_HW_I2C */

I just tested this and it works OK. Should I go ahead and open the PR for that?

olikraus commented 2 years ago

Should I go ahead and open the PR for that?

It would simplify my life :-) And yes, I would include the PR then.

dzalf commented 1 year ago

Dear @olikraus

I just ran into a similar issue, however, in this case on a Teensy 3.2 board. I get two warnings on "Size of symbol 'Wire' and 'Wire1' changed...

Do you have any idea on what preprocessor directive might help to include Teensy boards to avoid redeclarations?

Thank you so much for your kind help and your fantastic work

EDIT:

Apologies. I just found out that the line:

#define U8X8_HAVE_HW_I2C_TEENSY3

from U8x8lin.h must be uncommented, however, I am still having linker issues. I am unsure if these are related at all to your library. I am still investigating

This is the warning I get:

image

Cheers

olikraus commented 1 year ago

Probably the above define had been contributed via PR. I personally have not used teensy for many years.

dzalf commented 1 year ago

Dear @olikraus

Thank you so much for your prompt response. I might consider migrating my codebase to a different library, sadly 😢

I appreciate the continued support you give to the community

Cheers

dzalf commented 1 year ago

Dear @olikraus

I have found a solution to avoid using the i2c_t3.h library.

Should I share my solution in a new Issue or in a PR so that people facing the same problem can try it out?

Cheers

olikraus commented 1 year ago

I could also include any PR if u8g2 is not affected for other boards.