search

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
1.99k stars 637 forks source link

travis: test against more boards #300

Closed thijstriemstra closed 3 years ago

marcelstoer commented 4 years ago

Your test was successful, the CI build now fails 😜 This confirms the report in #296.

thijstriemstra commented 4 years ago

Results are not promising indeed: https://travis-ci.org/github/ThingPulse/esp8266-oled-ssd1306/builds/717596834

thijstriemstra commented 4 years ago

@marcelstoer the D3 pin issue can be solved by changing it to 3. Any objections to that?

thijstriemstra commented 4 years ago

Ah, probably needs to be imported for the other boards, let me check.

marcelstoer commented 4 years ago

The D<n> pins are only defined for the NodeMCU and WeMOS boards IIRC.

thijstriemstra commented 4 years ago

any idea what this error is about?

lib/esp8266-oled-ssd1306/src/SSD1306Wire.h:36:21: fatal error: algorithm: No such file or directory
thijstriemstra commented 4 years ago

can we merge this so the problems are identified and in the open? or do we want to fix these in this pull request.

marcelstoer commented 4 years ago

I am very reluctant to merge changes I know won't even compile.

thijstriemstra commented 4 years ago

I am very reluctant to merge changes I know won't even compile.

It's not a code change and it already doesn't compile on many boards, this just points that out.

or do we want to fix these in this pull request.

You didn't answer this question so I'll take a look at fixing them in this pull request.

marcelstoer commented 4 years ago

I am very reluctant to merge changes I know won't even compile.

It's not a code change and it already doesn't compile on many boards, this just points that out.

I know and since I know it will still break the build - again, not your fault of course - I prefer to have the fix in the same PR. That doesn't mean you're the (only) one who should contribute the fix.

thijstriemstra commented 3 years ago

@marcelstoer I simplified things and only updated the travis build config, please review.

marcelstoer commented 3 years ago

Coming back to my earlier comment

The D<n> pins are only defined for the NodeMCU and WeMOS boards IIRC.

I just took a brief look at the various example sketches in this project (and their init differences). The simple demo uses a useful pattern:

https://github.com/ThingPulse/esp8266-oled-ssd1306/blob/593ddc0ae6bca74f60853c7edd893ae511f5d830/examples/SSD1306SimpleDemo/SSD1306SimpleDemo.ino#L53-L57

Instead of relying on the D<n> pin definitions it relies on SDC/SCL and comments the alternatives. Would you agree that it makes sense to adjust the .inos to this? We could then enable the other boards again as you initially proposed. Could you do that in your thijstriemstra:patch-1 branch as I can't commit to it.

thijstriemstra commented 3 years ago

@marcelstoer made requested changes. But what to do about the Two screens example that uses 2 I2C ports.. ignore it, e.g. exclude it from travis? and there's still the algorithm dependency issue.

thijstriemstra commented 3 years ago

@marcelstoer can you also migrate this repository to travis.com, it'll hopefully fix the build trigger issue..

See https://docs.travis-ci.com/user/migrate/open-source-repository-migration

marcelstoer commented 3 years ago

what to do about the Two screens example that uses 2 I2C ports

Replace D<n> with an (arbitrary) numeric pin number?

can you also migrate this repository to travis.com, it'll hopefully fix the build trigger issue

The webhook still works (i.e. requests successful) but Travis doesn't start a build for PRs anymore. The whole travis-ci.org vs. travis-ci.com integration has become quite a nuisance for me :( I work for multiple orgs with both private and OSS repos. It's a mess currently.

thijstriemstra commented 3 years ago

all opensource repos using travis will migrate from .org to .com eventually. I found .com much more stable after recently migrating.

thijstriemstra commented 3 years ago

Replace D with an (arbitrary) numeric pin number?

This seems to be fixed.

Only error left is:

In file included from /tmp/tmphxqjkjzy/src/SSD1306SimpleDemo.ino:35:0:

lib/esp8266-oled-ssd1306/src/SSD1306Wire.h:36:21: fatal error: algorithm: No such file or directory

compilation terminated.

what about removing this import, is it really needed?

marcelstoer commented 3 years ago

what about removing this import, is it really needed?

Currently it is...the change was introduced with #291 by @geeksville

thijstriemstra commented 3 years ago

Seems like it's undefined on Arduino UNO. So maybe something like?

#if !defined(ARDUINO_ARCH_AVR)
#include <algorithm>
#endif

I guess this can be a different pull request, I'll only focus on esp32/esp8266 boards here.

thijstriemstra commented 3 years ago 
Environment          Status    Duration

-------------------  --------  ------------

nodemcuv2            SUCCESS   00:00:13.428

d1_mini              SUCCESS   00:00:13.497

esp-wrover-kit       SUCCESS   00:00:10.392

esp32doit-devkit-v1  SUCCESS   00:00:10.345

========================= 4 succeeded in 00:00:47.661 =========================

Ugh:

Generating partitions .pio/build/esp32doit-devkit-v1/partitions.bin

/tmp/tmp95di0upd/src/SSD1306OTADemo.ino:32:25: fatal error: ESP8266WiFi.h: No such file or directory
marcelstoer commented 3 years ago

First and foremost thank you so much for hanging in there and working through this 👍 👏 🥇

Seems like it's undefined on Arduino UNO. So maybe something like?

It's more than that I'm afraid. If I knew then what I know now I wouldn't have approved #291. Did you see the _min/_max changes there?

All the ifdefs for platform support start getting out of hand if you ask me. On the other hand, once all the issues uncovered here are fixed I can ask contributors to add a new board to .travis.yml when they add a platform to library.json.

Btw, the ESP8266 Arduino Core Arduino.h has this

#ifdef __cplusplus

#include <algorithm>
...
using std::min;
using std::max;
...
#define _min(a,b) ({ decltype(a) _a = (a); decltype(b) _b = (b); _a < _b? _a : _b; })
#define _max(a,b) ({ decltype(a) _a = (a); decltype(b) _b = (b); _a > _b? _a : _b; })
thijstriemstra commented 3 years ago

First and foremost thank you so much for hanging in there and working through this

Thanks. Any feedback on my other comments?

marcelstoer commented 3 years ago

I am closing this as I switched to GitHub Actions with 435ded3c222f17e64d102def76c30f411b890ad6. Then I addressed the issues discussed above in subsequent commits: https://github.com/ThingPulse/esp8266-oled-ssd1306/compare/435ded3c222f17e64d102def76c30f411b890ad6...00dc3ff9039b7b4b2bfb08a22e9ce259d55b20e4

Thanks again for your preparatory work and valuable input.