Skip to content

Eliminate extra tool-change park moves#11970

Closed
InsanityAutomation wants to merge 1 commit intoMarlinFirmware:bugfix-2.0.xfrom
InsanityAutomation:IDEX-Tool-Change-Optomization
Closed

Eliminate extra tool-change park moves#11970
InsanityAutomation wants to merge 1 commit intoMarlinFirmware:bugfix-2.0.xfrom
InsanityAutomation:IDEX-Tool-Change-Optomization

Conversation

@InsanityAutomation
Copy link
Contributor

Requirements

Dual X Carriage enabled

Description

Eliminate Unpark z raise variable
Eliminate duplicated moves
Ensure dual x carriage tool change function honors no move variable
Remove redundant logic

If this looks good, please let me know before merging and I will update all of the default / example configs today.

@InsanityAutomation
Copy link
Contributor Author

@Roxy-3D Is your machine back together enough to give this a test run? May also hit the cause of the z creep in #11412

@thinkyhead thinkyhead force-pushed the IDEX-Tool-Change-Optomization branch from 4d757a0 to b5699ee Compare October 1, 2018 01:50
@thinkyhead thinkyhead added the Needs: Testing Testing is needed for this change label Oct 1, 2018
@thinkyhead
Copy link
Member

The Dual X Carriage code was developed with heavy testing last time around, so this needs extensive testing before it can be trusted to be correct.

@InsanityAutomation
Copy link
Contributor Author

Agreed, I don't expect this in today! I did ping a couple people to try it out though.

@thinkyhead
Copy link
Member

Cool. I'll be sure to add any testers to a project group so we can ping them on future updates.

@thinkyhead thinkyhead changed the title Eliminate duplicated moves Eliminate extra tool-change park moves Oct 1, 2018
@Roxy-3D
Copy link
Member

Roxy-3D commented Oct 1, 2018

Is your machine back together enough to give this a test run? May also hit the cause of the z creep in #11412

Much of it is back together and working. The problem is my left extruder doesn't read the correct temperature. I'm not sure, but the controller board might have the wrong size pull up resistor. I don't know.... But I'm probably not too far away from being able to check this out.

@thinkyhead
Copy link
Member

thinkyhead commented Oct 1, 2018

Ensure dual x carriage tool change function honors no move variable

When using parking, there should still be park/unpark moves even with no_move. But I believe that the nozzle simply doesn't need to move to the same current position.

One thing in the code that has me dubious is the removal of fixing the position based on the hotend_offset for Y and Z. If the nozzle is offset compared to the other one, then it won't have the correct position without this.

@InsanityAutomation
Copy link
Contributor Author

The no move flag prevents crash moves for the most part, and I would expect it to be taken literally in the case of parking extruders, where a move command could then take out the intended tool. When X is homed, both extruders are parked, then calling T1 would only perform the z raise, x home, z lower commands. It was already home and parked, hence why no move was set, so all it would do is raise then lower z. This would then run a second time in the main tool change function due to the fixed 1.0mm raise for all except switching extruders. Plus add in the smaller, almost invisible at default settings of .2mm, unpark zlift which is buried by the 1.0mm move forced into the unparking move currently.

I did put back in the current_position change from the hotend offsets, im not sure what I was thinking when I pulled those 2 lines. I think I intended to move them at one point into the condition above, then decided they needed to apply all the time even with no move and mustve forgotten to put them back.

@SJ-Innovation
Copy link
Contributor

Following. Will test on my parking toolhead machine and report back.

@Roxy-3D
Copy link
Member

Roxy-3D commented Oct 1, 2018

Eliminate Unpark z raise variable

I'm OK with this defaulting to 0.00 mm. But having the option to have the nozzle rise some amount when it unparks and moves into position is a good thing. It helps guarantee the nozzle won't be scrapping across the part.

@InsanityAutomation
Copy link
Contributor Author

@Roxy-3D We always raise by the park amount if a move is allowed, so in this case you could more correctly say the two are merged I suppose. I considered a sanity check that if both are set, take the larger of the 2 as the raise amount.

@Roxy-3D
Copy link
Member

Roxy-3D commented Oct 1, 2018

OK! Understood!

One thing in the code that has me dubious is the removal of fixing the position based on the hotend_offset for Y and Z. If the nozzle is offset compared to the other one, then it won't have the correct position without this.

Unless this is compensated for some where else... I agree. In theory the IDEX extruders re-position exactly where the other one was when there is a tool change. But I know this isn't the case.

