SimplyPrint / OctoPrint-SimplyPrint

Official SimplyPrint plugin
GNU General Public License v3.0
11 stars 1 forks source link

M117 `branding` option not working + short vs. long `[SP]` #39

Closed AlbertMN closed 1 year ago

AlbertMN commented 2 years ago

The M117 branding option doesn't seem to be working. The setting is there in the rewrite now, and changing it successfully sends the "update" message via WS to the printer, which I, log-wise, have confirmed is received. But [SP] keeps being there even if the M117 branding setting is disabled.

It's very easily replicatable. Just go to; https://rewrite.simplyprint.io/panel/settings and change the "The printer screen" settings in the bottom

plugin_SimplyPrint (1).log

Bonus fix; seems like only the short [SP] is implemented. In the old code, it used the longer [SimplyPrint] when it had space enough (just defined by an argument). The longer [SimplyPrint] is 14 characters when you add the appended space, so I figured we could always show the long one if length of {message_to_display} <= 12 (if there's space for 26 characters on a normal LCD screen, which is what I recall, but I'll have to test it tomorrow)

jneilliii commented 2 years ago

Short vs long branding currently is configured as a flag to the set_printer_display callback. Is the goal here to now calculate that based on message length?

Relative to the reported bug, is this something that's changed how you're sending the settings, because what I'm seeing is this message in my plugin log...

2022-09-30 17:38:49,902 - octoprint.plugins.simplyprint - DEBUG - Unknown event: {"type":"printer_settings","data":{"has_psu":0,"has_filament_sensor":0,"display":{"enabled":1,"branding":1,"while_printing_type":0,"show_status":1}}}

My understanding of the logic in the current plugin implementation is that type should be demand and printer_settings is part of that payload. I could easily move where this is happening in the code, but want to make sure what I'm seeing is accurate.

AlbertMN commented 2 years ago

Hmh, that'd explain it. I haven't changed that method in a long time, so let's change it to non-demand

And yeah, I'm thinking a "calculate based on message length" would be the most smooth implementation.

jneilliii commented 2 years ago

Ok, well you let me know what that length of message is and I'll make those changes for calculated.

jneilliii commented 2 years ago

Seems you might also not be sending all the same data as before in that payload. Getting key error on updated_datetime missing after moving it to an event, please advise if that needs to be added on your side or removed from the plugin side?

jneilliii commented 2 years ago

I have the settings saving properly now with the above commit and removed that one property from the _sync_settings_from_simplyprint function. After further debugging had to change some of the settings logic to handle received 0/1 values rather than True/False. Will make further adjustments relative to short branding vs long branding once I get that length requirement.

jneilliii commented 2 years ago

adjusted the short branding options to account for anything longer than 7 characters along with the force parameter.