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

Add Blocks machines to ultimaker cura #11620

Merged
merged 44 commits into from
Jan 10, 2023

Conversation

jgrilo-blocks
Copy link
Contributor

First definitions of Blocks machines for Ultimaker Cura

Definition files to add Blocks printers
Extruder files for Blocks printers
Mesh files for Blocks printers
Quality setting files for Blocks printers for nozzle and print core variants
Variants files for Blocks printers that include nozzle and print core variants
@Ghostkeeper
Copy link
Collaborator

Belongs to Ultimaker/fdm_materials#195. They should be merged at the same time.

The tests are currently failing. It fails for a lot of files, but it seems that at least for some of them it's because the SettingVersion is outdated (should be 19 instead of 17). That indicates that you might have tested this in Cura 4.13 instead of in Cura 5.0. Do they work with Arachne?

@jgrilo-blocks
Copy link
Contributor Author

jgrilo-blocks commented Mar 16, 2022

Thanks for the feedback @Ghostkeeper, yes we tested them on Cura 4.13 but they work in Arachne, we will fix the SettingVersion of the files.
Should we do a new pull request after that?

@Ghostkeeper
Copy link
Collaborator

You can just update the master branch of BlocksTechnology's fork with new commits. Github will automatically update this Pull Request with your latest changes. That's also the easiest for us.

jgrilo-blocks and others added 6 commits March 23, 2022 10:26
Variants files for Blocks printers that include nozzle and print core variants
Variants files for Blocks printers that include nozzle and print core variants
SettingVersion updated to work with Cura 5.0
Quality setting files for Blocks printers for nozzle and print core variants
SettingVerison updated to work with Cura 5.0
Edited start Gcode of  Blocks Zero and Blocks R21
Updated Start GCODE
@braga-dev
Copy link
Contributor

@Ghostkeeper any updates when this branch will be merged?

Copy link
Member

@nallath nallath left a comment

Choose a reason for hiding this comment

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

Apologies for the slow review! I'm trying to get through all the open PR's again.

I've not checked all the settings, but I've found quite a few settings that re-define defaults. This makes it a lot harder to maintain the profile, so it's generally best to adhere to the DRY (Don't repeat yourself) as much as possible.

Furthermore, we also require buildplate meshes to be well below 1mb. Try to exclude any unneeded details like springs / screws.

[values]
layer_height = 0.1
layer_height_0 = 0.15
material_diameter = 1.75
Copy link
Member

Choose a reason for hiding this comment

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

This should not be set in a quality profile. It's already being set by the extruder. THese can (and should) be removed from all of them.

definition = blocks_base

[metadata]
setting_version = 19
Copy link
Member

Choose a reason for hiding this comment

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