In fact... I think I would almost argue for IDEX, we should put the HOTEND_OFFSET's together so everybody knows they can specify a Z offset:

  #define HOTEND_OFFSET_X {0.0,  0.00}  // (in mm) for each extruder, offset of the hotend on the X axis
  #define HOTEND_OFFSET_Y {0.0,  0.00}  // (in mm) for each extruder, offset of the hotend on the Y axis
  #define HOTEND_OFFSET_Z {0.0, -0.00}  // Z-offsets of the two hotends. The first must be 0.

@InsanityAutomation
Copy link
Contributor Author

@Roxy-3D On those offsets, I put that back in already. I agreed as well upon review and noted it was an erroneous removal from when I intended to move it and backtracked.

@thinkyhead
Copy link
Member

…we should put the HOTEND_OFFSETs together so everybody knows they can specify a Z offset…

That looks out of date. Current configs say…

//#define HOTEND_OFFSET_X {0.0, 20.00} // (mm) relative X-offset for each nozzle
//#define HOTEND_OFFSET_Y {0.0, 5.00}  // (mm) relative Y-offset for each nozzle
//#define HOTEND_OFFSET_Z {0.0, 0.00}  // (mm) relative Z-offset for each nozzle

@InsanityAutomation
Copy link
Contributor Author

@Roxy-3D any luck trying this on your end yet?

@Roxy-3D
Copy link
Member

Roxy-3D commented Oct 4, 2018

No. Not yet. My printer is broken. I'm having a problem with the reported temperature of the left extruder. So... I have wires hanging all over the place right now.

@InsanityAutomation
Copy link
Contributor Author

Ouch - I recall you mentioning that awhile ago. Have you tried putting thermistor wires directly into the db15 at the board? I've seen issues with the wires and sub boards before.

@Roxy-3D
Copy link
Member

Roxy-3D commented Oct 5, 2018

Ouch - I recall you mentioning that awhile ago. Have you tried putting thermistor wires directly into the db15 at the board? I've seen issues with the wires and sub boards before.

No... The problem is I have an early T-Rex 3 board. And it is upgrading a T-Rex 2+ printer. Nothing is 100% correct.

@InsanityAutomation
Copy link
Contributor Author

Fair enough. Was the db15 pinout retained? I've got a diagram laying around somewhere. Bypassing all that and connecting directly may at least narrow it down a bit.

@Roxy-3D
Copy link
Member

Roxy-3D commented Oct 5, 2018

Was the db15 pinout retained?

Do you mean the cables going to each extruder? I have a diagram of those pin assignments. Unfortunately, the pins are defined different depending upon which extruder the cable is connected to. You can't connect Extruder 0's cable to Extruder 1 with out damaging stuff.

But the bottom line is the answer is "No!" the DB-15 connector's are both changed. They needed extra pins to put the filament runout sensors on the extruders... and stuff moved around.

@InsanityAutomation
Copy link
Contributor Author

InsanityAutomation commented Oct 5, 2018

Those cables yes, but not between e0 and e1. I meant the same pinouts from the trex2+ db15 to the trex3 for the same tool head.

Aha! Edit while I was replying and answer the question! Lol shouldn't have needed to change the db15 pinout to get that, just a different output for the fan on t1 and branch heater - and fan + together to make the always on. If they did more, sounds like a waste.

I do have a good stack of spare parts if you need some. Just got a couple more boards in taking Colen's spares!

@Roxy-3D
Copy link
Member

Roxy-3D commented Oct 5, 2018

No.... Even between T-Rex 2+ and T-Rex 3 the pinouts changed. The main reason is they are adding a filament runout sensor to the extruder. So some wires were needed. Things moved. But independent of that, some pin numbers moved too.

@thinkyhead thinkyhead force-pushed the IDEX-Tool-Change-Optomization branch from 4dae7f6 to a98fb04 Compare October 7, 2018 01:19
@thinkyhead
Copy link
Member

Rebased and squashed.

@thinkyhead thinkyhead force-pushed the IDEX-Tool-Change-Optomization branch from a98fb04 to 10a6723 Compare October 7, 2018 01:21
@InsanityAutomation
Copy link
Contributor Author

Thanks - I'll update the test build I've sent out to the formbot groups as well with the latest. Still hoping for reports from any different manufacturers. Reports to date are good, but all from formbot users...

@thinkyhead thinkyhead force-pushed the IDEX-Tool-Change-Optomization branch from 10a6723 to a56838b Compare October 7, 2018 01:24
@thinkyhead
Copy link
Member

There's not a huge difference between Dual X Carriage implementations. If one of them has all the right behaviors, then we can safely assume they'll all work, assuming correct configuration.

@InsanityAutomation
Copy link
Contributor Author

