Limit LCD refresh while printing to avoid planner stalls

[wavexx]

Limit LCD reinitialization when the planner is busy

LCD operations, and to be precise the lcd_begin() call is extremely costly. When printing complex geometry or infill (such as gyroid) it can starve the planner and cause a stall.

This is also frequently visible during the 7x7 mesh leveling, where each move is individually planned and a single lcd refresh becomes noticeable as it prevents further moves to be scheduled.

We calculate a very rough estimate of the time taken by queued moves in planned_time. This is just overall length at the average nominal speed. If we have less than 250ms available, we skip the current refresh and wait for the next cycle. If a full reset is pending though we keep on probing every second, as it's likely we're printing complex geometry and we should attempt more frequently in order to catch an available time slot.

planned_time() is usually quite optimistic as acceleration is completely ignored, but it's otherwise accurate while printing high-resolution curvy geometry such as gyroid. We only need a true lower-bound anyway, and the function should be fast to avoid overloading the planner. This means we will be refreshing the LCD mostly during slow moves, outer/straight perimeters or long travels.

See issue #1928 for more details. Using just the queue size and/or planned length is not sufficient to prevent stalls and ensure the LCD is still refreshed regularly. LCD functions can still be sped up, so the issue cannot be closed completely with this PR yet.

[thess]

Funny you should do this -- I determined the ReinitLCD logic and support vars was totally unnecessary a couple of years ago. I still remove this re-draw hack from their firmware in custom builds and features. The constant display flickering from the erase-and-redraw procedures was annoying.

[leptun]

I am currently working on a better way to handle the lcd refresh in general. I will also tinker with the frequency at which the lcd is refreshing. The changes are not pushed yet as I haven’t really finished them. The refreshes shouldn’t block the loop function with my changes, avoiding planner starving.

[wavexx]

Note that it's not the refresh in general that creates big problems for the planner. No question that they're slow, but it's only the full re-init that takes enough time to stall.

As of why the lcd is fully re-initialized frequently, is that their internal state does get corrupted. I've seen this on other printers, although never on the mk3 so far.

I'm not entirely sure if that's because of the ribbon cable picking up noise or just low quality parts.

If you say that you never had this, I could also try to run with the reinit disabled.

[leptun]

It was the lcd reinit I was talking about :)

I don’t know why the lcd is reinitialized so frequently. Once a minute is enough and the reiniting when changing menus is kinda useless. I think only when entering (like when it times out) and exiting the status screen should it be reinited.

Still can’t test it though...

[wavexx]

On Mon, Jun 24 2019, Voinea Alex wrote: It was the lcd reinit I was talking about :)

Oh ok then =)

I don’t know why the lcd is reinitialized so frequently. Once a minute is enough and the reiniting when changing menus is kinda useless. I think only when entering (like when it times out) and exiting the status screen should it be reinited.

I never watched the LCD long enough to see how/when it corrupts exactly. What I can easily do is disable reinit completely and see how long it takes to happen.

Still can’t test it though...

As I happened to hit this problem by chance while doing other stuff with gyroid, I can wait :) Since you did a lot on this already you're in a better position to continue. Let me know if you have something you want some testing on.

[leptun]

I pushed to MK3_LCD_FAST branch.
My changes do as follows:

All lcd communications were moved to a separate planner. It has 8 blocks which can be filled with commands or characters for the lcd. These blocks are added to the ring buffer by the lcd_plan_data function. If the buffer is full, it will wait like before, but this should impact the mail loop much less since it only gets filled when lcd characters are printed to the lcd. lcd_begin() should stop the lcd from being updated untill the reset is finished, resulting in no downtime during reset.
The buffer has its own lcd_isr() ran alongside the stepper isr() that reads from the buffer, executes the content (prints data/commands to the lcd) and calculates when next command can be executed and saves it to the next block, regardless if it's filled or empty.

As for other improvements:

• VT100 codes support has been disabled to save flash and sram and can be enabled with define in Configuration.h. It is not properly implemented and only uses precious flash.
• RW support was removed since it isn't used on prusa printers and on most other lcds.
• 8bit bus support is still there, but it is only enabled when LCD_PINS_D4-7 are defined in pins_...board....h
• The lcd library no longer uses arduino pinMode and digitalWrite functions and uses fastIO macros instead.

If the changes I made still work (which I hope they do), it should fix the issue #1928
I would be greatful if @wavexx could test these and report back. Be prepared for some debugging :)

Will address lcd refreshes at a later time

[wavexx]

Can you open issues on your fw fork? I'd like to make a few comments and I have no idea where to put those. I would really avoid putting the lcd isr in timer1. At low speeds with LA any delay will kill advance steps, on fast moves when dual/quad stepping kicks in the timing gets really tight. And it's not like serial I/O that needs to drain a buffer quickly.

I think timer2 should be fast enough for this.

[leptun]

here you go:
leptun#1

[haarp]

Interesting, so #1406 is related to the LCD after all.

@leptun You might also wanna take a look at #1445

[leptun]

@haarp It’s interesting that other people attempted this before. I looked at #1445, but I like my approach more. Anyway, who am I to judge... :)

[wavexx]

I'm closing this for now. The current LCD implementation is vastly superior and doesn't delay the planner enough to warrant extra checks.