hd44780: allow to configure line length

[mhier]

This allows to use 16x4 displays rather than only 20x4.

Signed-off-by: Martin Hierholzer hier@beta-centauri.de

[KevinOConnor]

Thanks. In general it looks good to me. Some comments:

• What display/printer is this needed for?

• I don't think an arbitrary "line_length" is going to work well in practice (ie, any value other than 20 or 16). I wonder if adding a new lcd_type: hd44780x16 (or similar) may be more appropriate.

• Wont this also enable the _default_16x4 display group with the common st7920 glyph layout? (I guess there isn't a harm in that though.)

-Kevin

[mhier]

* What display/printer is this needed for?

My printer is a Renkforce RF1000 (cf #3496). There are more things to be added to Klipper to support it, and I intend to make more pull requests for it. This one is the simplest, just to get started (and understand the workflow) :-) The RF1000 has a 16x4 display.

* I don't think an arbitrary "line_length" is going to work well in practice (ie, any value other than 20 or 16).  I wonder if adding a new `lcd_type: hd44780x16` (or similar) may be more appropriate.

There are different lengths possible. I have an 40x2 display lying around (unrelated to 3D printing), which of course would also require to modify the number of lines (doable but not required right now). I personally would not restrict things unnecessarily. If you think this is too permissive, I could also restrict the line_length to be either 16 or 20 - this would allow to quickly extend to different line lengths if needed.

Wouldn't a new lcd_type require an extra module? I wouldn't like to copy the module code...

* Wont this also enable the `_default_16x4` display group with the common st7920 glyph layout?  (I guess there isn't a harm in that though.)

Not sure, what you mean by this. I am using this with the _default_16x4 display group, since the 20x4 would render too many characters. It seems to work fine, although I might not have a reference how it should look like ideally...

[KevinOConnor]

I personally would not restrict things unnecessarily. If you think this is too permissive, I could also restrict the line_length to be either 16 or 20 - this would allow to quickly extend to different line lengths if needed.

Makes sense. It's minor, but I think restricting to 16 or 20 would be worthwhile - just because I suspect other values would currently cause non-working results and may confuse users. FYI, one can use something like config.getchoice('line_length', {"16":16, "20":20}, "20") to validate the option.

Wouldn't a new lcd_type require an extra module?

FYI, it is possible to share the code - an example is in uc1701.py. That said, as above, I'm okay with keeping line_length.

Not sure, what you mean by this. [...] It seems to work fine

Ah, okay. I just wasn't sure if there would be visual quirks using _default_16x4 on the hd44780. If it works, then it works.

Separately, this will conflict with #3545 - I'll probably end up merging that one first.

-Kevin

UPDATE: Fix incorrect reference to #3545 .

[mhier]

I personally would not restrict things unnecessarily. If you think this is too permissive, I could also restrict the line_length to be either 16 or 20 - this would allow to quickly extend to different line lengths if needed.

Makes sense. It's minor, but I think restricting to 16 or 20 would be worthwhile - just because I suspect other values would currently cause non-working results and may confuse users. FYI, one can use something like config.getchoice('line_length', {"16":16, "20":20}, "20") to validate the option.

Ok thanks, it's done like this.

Ah, okay. I just wasn't sure if there would be visual quirks using _default_16x4 on the hd44780. If it works, then it works.

Just to verify, this is how the display looks like in idle position:
https://cloud.beta-centauri.de/index.php/s/RqN3o5PERnFnKAX

The special glyphs look fine. The layout looks like I would expect it from the configuration file.

Separately, this will conflict with #3545 - I'll probably end up merging that one first.

Sure, thanks for the heads up.

[mhier]

A simple rebase resolved the conflict. I noticed that the documentation was not yet reflecting the restriction to 16 or 20 characters, I fixed that, too. I think this is now good to go.

[KevinOConnor]

Thanks.

-Kevin