Then if you're comfortable with it, I'll confirm after rebasing that it's still good, maybe take a quick before / after video to show improvement and either merge, or wait till Roxy can confirm as well then merge. I assume she's still fighting that thermistor issue.

@thinkyhead
Copy link
Member

No rush. Better to have a solid confirmation than merge it and get a slew of reports.

@thinkyhead thinkyhead force-pushed the IDEX-Tool-Change-Optomization branch 2 times, most recently from 8ed4420 to a56838b Compare October 7, 2018 02:09
@Roxy-3D
Copy link
Member

Roxy-3D commented Oct 7, 2018

or wait till Roxy can confirm as well then merge. I assume she's still fighting that thermistor issue.

Things are not 100% correct. But I can print now. Do you have any suggestions on things to 'stress' ?

@thinkyhead
Copy link
Member

For Dual X Carriage, try all the things.

@InsanityAutomation
Copy link
Contributor Author

Just constant tool changes. Really this just eliminated the extra down then back up on every tool change, and the fixed 1mm raise regardless of configuration.

@InsanityAutomation
Copy link
Contributor Author

And I agree, all the things! Idex has been getting quite a bit of polish lately.

@thinkyhead thinkyhead force-pushed the bugfix-2.0.x branch 2 times, most recently from 579c7f3 to d52deeb Compare October 9, 2018 21:49
@InsanityAutomation
Copy link
Contributor Author

@Roxy-3D just bumped this to make it merge smoothly again. Let me know if youve had a chance to run it yet.

@InsanityAutomation
Copy link
Contributor Author

Just bumped and retested this, something changed that broke this. The lower is no longer functional. Ill figure it out and update this tonight or tomorrow.

@thinkyhead thinkyhead force-pushed the IDEX-Tool-Change-Optomization branch from 29db2a0 to 56d21c1 Compare October 14, 2018 01:16
@thinkyhead
Copy link
Member

git fetch origin
git checkout IDEX-Tool-Change-Optomization 
git reset --hard origin/IDEX-Tool-Change-Optomization 

@thinkyhead thinkyhead force-pushed the IDEX-Tool-Change-Optomization branch from 56d21c1 to 1235636 Compare October 14, 2018 01:21
@InsanityAutomation
Copy link
Contributor Author

@Roxy-3D working again btw

@Roxy-3D
Copy link
Member

Roxy-3D commented Oct 14, 2018

working again btw

You wouldn't believe all the problems I've been fighting. I'll probably take a day off. But I'll load it up and I have test scripts to test the the Z-Height issue with regard to tool changes. I'll beat on your stuff as soon as I have some energy back.

@InsanityAutomation
Copy link
Contributor Author

I have an espresso machine here, I'll make you a few straight shots :)

Fyi, assuming the lower hotends are the same, e3d chimera heatbreak work well to prevent issues with the lower ptfe tube. Biggest recurring issue I saw with formbot machines.

raised_parked_position[Z_AXIS] += TOOLCHANGE_UNPARK_ZLIFT;
#if ENABLED(MAX_SOFTWARE_ENDSTOPS)
NOMORE(raised_parked_position[Z_AXIS], soft_endstop_max[Z_AXIS]);
#endif
Copy link
Member

Choose a reason for hiding this comment

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

As I recall the reason we added this limit on Z movement was that someone complained about the lack of it. Maybe we should continue to include it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This raise move is being applied in the conditional above covered by a limit statement already. Since the z lower is not being performed between tools, the 2nd z raise is not necessary any longer. Were taking the current position as is, and no longer modifying it so the check is redundant.

@thinkyhead thinkyhead force-pushed the IDEX-Tool-Change-Optomization branch from 1235636 to a736821 Compare October 18, 2018 16:41
Co-Authored-By: InsanityAutomation <insanityautomation@users.noreply.github.com>
@thinkyhead thinkyhead force-pushed the IDEX-Tool-Change-Optomization branch from a736821 to 4999ddb Compare October 18, 2018 16:42
@thinkyhead
Copy link
Member

Rebased. Still holding for testing, but it sure looks simple enough.

@InsanityAutomation
Copy link
Contributor Author

I'll just have to send live Trex to Roxy's to chase her around till it gets tested on that end :)

@InsanityAutomation
Copy link
Contributor Author

FYI, ill leave this sit for any independent testing, but this is superseded by #12137

@InsanityAutomation
Copy link
Contributor Author

Im going to close this, as the other pr is about done and this is no longer relevant to put any attention to.

@InsanityAutomation InsanityAutomation deleted the IDEX-Tool-Change-Optomization branch December 31, 2018 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs: Testing Testing is needed for this change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants