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

Implements clean nozzle feature (Lulzbot's REWIPE) #4054

Merged
merged 4 commits into from
Jul 14, 2016

Conversation

jbrazio
Copy link
Contributor

@jbrazio jbrazio commented Jun 16, 2016

This is the implementation for the @alephobjects REWIPE feature discussed at #3553 by @scalez.

//
// Clean Nozzle Feature -- EXPERIMENTAL
//
// When enabled allows the user to send G12 to start the nozzle cleaning
// process, the G-Code accepts two parameters:
//   "P" for pattern selection
//   "S" for defining the number of strokes/repetitions
//
// Available list of patterns:
//   P0: This is the default pattern, this process requires a sponge type
//       material at a fixed bed location, the cleaning process is based on
//       "strokes" i.e. back-and-forth movements between the starting and end
//       points.
//
//   P1: This starts a zig-zag pattern between (X0, Y0) and (X1, Y1), "T"
//       defines the number of zig-zag triangles to be done. "S" defines the
//       number of strokes aka one back-and-forth movement. As an example
//       sending "G12 P1 S1 T3" will execute:
//
//          --
//         |  (X0, Y1) |     /\        /\        /\     | (X1, Y1)
//         |           |    /  \      /  \      /  \    |
//       A |           |   /    \    /    \    /    \   |
//         |           |  /      \  /      \  /      \  |
//         |  (X0, Y0) | /        \/        \/        \ | (X1, Y0)
//          --         +--------------------------------+
//                       |________|_________|_________|
//                           T1        T2        T3
//
// Caveats: End point Z should use the same value as Start point Z.
//
// Attention: This is an EXPERIMENTAL feature, in the future the G-code arguments
// may change to add new functionality like different wipe patterns.

Do not merge yet this PR as it still requires some tweaks, including the update of all example config files.

@jbrazio jbrazio added the Needs: Work More work is needed label Jun 16, 2016

#include "point_t.h"

extern void do_blocking_move_to(float x, float y, float z);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thinkyhead give me a hand here, when outside Marlin_main.cpp scope what is the best way to accomplish movement ?

Copy link
Member

@thinkyhead thinkyhead Jun 16, 2016

Choose a reason for hiding this comment

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

I feel like we're getting ahead of ourselves making every new thing into a class (especially non-static classes that pass a this to every method call). Could we make an effort from now on to make all the data and methods static in classes where we will only ever have a single instance? Otherwise, I will need to scrub all the classes and do it myself, which is tedious.

The other thing about putting every new thing into a class in its own compilation unit is, as you see, sacrificing the optimizations gained by implementing such things within Marlin_main alone. And I wonder if this effort at organization, placing everything into little classes, isn't going to come up and bite us in the ass in the long run. We are already bloating the code way beyond my initial hope of keeping it fitting on smaller boards for this release.

Anyway, if you want to access a function in Marlin_main.cpp, you will need to move its declaration to Marlin.h. If the compiler was inlining it, then it won't do that anymore, so we will lose any compiler optimization that was gained by having it declared in a single unit. Maybe some of these movement functions —the convenience functions which only wrap others— can be defined in Marlin.h as inline…. At least ensure they aren't expanded into a call from a call from a call…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first iteration I did was static but not knowing the future reverted back to non-static as the binary size was the same (we may have four nozzles, etc) but Ido agree with your principle for static and we should make clear that will be the norm.

I believe there is only one road to OOP, we must break with the past. I may be over killing with dynamic classes but the reason for that is that we dont know know the future or what we will require. The problem is every new class is bloated because old code does not re-use them.

GCC also inlines methods, there is no reason movement to be restrained in Marlin_main.cpp. IMO we must change the mindset that everything must be optimized, we must optimize code that runs most of the time for speed and the other part for size (80-20 rule).

Copy link
Member

@thinkyhead thinkyhead Jun 18, 2016

Choose a reason for hiding this comment

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

I agree we should avoid optimizing for speed where it matters little. But we should optimize for size at every opportunity. In some cases optimizing for size means replacing inline code with function calls, sacrificing a little performance, and I agree that is fine where code performance hardly matters, such as in the LCD and leveling routines. Easier-to-read code in homing and leveling will be better, even if it's slower.

I also agree, movement should be available all over the place, wherever we might need to utilize it in other classes. We can probably break out a lot of the general-purpose functions into separate compilation units from Marlin_main. In fact, I'm sure we will end up splitting it into a few different files based on types of functions - temperature, movement, etc.

@scalez
Copy link
Contributor

scalez commented Jun 16, 2016

@jbrazio I'm not sure if you have a wiper-pad setup, if you don't I can always help test and debug.

// /| /| /| (Xe, Ye)
// / | / | / |
// / | / | / |
// (Xs, Ys) / |/ |/ |
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Great idea to do a zig-zag pattern!

@jbrazio jbrazio force-pushed the feature/g12-clean-tool branch 2 times, most recently from 850335f to 295e1b4 Compare June 16, 2016 18:10
@jbrazio
Copy link
Contributor Author

jbrazio commented Jun 16, 2016

@scalez that would be great as I don't have the cleaning pad setup.

BTW, what type of material do you use for the cleaning pad, can we DIY something like that ?

@jbrazio jbrazio force-pushed the feature/g12-clean-tool branch 2 times, most recently from bd52a43 to ce1f498 Compare June 16, 2016 18:39
@scalez
Copy link
Contributor

scalez commented Jun 16, 2016

@jbrazio we have the instructions on how to make it here: https://devel.lulzbot.com/wiper_pads/wiper_pad_manufacture_SOP.odt
Here's the polyester strip that you apply the ABS juice to: http://www.mcmaster.com/#88085k311/=12vqw18
Or it can be purchased here (currently out of stock apparently, sorry about that, hopefully it'll be stocked soon): https://www.lulzbot.com/store/parts/lulzbot-mini-wiper-replacement-kit-0

@jbrazio
Copy link
Contributor Author

jbrazio commented Jun 17, 2016

@scalez it passes the Travis test now, did you had the opportunity to test ?

@scalez
Copy link
Contributor

scalez commented Jun 17, 2016

@jbrazio I'll try to test it today, if not Monday.

@scalez
Copy link
Contributor

scalez commented Jun 17, 2016

@jbrazio: I just tried it out a bit. It works really well. Here are my initial thoughts:

  • If you send G12 P1 S## but if (a < 5 || fabs(end.y - start.y) < 5) is true then it will still move to the initial wiping position. The zigzag member function should do the blocking moves after the return statement:
    static void zigzag(point_t const &start, point_t const &end, uint8_t const &triangles)
    __attribute__ ((optimize ("Os"))) {

      // Calculate the triangle side
      float const a = fabs(end.x - start.x) / triangles;

      // Don't allow the sides (a, b) to be smaller than 5mm
      if (a < 5 || fabs(end.y - start.y) < 5) return;

      // Move to the starting point
      do_blocking_move_to_xy(start.x, start.y);
      do_blocking_move_to_z(start.z);

      // Start the zig-zag pattern
      for (uint8_t i = 0; i < triangles; i++) {
        float const x = start.x + (a * (i + 1));
        do_blocking_move_to_xy(x, end.y);
        do_blocking_move_to_y(start.y);
      }
    }
  • Why limit the sides of the triangle? When I completely removed if (a < 5 || fabs(end.y - start.y) < 5) return; and my a == 0 it did some funky accelerations and stopped at those points where the hypotinuse == adjacent. This may actually be useful in some cases.
  • It would be excellent if the zigzag pattern could move back and forth.
  • An option to reverse the sawtooth pattern on X&Y would be nice:
      for (uint8_t i = 0; i < triangles; i++) {
        float const Y = start.Y + (B * (i + 1));
        do_blocking_move_to_xy(end.x, y);
        do_blocking_move_to_X(start.X);
      }
  • Since you're dividing strokes by two on L#53, an odd number as a parameter for S## when pattern == 0 would cause it to round down by one (to the even number below the odd number selected; e.g. S5 would produce S4). I think this should be noted in Configuration.h under the P0 comment.
  • I suggest adding a triangle wave pattern slightly different than the current sawtooth wave/right angle pattern; an isosceles pattern would be nice, like this:
             /\                /\
           /    \            /    \
(X0, Y0) /        \        /       \        / (X1, Y1)
                    \    /           \    /
                      \/               \/

@maukcc
Copy link
Contributor

maukcc commented Jun 20, 2016

I like this feature. we do it like this:
https://www.youtube.com/watch?v=-JJjylbQ8mc

I would like to see this after a filament change as well

@thinkyhead
Copy link
Member

thinkyhead commented Jun 20, 2016

I would like to see this after a filament change as well

This could lead to collisions in some cases. You need to be sure that the X (or XY) gantry cannot crash into the part on the build plate. Usually this means you can't lower the X (or XY) gantry back down to the bed (at many points) once the object reaches a certain height. If your wipe-surface moves with the gantry, this is not a problem. If the wipe surface is at the back of the bed, perhaps most of the time the object can be avoided. I suppose the machine could keep track of the farthest points where extrusion has occurred to "guess" the size of the object on the plate, and make sure to avoid that region.

@maukcc
Copy link
Contributor

maukcc commented Jun 21, 2016

Make "nozzle wipe after retraction" true or false and let the manufacturer of the machine make the decision. I would not try to do elaborate things in firmware.
default is false :)
Or make it a gcode macro that the manufacturer can program.

@thinkyhead
Copy link
Member

I would not try to do elaborate things in firmware.

I tend to agree. Nozzle wipe should be slicer-initiated as part of the full control program, when switching nozzles or whatever. Software that generates the GCode should decide where to move based on configured clearance values, and whether a wipe is possible given the position of the wipe surface.

I'm not sure whether this is an option in current slicers.

@jbrazio
Copy link
Contributor Author

jbrazio commented Jun 22, 2016

OK we have a new pattern generator for G12 which includes @scalez suggestions.

//   P1: This starts a zig-zag pattern between (X0, Y0) and (X1, Y1), "T"
//       defines the number of zig-zag triangles to be done. "S" defines the
//       number of strokes aka one back-and-forth movement. As an example
//       sending "G12 P1 S1 T3" will execute:
//
//          --
//         |  (X0, Y1) |     /\        /\        /\     | (X1, Y1)
//         |           |    /  \      /  \      /  \    |
//       A |           |   /    \    /    \    /    \   |
//         |           |  /      \  /      \  /      \  |
//         |  (X0, Y0) | /        \/        \/        \ | (X1, Y0)
//          --         +--------------------------------+
//                       |________|_________|_________|
//                           T1        T2        T3

Please note that now the zig-zag will return to the "initial" position from where G12 was called from, should we keep this behavior or simply stop after generating the pattern like it is with G12 P0

@jbrazio
Copy link
Contributor Author

jbrazio commented Jun 23, 2016

Youtube video:
G12 video

@jbrazio jbrazio force-pushed the feature/g12-clean-tool branch 3 times, most recently from 5ed899b to 35d8292 Compare June 23, 2016 01:05
@jbrazio
Copy link
Contributor Author

jbrazio commented Jul 13, 2016

@thinkyhead travis is running, if green then this PR is ready to be merged.

@jbrazio jbrazio added S: Please Merge and removed Needs: Work More work is needed labels Jul 13, 2016
@jbrazio jbrazio added this to the 1.1.0 milestone Jul 13, 2016
@thinkyhead
Copy link
Member

Yes, it certainly is!

@thinkyhead thinkyhead merged commit 8bf6861 into MarlinFirmware:RCBugFix Jul 14, 2016
@jbrazio jbrazio deleted the feature/g12-clean-tool branch July 14, 2016 22:36
@ghost
Copy link

ghost commented Jul 15, 2016

When HAS_BED_PROBE is enabled and AUTO_BED_LEVELING_FEATURE is disabled, compilation error occurs.

error message

AppData\Local\Temp\build6674025c445f92d0b07185e4e4080804.tmp\sketch\Marlin_main.cpp: In function 'void process_next_command()':

Marlin_main.cpp:6878: error: 'gcode_G12' was not declared in this scope

           gcode_G12(); // G12: Clean Nozzle

                     ^

@Blue-Marlin
Copy link
Contributor

Blue-Marlin commented Jul 15, 2016

Historically this is correct, because this nozzle-probes, where the feature is made for, are only used for probing - never for homing. This systems have a separate z-endstop for homing.
The solution could be

  • a sanity check.
  • a altered condition for G12
  • a relation to a special kind of probe (has to be defined)
  • a define like: Probe/Nozzle_can_be_cleaned.

@jbrazio
Copy link
Contributor Author

jbrazio commented Jul 15, 2016

This PR needs "rebasing" to take advantage of the do_blocking_move_to_*() being now free of HAS_BED_PROBE.

@esenapaj @Blue-Marlin do you agree we can have the nozzle clean feature without a probe right ? For instance I have replaced my start-gcode clean/purge procedure with a call to G12.

@Blue-Marlin
Copy link
Contributor

#if ENABLED(NOZZLE_CLEAN_FEATURE)
alone should be ok. Logically non of the other categories really fits.
ENABLED(AUTO_BED_LEVELING_FEATURE) can be dropped now if was only to use to use do_blocking_moves_to().

*to_xy is still lingering somewhere around.

@ghost
Copy link

ghost commented Jul 16, 2016

I think that it's better that this feature is freed from restrict of AUTO_BED_LEVELING_FEATURE,
but I can't be determined whether this feature needs to be freed from restrict of HAS_BED_PROBE or not .
I recognize this feature is for "electrical probing"? in LulzBot printers,
if it be so it seems that this feature needs to be not freed from restrict of HAS_BED_PROBE.
Even if nozzle is rubbed with cleaning pad, probably sludge of filament on inside of nozzle isn't detached.

...But, possibly sludge of filament on outside of nozzle can be removed if strong organic solvent is used with cleaning pad.
...Finally, I think that maybe less restrict is better...

@thinkyhead
Copy link
Member

thinkyhead commented Jul 16, 2016

needs to be not freed from restrict of HAS_BED_PROBE

Possibly HAS_BED_PROBE makes sense as the last limitation on the do_blocking_move* functions now. But perhaps Z-raise + XY move + Z-lower is something with non-probe use-cases too…?

I think that maybe less restrict is better

It's actually fine for functions only used in a single compilation unit (.cpp file), because unused functions will be stripped by the compiler. But if they are made extern or exposed in a .h header then they are always included, even if never used.

probably sludge of filament on inside of nozzle isn't detached

Menu > Clean Nozzle > Cold Pull ?

  • Heat up to melting point
  • Extrude 1-2cm
  • Wipe nozzle (optional)
  • Cool down to ~160 (below melting point)
  • Eject filament quickly, or
    • Alert user to pull the filament straight out

@ghost
Copy link

ghost commented Jul 16, 2016

Menu > Clean Nozzle > Cold Pull

It's interesting, but in case of using a "filament for cleaning" like a Dyna-Purge, it needs more length of extrude.
dscn1679

And, I think the feature can be used for pri-process of printing and change-process of multi-nozzle like this...?

@ghost
Copy link

ghost commented Jul 16, 2016

But, I feel that these recherche features might be as well considered after 1.1.0...

@scalez
Copy link
Contributor

scalez commented Jul 18, 2016

@esenapaj so your wipe is done during toolhead changes. Do you depend on wiping for your probing scheme? Or would you do a purge and cold pull before each probe?

@ghost
Copy link

ghost commented Jul 18, 2016

so your wipe is done during toolhead changes.

Sorry, above video isn't mine. I introduced above video for technological study.
My machine is delta. https://www.youtube.com/watch?v=VcoUde3wNsI

@jbrazio
Copy link
Contributor Author

jbrazio commented Jul 18, 2016

@scalez that video was just an example @esenapaj was giving us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants