Skip to content

Comments

IDEX Improvements#11848

Merged
thinkyhead merged 2 commits intoMarlinFirmware:bugfix-2.0.xfrom
Roxy-3D:bugfix-2.0.x
Sep 17, 2018
Merged

IDEX Improvements#11848
thinkyhead merged 2 commits intoMarlinFirmware:bugfix-2.0.xfrom
Roxy-3D:bugfix-2.0.x

Conversation

@Roxy-3D
Copy link
Member

@Roxy-3D Roxy-3D commented Sep 16, 2018

Many improvements. Everything from cleaner (and less buggy) tool changes to LCD Menu options to control IDEX modes and settings.

Everything here has been well tested.

@thinkyhead
Copy link
Member

Following up on the other discussion… How does it make sense for bed leveling to work in Duplication Mode? It seems to me that it would be wisest to disable leveling compensation when that mode is active.

@Roxy-3D
Copy link
Member Author

Roxy-3D commented Sep 16, 2018

Well... I believe I can make it 'split the difference' between what each nozzle would prefer.
It isn't as good as what you get with a single nozzle active. But it should be better than having no correction.

That is what I'll be looking at soon.

@thinkyhead
Copy link
Member

I'm skeptical that any approach will be useful, but I'll reserve judgment till it's been implemented and tested.

@Roxy-3D
Copy link
Member Author

Roxy-3D commented Sep 16, 2018

:) I think it will work!

@thinkyhead
Copy link
Member

What I'm imagining is a lumpy left side of the bed and a nice flat right side, so that half of the lumpiness gets transmitted to the right side. For dual printing, it would generally be better to add a raft in the slicer and over-extrude on the bottom layer.

@Roxy-3D
Copy link
Member Author

Roxy-3D commented Sep 16, 2018

What I'm imagining is a lumpy left side of the bed and a nice flat right side, so that half of the lumpiness gets transmitted to the right side. For dual printing, it would generally be better to add a raft in the slicer and over-extrude on the bottom layer.

@thinkyhead

Oh ye of little faith!!!! Soon we will know....

I think you can merge #11578 now... This passes Travis! Let's hope the conflicts from #11578 aren't too horrible!

@Roxy-3D
Copy link
Member Author

Roxy-3D commented Sep 17, 2018

I didn't catch this. This can be deleted:

  /*
  SERIAL_ECHO("start of resume_print()\n");
  SERIAL_ECHOPAIR("\ndual_x_carriage_mode:", dual_x_carriage_mode);
  SERIAL_ECHOPAIR("\nextruder_duplication_enabled:", extruder_duplication_enabled);
  SERIAL_ECHOPAIR("\nactive_extruder:", active_extruder);
  SERIAL_ECHO("\n\n");
  */

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we already have a method to accomplish what this is doing, or if needed we can add one to the Planner.

Copy link
Member Author

@Roxy-3D Roxy-3D Sep 17, 2018

Choose a reason for hiding this comment

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

Actually.... I think I can delete that. But the current code is well tested so I left it alone.
The next Pull Request will most likely have that jog back and forth removed. Originally... The reason that was needed was because the steppers.set_direction() wasn't being called everywhere it was needed. But I ----think---- that is not true any more.

@thinkyhead
Copy link
Member

Not quite yet. There are some bugs, which I'm patching now…

Copy link
Member

Choose a reason for hiding this comment

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

How does babystepping behave when there's no active printing?
Did we already add code to activate steppers when babysteps are received? (I think so…)

Copy link
Contributor

Choose a reason for hiding this comment

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

Did we already add code to activate steppers when babysteps are received? (I think so…)

If that was not removed ... . At least in the initial introduction of babystepping that was included.

Copy link
Member Author

@Roxy-3D Roxy-3D Sep 17, 2018

Choose a reason for hiding this comment

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

I have only used that for Z-BabyStepping. But it works just fine. And I have been using that patch for 5 or 6 weeks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can confirm that it works just fine with those additional conditionals removed.

@thinkyhead
Copy link
Member

Rebased and squashed. Proceed with caution.

@Roxy-3D
Copy link
Member Author

Roxy-3D commented Sep 17, 2018

stepper.set_directions() can not be 'private:'. It needs to be available a bunch of places to make sure the extruders are going to head in the right direction. For example... That jog back and forth when setting the IDEX mode... That was needed because without calling stepper.set_directions() depending on which way you were going move things... one of the nozzles would head the wrong direction.

@thinkyhead
Copy link
Member

That was my original patch, but the friend patch was better since it reserved privileges. Making it public is better if it's going to be used in many places. As a low-level behavior, it would be better to have it only get called from functions that are "closer to the metal" and not in any higher-level code.

@AnHardt
Copy link
Contributor

AnHardt commented Sep 17, 2018

Step back and take a look at Marlins structure. Most contributors will nor see it and i can see that from there contributions. And those who can see it, still are embarrassed about it - partially - where they understand it.
Marlins code has much to much side effects. Limiting visibility/usability of functions seems to be into the right direction.
Structure needs room we don't have at the AVRs.
# frustrated_mode ON
My latest discovery: DELTA homing, quick homing, Dual ?-axis dual endstop homing, triple z triple endstop homing are all the same. Home a group of axes until the first stop triggers, then home individually and back off an endstop adjustment. I guess that's distributed to tree functions.
The oldest one is: Bed-, extruder-, chamber- heaters are the same. We could get rid of about half of the heater code. It's just a matter of indexing and allowing the solitair features to all.
For codesize fastio is a killer because you can't make loops over pins. Do we need fastIO in the less time critical parts of the code?
Can't find the relevant code in PR, because covered under n->infinity changes in configs and pins. Shod those be in a separate repo or just in separate commits?
# frustrated_mode OFF

@thinkyhead
Copy link
Member

Too bad we can't program it in Haskell!

@tcm0116
Copy link
Contributor

tcm0116 commented Sep 17, 2018

I think Lisp would be fun!

@thinkyhead
Copy link
Member

LOL. You are clearly a sadomasochist.

@Roxy-3D
Copy link
Member Author

Roxy-3D commented Sep 17, 2018

I think Lisp would be fun!

I know that was a joke. But we aren't re-writing Marlin in Lisp.

@Roxy-3D
Copy link
Member Author

Roxy-3D commented Sep 17, 2018

As a low-level behavior, it would be better to have it only get called from functions that are "closer to the metal" and not in any higher-level code.

Yeah. But the fact is when the IDEX functionality changes modes, we need to get both extruders moving in the right direction. stepper.set_direction() is a clean, easy way to do that. We don't want that to be 'Private:'. We don't want to call that from a bunch of places. But when we need to get all the different things (steppers) on the same page... It needs to be available.

Roxy-3D and others added 2 commits September 17, 2018 00:47
Many improvements.   Everything from cleaner (and less buggy) tool changes to LCD Menu options to control IDEX modes and settings.

Everything here has been well tested.
@thinkyhead
Copy link
Member

We don't want to call that from a bunch of places.

Sorry, I meant for that aspect to be implied by "functions that are 'closer to the metal' and not in any higher-level code."

@thinkyhead thinkyhead merged commit 0780913 into MarlinFirmware:bugfix-2.0.x Sep 17, 2018
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.

4 participants