setting version is 20 now (not a big deal if you don't fix it, but it will make the tests fail right now)

resources/definitions/blocks_pros30.def.json Outdated Show resolved Hide resolved
"travel_avoid_supports": { "value": true },
"travel_retract_before_outer_wall": { "value": true },

"retraction_enable": { "value": true },
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"retraction_enable": { "value": true },

This is already the default in fdmprinter, no need to repeat yourself.


"retraction_speed": {
"deafult_value": 40,
"maximum_value_warning": "machine_max_feedrate_e if retraction_enable else float('inf')",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"maximum_value_warning": "machine_max_feedrate_e if retraction_enable else float('inf')",

Already the default, no need to repeat yourself

Comment on lines 59 to 60
"speed_wall": { "value": "speed_print / 2" },
"speed_wall_0": { "value": "speed_wall" },
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"speed_wall": { "value": "speed_print / 2" },
"speed_wall_0": { "value": "speed_wall" },

Comment on lines 62 to 63
"speed_topbottom": { "value": "speed_print / 2" },
"speed_roofing": { "value": "speed_topbottom" },
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"speed_topbottom": { "value": "speed_print / 2" },
"speed_roofing": { "value": "speed_topbottom" },

Comment on lines 79 to 80
"line_width": { "value": "machine_nozzle_size" },
"wall_line_width": { "value": "machine_nozzle_size" },
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"line_width": { "value": "machine_nozzle_size" },
"wall_line_width": { "value": "machine_nozzle_size" },

"machine_height": { "default_value": 290 },
"machine_depth": { "default_value": 310 },

"retraction_retract_speed": { "value": 40, "maximum_value_warning": 130 },
Copy link
Member

Choose a reason for hiding this comment

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

Why not just define the maximum value_warning in the base file? THey seem to be the same for all machines.


"retraction_retract_speed": { "value": 40, "maximum_value_warning": 130 },
"retraction_prime_speed": { "value": 20, "maximum_value_warning": 130 },
"speed_print": { "value": 50 },
Copy link
Member

Choose a reason for hiding this comment

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

It's the same for all printers. So why not just put it in the base file?

@braga-dev
Copy link
Contributor

@nallath changes were made and machines added. Thank you so much for the help

@braga-dev
Copy link
Contributor

@nallath any chance to review the printer and quality profiles? thank you

@Joeydelarago Joeydelarago added the PR: Printer Definitions 🏭 A PR that introduces or changes settings and printer definitions label Nov 11, 2022
@jellespijker
Copy link
Member

jellespijker commented Nov 29, 2022

Can you sync your fork, with our main. We have added a printer-linter tool for our printer definitions and profiles. This should help us with processing our PR's. It performs some standard diagnostic checks and apply auto-formatting on the changed files in this PR, but you can also run it locally. See https://github.com/Ultimaker/Cura/tree/main/printer-linter

If it finds issues for which it knows a fix it should post a comment with that suggested fix such that you can easily commit that fix if you're okay with it.
But please use commonsense with the suggestions because the tool is still in the early stages of development.

@braga-dev
Copy link
Contributor

@jellespijker how are you? is it possible to approve the workflow to check if there is more diagnostics with printer-liner? thank you

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

resources/definitions/blocks_base.def.json Outdated Show resolved Hide resolved
resources/definitions/blocks_base.def.json Outdated Show resolved Hide resolved
resources/definitions/blocks_base.def.json Outdated Show resolved Hide resolved
resources/definitions/blocks_base.def.json Outdated Show resolved Hide resolved
resources/definitions/blocks_base.def.json Outdated Show resolved Hide resolved
resources/definitions/blocks_base.def.json Outdated Show resolved Hide resolved
resources/definitions/blocks_base.def.json Outdated Show resolved Hide resolved
braga-dev and others added 2 commits December 30, 2022 11:10
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@braga-dev
Copy link
Contributor

@jellespijker just updated with the last commits! Let's hope its the final one :D

Contributes to CURA-10101
The settings:
 - travel_compensate_overlapping_walls_0_enabled
 - fill_perimeter_gaps
 - filter_out_tiny_gaps

Are no longer used with the new Arachne engine. That these settings were
still set in the blocks_base definition is an indication that the current
profiles weren't actually tested with the new Arachne engine.

Please consider doing some actual quality tests with these profiles on your
printers. This is for the benefit of your users experience.

Contributes to CURA-10101
Contributes to CURA-10101
Contributes to CURA-10101
Copy link
Member

@jellespijker jellespijker left a comment

Choose a reason for hiding this comment

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

Still multiple issues and failing Unit Tests so I can't press merge :-(

To help you guys out a bit I made a PR against your fork. See BlocksTechnology#1

If you agree with these suggested changes feel free to merge it on your fork, this should also update this PR.

I will convert this PR to a WIP PR. You can convert it back once it is ready to be reviewed again.

@jellespijker jellespijker marked this pull request as draft December 30, 2022 23:24
@jellespijker
Copy link
Member

@braga-dev and @jgrilo-blocks Do you guys need additional help with this PR? Or is merging my PR on your fork enough? BlocksTechnology#1

It would be nice if we can add your printers in Cura 5.3.0

@braga-dev
Copy link
Contributor

hello @jellespijker sorry for the delay. We just fixed the last issue so its all probably fine. We already tested these print profiles in Cura 5 and they are working fine so there won't be a problem.

For future updates, its going to be easier/faster than the first ones right?

@jellespijker
Copy link
Member

Don't worry. The main delay was originally on our side. We are finally catching up.

Yes, updates and new printers should now be processed way faster due to our improved workflow.

I will take a look at the fixes first thing o Monday. Good to hear that they have been tested on Cura 5.x.

If all goes well, these printers should be available in Cura 5.3

@jellespijker
Copy link
Member

just mark it ready for review when you're finished so we know that we can review it.

@braga-dev
Copy link
Contributor

braga-dev commented Jan 6, 2023

Hello @jellespijker perfect. Great to hear that. I don't have write access so I can't make it ready for review.

@jellespijker jellespijker marked this pull request as ready for review January 7, 2023 12:24
Copy link
Member

@jellespijker jellespijker left a comment

Choose a reason for hiding this comment

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

LGTM;

Two minor fixes which I will quickly fix myself when I merge it.
Congrats guys, your printers should be available in Cura 5.3

"retraction_hop_enabled": { "value": "False" },
"retraction_prime_speed":
{
"deafult_value": 20,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"deafult_value": 20,
"default_value": 20,

Copy link
Member

Choose a reason for hiding this comment

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

I will change these myself after I merge them

},
"retraction_speed":
{
"deafult_value": 40,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"deafult_value": 40,
"default_value": 40,

Copy link
Member

Choose a reason for hiding this comment

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

I will change these myself after I merge them

@jellespijker jellespijker dismissed stale reviews from nallath and github-actions[bot] January 10, 2023 07:32

Applied changes where necesarry

@jellespijker jellespijker merged commit a28fc03 into Ultimaker:main Jan 10, 2023
@braga-dev
Copy link
Contributor

Hello @jellespijker ! Thank you very much! Can't wait.

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: Printer Definitions 🏭 A PR that introduces or changes settings and printer definitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants