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.

Sign up for GitHub

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

Jump to bottom

Create AddCoolingProfile.py #15853

Closed
wants to merge 7 commits into from
Closed

Create AddCoolingProfile.py #15853

wants to merge 7 commits into from

Conversation

GregValiant
Copy link
Collaborator

@GregValiant GregValiant commented Jun 22, 2023 •
edited
Loading

Advanced fan control post-processor. "By Layer" or "By Feature" for up to 4 fan circuits. This is in response to numerous requests such as #2149 , #5140 , #8527, #8777, #3624
September 10 commit adds an exit if the "print_sequence" is set to "One at a Time" in Cura.
September 14 commit removes the exit code and allows use with "One at a Time".

Description

A new post-processor to control layer cooling fans.

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

Type of change

  • [ X] New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • [ X] Test A
  • Tested with Cura 4.x and Cura 5.x

Test Configuration:

  • Operating System: Win 10 Home, Ender 3 Pro with Marlin firmware.

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 Jun 22, 2023
@GregValiant GregValiant mentioned this pull request Jul 2, 2023
Open
@katherinehackworth
Copy link

I think adding more description, or a screenshot, would be extremely helpful

@GregValiant
Copy link
Collaborator Author

GregValiant commented Jul 2, 2023 •
edited
Loading

You can turn the fan on or off 8 times when by "Layer Number" (you can run multiple instances of the post processor to increase that).
"By Feature" will change the fan speed depending on the feature (Outer-Wall, Skin, Infill, etc.). There are options to turn the fan off during combing moves and to turn it on just for a raft. You can pick the layer to start and end the fan control. Multiple instances allow you to configure part of a print "By Layer Number" and then the next part "By Feature".
A screenshot of "By Feature".
image

This is how it looks by "Layer Numbers".
image

Unzip and put the py file in the postprocessingplugins/Scripts folder.
(Download link removed as stale.)

@GregValiant GregValiant requested a review from Ghostkeeper July 3, 2023 20:09
@GregValiant
Create AddCoolingProfile.py
1400598 
Advanced fan control post-processor.  "By Layer" or "By Feature" for up to 4 fan circuits.

Update AddCoolingProfile.py

Fixed a problem in the AddNonMesh section.

Update AddCoolingProfile.py

Added code to skip M20? accel and jerk lines when calculating travel moves.

Update AddCoolingProfile.py

Remove the line to add the name to the gcode file.

Update AddCoolingProfile.py

Move the initial fan shutoffs so they are before the "Layer_count" line.
@GregValiant
Update AddCoolingProfile.py
0eeffcd 
Remove caps from variable names.
Removed Logger.
@GregValiant
Update AddCoolingProfile.py
34d348d 
Add a kick-out if "One at a Time" is enabled in Cura.
@GregValiant
Revert "Update AddCoolingProfile.py"
fe4ca81 
This reverts commit 34d348d.
@GregValiant
Update AddCoolingProfile.py
3ee0d3e 
Added ability to handle "One at a Time".
Removed the One at a Time kickout code.
@GregValiant GregValiant requested review from casperlamboo and rburema and removed request for Ghostkeeper September 14, 2023 14:00
@GregValiant
Copy link
Collaborator Author

It's long, it's involved, it's industrial strength, it works.

rburema
rburema requested changes Dec 13, 2023
View reviewed changes
Copy link
Member

@rburema rburema left a comment

Choose a reason for hiding this comment

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

Hi Greg!

I find that one of the reasons that it takes me so long to get to these each time, is that on the one hand they're useful and I can see a lot of work has gone into them. (Also, the code is at least quite readable, and I think to a certain extent also maintainable.)

