Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace u8glib with u8g2 #2184

Merged
merged 10 commits into from
Jul 28, 2018
Merged

Replace u8glib with u8g2 #2184

merged 10 commits into from
Jul 28, 2018

Conversation

devsaurus
Copy link
Member

Fixes #1904.

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/en/*.

This PR replaces u8glib with u8g2 for reasons that

  • u8glib is not maintained anymore
  • fixes and new displays are only available for the successor library u8g2
  • u8g2 offers less complex display routines with full RAM buffer

Lua developers need to update their scripts:

  • move from u8g to u8g2 module with different display constructors (ref u8g2 module docs)
  • apply the generic porting instructions

Upstream u8g2 source is included as a git submodule. As a side effect firmware developers need to consider these points to avoid pitfalls:

  • run git submodule update --init after pulling dev once after this PR is merged
  • use git clone --recurse-submodules when starting from scratch
  • the firmware won't build anymore when sources were downloaded as zip from GitHub since the resulting tree is incompatible with git in general and can't pull submodules

This is of course not specific to u8g2. It's more a strategic question of whether we continue with verbatim copies of upstream code or include them as submodules.

Last but not least, the configuration files changed a bit compared to former u8g integration - I expect that the cloud build service needs to be adapted to provide equivalent support for display and font selection.

//
// Add a U8G2_FONT_TABLE_ENTRY for each font you want to compile into the image
#define U8G2_FONT_TABLE \
U8G2_FONT_TABLE_ENTRY(font_6x10_tf) \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add something like the below. This allows you to define the extra fonts that you want and include them in your config that is included everywhere (or via -include on the command line). This means that this file then doesn't need to be modified to choose fonts.

#ifndef U8G_FONT_TABLE_EXTRA
#define U8G_FONT_TABLE_EXTRA
#endif
 #define U8G_FONT_TABLE_ENTRY(font)
 #define U8G_FONT_TABLE                          \
     U8G_FONT_TABLE_ENTRY(font_6x10)             \
    U8G_FONT_TABLE_EXTRA       \
     U8G_FONT_TABLE_ENTRY(font_chikita)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, seams to work.

@pjsg
Copy link
Member

pjsg commented Jan 3, 2018

Looks good to me.....

@androdev88
Copy link

Works well for me, but we should consider using https://github.com/pasko-zh/brzo_i2c.

I did a quick POC and the difference is huge.
https://www.youtube.com/watch?v=OeXQYvId7X8&feature=youtu.be

My POC uses a hack to buffer bytes as u8g2 was splitting command into multiple calls of U8X8_MSG_BYTE_SEND and brzo_i2c_write stops transmission in the call, so I had to buffer data and send it in U8X8_MSG_BYTE_END_TRANSFER.

I think the proper solution would be to modify brzo_i2c_write and move stop to brzo_i2c_end_transaction then replace original platform_i2c with support for FAST and FASTPLUS. This change most likely will be separate PR.
Unfortunately, I don't have much time so If anyone is interested POC code can be found here: https://github.com/elektro255/nodemcu-firmware/commit/24cf5f8cbaa62d35eb7c4e8ba3b03253ae3120ce

@devsaurus
Copy link
Member Author

@pjsg I extended your proposal also to the display drivers. You can now overload all config defines from an externally included file.

@elektro255 thanks for the link to optimized i2c driver. I'd suggest to handle this in a separate issue.
If and once that's sorted out, the adaption of u8g2 would be quite simple since I implemented the buffered approach already for the esp32 port.

@marcelstoer
Copy link
Member

marcelstoer commented Jan 16, 2018

I expect that the cloud build service needs to be adapted to provide equivalent support for display and font selection.

Yep, indeed. I don't have time for this atm, sorry.

The submoduling stuff also means changing the cloud builder build script and the Docker build image. Those changes would be trivial and not time-consuming, though.

Our .travis.yml doesn't need updating because Travis seems to pull in submodules recursively by default. Didn't find that behavior described in their docs but the proof is here: https://travis-ci.org/nodemcu/nodemcu-firmware/builds/328820409#L441

@devsaurus
Copy link
Member Author

Is there anything I can do to support getting this merged?

@pjsg
Copy link
Member

pjsg commented Mar 20, 2018

androdev88: Works well for me, but we should consider using https://github.com/pasko-zh/brzo_i2c.

Note that the license for the https://github.com/pasko-zh/brzo_i2c code is incompatible with the nodemcu license -- so this is a non-starter.

@FrankX0
Copy link
Contributor

FrankX0 commented Jun 11, 2018

Can anyone verify if u8g2 works in combination with spi?

With i2c a connected display works correctly.
However, with spi I cannot get it to work.
Checking the data and clock line, there is no activity.
With just the same spi master enabled and sending plain data I do see correct activity.
So it seems the u8g2 driver is not sending any data through spi.

My bad: I did not set the spi-master as first parameter. This seems to differ from current u8g and ucg implementations.

@devsaurus time to merge?

@devsaurus
Copy link
Member Author

@FrankX0 my SPI display works well for me with the code in this branch.
How do you initialize the display driver? The SPI id is needed as first parameter:

    -- Hardware SPI CLK  = GPIO14
    -- Hardware SPI MOSI = GPIO13
    -- Hardware SPI MISO = GPIO12 (not used)
    -- Hardware SPI /CS  = GPIO15 (not used)
    -- CS, D/C, and RES can be assigned freely to available GPIOs
    local cs  = 8 -- GPIO15, pull-down 10k to GND
    local dc  = 4 -- GPIO2
    local res = 0 -- GPIO16

    spi.setup(1, spi.MASTER, spi.CPOL_LOW, spi.CPHA_LOW, 8, 8)
    -- we won't be using the HSPI /CS line, so disable it again
    gpio.mode(8, gpio.INPUT, gpio.PULLUP)

    disp = u8g2.ssd1306_128x64_noname(1, cs, dc, res)

@FrankX0
Copy link
Contributor

FrankX0 commented Jun 17, 2018

@devsaurus I indeed did not put the SPI id as first parameter.
It's working fine now.
Do you think we can merge now?

@marcelstoer
Copy link
Member

Do you think we can merge now?

Triggered by Philip's comment about the Brzo I2C license I checked the U8g2 license. The library/repository doesn't declare any license at all but several files carry a "GPL" or "GPL v2" license header. Do we need to ask Oli to clarify this first (and ask for a more permissive license)?

Apart from that if people are willing to break the cloud builder and the Docker build image with this change than I guess this is ready for merging. I'm biased of course 😉and won't judge. Ideally I'd get PRs for U8g2-support in those projects (maintaining backwards compatibility) beforehand, though.

@devsaurus
Copy link
Member Author

I checked the U8g2 license. The library/repository doesn't declare any license at all

Well, there's https://github.com/olikraus/u8g2/blob/master/LICENSE stating

The U8g2lib code (http://code.google.com/p/u8g2/) is licensed under the terms of
the new-bsd license (two-clause bsd license).
See also: http://www.opensource.org/licenses/bsd-license.php

Reading the full content I get the impression that we just need to add the copyright notice in the U8g2lib API chapter to cover both redistribution of source code and in binary form. Right?

Ideally I'd get PRs for U8g2-support in those projects

Which artifacts exactly would need a change? I identified the set-display.sh and set-fonts.sh scripts at https://github.com/marcelstoer/nodemcu-custom-build/ so far. What else?

@marcelstoer
Copy link
Member

Well, there's https://github.com/olikraus/u8g2/blob/master/LICENSE

Ouch, quite embarrassing that I missed the most obvious file, sorry 🤦‍♂️

I believe your assessment regarding the licensing constraints is correct.

What else?

I haven't checked whether we'd need additional tools at https://github.com/marcelstoer/docker-nodemcu-build/blob/master/Dockerfile#L11. If not then the Docker build is probably safe.

As for the cloud builder the scripts you identified are the only ones affected AFAICT. I'm a little concerned about the font list, though. I can pull all images from the wiki repo (displayed at https://github.com/olikraus/u8g2/wiki/fntlistall) but I seriously hope the available fonts are more or less the same so that I don't have to adjust the HTML and PHP for the front end.

@TerryE
Copy link
Collaborator

TerryE commented Jun 25, 2018

I've been reviewing this because of my plans to include a compression / decompression library in NodeMCU and here are a couple of useful summaries that I came across (1), (2). I know that this has been raised before, so we should have a consistent policy on this.

IMO the td;dr is that both are permissive licences, so the issues are limited, but our project MIT license allows for distribution without contribution credits, whereas BSD doesn't.

So as far as I can see it, so long as we keep the BSD libraries in their own folders, and these contain the correct folder-based licence and attribution, then we should be OK, so long as we keep the lua and modules hierarchies themselves MIT-based.

This being said, I see that we already have BSD code in our distro: the tsl2561 library, the switec driver, and the tcs34725 module, plus some code in our libc c_string, c_stdio and dbg_printf sources which should really be replaced with MIT versions.

@TerryE
Copy link
Collaborator

TerryE commented Jul 4, 2018

Makefile Changes

  • app/u8g2lib/Makefile. Use the dev version. This has been updated since you created this patch. If now conditionally makes the subdirs for modules listed in OPT_MKTARGETS and OPT_MKLIBTARGETS if module the corresponding module has been enabled.
    This considerably speeds up the make, since most firmware builds use few if any of this listed subdirs and their libraries. So just replace your app/Makefile with the current dev one. In order to keep this simple in this case, if the module is u8g2.c then the subdir must be u8g2clib and the corresponding library must be libu8g2lib.a. If you follow this convention then youos won't need to change the dev version.
    An issue here is that you have broken the convention by remaining your library and subdir u8g2clib, so you either need to make the module, subir and library names consistent or add some exception logic in this Makefile

  • app/u8glib/Makefile. Use your version, but you need to follow the libu8g2lib.a convention (or your coded exception) so update the GEN_LIBS assignment accordingly.

Using submodules will break most u8g2 users builds

Most developers either use one of three build paths, and adding a git submodule will break all three:

  • Cloud builder. @marcelstoer will need to modify his nodemcu-custom-build file .travis.yml to add a --recursive option to his git clone to pull in the additional u8g2 submodule
  • git clone users. This will need to do the same if they use u8g. (If they don't then the subdir isn't made anyway).
  • Download ZIP users. The zip download returns an empty u8g2 subfolder so u8g users also will need to download this sub-project separately.

@devsaurus
Copy link
Member Author

I finally found time to update the PR:

  • Re-based onto latest dev
  • Adapted to Terry's optional library building
    As far as I can see, the u8g2lib branch build is skipped when the module is not selected.
  • Added copyright notice to the docs.

@devsaurus
Copy link
Member Author

@TerryE regarding your inputs on submoduling u8g2lib.
I agree on your items above, and would rate the first two as topics for documentation and user support.
The incomplete zip file is more like a bugger. Github doesn't offer any automated way, and downloading the u8g2 zip file separately opens up (in)consistency pitfalls: u8g2 integration into Nodemcu might lag behind the upstream releases. While the git submodule is locked to a specific commit which is consistent with the firmware, latest&greatest u8g2 upstream might break the build. Users would need to download the correct release zip.

Having u8g2 as git submodule is not a prerequisite for integration, of course. It's just to get rid of verbatim source copies and avoid yet another binary blob sync mechanism similar to the SDK.

@TerryE TerryE merged commit c6f6c54 into nodemcu:dev Jul 28, 2018
@marcelstoer marcelstoer added this to the next-release milestone Jul 28, 2018
@tteksoy
Copy link

tteksoy commented Sep 25, 2018

I can not get this library to work with WEMOS OLED Shield 64x48
Error message:

dofile("graphics_test.lua")
graphics_test.lua:19: attempt to call field 'ssd1306_i2c_64x48_er' (a nil value)
stack traceback:
graphics_test.lua:19: in function 'init_i2c_display'
graphics_test.lua:259: in main chunk
[C]: in function 'dofile'
stdin:1: in main chunk

Section of code:

-- setup I2c and connect display
function init_i2c_display()
-- SDA and SCL can be assigned freely to available GPIOs
local sda = 2
local scl = 1
local sla = 0x3c
i2c.setup(0, sda, scl, i2c.SLOW)
disp = u8g2.ssd1306_i2c_64x48_er(0, sla)
end

@FrankX0
Copy link
Contributor

FrankX0 commented Sep 26, 2018

First of all: this pull request has been closed. For actual (firmware) issues please open a new defect.

But judging from the response ('ssd1306_i2c_64x48_er' (a nil value)), this suggests your firmware does not contain the mentioned driver.
Please check your u8g2_displays.h file: is it added to #define U8G2_DISPLAY_TABLE_I2C ?

@ajanulis
Copy link

Dear Team,

I believe that your intentions are good, but the new u8G2 just F***C everything I did in a half a year... I've been using @marcelstoer cloud base builder for 2 years and was very happy and grateful indeed until yesterday night... I've added some Json functionality to my project and, as always, headed to cloud based tool for a built. Downloaded it, flashed and crashed... Spent all day long to realise that it's not me stupid - it's your dirty games :) Uff, sooo mad. Can you, please, provide at least simplest example how to use this u8g2 with lua - now it crashes every time even if I only declare display like this:
disp = u8g2.ssd1306_i2c_128x64_noname(0, 0x3C). Is there any way to get the old u8g? Any workaround? Anything?

@nwf
Copy link
Member

nwf commented Sep 28, 2018

@ajanulis

  • This is not a support forum.
  • We do not play "dirty games"; the firmware is developed in the open and this PR is a year old.
  • Tailored help likely requires contractual arrangements; we develop the firmware for our own ends.
  • "It crashes" is not sufficiently informative to make a good bug report; we can only guess what has gone wrong.
  • As per the example provided in https://nodemcu.readthedocs.io/en/dev/en/modules/u8g2/#i2c-display-drivers , please be sure you are initializing the I2C bus as well as the display.
  • You can get the old module by checking out this tree and rolling back to before it was removed. It may. also, be possible to selectively revert the merge of u8g2.

@ajanulis
Copy link

@nwf - thank you very much for the answer and sorry, do not want to insult anyone - I appreciate your contribution to the community and pretty sure a lot of us will benefit from your input.

  • sorry if it looks like a spam here - if you will point me where I should ask for support, I'll do it there (I posted here as I've search for similar problems and found @tteksoy has similar one and @FrankX0 pointed to the problem)

  • truly believe you are doing this one already a year (can see post dates), BUT the cloud based tool by @marcelstoer was changed just a few days ago and for me that was just out of the blue - I've built few binaries a couple days ago and everything was fine and worked (as it still was u8g). Unfortunately I can't use these builds I have as I need a JSON module to be included.

  • I am not asking for tailored support - I am just informing that using cloud based tool and documentation provided at https://nodemcu.readthedocs.io/en/master/ is not sufficient enough to use it (or, most probably I am too dumb to understand how to use it).

  • by "crashing" I literally mean crashing:

init_display()

ets Jan 8 2013,rst cause:2, boot mode:(3,6)

load 0x40100000, len 26876, room 16
tail 12
chksum 0x2d
ho 0 tail 12 room 4
load 0x3ffe8000, len 2332, room 12
tail 0
chksum 0x99
load 0x3ffe891c, len 136, room 8
tail 0
chksum 0x93
csum 0x93

under function init_display() is this code:

function init_display()
sda = 3
scl = 4
id = 0
sla = 0x3C
i2c.setup(0, sda, scl, i2c.SLOW)
disp = u8g2.ssd1306_i2c_128x64_noname(id, sla)
disp:setFont(u8g2_font_unifont_t_symbols);
disp:drawStr(0,15,"Hello World!")
end

  • I do initialise I2C. Should I start it and also add the direction? Anything else?

  • most probably I am in the wrong place as I do not know how to roll back the cloud based service. I just realised that it is possible to use 1.5.4.1-final (frozen, for 512KB flash) and I can see that there is an old u8g, just not sure what this "for 512KB flash" means - ESP01 only?

  • would appreciate a lot if you can spend 30 seconds and make an example there (or, even better in documentation) how to simple print "hallo world" on the Oled just using u8g2 and lua (don't know why there are a lot of stuff regarding Arduino it the documentation mentioned above)

@FrankX0
Copy link
Contributor

FrankX0 commented Sep 28, 2018

It seems the crashing is caused by not or (in your case) incorrectly setting an (initial) font.
In your program, replace
disp:setFont(u8g2_font_unifont_t_symbols);
with
disp:setFont(u8g2.font_unifont_t_symbols)
For me this stopped the crashing.

Furthermore you might want to clear the display buffer first, and updating the display by sending the buffer.
Try this:

function init_display()
sda = 3
scl = 4
id = 0
sla = 0x3C
i2c.setup(0, sda, scl, i2c.SLOW)
disp = u8g2.ssd1306_i2c_128x64_noname(id, sla)
disp:setFont(u8g2.font_unifont_t_symbols)
disp:clearBuffer()
disp:drawStr(0,15,"Hello World!")
disp:sendBuffer()
end

Hope this helps.

@ajanulis
Copy link

Thanks, @FrankX0! It helped indeed! Share your Paypal address - will buy you a beer!

@devsaurus devsaurus deleted the u8g2_port branch September 30, 2018 20:38
@trpster
Copy link

trpster commented Oct 16, 2018

@FrankX0 , how does one check to see if u8g2 is in the firmware build? I used the NodeMcu custom build site, and following the build it said u8g2 was included. Where would I find u8g2_displays.h file, or verify that it included #define U8G2_DISPLAY_TABLE_I2C ?
Thanks,
Tom

@FrankX0
Copy link
Contributor

FrankX0 commented Oct 17, 2018

If you are using nodemcu-build.com you need to select the U8G2 module and select the display driver for I2C and SPI:
image
Only when you are building your own firmware you need to do this by yourself through u8g2_displays.h.

@trpster
Copy link

trpster commented Oct 17, 2018

Thanks for the reply, @FrankX0. That's what I was assuming. I did select the u8g2 module and the ssd1306 i2c display, so any idea why I get the 'ssd1306_i2c_128x64_noname' (a nil value)
error even when I try the exact code you listed above? I guess I'll try reflashing...when in doubt, push a pawn.

@trpster
Copy link

trpster commented Oct 17, 2018

I've never been a big fan of mysterious cures, but I requested a new build, reflashed, and now it works. Ain't life grand!

@nodemcu nodemcu locked as resolved and limited conversation to collaborators Oct 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.