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
2k stars 638 forks source link

Add drawIco16x16, fix updateInterval overflow, optimize mult. operations #236

Closed ElderJoy closed 5 years ago

ElderJoy commented 5 years ago
  1. Add drawIco16x16 and setPixelColor functions
  2. Optimize operations mult. 2, 4, 8
  3. Fix updateInterval overflow when frame rate lower than 8
stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

ElderJoy commented 5 years ago

Anybody still support this library? Please apply this fix.

marcelstoer commented 5 years ago

We can't, there are merge conflicts in four files. Please resolve them so we can take a look afterwards.

ElderJoy commented 5 years ago

Hi @marcelstoer,

Thank you, I didn't mention on merge conflicts. Now I resolved all conflicts and it looks ready to merge into master. Please take a look on it.

ElderJoy commented 5 years ago

One of the reasons is that I used your library (with my changes) in https://github.com/xoseperez/espurna.git project. So, now it has dependency to my github clone of your library. I would prefer to merge my changes into original library and setup dependency in espurna project to it.

marcelstoer commented 5 years ago

Thanks for the contribution, they look sane. We would appreciate if individual bug fixes or improvements were offered as separate MRs (3 in your case). That makes it easier to reason about each one separately.

ElderJoy commented 5 years ago

Thank you for merging my fixes. Agree with you as for individual commits.

DaveSprague commented 4 years ago

It appears that the modifications to change the multiplications by small powers of 2 were based on a version of the code before my merge [7341361aa92d8db9edf155bcbbc26a9c83b27300] that fixed the circle drawing algorithm (replacing the pre-increments and pre-decrements with post-increments and post-decrements. The result is that the circle drawing routine once again draws circles that are squashed. I'll submit another pull request to fix that again.

DaveSprague commented 4 years ago

Here's an image from this merge showing that when the multipications by 2 were replaced by left-shifts, the post-increments and post-decrements were inadvertently changed to back to pre-increments and pre-decrements which causes the circles to not be drawn correctly.

What's the best way to fix this? If you want me to submit another pull request for this fix, I'll do that.

drawCircleChanges

marcelstoer commented 4 years ago

If you want me to submit another pull request for this fix, I'll do that.

That would be great. Thanks for the analysis.

DaveSprague commented 4 years ago

In looking at this further looking at this today. In addition to the reverting of the post-increments/decrements back to pre-increments, there are two other problems with these code changes: 1) multiplication is higher precedence than addition but bit shifting is lower precedence than addition so in order for the computations to work correctly with the shifts replacing the multiplications you have to enclose them in parentheses, i.e. instead of: (x++) << 1 + 3 //shifts by 4 you need to write it as: ((x++) << 1) + 3 // shifts by 1 and then adds 3 to result.

2) I did testing on an ESP8266 type board (NodeMCU) as well as on an Arduino Uno and a Adafruilt Feather M0 and there were no cases where the << bit shift was faster than multiplication. In fact, for the NodeMCU (8266) board, the multiplication by two was significantly faster than left shift by 1. I'm not sure why this is the case but it probably has to do with the compiler already doing some optimizations to speed up multiplications.

As a result, I'm going to submit a pull request to revert the circle drawing algorithms back to the way I had them in my earlier pull request (https://github.com/ThingPulse/esp8266-oled-ssd1306/pull/197) with the post-increments/decrements and with multiplications instead of bit shifting. I'm not planning to go through and revert the other places where multiplication was replaced with bit shifting in this pull request -- just the ones in the circle calculations.

helmut64 commented 4 years ago

Todays compilers optimise the code, a manual bit shifting is not needed anymore. For putting in the right math code provides better documentation and and the compiler will optimize it.

marcelstoer commented 4 years ago

@helmut64 I fully agree with you. That's one of the reasons I was so reluctant to merge this in the first place. If @ElderJoy had hashed out his changes in three individual PRs as I suggested I would have rejected that 2nd one:

Optimize operations mult. 2, 4, 8

ElderJoy commented 4 years ago

Sorry, just saw this topic. If it needed, I can prepare pull request to revert back rest optimization changes I've made.

marcelstoer commented 4 years ago

Having explicit multiplications and divisions rather than bit shifts in OLEDDisplay.cpp would be preferable IMO. So, yes a PR to that effect would be welcome 😄

DaveSprague commented 4 years ago

Hi Dmitry,

You should make sure to start from the current version of OLEDDisplay.cpp and put your revisions onto it. I've already revised the shifts back to multiplies in the three circle drawing functions so no need to change those.

Thanks Dave

On Mon, Oct 14, 2019 at 4:49 AM Marcel Stör notifications@github.com wrote:

Having explicit multiplications and divisions rather than bit shifts in OLEDDisplay.cpp would be preferable IMO. So, yes a PR to that effect would be welcome 😄

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ThingPulse/esp8266-oled-ssd1306/pull/236?email_source=notifications&email_token=AALBSCWNXTRA4GJA2DO45ZTQOQXCBA5CNFSM4G3JP75KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBDYSUI#issuecomment-541559121, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALBSCUY3CIFWA6XBCWGZ33QOQXCBANCNFSM4G3JP75A .

ElderJoy commented 4 years ago

Hi Dave, sure, I've got latest sources. I prepared reverting pull request https://github.com/ThingPulse/esp8266-oled-ssd1306/pull/258 take a look at it please.

DaveSprague commented 4 years ago

HI Dmitry, I checked it and it appears to be correct. Thanks,

Dave

On Tue, Oct 15, 2019 at 7:14 AM DmitryBlinov notifications@github.com wrote:

Hi Dave, sure, I've got latest sources. I prepared reverting pull request

258 https://github.com/ThingPulse/esp8266-oled-ssd1306/pull/258 take a

look at it please.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ThingPulse/esp8266-oled-ssd1306/pull/236?email_source=notifications&email_token=AALBSCSCYGB5VQCTVBE64ETQOWQYTA5CNFSM4G3JP75KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBIL2HA#issuecomment-542162204, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALBSCWFI4WH5YOARDMALWLQOWQYTANCNFSM4G3JP75A .