duinoWitchery / hd44780

Extensible hd44780 LCD library
GNU General Public License v3.0
238 stars 57 forks source link

Declaring hd44780_I2Cexp in header #3

Closed dromer closed 7 years ago

dromer commented 7 years ago

I've been trying to use the library in a platformio project and ran into some issues when refactoring my main.cpp to various submodules. I think because of the autoconfigure method I am unable to declare an lcd object in my header file.

multiple definition of `hd44780_I2Cexp::AutoInst'

When I disable the 3 lines with AutoInst (declaration, use and definition) in hd44780_I2Cexp.h I am able to declare my screen (I use manual addressing in the definition).

How should I be able to declare the instance without wrecking the library? :)

bperrybap commented 7 years ago

Yeah, static class variables are messy. I probably need to do some changes to the code to make this work correctly when included in multiple modules. Can you provide a simplified example of some code that is having the issue, so that I replicate the issue?

dromer commented 7 years ago

Ok something like this (btw I have to include Wire.h for the lib to work like this):

main.cpp:

#include <function.h>

extern hd44780_I2Cexp lcd;

void setup(){
    lcd.begin(2, 16);
}
void loop(){}

function.h:

#include <Wire.h>
#include <hd44780.h>
#include <hd44780ioClass/hd44780_I2Cexp.h>

extern hd44780_I2Cexp lcd;

function.cpp:

#include <function.h>

hd44780_I2Cexp lcd(0x3F);

Results in:

Compiling .pioenvs/d1_mini/src/function.o
Linking .pioenvs/d1_mini/firmware.elf
.pioenvs/d1_mini/src/main.o:(.bss._ZN14hd44780_I2Cexp8AutoInstE+0x0): multiple definition of `hd44780_I2Cexp::AutoInst'
.pioenvs/d1_mini/src/function.o:(.bss._ZN14hd44780_I2Cexp8AutoInstE+0x0): first defined here
collect2: error: ld returned 1 exit status
*** [.pioenvs/d1_mini/firmware.elf] Error 1
bperrybap commented 7 years ago

This is an issue in the library due to the way I use the header and the static variable declaration. static class variables can be a PITA in C++ Not quite sure yet how I'm going to fix this. As long as you only are using a single instance or are not using auto locate with multiple instances you can work around it by:

After that patch, you will still be able to auto locate the i2c address of the backpack when using a single instance. And you can use multiple instances, but when using multiple instances, you must create each lcd object using a specified address.

In the mean time, I'll work on fixing the code to work correctly when using multiple modules.

dromer commented 7 years ago

I do not use the autolocate feature. This didn't work in my setup. Maybe because I have other I2C devices on the bus, non-hd44780 expander type.

bperrybap commented 7 years ago

Please open another issue for this. Autolocate should work as long as there are no other PCF8574 or MCP23008 devices on the bus that are not connected to hd44780 LCDs.

dromer commented 7 years ago

Sure. However I do not mind using a specific address. My main issue is with not being able to declare the lcd object in my headers, but your suggestion on that works here. Maybe I will temporarily keep a fork with these changes that I can use in my projects until there is a permanent solution for this.

bperrybap commented 7 years ago

What I'll probably do for this static variable issue is put something in quickly to resolve it. Then take a bit more time to work out a more elegant permanent fix.

dromer commented 7 years ago

Cool, will look forward to your commit :)

bperrybap commented 7 years ago

Can you try out this patch to make sure it works for your environment? comment out/remove the the existing declaration and definition for AutoInst in the header file. And then add a static declaration to ioinit() like this:

int ioinit()
{
int status = 0;
static uint8_t AutoInst;

This should allow everything to work without creating multiple definitions for the AutoInst variable.

--- bill

dromer commented 7 years ago

You mean like so?

301c301
< static uint8_t AutoInst; // declaration only, definition is outside class
---
> //static uint8_t AutoInst; // declaration only, definition is outside class
329a330
> static uint8_t AutoInst;
1147c1148
< uint8_t hd44780_I2Cexp::AutoInst; // define the static auto instance variable
---
> //uint8_t hd44780_I2Cexp::AutoInst; // define the static auto instance variable

This seems to work fine indeed.

bperrybap commented 7 years ago

Exactly. I'll create a new release later this evening with this patch and a few other minor updates.

dromer commented 7 years ago

Cool, I'll close the issue then :)

bperrybap commented 7 years ago

Work around for this issue is in release 0.8.4