And, like you said, they work (though I'm not sure I get what you mean with 'Industrial Strength'). Yet another thing is that the scripts don't quite need to adhere to the level of code-standards we use in the rest of the code, since they're --in a way-- separate.

On the other hand (and this gets me to my point, which is that) I don't think I'd be accepting any of this without alterations if it was for the main code base. I'm on the fence as it is. (Too much of my time is spend actually sitting on that metephorical fence, which I realize says a whole lot more about me then about your code, but here we are ... 😅 )

I've decided to finally bite the bullet and go through the things I'd really like to have changed, as I should have in the first place. We can have discussions about most of these, but at the very least I'd like to get rid of the repetition of the '9999..' string.

plugins/PostProcessingPlugin/scripts/AddCoolingProfile.py Outdated
t0_fan = " P0"; t1_fan = " P0"; t2_fan = " P0"; t3_fan = " P0"; is_multi_extr_print = True

#Get some information from Cura-----------------------------------
extrud = Application.getInstance().getGlobalContainerStack().extruderList
Copy link
Member

Choose a reason for hiding this comment

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

If using this Application.getInstance().getGlobalContainerStack() multiple times, you should request it once, and put it in a variable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

plugins/PostProcessingPlugin/scripts/AddCoolingProfile.py Outdated
Comment on lines 329 to 360
fan_1st = self.getSettingValueByKey("fan_1st")
if "/" in fan_1st:
fan_list[0] = check_layer.layer_checker(fan_1st, "l", fan_mode)
fan_list[1] = check_layer.layer_checker(fan_1st, "p", fan_mode)
fan_2nd = self.getSettingValueByKey("fan_2nd")
if "/" in fan_2nd:
fan_list[2] = check_layer.layer_checker(fan_2nd, "l", fan_mode)
fan_list[3] = check_layer.layer_checker(fan_2nd, "p", fan_mode)
fan_3rd = self.getSettingValueByKey("fan_3rd")
if "/" in fan_3rd:
fan_list[4] = check_layer.layer_checker(fan_3rd, "l", fan_mode)
fan_list[5] = check_layer.layer_checker(fan_3rd, "p", fan_mode)
fan_4th = self.getSettingValueByKey("fan_4th")
if "/" in fan_4th:
fan_list[6] = check_layer.layer_checker(fan_4th, "l", fan_mode)
fan_list[7] = check_layer.layer_checker(fan_4th, "p", fan_mode)
fan_5th = self.getSettingValueByKey("fan_5th")
if "/" in fan_5th:
fan_list[8] = check_layer.layer_checker(fan_5th, "l", fan_mode)
fan_list[9] = check_layer.layer_checker(fan_5th, "p", fan_mode)
fan_6th = self.getSettingValueByKey("fan_6th")
if "/" in fan_6th:
fan_list[10] = check_layer.layer_checker(fan_6th, "l", fan_mode)
fan_list[11] = check_layer.layer_checker(fan_6th, "p", fan_mode)
fan_7th = self.getSettingValueByKey("fan_7th")
if "/" in fan_7th:
fan_list[12] = check_layer.layer_checker(fan_7th, "l", fan_mode)
fan_list[13] = check_layer.layer_checker(fan_7th, "p", fan_mode)
fan_8th = self.getSettingValueByKey("fan_8th")
if "/" in fan_8th:
fan_list[14] = check_layer.layer_checker(fan_8th, "l", fan_mode)
fan_list[15] = check_layer.layer_checker(fan_8th, "p", fan_mode)
Copy link
Member

Choose a reason for hiding this comment

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

Extremely similar code, please replace with loop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

plugins/PostProcessingPlugin/scripts/AddCoolingProfile.py
Comment on lines +413 to +416
T0_used = False
T1_used = False
T2_used = False
T3_used = False
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure machines can have a maximum of 4 fans. My issue is mostly that things like this are completely opaque to any user that doesn't dive into the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tool changer machines can have as many fans as they have toolheads. Large format machines can be configured in a lot of ways. I saw no reason not to support that. It's part of the "Industrial Strength" thing. Not everyone who uses Cura is printing dnd characters with it. For example, the 3D printing group at Ford Motor Company has 4 Modix 1800 printers and a variety of smaller printers (no Chinese crap). When things get large then needs change.

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is that there may be more than 4 fans.

plugins/PostProcessingPlugin/scripts/AddCoolingProfile.py
Comment on lines +420 to +427
if "T0" in lines:
T0_used = True
if "T1" in lines:
T1_used = True
if "T2" in lines:
T2_used = True
if "T3" in lines:
T3_used = True
Copy link
Member

Choose a reason for hiding this comment

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

Also, it's probably a good idea to put these in a list anyway (T_used or Tx_used or something like that).

plugins/PostProcessingPlugin/scripts/AddCoolingProfile.py Outdated
Comment on lines 428 to 437
if T0_used and (T1_used or T2_used or T3_used):
is_multi_extr_print = True
elif T1_used and (T0_used or T2_used or T3_used):
is_multi_extr_print = True
elif T2_used and (T0_used or T1_used or T3_used):
is_multi_extr_print = True
elif T3_used and (T0_used or T1_used or T2_used):
is_multi_extr_print = True
else:
is_multi_extr_print = False
Copy link
Member

Choose a reason for hiding this comment

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

Case in point: You could just count the number used here by looping over that array.

plugins/PostProcessingPlugin/scripts/AddCoolingProfile.py Outdated
Comment on lines 703 to 706
if line == "T0": this_fan = t0_fan
if line == "T1": this_fan = t1_fan
if line == "T2": this_fan = t2_fan
if line == "T3": this_fan = t3_fan
Copy link
Member

Choose a reason for hiding this comment

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

Please loop.

plugins/PostProcessingPlugin/scripts/AddCoolingProfile.py Outdated
Comment on lines 729 to 773
if ";TYPE:SKIRT" in line:
modified_data += line + "\n"
modified_data += fan_sp_skirt + this_fan + "\n"
current_fan_speed = str(fan_sp_skirt.split("S")[1])
elif ";TYPE:WALL-INNER" in line:
modified_data += line + "\n"
modified_data += fan_sp_wall_inner + this_fan + "\n"
current_fan_speed = str(fan_sp_wall_inner.split("S")[1])
elif ";TYPE:WALL-OUTER" in line:
modified_data += line + "\n"
modified_data += fan_sp_wall_outer + this_fan + "\n"
current_fan_speed = str(fan_sp_wall_outer.split("S")[1])
elif ";TYPE:FILL" in line:
modified_data += line + "\n"
modified_data += fan_sp_fill + this_fan + "\n"
current_fan_speed = str(fan_sp_fill.split("S")[1])
elif ";TYPE:SKIN" in line:
modified_data += line + "\n"
modified_data += fan_sp_skin + this_fan + "\n"
current_fan_speed = str(fan_sp_skin.split("S")[1])
elif line == ";TYPE:SUPPORT":
modified_data += line + "\n"
modified_data += fan_sp_support + this_fan + "\n"
current_fan_speed = str(fan_sp_support.split("S")[1])
elif ";TYPE:SUPPORT-INTERFACE" in line:
modified_data += line + "\n"
modified_data += fan_sp_support_interface + this_fan + "\n"
current_fan_speed = str(fan_sp_support_interface.split("S")[1])
elif ";MESH:NOMESH" in line or ";MESH:NONMESH" in line: #compatibility with 5.3.0 'NOMESH'
if fan_combing == True:
modified_data += line + "\n"
modified_data += "M106 S0" + this_fan + "\n"
current_fan_speed = "0"
else:
modified_data += line + "\n"
elif ";TYPE:PRIME-TOWER" in line:
modified_data += line + "\n"
modified_data += fan_sp_prime_tower + this_fan + "\n"
current_fan_speed = str(fan_sp_prime_tower.split("S")[1])
elif line == ";BRIDGE":
modified_data += line + "\n"
modified_data += fan_sp_bridge + this_fan + "\n"
current_fan_speed = str(fan_sp_bridge.split("S")[1])
#If an end layer is defined - Insert the final speed and set the other variables to Final Speed to finish the file
elif line == ";LAYER:" + str(the_end_layer):
Copy link
Member

Choose a reason for hiding this comment

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

For the similar bits of code here, you can make the three (often) repeated lines into a function, then do the same 'trick' I described earlier with the mapping (and/or another lambda/function).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have no idea what a lambda is beyond it being a Greek letter.

plugins/PostProcessingPlugin/scripts/AddCoolingProfile.py Outdated
fan_string_l = str(fan_string.split("/")[0])
try:
if int(fan_string_l) <= 1: fan_string_l = "1"
if fan_string_l == "": fan_string_l = "999999999"
Copy link
Member

Choose a reason for hiding this comment

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

Repeated string 2

plugins/PostProcessingPlugin/scripts/AddCoolingProfile.py Outdated
if int(fan_string_l) <= 1: fan_string_l = "1"
if fan_string_l == "": fan_string_l = "999999999"
except ValueError:
fan_string_l = "999999999"
Copy link
Member

Choose a reason for hiding this comment

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

Repeated string 3

plugins/PostProcessingPlugin/scripts/AddCoolingProfile.py
}
}"""

def execute(self, data):
Copy link
Member

Choose a reason for hiding this comment

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

I'd greatly prefer it if this main execute function was broken up into more sub-functions.

Copy link
Collaborator Author

@GregValiant GregValiant Dec 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'll take another look. I learn as I go. Todays commit was housecleaning. I'll change this to a draft.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright. I've removed the secondary classes and split 4 functions out of the long script. It's still long but it's a little neater.

Copy link
Member

Choose a reason for hiding this comment

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

I've got time to take a look now, but I noticed you closed a bunch of plugins. I hope I didn't come on too strong in the reviews?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nah. I'll never be a software engineer and I'm not going to bother to try. The only thing I know about a "lambda" is that it's a Greek letter. The posts work for me and I'm good with that.
For something like the fan control it's been requested for over six years and I agreed it would be a really good idea so I gave it a shot. I use it and DispalyInfoOnLCD on every print.

I'm sure all the UM code looks nice. Then there are things like this example. That bothered me on a very basic level.

Copy link
Member

Choose a reason for hiding this comment

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

UM code is far from perfect. There are tons of things I'd change if I'd had the time (and energy).

It is a really goof idea, and we'd like to have it in here. The other reason I made my comments the way they are is that in the past, you'd indicated that you'd like to learn.

I'm really sorry this is taking all so long -- ideally, that should not happen, but it is what it is. Next week the team will have a meeting about what to do about the slowness of the interaction with parts of our work outside of that what UM itself mandates (largely community focussed).

I'm not sure if I said it before (I hope I did), but none of this is really bad. I was debating myself whether to just merge it in wholesale without saying anything, but then I wouldn't have done due diligence.

I've finally rested up enough to prepare for work again. As I feel I have some unfinished business here, since I feel like I let you down, I'm going to try and make some small adjustments here and perhaps get it merged this weekend.

@GregValiant
Update AddCoolingProfile.py
d367055 
Additional cleanup throughout.
Altered some variable names.
Eliminated the 9999999 strings.
@GregValiant GregValiant marked this pull request as draft December 15, 2023 19:11
@GregValiant
Update AddCoolingProfile.py
e0a9c67 
Split the main section into sub routines.
Eliminated 2 classes.
@GregValiant GregValiant marked this pull request as ready for review December 17, 2023 22:32
@GregValiant GregValiant marked this pull request as draft December 18, 2023 20:09
@GregValiant GregValiant deleted the AddCoolingProfile branch December 19, 2023 02:03
@Coopski101
Copy link

Hey man, don't give up on this thing, it rules. I appreciate it

@GregValiant
Copy link
Collaborator Author

GregValiant commented Jan 7, 2024 •
edited
Loading

@rburema I made some of the changes you suggested. There was a lot of breakage but I think I'll take another look and try to get it squared away again.

EDIT: I find the entire "pull request" thing to be maddening. I'm attaching my latest version. There are no substantial changes to the script but I did split 4 functions out of the main "execute" function. I found some issues with the regex searches and replacements and those are fixed.
@Coopski101 you will want this as well.
AddCoolingProfile.zip

Remco, as a side note; I've added functionality to Display Info on LCD, Add Cura Settings, and Little Utilities.
Add Cura Settings should be an easy review because it doesn't do anything to the gcode, it just adds all the Cura settings to the end of the file. The added functions are to add the "Max and Min print speeds" and some file statistics.
I won't re-open the pull requests unless you ask, and then I'll try. When UM changed the "license" lines everything broke in the pull requests for the altered "stock" scripts and I couldn't figure out how to get by that. My resulting frustration is what caused me to close them all. I'm still pissed off, so if I run into further issues I don't intend to spend any more of my time trying to figure it out.

rburema pushed a commit that referenced this pull request Jan 7, 2024
@GregValiant @rburema
Add 'Advanced Cooling Fan Control' post-processing script.
1a5370d 
See discussions in #15853 (on github) and (internally) tracking-ticket CURA-11520
@rburema rburema mentioned this pull request Jan 7, 2024
Merged
@rburema
Copy link
Member

rburema commented Jan 9, 2024

@GregValiant I could've sworn I did download the latest zip (that you put here) but when I redownloaded & ran a compare on the file I have and the new one there where indeed some differences that I'd like to be in. Not sure how that happened. I'll take a look shortly.

As for handling PR's in general, for everyone involved: I can imagine; source-control seems like it should be a solved problem, and yet, here we are.

I'll try to squeeze in a look at the other one you mentioned as well, but I won't promise anything right now.

rburema pushed a commit that referenced this pull request Jan 9, 2024
@GregValiant @rburema
Update 'Advanced Cooling Fan Control' post-processing script.
2ad2afd 
See discussions in #15853 (on github) and (internally) tracking-ticket CURA-11520
@GregValiant
Copy link
Collaborator Author

It got caught so that's a good thing. That bug you caught needs to be caught again.

@rburema
Copy link
Member

rburema commented Jan 9, 2024

Yes, I merged the new stuff with main already, I took care that my bugfix is still in there.

Just so you don't have to search for what I did; the thing I changed (well, besides removing a spurious space somewhere) is the initialize-function (not the most elegant fix, but at least it doesn't crash anymore):

    def initialize(self) -> None:
        super().initialize()
        scripts = Application.getInstance().getGlobalContainerStack().getMetaDataEntry("post_processing_scripts")
        if scripts != None:
            script_count = scripts.count("AddCoolingProfile")
            if script_count > 0:
                ## Set 'Remove M106 lines' to "false" if there is already an instance of this script running.
                self._instance.setProperty("delete_existing_m106", "value", False)

@GregValiant
Copy link
Collaborator Author

I used a "try / except / pass" which I guess did the same thing.
Funny I never had that come up but as soon as I did what you did, there it was.

@rburema
Copy link
Member

rburema commented Jan 10, 2024

That's why I like working with QA -- you can never find it all as a solo dev.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rburema rburema rburema requested changes

@casperlamboo casperlamboo Awaiting requested review from casperlamboo

Assignees
No one assigned
Labels
PR: Community Contribution 👑 Community Contribution PR's
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@GregValiant @katherinehackworth @Coopski101 @rburema