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

Z_RAISE_BEFORE_HOMING -> MIN_Z_HEIGHT_FOR_HOMING #3107

Merged
merged 1 commit into from
Mar 9, 2016

Conversation

AnHardt
Copy link
Member

@AnHardt AnHardt commented Mar 7, 2016

Replace Z_RAISE_BEFORE_HOMING with MIN_Z_HEIGHT_FOR_HOMING

This was made as a fix for #3001. @jbrazio was faster.

Nevertheless i'll take the opportunity to simplify z-raising in G28 a bit.

Make raising z work when AUTO_BED_LEVELING_FEATURE is disabled.
Raise for homing every axis, but only once and only if not already high enough.
Clean out some extra complications when Z_SAFE_HOMING is defined.


// Raise Z before homing any other axes and z is not already high enough (never lower z)
if (current_position[Z_AXIS] <= MIN_Z_HEIGHT_FOR_HOMING) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Clever, the planner may already have an estimation where the z-axis position could be at.

@thinkyhead
Copy link
Member

This looks good to me. I'm always worried with such changes that it will work on some configurations but cause others to behave strangely. How confident are we that this is effective for all configurations?

This feature partly mirrors the "simplified clearance options" that I was working on before. (See MarlinFirmware/MarlinDev#366.) In that PR I used the name Z_RAISE_FOR_OBSTACLES instead of MIN_Z_HEIGHT_FOR_HOMING. Then I had Z_PROBE_CLEARANCE as the amount of clearance needed to deploy or stow a Z probe on a servo arm. While this is more specific about the raise being related to homing, I think that's alright. Homing and probing are really the only two places where these raises will tend to apply.

Once this is updated I will merge it for testing.

Seems we'll need to make another PR for dev soon, collecting together all the patches made to RCBugFix that fix existing issues there.

@Blue-Marlin
Copy link
Contributor

Names are always difficult to me.
It's not exactly a RAISE but a MIN_HEIGHT over the z-boot-position or (later) the bed, now.
A min_height_for_automated_(nonprinting_)xy_moves?
How about MIN_HEIGHT_FOR_OBSTACLES ?

Z_PROBE_CLEARANCE is also not very precise. It could be misunderstood as the space between the bed and a already deployed probe. MIN_HEIGHT_FOR_PROBE_HANDLING?

I'm quite confident it will work for all systems but DELTAs. But DELTAs have their on homing and they never can home towards a z-min-endstop

@jbrazio
Copy link
Contributor

jbrazio commented Mar 8, 2016

I vote in favor of MIN_Z_HEIGHT_FOR_HOMING as I'm not a native English speaker and I find it easy to break down the concept:

  • Minimal
  • Z position
  • Required for homing

What can be misunderstood is, are we speaking about the clearance from the bed to the nozzle or from the bed to the probe ?

@Blue-Marlin
Copy link
Contributor

@jbrazio
The number is the distance from the nozzles tip to the bed. If you want to drive around with a extended probe simply increase the number until the probe has enough play.

We are talking about is a save height to operate the xy-moves, regardless of nozzle or probe.

See bed-screws/clamps as standing obstacles and the probe as hanging one.

rebased
corrected spelling
changed to #elif for error

Still with MIN_Z_HEIGHT_FOR_HOMING but con be done with (folder wide) search-replace when we have a better name.
@AnHardt
Copy link
Member Author

AnHardt commented Mar 8, 2016

Rebased
Corrected spelling
Changed to #elif for error

Still with MIN_Z_HEIGHT_FOR_HOMING but can be done with (folder wide) search-replace when we have a better name.

@AnHardt
Copy link
Member Author

AnHardt commented Mar 8, 2016

Copy, paste, increase one number or add your new name for this define with a 1.
If you want to change your mind decrease the one, increase the other
Poll:

Z_RAISE_BEFORE_HOMING        0
MIN_Z_FOR_HOMING             0
MIN_Z_HEIGHT_FOR_HOMING      0
MIN_HEIGHT_FOR_OBSTACLES     0
Z_RAISE_FOR_OBSTACLES        0
MIN_Z_FOR_AUTOMATED_XY_MOVES 1
MIN_Z_FOR_XY_MOVES           0

@ruggb
Copy link

ruggb commented Mar 8, 2016

The purpose of increasing Z is to make sure it won't hit something while XY homes. It could already be too low, the bed may not be level, there could be a print on the bed.
The function would increase Z with relative positioning, then home XY, then home Z.
I use "INCREASE" because some printers RAISE the bed and some LOWER the nozzle.
I vote for MIN_Z_FOR_HOMING

MIN_Z_FOR_AUTOMATED_XY_MOVES & MIN_Z_FOR_XY_MOVES are not relative. We want It for homing only. If u want it for other moves it can be setup in Repeteir or whatever. I can't change G28.

MIN_HEIGHT_FOR_OBSTACLES & Z_RAISE_FOR_OBSTACLES only implies 1 problem

MIN_Z_HEIGHT_FOR_HOMING adds HEIGHT which is not necessary and may add confusion

Z_RAISE_BEFORE_HOMING uses RAISE which may be LOWER for some printers - like mine

@AnHardt
Copy link
Member Author

AnHardt commented Mar 8, 2016

@ruggb
We are sarching for a name that also could be used for the height/raise/z-min of travel moves in G29, M48, ... . So HOMING seems not to be ideal.
Together with only one other height/raise/z-min what describes the amount of space we need to handle/deploy/stow/retract a probe (servo probe) we hope to eliminate all other Z_RISE_???_PROBING? defines. At the moment we search for a name that also could be used instead of Z_RAISE_BETWEEN_PROBINGS. The other is ought to replace Z_RAISE_BEFORE_PROBING and Z_RAISE_AFTER_PROBING (f.e. MIN_Z_FOR_HANDLING_PROBE).

@ruggb
Copy link

ruggb commented Mar 8, 2016

If it is for something other than homing, then it is being done AFTER homing on a clean bed.
If u r probing then the Z height is probably going to be different then when homing.
Since I don't have ABL implemented it is not on my radar. I just want it for G28.
So any of the other tags would probably go right by me as a variable I can use since it doesn't mention HOMING.

How about 2 separate variables
MIN_Z_FOR_ABL_TRAVEL
and
MIN_Z_FOR_HOMING

@ruggb
Copy link

ruggb commented Mar 8, 2016

Also, wouldn't adding that change the specs of G28 and G29.
I don't know how this works but if it an accepted spec doesn't that get involved.
If the default for both MIN_Z_FOR_ABL_TRAVEL and MIN_Z_FOR_HOMING were 0 then it would conform and no spec change would be necessary. It would just be a feature of Marlin until everyone else figured it was a good idea.

thinkyhead added a commit that referenced this pull request Mar 9, 2016
Z_RAISE_BEFORE_HOMING -> MIN_Z_HEIGHT_FOR_HOMING
@thinkyhead thinkyhead merged commit bedd4da into MarlinFirmware:RCBugFix Mar 9, 2016
@thinkyhead
Copy link
Member

Please roll this for dev also, when you find the time. We will do a round of compare-merges after RC4 to bring dev and RCBugFix into better sync.

@AnHardt
Copy link
Member Author

AnHardt commented Mar 9, 2016

@thinkyhead
Sorry.
I'm missing a merged example without a complain from an other party.
Neither the branch in dev nor the configs to change are clear to me.

Since all the work done there, is done by direct commits and not by PRs and the PRs are piling up.
Since i was forced to use feature branches, that where deleted without warning than.
Since this cleanups permanently breaked my branches,
Since i left the group of collaborators to be not responsible for that crap.

  • i'm not very interested in Dev

Ommmmmmm. Ommmmmmm.
I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore. I'm not angry anymore.

It doesn't help. I'm still angry.

@thinkyhead
Copy link
Member

It's no big deal, we'll compare them along the way.

I'm trying to remember now —since no one has pointed out otherwise— if dev was the cleaned up version of master at one point. I've been making all my PRs for dev and ignoring master, and they had been getting merged.

Anyway, I take it you don't like the new file layout...?

I have been preserving backward compatibility for Marlin 1.1 so it can build on older toolsets, but it is very tempting to use subfolders now that Arduino 1.6.7 allows it. Even if Marlin isn't packaged in library form, I have no problem making Marlin 1.2 dependent on Arduino 1.6.7 and up.

Good news is I believe Dev has gone back to a normal PR-based development model, with no more abuse of direct commits. Everything should now be vetted and more considered. Perhaps some developers will start to float back in.

@jbrazio
Copy link
Contributor

jbrazio commented Mar 10, 2016

The current model gives you a lot of additional manual work, you are the one making sure RCBugFix is in sync with dev, and of course that lags behind because all the bleeding edge users are using RCBugFix to have all the new features.

Nevertheless IMHO the code base between the two has diverged and the development branch seems more like a tech preview than a dev branch, in the other hand RCBugFix seems to be the dev branch.

@AnHardt AnHardt deleted the z_raise_before_homing branch March 10, 2016 14:28
@AnHardt AnHardt restored the z_raise_before_homing branch March 10, 2016 20:31
@AnHardt AnHardt deleted the z_raise_before_homing branch March 10, 2016 20:32
@paulusjacobus
Copy link
Contributor

Nevertheless IMHO the code base between the two has diverged and the development branch seems more like a tech preview than a dev branch, in the other hand RCBugFix seems to be the dev

branch

@jbrazio that is exactly what @AnHardt has warned us all the time for. In a defined and strict project environment, releases will work but in a loose open source development environment without a project release manager things start to diverge. @thinkyhead and @AnHardt are acting as our default project owners and cleaning it up (while issues keeps flowing in). I'm very greatful for the huge amount of work they put into it on a daily basis, without that work Marlin would be dead by now.

@jbrazio
Copy link
Contributor

jbrazio commented Mar 11, 2016

@paulusjacobus I'm a true believer on the bazaar model without well defined goals to be reached. But I also believe that this type of model works better using a rolling release development cycle where there is one repository which gets fine tuned and polished into a version release, afterwards a new dev cycle starts going in (features changes whatever), then polishing starts until another release.. iterate.

Well we're going really off topic here. :-)

@paulusjacobus
Copy link
Contributor

No worries, I'm always mesmerized by the fact that we get something like
Marlin developed and working! I wish we could do that at work... :)

On 11 March 2016 at 11:51, João Brázio notifications@github.com wrote:

@paulusjacobus https://github.com/paulusjacobus I'm a true believer on
the bazaar model without fixed management or well defined goals to be
reached. But I also believe that this type of model works better using a
rolling release development cycle where there is one repository which gets
fine tuned and polished into a version release, afterwards a new dev cycle
starts going in (features changes whatever), then polishing starts until
another release.. iterate.

Well we're going really off topic here. :-)


Reply to this email directly or view it on GitHub
#3107 (comment)
.

@ruggb
Copy link

ruggb commented Mar 11, 2016

@AnHardt
Why did u close this? The z raise has broken babystepping. see #3060

@thinkyhead
Copy link
Member

Why did u close this?

When you merge changes it closes the PR automatically.

The z raise has broken babystepping.

Post hoc ergo propter hoc? I see nothing in this PR that could affect BABYSTEPPING. I suggest we look at the BABYSTEPPING more closely in a new issue out what exactly is broken.

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.

6 participants