Compiler Bug? local arrays not initialised correctly

Discussion about Z-Uno product. Visit http://z-uno.z-wave.me for more details.
petergebruers
Posts: 255
Joined: 26 Jul 2015 17:29

Re: Compiler Bug? local arrays not initialised correctly

Post by petergebruers »

The backlight control was incorrect, I think I've fixed it. This bug may already have been in the original code, I did not check.

Demo sketch: LCD + AMS2320 temperature + humidity sensors. No elegant code, just a test...
ZUNO_LCD_I2C.zip
(4.16 KiB) Downloaded 272 times
AM2320_LCD.zip
(1.13 KiB) Downloaded 316 times
pjpankhurst
Posts: 31
Joined: 07 Sep 2017 00:40

Re: Compiler Bug? local arrays not initialised correctly

Post by pjpankhurst »

Looks like we both tested this in parallel :-)

The backlight did used to work fine, the bug was introduced during the refactoring..

The original code did this:

void ZUNO_LCD_I2C::expanderWrite(byte _data){
Wire.beginTransmission(_Addr);
Wire.write((int)(_data) | _backlightval);
Wire.endTransmission();
}

I think a neater solution to fix this is to make the following 2 functions private to the class so that _backlightval can simply be bitwise OR'd in as previously like this:

void ZUNO_LCD_I2C::lcd_aux_expander_write(void)
{
Wire.beginTransmission(g_lcd_aux_addr);
Wire.write(g_lcd_aux_data | _backlightval);
Wire.endTransmission();
}
void ZUNO_LCD_I2C::lcd_aux_write4bits(void)
{
lcd_aux_expander_write();
g_lcd_aux_tmp = g_lcd_aux_data;
g_lcd_aux_data |= En;
delayMicroseconds(1);
lcd_aux_expander_write();
g_lcd_aux_data = g_lcd_aux_tmp;
g_lcd_aux_data &= ~En;
lcd_aux_expander_write();
delayMicroseconds(50);
}

This means that in the case of multiple ZUNO_LCD_IC2 objects being created their backlight attributes are handled properly.
p0lyg0n1
Posts: 242
Joined: 04 Aug 2016 07:14

Re: Compiler Bug? local arrays not initialised correctly

Post by p0lyg0n1 »

Don't do them private! When you add them to class you increase the parameter list. They have to work fine with multiple displays too, because the parameters will be buffered anyway. Please, modify the code carefully.
The right way is to add additional global variable for current backlight, or add this when you fill data in macros.
Oh, I am sorry. I forgot this when I try to simplify interface.

UPD:
Peter did it right way already. See his attached files! It's cool! Thank you, Peter!
pjpankhurst
Posts: 31
Joined: 07 Sep 2017 00:40

Re: Compiler Bug? local arrays not initialised correctly

Post by pjpankhurst »

Perhaps I'm missing something here..

with the method Peter is using, when the backlight value is set it acts globally so will affect ALL displays.

Surely you would need to store the backlight value privately and assign it to a global in the macro when you fill data?
petergebruers
Posts: 255
Joined: 26 Jul 2015 17:29

Re: Compiler Bug? local arrays not initialised correctly

Post by petergebruers »

Thanks for reporting back! I think you both make valid points!

At the moment, the code I wrote, works with only one display. Theoretically you could connect 8 of them to one I2C bus. I only have one of these, a 2nd one is one its way. The LED back-light is a bit of an oddball on those displays. It is not connected to the display controller, as you might expect, it is wired to "bit 3" of the I2C expander. Because the LED signal is on the PCF8574 and this chip does not support masking, each time you send something to the display you also have to send the LED bit. You could read it back, but then each I2C write has to be preceded by an I2C read, I do not think this is a good solution for a library. As a quick fix, if you need it, I'd go with the suggestion of making the byte that buffers this bit a "private" variable. For a more Z-Uno friendly fix, that would be worthy of an "official" library, I'd have to think longer and harder. The I2C expander chip can have max 8 different addresses, so maybe one global could store the 8 LED bits. But I cannot imagine that this would make pretty code... How important is it to support multiple displays?

EDIT: I'm sorry I forgot to mention that my modification only works if you have only one display. I was aware of that, and because I was fixing a library, I really should have clearly mentioned it. As you can see, I am new to developing libraries... I missed the importance of "not breaking the contract" of the existing library. Such a modification would be less annoying if it was an individual sketch, it would make one person unhappy, while a library could potentially make a lot of people unhappy...
pjpankhurst
Posts: 31
Joined: 07 Sep 2017 00:40

Re: Compiler Bug? local arrays not initialised correctly

Post by pjpankhurst »

Thanks for the feedback Peter.

I think the attached fixes the problem so it works for multiple displays (I've not tested it with more than 1) and retains the optimisations that p0lyg0n1 was concerned about.

It simply fills in the global variable with the value that is held privately within the object. It does this within each of the macros.

@p0lyg0n1 - if you have a couple of mins I'd be grateful if you could explain why the functions being global versus private makes a difference to stack usage..I had assumed that the only difference was that the compiler restricted the scope in the case that it was private. Understanding this would allow us to optimise things in future ourselves :-)

Cheers
Paul
Attachments
ZUNO_LCD_I2C.zip
(4.01 KiB) Downloaded 259 times
petergebruers
Posts: 255
Joined: 26 Jul 2015 17:29

Re: Compiler Bug? local arrays not initialised correctly

Post by petergebruers »

Thank you for continuing the good work! I still have to test your latest code, but can I say something regarding the previous idea? I implemented the "private" functions and I came to the conclusion that it costs 6 extra bytes on the stack. Each class function needs a hidden parameter, v_this and I think the compiler tries to optimize this. But it still requires respectively 2 and 4 bytes to save registers. The assembler is a bit hard to decipher for me, I am rather new you Z-Uno. To implement C++ a lot of name mangling goes on under the hood. I think 6 bytes of extra stack space for library is a lot... Can't remember the exact avilable space, it is in some other topic... To be continued.
petergebruers
Posts: 255
Joined: 26 Jul 2015 17:29

Re: Compiler Bug? local arrays not initialised correctly

Post by petergebruers »

Here's an excerpt from the assembly listing, edited for clarity.

This is the optimized version:

; C:\Users\peter\AppData\Local\Temp\arduino_build_729081\ZUNO_LCD_I2C_ucxx.c:649: lcd_aux_expander_write();
lcall _lcd_aux_expander_write

So this cost you the stach-k space to store the return address.

This is what happens, if you make those 2 functions part of the class:

First, the parameter passing seems optimized, as if this would cost you no stack space:

992 ;------------------------------------------------------------
993 ;Allocation info for local variables in function '__cxx__ZUNO_LCD_I2C__method__lcd_aux_expander_write_00'
994 ;------------------------------------------------------------
995 ;v__this Allocated to registers r6 r7
996 ;------------------------------------------------------------

But... when the other function, __cxx__ZUNO_LCD_I2C__method__lcd_aux_write4bits_00, calls the first on:

push ar7
push ar6
lcall ___cxx__ZUNO_LCD_I2C__method__lcd_aux_expander_write_00
pop ar6
pop ar7

It saves the v_this on the stack, it costs you 2 bytes.

Then, when another funtion calls write4bits, this is what happens:

push ar7
push ar6
push ar5
push ar4
lcall ___cxx__ZUNO_LCD_I2C__method__lcd_aux_write4bits_00

That is 4 bytes extra. So now we already need 6 bytes extra, plus 4 bytes for the return addresses.

I think this is the reason why P0lyg0n1 advises to use macro's instead of functions (reduces call level) and to avoid class functions (avoids v_this), at least when possible, in libraries... That's my idea anyway.
pjpankhurst
Posts: 31
Joined: 07 Sep 2017 00:40

Re: Compiler Bug? local arrays not initialised correctly

Post by pjpankhurst »

Thanks for that Peter..most useful :-)

The macro versus function, I could easily see how that saved stack by not having to push any params or return addresses.

It was the private versus global that I didn't get, it seems that it is the 2 bytes for v_this that are the difference.

I seem to recollect seeing somewhere that the stack size was only 50.. Cant find it now..can anyone confirm the stack size available?
Cheers
Paul
petergebruers
Posts: 255
Joined: 26 Jul 2015 17:29

Re: Compiler Bug? local arrays not initialised correctly

Post by petergebruers »

Thanks. Glad I could help.

I've found back the numbers: "Z-Uno has just about 130 bytes of stack and it runs two firmwares in the same time: main firmware that makes all Z-Wave processing and the sketch."

If a library is based on other libraries and nests calls, you can exceed this. This happened on the older firmware, when printing a float to an oled display. ZME rewrote the libraries and it works on 2.1.1.
Post Reply