Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update DisplayInfoOnLCD.py #18774

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

GregValiant
Copy link
Collaborator

@GregValiant GregValiant commented Mar 31, 2024

Bug fix for "remaining_time".
Affected M118 and M73_time
Bug was pointed out in #18766

Description

This repairs a problem with M118 lines being added if "remaining_time" was not selected. The bug affected M118 and M73_time insertions.
There was a request to add the "A" and "P" parameters to the M118 lines as they might be required by Octoprint.
Adding M117 is now an option.

This fixes... OR This improves... -->

Type of change

  • [ X] Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Cura 5.7beta and 4.13.1

Test Configuration:

  • Operating System: Win 10 Pro

Checklist:

  • [ X] My code follows the style guidelines of this project as described in UltiMaker Meta and Cura QML best practices
  • [X ] I have read the Contribution guide
  • [ X] I have commented my code, particularly in hard-to-understand areas
  • [ X] I have uploaded any files required to test this change

@github-actions github-actions bot added the PR: Community Contribution 👑 Community Contribution PR's label Mar 31, 2024
@GregValiant GregValiant added PR: BooBoos 😇 Suggestions for Typos, like the NoMesh or there is a suggested refactor PR: Post Processing ➕ Like adding beeps, more tunability or different Gcode pause at heights labels Mar 31, 2024
@GregValiant GregValiant marked this pull request as draft April 3, 2024 13:01
@GregValiant
Copy link
Collaborator Author

Most of these changes were requested by #18766 while other are because I'm unable to keep my fingers out of it.
The changes have missed 5.7.0 so I will move it back to active when I eventually quit fooling with it.

Bug fix for "remaining_time".
Affected M118 and M73_time

Update DisplayInfoOnLCD.py

Add options for Time-to-pause to include Filament Change.

Update DisplayInfoOnLCD.py

Make M117 optional.
Combine M73 R and M73 P lines into a single line.
Add A and P parameters for M118 lines.

Update DisplayInfoOnLCD

A TouchUp.

Update DisplayInfoOnLCD.py

TouchUp

Update DisplayInfoOnLCD.py

TuneUp
Should not have been included.
@GregValiant
Copy link
Collaborator Author

I see no reason why Remco and Casper have to be the only ones horrified by my coding style.

@GregValiant GregValiant marked this pull request as ready for review April 7, 2024 13:08
Comment on lines +243 to +249
if add_m118_line:
if add_m118_a1 and not add_m118_p0:
m118_str = "M118 A1 "
if add_m118_p0 and not add_m118_a1:
m118_str = "M118 P0 "
if add_m118_p0 and add_m118_a1:
m118_str = "M118 A1 P0 "
Copy link
Contributor

@HellAholic HellAholic Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be a bit simplified either like below

Suggested change
if add_m118_line:
if add_m118_a1 and not add_m118_p0:
m118_str = "M118 A1 "
if add_m118_p0 and not add_m118_a1:
m118_str = "M118 P0 "
if add_m118_p0 and add_m118_a1:
m118_str = "M118 A1 P0 "
if add_m118_line:
if add_m118_a1:
m118_str += "A1 "
if add_m118_p0:
m118_str += "P0 "

Or, you can check for both conditions being true at the same time in the first if, then use elif for individual conditions.

Comment on lines +303 to +308
if add_m118_a1 and not add_m118_p0:
m118_str = m118_text.replace("M118 ","M118 A1 ")
if add_m118_p0 and not add_m118_a1:
m118_str = m118_text.replace("M118 ","M118 P0 ")
if add_m118_p0 and add_m118_a1:
m118_str = m118_text.replace("M118 ","M118 A1 P0 ")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

elif variation, first checks for both being true, otherwise goes for the other conditions.

Suggested change
if add_m118_a1 and not add_m118_p0:
m118_str = m118_text.replace("M118 ","M118 A1 ")
if add_m118_p0 and not add_m118_a1:
m118_str = m118_text.replace("M118 ","M118 P0 ")
if add_m118_p0 and add_m118_a1:
m118_str = m118_text.replace("M118 ","M118 A1 P0 ")
if add_m118_p0 and add_m118_a1:
m118_str = m118_text.replace("M118 ","M118 A1 P0 ")
elif add_m118_p0:
m118_str = m118_text.replace("M118 ","M118 P0 ")
elif add_m118_a1:
m118_str = m118_text.replace("M118 ","M118 A1 ")

@HellAholic
Copy link
Contributor

HellAholic commented Apr 8, 2024

I see no reason why Remco and Casper have to be the only ones horrified by my coding style.

Did a quick check to share the horror :P But I'm not familiar enough with the code to make a full review on it.

@GregValiant
Copy link
Collaborator Author

Thanks for looking anyway.
I'll make changes locally until a full review. Maybe I can get by with just a single additional commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: BooBoos 😇 Suggestions for Typos, like the NoMesh or there is a suggested refactor PR: Community Contribution 👑 Community Contribution PR's PR: Post Processing ➕ Like adding beeps, more tunability or different Gcode pause at heights
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants