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

Optimize graphical display with selective rendering #5288

Merged
merged 5 commits into from
Nov 26, 2016

Conversation

thinkyhead
Copy link
Member

@thinkyhead thinkyhead commented Nov 24, 2016

The draw loop wastes a lot of cycles because it re-calculates the whole display 2 or more times per screen render. This PR employs two strategies to reduce rendering time:

  • Add bounds-checking so that only items within the current page being rendered are drawn.
  • For XYZ —overlapping both halves of the screen— the value strings are cached.
  • Use a frame with normal text instead of a solid box with inverse text.
  • Reduce the delay between bytes sent over SPI by 5-6µs.

Added up, these optimizations save at least ~19ms (380,000 cycles) on screens with 2 stripes.

Additionally:

  • Use uint8_t for u8g coordinates. (u8g also supplies a u8g_uint_t type)
  • Use const for function arguments and locals, where appropriate.

I look forward to your test results!

@Sebastianv650
Copy link
Contributor

This one is better than #5279: It saves 2ms (82.5 to 80.5ms) on my printer.

@thinkyhead
Copy link
Member Author

thinkyhead commented Nov 24, 2016

2ms is significant! That's 32,000 cycles.

@olikraus suggests we can do even better with u8g2. But it is still a work-in-progress…

Please read olikraus/u8glib#447 (comment)

@thinkyhead
Copy link
Member Author

thinkyhead commented Nov 24, 2016

Also @yhfudev take note! U8g2 apparently has some helpful features that may make your work easier. Specifically:

  • "UTF-8 support, Chinese fonts are already included, limit of 256 glyphs per font removed."

Proper UTF-8 support means we don't have to do so much remapping.

  • Very fast, LiquidCrystal like char based API (U8x8) if graphics output is not required.

Only the Info screen needs graphics. Other screens can take advantage of this optimization.

olikraus/u8glib#447 (comment)

@AnHardt
Copy link
Member

AnHardt commented Nov 24, 2016

Very good!

Todays tests:

Todays test of the GLCD enhacements

RCBugFix Status screen
2 Stripes 77ms
4 Stripes 86ms
8 Stripes 110ms

Selective rendering
2 Stripes 75ms (-2ms)
4 Stripes 76ms (-10ms)
8 Stripes 85ms (-25ms)

Selective + unroll
2 Stripes 78ms (+1ms)

Selective + unroll + decrease delay
2 Stripes 76ms (-1ms)

Selective + decrease delay
2 Stripes 73ms (-4ms)

Detailed timings in the commits comments. (AnHardt#67)

Conclusion:
Selective rendering is a small win for the two stripe ST7920 but makes a huge difference for displays with more stripes. Go for it.
Unrolling the loops seems to make the updates a tiny bit slower. Don't merge.
Decreasing the 10µs-delay to 5Ms speeds up ~2ms per complete update. Today i could not go below 5µs. If we should do that - with the original 10 µseconds and a big fat warning - decrease on your own risk.

@ghost
Copy link

ghost commented Nov 24, 2016

I also tested.

Selective + 10μs delay Selective + 5μs delay Selective + 4μs delay Selective + 3μs delay
2 Stripes 2 Stripes 2 Stripes 2 Stripes
74 ~ 75ms 73 ~ 74ms 72 ~ 73ms 72 ~ 73ms
Displaying is broken It's displayed properly It's displayed properly , but bit noisy when encoder is rotated It's displayed properly , but bit noisy when encoder is rotated Displaying is broken
dscn1749

A branch that used for testing: https://github.com/esenapaj/Marlin/tree/glcd-testes

And, I created a branch that it doesn't include benchmark and 5μs patch and tested. https://github.com/esenapaj/Marlin/tree/glcd-testes2
Displaying is broken. It's displayed properly.
Video clip:

@AnHardt
Copy link
Member

AnHardt commented Nov 25, 2016

@thinkyhead
Would you pleas check AnHardt#67 (comment).

@AnHardt
Copy link
Member

AnHardt commented Nov 25, 2016

@esenapaj
Please reset the display (shut down power completely) and retest Selective with the normal 10µs delay. Simply rebooting the Arduino sometimes does not work when the delays previously have been too short..

@ghost
Copy link

ghost commented Nov 25, 2016

@AnHardt
Sorry and thanks for the advice, now I've re-tested and confirmed that both this PR and this PR + 5μs patch works without any problems.

@ghost
Copy link

ghost commented Nov 25, 2016

@AnHardt
I've tested your "frame" solution.

Selective + 5μs delay + "frame"
2 Stripes
58 ~59ms (-19ms)
It's displayed properly.

Selective + 4μs delay + "frame"
2 Stripes
58 ~59ms (-19ms)
It's displayed properly.

Selective + 3μs delay + "frame"
2 Stripes
57 ~58ms (-20ms)
Displaying is broken.

A branch that used for testing: https://github.com/esenapaj/Marlin/tree/glcd-testes3

@AnHardt
Copy link
Member

AnHardt commented Nov 25, 2016

Tomorrow i'll make some experiments with 'hollow' extruder and sd-card symbols.

@thinkyhead
Copy link
Member Author

thinkyhead commented Nov 25, 2016

These are really encouraging results! A savings of 19ms frees up 304,000 CPU cycles. Wow.

Can you also test with the options USE_BIG_EDIT_FONT and USE_SMALL_INFOFONT to make sure the display still works correctly, including menus and edit items?

Also, try the display with and without LCD_SCREEN_ROT_180. Oliver suggested that the page_y0 and page_y1 values might be reversed in that case.

I've added an option DOGM_SPI_DELAY_US for tuning that 10µs delay. It's in Configuration_adv.h.

Drawing the frame instead of a solid box is also very smart. As another alternative I was also thinking of trying pre-clipping the solid box. That is, instead of drawing the whole box, starting at 30 with a height of INFO_FONT_HEIGHT + 2, make sure to only draw the portion that's within the current stripe.

Or… place that part of the display at the very top of the screen instead of in the middle, so it's only ever drawn once (including the XYZ strings).

@ghost
Copy link

ghost commented Nov 25, 2016

I got a compilation error.

error message
In file included from \AppData\Local\Temp\arduino_build_683885\sketch\ultralcd_impl_DOGM.h:46:0,

                 from \AppData\Local\Temp\arduino_build_683885\sketch\ultralcd.cpp:56:

\AppData\Local\Temp\arduino_build_683885\sketch\ultralcd_st7920_u8glib_rrd.h: In function 'void marlin_SPI_delay()':

ultralcd_st7920_u8glib_rrd.h:112: error: '_delay_system_ticks_sub' was not declared in this scope

       _delay_system_ticks_sub((CYCLES_PER_MICROSECOND) * (DOGM_SPI_DELAY_US));

                                                                             ^

@ghost
Copy link

ghost commented Nov 25, 2016

Still got a compilation error

error message
In file included from \AppData\Local\Temp\arduino_build_498533\sketch\ultralcd_impl_DOGM.h:46:0,

                 from \AppData\Local\Temp\arduino_build_498533\sketch\ultralcd.cpp:56:

\AppData\Local\Temp\arduino_build_498533\sketch\ultralcd_st7920_u8glib_rrd.h: In function 'void marlin_SPI_delay()':

ultralcd_st7920_u8glib_rrd.h:112: error: 'delay_system_ticks' was not declared in this scope

       delay_system_ticks((CYCLES_PER_MICROSECOND) * (DOGM_SPI_DELAY_US));

                                                                        ^
, thus I avoid it tentatively.
Selective + 5μs delay + "frame" Selective + 5μs delay + "frame" Selective + 4μs delay + "frame" Selective + 3μs delay + "frame"
without both USE_BIG_EDIT_FONT and USE_SMALL_INFOFONT USE_BIG_EDIT_FONT and USE_SMALL_INFOFONT USE_BIG_EDIT_FONT and USE_SMALL_INFOFONT USE_BIG_EDIT_FONT and USE_SMALL_INFOFONT
without LCD_SCREEN_ROT_180 without LCD_SCREEN_ROT_180 without LCD_SCREEN_ROT_180
2 Stripes 2 Stripes 2 Stripes
58 ~ 59ms 55 ~ 56ms 54 ~ 55ms 54 ~ 55ms
It's displayed properly It's displayed properly but adjustment is needed It's displayed properly but adjustment is needed Displaying is broken
dscn1754 dscn1751 dscn1751
without this PR Selective + 10μs delay + "frame" Selective + 10μs delay + "frame" Selective + 5μs delay + "frame"
USE_BIG_EDIT_FONT and USE_SMALL_INFOFONT without both USE_BIG_EDIT_FONT and USE_SMALL_INFOFONT USE_BIG_EDIT_FONT and USE_SMALL_INFOFONT USE_BIG_EDIT_FONT and USE_SMALL_INFOFONT
with LCD_SCREEN_ROT_180 with LCD_SCREEN_ROT_180 with LCD_SCREEN_ROT_180 with LCD_SCREEN_ROT_180
2 Stripes 2 Stripes 2 Stripes 2 Stripes
- 45 ~ 46ms 39 ~ 40ms 37 ~ 38ms
It's displayed properly It isn't displayed properly It isn't displayed properly It isn't displayed properly
dscn1753 dscn1752 dscn1752

A branch that used for testing: https://github.com/esenapaj/Marlin/tree/glcd-testes4

@jbrazio
Copy link
Contributor

jbrazio commented Nov 25, 2016

@thinkyhead

U8g2 apparently has some helpful features

Last time I checked they were still missing PROGMEM support.

@thinkyhead
Copy link
Member Author

Still got a compilation error

Oops, forgot to push. Should be better now.

@thinkyhead
Copy link
Member Author

thinkyhead commented Nov 25, 2016

@esenapaj Helpful results. So, the rotated screen needs to be accounted for. Apparently there's a way to access the page bounds that will work with any rotation…

u8g.getU8g()->current_page.y0
u8g.getU8g()->current_page.y1

…so I'll try adapting the code to use these.

I can also try to fix the positioning of the smaller font.

Are the glyphs for X Y Z broken with the small font?

@thinkyhead
Copy link
Member Author

thinkyhead commented Nov 25, 2016

they were still missing PROGMEM support

Oliver is being very responsive! He has already submitted this for us:

The AVR-specific optimizations that make u8g faster than u8g2 are still extant, however.

@thinkyhead thinkyhead force-pushed the rc_selective_rendering branch 2 times, most recently from 03a316a to c4532c1 Compare November 25, 2016 18:45
@thinkyhead
Copy link
Member Author

thinkyhead commented Nov 26, 2016

Maybe this?

#if DISABLED(DISPLAY_CHARSET_ISO10646_1)
  #undef USE_BIG_EDIT_FONT
  #undef USE_SMALL_INFOFONT
#endif

That would correspond with:

#if DISABLED(SIMULATE_ROMFONT) \
 && DISABLED(DISPLAY_CHARSET_ISO10646_1) \
 && DISABLED(DISPLAY_CHARSET_ISO10646_5) \
 && DISABLED(DISPLAY_CHARSET_ISO10646_KANA) \
 && DISABLED(DISPLAY_CHARSET_ISO10646_GREEK) \
 && DISABLED(DISPLAY_CHARSET_ISO10646_CN) \
 && DISABLED(DISPLAY_CHARSET_ISO10646_TR)
  #define DISPLAY_CHARSET_ISO10646_1 // use the better font on full graphic displays.
#endif

…and…

// Western only. Not available for Cyrillic, Kana, Turkish, Greek, or Chinese.

@ghost
Copy link

ghost commented Nov 26, 2016

Maybe this?

Maybe so. At least, I've confirmed that USE_BIG_EDIT_FONT works properly, and "top line missing" problem is solved in English just now.
Also LCD_SCREEN_ROT_180 is ok.

About "g", I test now.

@ghost
Copy link

ghost commented Nov 26, 2016

How about?

USE_BIG_EDIT_FONT without USE_BIG_EDIT_FONT
with LCD_SCREEN_ROT_180 with LCD_SCREEN_ROT_180
dscn1772 dscn1773

@thinkyhead
Copy link
Member Author

Looks good. I count 13 pixels tall for the big font. I'll see about getting it better-centered based on that info, and perhaps we can get one less render on 8-stripe displays too.

@ghost
Copy link

ghost commented Nov 26, 2016

It looks like that

#if DISABLED(DISPLAY_CHARSET_ISO10646_1)
  #undef USE_BIG_EDIT_FONT
  #undef USE_SMALL_INFOFONT
#endif

is forgotten.

@thinkyhead
Copy link
Member Author

thinkyhead commented Nov 26, 2016

It looks like that … is forgotten

I'm suggesting that you try using those lines in place of these:

#if DISABLED(MAPPER_C2C3) && DISABLED(MAPPER_NON) && ENABLED(USE_BIG_EDIT_FONT)
  #undef USE_BIG_EDIT_FONT
#endif

@ghost
Copy link

ghost commented Nov 26, 2016

Well... I used it already at test in #5288 (comment)

Perhaps I may be misunderstanding the interaction with you cause by my bad English.

@Blue-Marlin
Copy link
Contributor

Blue-Marlin commented Nov 26, 2016

@@ -52,12 +52,13 @@
 
 #if ENABLED(SHOW_BOOTSCREEN) && ENABLED(SHOW_CUSTOM_BOOTSCREEN)
   #include "_Bootscreen.h"
 #endif
 
-#if DISABLED(MAPPER_C2C3) && DISABLED(MAPPER_NON) && ENABLED(USE_BIG_EDIT_FONT)
+#if DISABLED(DISPLAY_CHARSET_ISO10646_1)
   #undef USE_BIG_EDIT_FONT
+  #undef USE_SMALL_INFOFONT
 #endif
 
 #if ENABLED(USE_SMALL_INFOFONT)
   #include "dogm_font_data_6x9_marlin.h"
   #define FONT_STATUSMENU_NAME u8g_font_6x9

is enough to bring back USE_BIG_EDIT_FONT to RCBugFix. Tested with en and de.
It's the most direct condition we can test. Small and big font are both coded in ISO10646_1 so only when ISO10646_1 is used for the 'main' font it makes sense to use the other.

@ghost
Copy link

ghost commented Nov 26, 2016

Also, I confirmed no problem with kana and kana_utf8.

@thinkyhead
Copy link
Member Author

Perhaps I may be misunderstanding the interaction with you cause by my bad English.

From your statement "It looks like that (code) is forgotten" I thought you meant something else. The words "is forgotten" are quite poetic in this context.

I'm glad to hear that fixes it. I will add that to the PR and merge this thing. We can continue to patch this up in the coming days, but I think it's showing great promise.

@AnHardt
Copy link
Member

AnHardt commented Nov 26, 2016

I'll make a separate PR to fix 'USE_BIG_EDIT_FONT' only.

@ghost
Copy link

ghost commented Nov 26, 2016

The words "is forgotten" are quite poetic in this context.

Oh sorry...

@thinkyhead
Copy link
Member Author

@AnHardt AnHardt mentioned this pull request Nov 26, 2016
@thinkyhead thinkyhead merged commit 668e737 into MarlinFirmware:RCBugFix Nov 26, 2016
@thinkyhead thinkyhead deleted the rc_selective_rendering branch November 26, 2016 13:01
@AnHardt
Copy link
Member

AnHardt commented Nov 26, 2016

And we should check the edit items that need 2 lines to display (because the label is long).

Works.

@AnHardt
Copy link
Member

AnHardt commented Nov 27, 2016

About u8g2:

Last time I checked they were still missing PROGMEM support.

the solution for this is looking like:

#define printP(s) print(F((s)))
#define TEST_STR "Test"

u8g2.printP(TEST_STR);
// or
u8g2.printP("An other String");

Fonts are already in PROGMEM.

Something else missing? - besides of speed?

Last night's patch for u8g2 is a big step forward - but not enough. :-(
https://www.youtube.com/watch?v=c6XYxvCf2Fc

@landodragon141
Copy link

landodragon141 commented Nov 27, 2016

It looks like he just posted another performance update for SW SPI.
I don't think there is a video up yet.

@AnHardt
Copy link
Member

AnHardt commented Nov 28, 2016

u8g vs u8g2 - now maybe a bit too fast - somewhere
https://www.youtube.com/watch?v=ZChlUhMJ3fQ
Downloaded u8g2 ~0:00UTC

@olikraus

if ( b == 0 )
{
  *arduino_data_port &= arduino_data_n_mask;
  for( i = 0; i < 4; i++ )
  {
    *arduino_clock_port |= arduino_clock_mask;      
    *arduino_clock_port &= arduino_clock_n_mask;
          __asm__("nop\n\t");                             ///////////////////////////
    *arduino_clock_port |= arduino_clock_mask;      
    *arduino_clock_port &= arduino_clock_n_mask;
  }
}
else

A nop in both of the 0-loops seems to be enough to have no pixel errors on my display.

@thinkyhead thinkyhead mentioned this pull request Nov 28, 2016
@fiveangle fiveangle mentioned this pull request Sep 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants