Skip to content

Update AddCoolingProfile.py #20093

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

Merged
merged 10 commits into from
Jun 16, 2025
Merged

Conversation

GregValiant
Copy link
Collaborator

@GregValiant GregValiant commented Jan 1, 2025

Add control for a Build Volume fan.
Add 'Idle speed' for the inactive extruder rather than always going to "0".
General cleanup in the code and changed variable "curaApp" to "global_stack".

Type of change

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

Test Configuration:

  • Operating System:
    Win 10 Pro
    All Cura versions

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 Jan 1, 2025
@GregValiant GregValiant added the PR: Post Processing ➕ Like adding beeps, more tunability or different Gcode pause at heights label Jan 1, 2025
@GregValiant GregValiant requested a review from HellAholic January 1, 2025 17:14
Copy link
Contributor

github-actions bot commented Jan 1, 2025

Test Results

23 881 tests  ±0   23 879 ✅ ±0   49s ⏱️ +3s
     1 suites ±0        2 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit 7fc8bdd. ± Comparison against base commit ae60c7f.

♻️ This comment has been updated with latest results.

Add control for a Build Volume fan.

Update AddCoolingProfile.py

Oops.  Left in a debugging line.
@HellAholic
Copy link
Contributor

Code could use a bit more cleaning up imo, some of the if statements for example could be simplified. You could also do the init() and define self.curaApp and use that instead of Application.getInstance().getGlobalContainerStack() or setting the curaApp in different functions or in the if statements.

Made changes per request.
@GregValiant
Copy link
Collaborator Author

Changes made.
I also added the "if ';POSTPROCESSED' in data[0]" exit line so the script can't run twice.

A little history...This was the first script I'd written. I took it on because I needed it for PETG, and because it had been a feature request here since 2017 and no one else had bothered. I'm quite pleased that there has never been a bug report regarding it (knock-on-wood).

Changed variable name "curaApp" to "global_stack".

Update AddCoolingProfile.py

Add except for 'Build Volume Fan' for previous versions.
Re-worked the Build Volume fan code to include a printers Auxiliary fan if there is one.
@HellAholic
Copy link
Contributor

Some minor comments, looks good overall. Added some space after the # in a commit.

Update per requested changes.
Comments are consistent with "# ".
global_stack, extruder_list, extruder_count are assigned to "self".
@GregValiant
Copy link
Collaborator Author

The last commit should do the job. All of your suggestions were implemented.

@GregValiant
Copy link
Collaborator Author

Take one more look. I had missed a couple of change requests.
Honest... I think that's got it now.

@HellAholic
Copy link
Contributor

Mainly looked at the changes introduced by the PR, but going through the entire code with a finer comb:

  • Take a look at the try/except clauses -> Only should catch the specific exceptions you expect (e.g, ValueError, AttributeError,...) depending on the try content.
  • Parts of the code such as the functions (eg. _multi_fan_by_feature) that handle multi extruder setups and fan control could be refactored to reduce the repetitive code -> this helps with future development and debugging as you would only change the logic of the helper functions instead of applying it to the main function. It lowers the complexity of the code and makes it also easier to follow and maintain.
  • There are unused variables in the function definitions. Some examples '_multi_fan_by_layer: fan_mode', '_single_fan_by_feature: fan_list, index = 1', '_multi_fan_by_feature: layer_0_index, the_end_is_enabled, fan_list, fan_mode'
  • Functions could use a bit of documentation, referencing the comment under the function that explains in a wide scope what the function does and what the parameters are and what the function returns:
def _single_fan_by_layer(self, data: str, layer_0_index: int, fan_list: str, t0_fan: str) -> str:
    """
    Adjusts fan speeds for a single fan by layer.

    :param data: The G-code data as a list of strings.
    :param layer_0_index: The index of the first layer in the G-code.
    :param fan_list: A list of layer/fan speed pairs.
    :param t0_fan: The fan identifier for the first extruder.
    :return: The modified G-code data.
    """
  • Optional: You could use the logger to indicate when a function starts, helps with troubleshooting later and tracking the code execution during final testing.

@GregValiant
Copy link
Collaborator Author

We run into my ignorance here. I have no idea what an "attribute error" is.

At one point Remco asked that I try to simplify sections of the code. That's when I split off the four separate 'main' functions. Yes, I may have missed variables that became obsolete during those changes.

I'm not in the mood for going back over it again but I will at some point. It's a bastard to debug and right now it works fine even though the code remains pretty ugly.

I've been working on ChangeAtZ and PauseAtHeight. I figured I'd submit them after 5.10 stable is out. Once I get that far I'll return to this script and work on it some more.

@HellAholic
Copy link
Contributor

I went over it since you made the request, take your time and see what you think could be added. It's best to work on things when you're motivated.
Probably you know "attribute error" but just not by the name. Think of it this way, what sort of error message shows up in the logs (or when you run the script) if you don't include the try/except, that's the type of exception you should handle in the except part.

Some General rambling to clarify.
examples for attribute error

class Car:
    def __init__(self, brand):
        self.brand = brand

my_car = Car("Toyota")
print(my_car.brannd)  # Typo: 'brannd' instead of 'brand'

AttributeError: 'Car' object has no attribute 'brannd'

class Person:
    def __init__(self, name):
        self.name = name

p = Person("Alice")
print(p.age)  # 'age' was never defined in the class

AttributeError: 'Person' object has no attribute 'age'

class Animal:
    def speak(self):
        return "Some sound"

dog = Animal()
print(dog.run())  # 'run()' is not defined in the class

AttributeError: 'Animal' object has no attribute 'run'

num = 10
print(num.append(5))  # Integers don't have an 'append' method

AttributeError: 'int' object has no attribute 'append'

@GregValiant
Copy link
Collaborator Author

Another effort.

@GregValiant
Copy link
Collaborator Author

I had changed some of the "except" lines to indicate the error to trap. I was wrong in regards to line 782. It had looked like it would be an IndexError (and it still might throw one) but it kicked out on a ValueError. I have reverted back to the simple "except:" which did work.

Update AddCoolingProfile.py

Un-trapped ValueError in line 782.  It might also be an IndexError.  I left it open.

Update AddCoolingProfile.py

Change an IndexError to a ValueError
@GregValiant
Copy link
Collaborator Author

Changed an exception from IndexError to a ValueError.

@GregValiant
Copy link
Collaborator Author

GregValiant commented Apr 16, 2025

Commit 8 is a bug fix.

Found a bug.  The ";LAYER:" line was not being added to the "modified data" string when in "single_fan_by_feature" mode.

Update AddCoolingProfile.py

bug fix for the bug fix.
@HellAholic HellAholic merged commit ee021f7 into Ultimaker:main Jun 16, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

2 participants