Marlin: RC8 reprapdiscount full graphic smart controller problem

[playlet]

Hi!

I just tried the latest release and there is a strange thing going on with this lcd display.

RCBugFix from Nov 27 that I installed has full text bacground shading and scrolls fast through Control - Motion menu.

RC8 only has 2 lines - top and bottom (indicating the selected item). That might be intentional but the following is definitely strange. When you scroll through the menu items (Control - Motion menu), only the first item gets updated fast, the rest are frozen and break up until you stop scrolling the encoder which is when everything gets updated on the lcd again and works ok.

Scrolling through the menu generally feels laggy compared with the previous bugfix version I tried (Nov 27).

Is this a bug or a "feature"? :)

Using prusa i3: Mega2560, RAMPS 1.4.

Best regards,

Ivan from Croatia

[playlet]

If I undo these changes: https://github.com/MarlinFirmware/Marlin/pull/5389/files
... everything works fast & responsive & beautifully again. :)

[esenapaj]

I can't reproduce the broken third line, but I see that refresh of GLCD looks like slowly.
And I also revert PR #5389, problem seems to be solved.

[playlet]

All text lines (except for the first one) are not updating as you scroll the menu and are updated only after the encoder stops rotating. If you scroll a menu that has more than 5 items (like motion control menu) in a medium slow manner, everything below the 1st text line gets jumbled up. If you do it fast, it only updates the screen when you stop. If you rotate the encoder very slowly (1 item per second), then it works ok.

For me, redrawing the lcd takes about 0.5 seconds so yeah, very slow. :)

Thanks for looking into this! :)

[AnHardt]

A little delay in between the updates of the single stripes of an entire screen is the purpose of #5389.

It makes the unbreakable portions of the screen-update smaller and introduces breaks where the planner is able to refill the planner buffer. This ought to prevent the planner buffer running empty during the screen updates (stuttering).

If we'd always update the complete screen, even if we know the actual screen content has already changed, the lag, until the 'right' content is displayed, would be even larger.

[AnHardt]

You could try

@@ -2756,11 +2756,15 @@ void lcd_update() {
     }

   #endif //SDSUPPORT && SD_DETECT_PIN

   millis_t ms = millis();
-  if (ELAPSED(ms, next_lcd_update_ms)) {
+  if (ELAPSED(ms, next_lcd_update_ms)
+    #if ENABLED(DOGLCD)
+      || drawing_screen
+    #endif
+    ) {

     next_lcd_update_ms = ms + LCD_UPDATE_INTERVAL;

     #if ENABLED(LCD_HAS_STATUS_INDICATORS)
       lcd_implementation_update_indicators();

This will minimize the lag, but gives the planer only one single chance, in between of the stripes, to refill the buffer. If that has still the wanted effect is questionable and needs retesting.

[AnHardt]

An more adjustable alternative could also be:

@@ -2865,11 +2869,11 @@ void lcd_update() {
       #if ENABLED(ULTIPANEL)
         currentScreen == lcd_status_screen &&
       #endif
         !lcd_status_update_delay--
     ) {
-      lcd_status_update_delay = 9;
+      lcd_status_update_delay = (1000 - LCD_UPDATE_INTERVAL) / LCD_UPDATE_INTERVAL;
       lcdDrawUpdate = LCDVIEW_REDRAW_NOW;
     }

     if (LCD_HANDLER_CONDITION) {

and decreasing LCD_UPDATE_INTERVAL in ultralcd.h.
Currently there are 100ms -> 10 time slots per second.
Decreasing to 50 would mean maximum lag would decrease from 400 to 200ms and 20 time slots.
Don't go below 20ms - else the effect will be about the same as https://github.com/MarlinFirmware/Marlin/issues/5403#issuecomment-265203647.

How does it feel to you with

  #define LCD_UPDATE_INTERVAL 50

or

  #define LCD_UPDATE_INTERVAL 25

?

[thinkyhead]

User experience is the first priority. It is the main thing that gives an impression whether or not we know what we're doing. Let's make sure this is patched, and soon.

[playlet]

Here is an example how this looks (video). Veeeeery slow. :) This is not in any way a deal breaker for me, it just feels kinda funny to use. You have to stop scrolling to see where you're at so it takes some time. Especially when you need to find that one setting buried deep in the menu far far away. :)
https://youtu.be/Y1HH9EZUPWs

@esenapaj your fork feels quite nice. I can't complain at all. It is the same as @AnHardt 's first example. There is some small text breakup but fine with me. I pick this one although I don't know if this is good for the printing buffer and all that important stuff I don't know much about...

@AnHardt first example is quite good (take a look at my comment for esenapaj above). Your second example with update interval at 50 has the same text breakup like the release version but is a tiny bit faster at refreshing the lcd. At 25 the breakup is better but still very noticeable. Refresh rate is fast enough to look for that one setting in a menu and actually see where you're at although still looking a bit funny. I suppose I could live with it.

I will let you guys decide what is best for the official patch. Maybe there is yet another idea to be discovered. Thank you for taking the time to work on this. :)

[AnHardt]

@thinkyhead
What user experience do you mean? With printing short fast moves or with playing with the display?
Like so many things in Marlin it's a a compromise und in this case it's relative easy to adjust it to the one or the other side. And maybe, if we can't find a one size fits all adjustment we need to make it configurable.

I suggest you try it by your self when you have access to your graphical display again.
I'd be exited to hear from someone who had the stuttering problem before but does not use ENSURE_SMOOTH_MOVES where the default definitions are also not ideal - for a delta for example.

Calling a well known side effect, which i have talked about before, a bug completely demotivates me to try to do something similar for the character based displays - updating line by line and bring down the time for a single line below of 10ms.

Currently the graphical displays should perform better than the character based ones - if you compare stuttering.

[thinkyhead]

What user experience do you mean?

All of the above. I'm doing my best Steve Jobs impression here.

Of course, I do understand that there is some compromise involved. My hope is that, nevertheless, we may be clever enough to accomplish both reliable movement and a pretty display. Or at least minimize the ugly. I realize that with the current approach we can't ensure that delays won't occur in-between stripes. And, when we re-draw the screen for controller events, we do have to start over. I just have this gut feeling that just out of reach there's an insanely great idea we haven't thought of yet.

[thinkyhead]

updating line by line and bring down the time for a single line below of 10ms.

That seems like an interesting notion too. I'm surprised to hear that now there's actually _more_ stuttering with character displays than graphical ones. A testament to the effectiveness of our optimization. But I was also under the impression that character displays were speedy, only needing to send 80 bytes per-update.

Is this a case where we could use an 80-byte screen buffer and just send bytes to the display one line at a time as a side-task?

[esenapaj]

By the way I'm not sure this is related to PR #5389 or other recent GLCD tuning or specification of ENSURE_SMOOTH_MOVES…

But when ENSURE_SMOOTH_MOVES is enabled and ALWAYS_ALLOW_MENU is disabled, screens of M600 don't display properly.

First screen is broken:

…and screens of "Wait for filament load" and "Wait for filament extrude" are skipped at least.

But when both ENSURE_SMOOTH_MOVES and ALWAYS_ALLOW_MENU are enabled, It looks like that those display properly.

[AnHardt]

Todays RCBugFix with REPRAP_DISCOUNT_SMART_CONTROLLER and activated bed takes about 23-24ms in a single block. REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER takes about 60ms in 4 chunks of about 15ms. The trick is to refill the planner buffer in the breaks between the chunks. With https://github.com/MarlinFirmware/Marlin/issues/5403#issuecomment-265203647 only one command can be refilled (see https://github.com/MarlinFirmware/Marlin/issues/5405 ). With https://github.com/MarlinFirmware/Marlin/issues/5403#issuecomment-265231490 there is some more time to refill the buffer until LCD_UPDATE_INTERVAL gets <= then it takes to draw and transfer one stripe (15ms).

[thinkyhead]

Calling a well known side effect, what i have talked about before, a bug

Took me a while to parse this. I didn't mean to call the side-effect a "bug." But users might, because they're not as savvy as we. My only consideration is to minimize the impression of a malfunction.

[soimy]

It would be great to have this fixed.
I just upgraded from RC7 and find high responsive lag very annoying. When changing values quickly from LCD menu, the number just unable to update until you stop changing it.

[thinkyhead]

find high responsive lag very annoying

Agreed. User experience should be our highest priority, and a responsive controller is essential to that.

I find this issue to be highly motivating. I will be back with my printers and graphical LCD starting tomorrow, and will continue to work on this in earnest.

[thinkyhead]

when ENSURE_SMOOTH_MOVES is enabled and ALWAYS_ALLOW_MENU is disabled, screens of M600 don't display properly.

We'll have to look at that closely. The conditions are determined by:

#if ENABLED(ENSURE_SMOOTH_MOVES) && ENABLED(ALWAYS_ALLOW_MENU)
  #define STATUS_UPDATE_CONDITION planner.long_move()
#else
  #define STATUS_UPDATE_CONDITION true
#endif
#if ENABLED(ENSURE_SMOOTH_MOVES) && DISABLED(ALWAYS_ALLOW_MENU)
  #define LCD_HANDLER_CONDITION planner.long_move()
#else
  #define LCD_HANDLER_CONDITION true
#endif

And the way planner.long_move() works is:

if (blocks_queued()) {
  return block_buffer_runtime_us > (LCD_UPDATE_THRESHOLD) * 1000UL + (MIN_BLOCK_TIME) * 3000UL;
}
else
  return true;

What about this would prevent the screen updating? Could LCD_UPDATE_THRESHOLD or MIN_BLOCK_TIME be too large?

@esenapaj Does it make any difference if lcd_goto_screen changes like…

- lcdDrawUpdate = LCDVIEW_CALL_REDRAW_NEXT;
+ lcdDrawUpdate = LCDVIEW_CLEAR_CALL_REDRAW;

…?

[esenapaj]

- lcdDrawUpdate = LCDVIEW_CALL_REDRAW_NEXT;
+ lcdDrawUpdate = LCDVIEW_CLEAR_CALL_REDRAW;

I've tested just now, it does not improve anything, and it does not seem to make anything worse.

[esenapaj]

Could LCD_UPDATE_THRESHOLD or MIN_BLOCK_TIME be too large?

When I set those to

  #define LCD_UPDATE_THRESHOLD 0
  #define MIN_BLOCK_TIME 0

, First screen display properly but "Wait for filament unload" and "Wait for filament load" and "Wait for filament extrude" are still skipped.

[thinkyhead]

"Wait for filament unload" and "Wait for filament load" and "Wait for filament extrude" are still skipped.

Could it be that lcd_clicked is never getting cleared when ALWAYS_ALLOW_MENU is disabled? Try adding:

  void lcd_goto_screen(screenFunc_t screen, const uint32_t encoder = 0) {
    if (currentScreen != screen) {
+     lcd_clicked = false;

[thinkyhead]

I've tested just now, it does not improve anything

I see we clear the screen in lcd_goto_screen already anyway!

[esenapaj]

  void lcd_goto_screen(screenFunc_t screen, const uint32_t encoder = 0) {
    if (currentScreen != screen) {
+     lcd_clicked = false;

It didn't improve anything unfortunately.

[AnHardt]

M600, with all screens, works always for me if ALWAYS_ALLOW_MENU is defined.
To find out more about the M600 problems i added

        }
    } // LCD_HANDLER_CONDITION
    else
      SERIAL_ECHOLNPGM("LCD blocked!");
  } // ELAPSED(ms, next_lcd_update_ms)
}

The result seems to be a bit scary.
If makes a difference if a do print a 'normal' part or if i print a test part with too short segments by purpose. In the first case the missing screens appear, but not so in the second - i get the blocked messages.

If already in M600 this should not matter. Searching for the reasons.

[Sebastianv650]

I just stumbled across this thread. I have no quick answer what could go wrong with ENSURE_SMOOTH_MOVES and the M600 combination when it works with normal menu and status screen.

But I'm asking myself if we should invest a lot of time into this. The intention of ENSURE_SMOOTH_MOVES was to compensate for slow display updates. If @AnHardt updates (where I see the future) eliminate this issue, I would be glad to revert my work around again - only leaving the option to set a minimum block time as it is at 6ms at the moment. With a block buffer of 16 and enough time between the stripes to refresh the buffer we have at least 6*16=96ms inside the buffer - more than enough to keep alive during a stripe update.

This would also be more universal for all users: The 6ms due to the new block calculation should be very simmilar on all machines, where the LCD refresh time might be different depending on enabled features and so on.

If you agree with this, I would prepare the PR, which could be merged when we found a solution between refresh lag and print performance.

[AnHardt]

Could not find out why/how there can be a condition where he planner buffer is not empty, but the summ of the times in it is zero. Likely one move - active running - so already subtracted.

@@ -391,11 +391,11 @@ class Planner {
         return NULL;
     }

     #if ENABLED(ENSURE_SMOOTH_MOVES)
       static bool long_move() {
-          if (blocks_queued()) {
+          if (blocks_queued() && block_buffer_runtime_us) {
             return block_buffer_runtime_us > (LCD_UPDATE_THRESHOLD) * 1000UL + (MIN_BLOCK_TIME) * 3000UL;
           }
           else
             return true;
       }

seems to help.

[AnHardt]

@Sebastianv650
I doubt 6ms is the right value. It seems to make deltas slower - which means they can plan faster.

The best of the now shorter display updates seems to be - SLOWDOWN seems to work again - the buffer is not drained that sudden anymore.

Yesterday i tried to make some performance benchmarks, but got lost some when - too many parameters with too tiny differences on the result. (https://github.com/AnHardt/Marlin/commits/performance)

[Sebastianv650]

6ms is a worst case value, that's for sure. I wanted to be on the save side - if a print takes a tiny little bit longer that's maybe not a big deal, but if it starts to stutter quality goes down.

If slowdown is working safely in any circumstates again that would be perfect. I will also do some tests as soon as I find a free hour. I'm a little bit blocked printing christmas presents...

[AnHardt]

@Sebastianv650
How did you measure the time?
Did you subtract the time spent here in?

  // If the buffer is full: good! That means we are well ahead of the robot.
  // Rest here until there is room in the buffer.
  while (block_buffer_tail == next_buffer_head) idle();

If not you may have measured the time for planning + a part of the time for stepping an other move + ~1/2 an idle().
Alternatively one could drop these times when the buffer is full.

[thinkyhead]

How did you measure the time?

It's the estimated time of the moves already in the planner. It might be too idealized, and doesn't account for the fact that the moves might accelerate and/or decelerate (i.e., it could be too small). It may also not account for the fact that the current block might be at any point in its processing by the ISR (i.e., it could be too large).

[AnHardt]

@thinkyhead

How did you measure the time?

It's the estimated time of the moves already in the planner. ...

I tried to talk about the 6ms for planning a move - not the sum of times for the moves in the planner buffer. (https://github.com/MarlinFirmware/Marlin/issues/5403#issuecomment-265849324)

[AnHardt]

The result seems to be a bit scary.
If makes a difference if a do print a 'normal' part or if i print a test part with too short segments by purpose. In the first case the missing screens appear, but not so in the second - i get the blocked messages.

It's more a random effect. But it's much more likely when lots of small moves are done.
block_buffer_runtime_us is mangled in the planer and in the stepper-ISR - so needs to be volatile and Interrupt protected. PR will follow.

[thinkyhead]

I tried to talk about the 6ms for planning a move

Ahhhh.

[Sebastianv650]

@AnHardt

How did you measure the time?
Did you subtract the time spent here in?

I'm starting the time measurement just after the while / idle(); line. I'm using the micros() function to get start/end time and to get worst case numbers I do the measurement while the printer is doing print moves with LIN_ADVANCE enabled. Furthermore I'm only counting moves where we have X, Y and E components.

[playlet]

Hi guys!
I have another problem that started just after the move menu was rearranged. Manual bed leveling is impossible to do. It hangs mid homing (lifts z and stops) unresponsively. Problem already described here: https://github.com/MarlinFirmware/Marlin/issues/5518

[SwiftNick]

I have the same issues on my k8400/3DualDrag controller with the Reprap full Graphic display using the latest RC Bugfix.
1) Menu items are marked with top/bottom line rather than inverse text
2) Menu response is slow
3) Occasional corruption of display during menu changes
4) Occasional pause during print (I am printing from SD Card)

It was fine in releases prior to RC8.

I can test specific builds code changes so let me know.

On the plus side, bed levelling seems more consistent now :-)

[Blue-Marlin]

@SwiftNick
Please test RCBugFix - that got several improvements.
Inverse text is slooooow - but if you want it

  // Enable to save many cycles by drawing a hollow frame on the Info Screen
  #define XYZ_HOLLOW_FRAME

  // Enable to save many cycles by drawing a hollow frame on Menu Screens
  #define MENU_HOLLOW_FRAME