Change ST9720 delay settings priorities

[dotdash32]

Description

Changes priority of psuedo-switch statement of ST9720 display delays in pins_postprocess.h.

Used to use LCD, then BOARD, then CPU defined timings, but this seems to only ever use LCD timings, and not allow for specific board based overrides.

This new arrangement works on my system (SKR2, CR10_STOCKDISPLAY, but requires verification for other boards/LCDs.

If this does not work, may require logic to switch between whichever delay is longest between the three. At current, unsure if this is necessary or possible to do a preprocessor comparison of DELAY_NS() functions.

Requirements

Requies U8GLIB_ST7920 display and board with different delay timing requirements

Benefits

Better handling of display timing delays.

Configurations

Related Issues

See #22710

[thisiskeithb]

This new arrangement works on my system (SKR2, CR10_STOCKDISPLAY, but requires verification for other boards/LCDs.

The old code worked on my CR10_STOCKDISPLAY + SKR2 builds, but this also seems to work fine on the same hardware combination.

I haven't tested these changes on other board/LCD combos.

[thinkyhead]

It is intentional that the "LCD" values in Conditionals_LCD.h should take precedence over the more general "BOARD" values, which take precedence over the most general "CPU" values. Maybe you want to review the "LCD" values instead, or look at the rationale for those values and figure out what they mean, and post queries to the original authors of those changes before jumping ahead and changing the order of precedence.

[dotdash32]

If you can direct me to the written docs on this, that would be great. I can't find anything attached to the original PR about the reasoning behind the preference order. In the linked issue on the page, there is talk of the delays being more closely related to the boards rather than the displays (which I tried to replicate here).

I believe a potentially better fix than the current one in this PR is to choose the longest delay for the situation. It seems like in this situation, the simplest fix is to just use a user defined value in a top level config, but I'm unsure if that is fully accessible to all users, and doesn't address the root of the issue.

Looking through the code base, the "LCD" values have only 2 values in Conditionals_LCD.h, which are in general lower than the ones I've found for "BOARD" or "CPU". The results on hardware tests also seem to be inconsistent, based on thisiskeithb's response of a similar system working before and after. My base assumption based on ellensp's response on my bug report is that longer delays will tend to be more stable, so that is the preference when necessary.

At this point, it seems like the longer term solution is to choose the most appropriate delay, which intuitively seems like the longest value. While the current situation is to choose the value with the highest precedence, it doesn't seem to work 100% of the time. Particulary, the SKR2 needs a 580 ns delay for ST7920_DELAY_3, while the standard is only 125.

One potential solution is to redefine XXX_ST7920_DELAY_N from DELAY_NS(yyy) to just the numerical value, yyy, and then in pins_postprocess.h change the if statements to something like

#define PRE_ST7920_DELAY_1    (MAX(MAX(BOARD_ST7920_DELAY_1, LCD_ST7920_DELAY_1), CPU_ST7920_DELAY_1))
#define ST7920_DELAY_1        DELAY_NS(PRE_ST7920_DELAY_1)

I can do some testing to see if this works (setting delays as {125, 125, 580}), using just a user override, but would appreciate feedback on whether this flow fulfills the original intention.

Also apologies for the strongly worded PR title, it carried over from my convcommit category.

[thinkyhead]

Digging through the line history and git blame, the original concept of overriding the delays at the LCD level comes from #7824 by @jmdearras. It would seem to make sense that longer delays should take precedence over shorter ones, but that would mean redefining all the delays as integers representing nanoseconds instead of having them wrapped in DELAY_NS and then applying _MAX.

[thinkyhead]

Ok, following our notional consensus the default delays are now defined as integers, applying _MAX to pick the longest delay from the list. Users can still define their own overrides with DELAY_NS so that no one needs to update their documentation.

[dotdash32]

I was going to offer to help change the defines, but it looks like you got it covered, that was fast! Thanks.

I did some preliminary testing, and my system works when increasing delay (delays of {125 125 580}, as well as {1000 1000 1000}). So while a small scale, I believe this method should be robust. This appears to be supported based on the code using this to just slow down the data stream, so slower data should improve display stability, with (hopefully) minor impacts on general code execution time.

[dotdash32]

Whoops, misread the program flow, realized while I was out. The #ifndef doesn't need an else to define, since the else situation is that it is defined. Should be fixed (back at your original flow).

[thinkyhead]

The conceptual shift works for me. In retrospect the LCD_ST7920_DELAY... seems like it should be the best of all ideas, since the display timing should be most closely linked to the LCD, not to the MCU or the board. It may be that the general HAL Delay.* code was just not as robust and consistent across MCUs in the past, but should be in its best state now. At some point we'll have to do a more thorough audit of all the boards and LCDs that Marlin supports and get all the relevant information about them catalogued so we have a quick reference. It will help to be able to quickly look up what interface they use, the mode and data rate for SPI displays, the most reliable ST7920 delay values, and so on.