MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.29k stars 19.24k forks source link

[BUG] Babystepping still not working when idle #13403

Closed ruggb closed 4 years ago

ruggb commented 5 years ago

Marlin-bugfix-2.0.x.9a515cb

I see code change for babystepping in this build I have changed the items flagged with //** below

define BABYSTEPPING //**

if ENABLED(BABYSTEPPING)

//#define BABYSTEP_WITHOUT_HOMING //#define BABYSTEP_XY // Also enable X/Y Babystepping. Not supported on DELTA!

define BABYSTEP_INVERT_Z false // Change if Z babysteps should go the other way

define BABYSTEP_MULTIPLICATOR 4 //** Babysteps are very small. Increase for faster motion.

define DOUBLECLICK_FOR_Z_BABYSTEPPING //** Double-click on the Status Screen for Z Babystepping.

if ENABLED(DOUBLECLICK_FOR_Z_BABYSTEPPING)

#define DOUBLECLICK_MAX_INTERVAL 2250   //** Maximum interval between clicks, in milliseconds.
                                        // Note: Extra time may be added to mitigate controller latency.
#define BABYSTEP_ALWAYS_AVAILABLE       //** Allow babystepping at all times (not just during movement).
//#define MOVE_Z_WHEN_IDLE              // Jump to the move Z menu on doubleclick when printer is idle.
#if ENABLED(MOVE_Z_WHEN_IDLE)
  #define MOVE_Z_IDLE_MULTIPLICATOR 1   // Multiply 1mm by this factor for the move step size.
#endif

endif

//#define BABYSTEP_ZPROBE_OFFSET // Combine M851 Z and Babystepping

if ENABLED(BABYSTEP_ZPROBE_OFFSET)

//#define BABYSTEP_HOTEND_Z_OFFSET      // For multiple hotends, babystep relative Z offsets
//#define BABYSTEP_ZPROBE_GFX_OVERLAY   // Enable graphical overlay on Z-offset editor

endif

endif

this doesn't appear to work

define BABYSTEP_ALWAYS_AVAILABLE //** Allow babystepping at all times (not just during movement).

Or do I need something else changed? BTW wouldn't "printing" be a better description than "movement"

ruggb commented 5 years ago

In looking at Menu.cpp line 229 if (should_babystep && can_babystep)

shouldn't that be if (should_babystep || can_babystep)

as written it would require that I enable //#define BABYSTEP_WITHOUT_HOMING also which doesn't seem right.

ruggb commented 5 years ago

well it works that way...........

boelle commented 5 years ago

as far i know babystepping is a feature jused to adjust height while PRINTING

so it might not be so strange that it does not work while not printing(idle)

boelle commented 5 years ago

personal i dont use babystepping, i adjust my probe offset so that first layer is correct

ruggb commented 5 years ago

@boelle thanks for that info, but it is really pretty irrelevant. I am sure there are lots of things in the program you do not use. Does that mean it doesn't matter if any of them work? If you don't use it, how would you know what ppl use it for? The && is OBVIOUSLY a programming error. There would not be 2 separate defines for a function if you were going to AND them. Do you think you can fix it?

boelle commented 5 years ago

no i'm not a programmer but maybe @thinkyhead ,@p3p or @InsanityAutomation can

ruggb commented 5 years ago

thanks

boelle commented 5 years ago

Maybe edit title so it starts with [BUG] now that we know it's an error

ruggb commented 5 years ago

It is just my assumption but no programmer has confirmed it, though I am 99% sure it is a bug

boelle commented 5 years ago

either that or there are a difference in opinion between you and the programmers on how it should work

time will tell, those 3 guys are basicly working for free so it can be a day or 2 before they chime in

ruggb commented 5 years ago

IF the program is correct, then it is sloppy programming. Why would one set up 2 define options, //#define BABYSTEP_WITHOUT_HOMING

define BABYSTEP_ALWAYS_AVAILABLE // Allow babystepping at all times (not just during movement printing).

then AND them. It is easier to see a mind jerk then sloppy programming. Besides, this has been an issue for a long time. look at #5152, #5217, #5925, #6186, #7998, #10064, #13129. Those are just ones I have participated in.

Does that make sense to you?

boelle commented 5 years ago

what i'm worried most about is if it should work while not printing, that must be the number one concern if the programmers has decided that babystepping should only work when printing then its a question about creating a FR for it to be able to work when not printing also

the //#define BABYSTEP_WITHOUT_HOMING yes... i can see one usecase for it

when you power up the printer it will think the nozzle is a Z0, and thus it will not allow it to move down even if the nozzle is at Zmax

with #define BABYSTEP_WITHOUT_HOMING it would allow you to move Z even if Z has not been homed so it knows there nozzle really is

So yes at least there is a documentation issue, I have been assigned to do documentaion but i'm not even done with configuration.h yet. my plan is to make sure that the documentation website is in sync with the config files so that user can start at the top of both site and file and den work their way down, if the explanation in the config is not enough there will/should be a better explanation on the site

InsanityAutomation commented 5 years ago

@boelle the debate was already had for having the option to make it always available. I do plan to tweak this a little more in the near future, but other tasks are higher priority before MRRF. Ill be back to this soon enough though.

ruggb commented 5 years ago

THAT is the whole point boelle!!!!! YES, it should work while all the time, printing or not. The disclaimer is there - BS does not respect endstops - you MUST enable it to always work, thereby AGREEING to the disclaimer - you are now responsible for screwing things up.

It is a testing/setup tool for me. I use it to fine tune my Zoffset and test the Z0 b4 I even heat the nozzle. I also use it while printing the first layer brim. Part of the problem is that if I cannot get it onto BS until it actually starts printing, I am now potentially into the actual print b4 I can even get to BS.

@boelle GOOD LUCK with documentation! I once tried to do that in V1 = too much changed to keep up with it. V2 is better, but it is still too fragmented and keeping up with docs is going to be a challenge. Since I am not a programmer either, I don't know if it is possible to set this up so that ALL defines are in 1 or 2 files. The way I would like to see it, is that each function is in a separate file. It is getting there, but some ppl keep placing special steppers and other things in the config files.

There are a few defines in other files, like the timeout for the boot screen, that I modify on every commit I use. I suggested moving that to a config file, but got no response.

The problem is that all defines for a specific item/function are not in the same place, sometimes not even in the same file. I do not know why this can't be done, but would like someone to tell me.

Developer think different than users and some appear to believe the only ppl modifying these files are other engineers who speak their language. NOT SO. EG,

define BABYSTEP_ALWAYS_AVAILABLE //** Allow babystepping at all times (not just during movement).

Someone knows why they put "movement" in the above, but that is not what it is, it is "printing". BS creates "movement". There is also too much explanation in the config file and too much code. EG, the dissertation RE thermistors. It just is too much in the config file. The Doc files are the best place for that.

I would be glad to collaborate with you offline if you think it would help. I have done a lot of technical writing in my career.

ruggb commented 5 years ago

@InsanityAutomation I think I can say this is a simple change. I have made it and tested it and it works. I think the logic is self explanitory so it might take 30 sec to fix on the next commit. change && to || in menu.cpp in Marlin-bugfix-2.0.x.9a515cb currently at line 229 if (should_babystep && can_babystep)

InsanityAutomation commented 5 years ago

I'm also planning to setup stepper timeout defer when in the babystepping screen when I'm in it next time, and go over the interaction of move z when idle vs babystepping conditions. I think there's still more room for buttoning up here.

ruggb commented 5 years ago

I am not seeing the need for move Z when Idle. Isn't stepping Z the same thing? Maybe it is just a LCD thing vs a computer thing. Or should it be called move Z before Homing? It is not the same as BS because BS can go negative.

The BS screen simply allows moving Z, Why would you want to add another function to that screen? If I DoubleClick to get to BS, I don't want to have to choose between another function there. That defeats the DC purpose.

InsanityAutomation commented 5 years ago

The difference is when M851 is merged with babystepping. BS then sets a stored value for home offset. Move z when idle moves position without updating this. When position is not known and you just want to move the axis, use jog. When position is known and youre making minor adjustments that you want stored then BS is desired.

If M851 is not tied to babystepping, its irrelevant, but most configurations do enable this. When that flag is on, precision is required and the z probe offset should not be updated when position is not known.

ruggb commented 5 years ago

OK, so I am not aware of M851. Where can I find the detail? I am not using BS to set an offset, mostly because I don't know about it, but I may find it useful. There might be another issue here. Repetier has a problem with Zoffset. Dev is aware of it and has it on schedule to fix, but when?? Not sure if it will affect the BS process.

InsanityAutomation commented 5 years ago

M851 merges the z_probe_offset with the z_offset that babystepping controls, and allows the value to be stored to eeprom. So if I babystep to -0.65, then store settings, power cycle, home and start the next print it is at -0.65 still. It allows setting the gap between the probe and the nozzle, and should be utilized when the probe is the z endstop as well.

ruggb commented 5 years ago

is that merge an enable or is it hard coded? Is the enable currently in the bf commit? If so, where? I haven't stumbled across it.

Roxy-3D commented 5 years ago

I think InsanityAutomation is referring to the #define BABYSTEP_ZPROBE_OFFSET in Configuration_adv.h

ruggb commented 5 years ago

got it - thanks

InsanityAutomation commented 5 years ago

Yup, thats exactly the one I was referring to!

Roxy-3D commented 5 years ago

@boelle

@ruggb always provides actionable and accurate data. Please give him the benefit of the doubt on technical matters.

boelle commented 5 years ago

@ruggb i assume this is still to be fixed?

ruggb commented 5 years ago

Well, I don't know - I changed line 228 in build 9a5125cb by replacing the & with a || and that works for me. But I don't know if y'all fixed it. I just keep replacing that file but have not tried your version lately.

boelle commented 5 years ago

i will have a look at this after current print is done ie i will see if it works out the box or if line 228 in Menu.cpp needs a change if the later is the case i will try and send in a PR to fix it

boelle commented 5 years ago

changing my config to test for this

i remove # in front of:

define BABYSTEPPING

define DOUBLECLICK_FOR_Z_BABYSTEPPING

define BABYSTEP_ALWAYS_AVAILABLE

but then i see

//#define MOVE_Z_WHEN_IDLE // Jump to the move Z menu on doubleclick when printer is idle.

a bit confusing to have babystepping always and then move Z on idle, both should do the same thing right? if i'm wrong let me know

but i will test both cases ie with #define MOVE_Z_WHEN_IDLE commented out and with the # removed

ruggb commented 5 years ago

yea, that confused me also but I just ignored it.

boelle commented 5 years ago

just flashed with the above settings and i can move Z when not printing

define BABYSTEPPING

define DOUBLECLICK_FOR_Z_BABYSTEPPING

define BABYSTEP_ALWAYS_AVAILABLE

ruggb commented 5 years ago

Neat, I'll try it when the sky stops falling...........

boelle commented 5 years ago

Sky is not falling where i live :-D

ruggb commented 5 years ago

OK, my car has now been fixed with the installation of 2 new wiring harnesses at a cost of $8000. due to hungry squirrels My new bed and frame is now fully installed due to a water leak in my 36 yo water bed My bedroom ceiling has now been repaired and the whole room repainted and the causal water leak fixed. My wife's desire to remove the wallpaper and paint the master bath has been completed The rotten wood around my chimney has been replaced and painted The water damaged fascia boards under the gutters have been replaced and painted due to squirrels chewing on my roof shingles My wife is back from her girls vacation My niece has come and gone. Now I can get back to my printer.....................but the garage still need cleaning out of the water bed remains and the roof needs to be replaced.

ruggb commented 5 years ago

@boelle I am sad to say - it is pretty screwed up at this point.

Tests done with BS & dbl click enabled all these tests were done at idle/not printing BS BABYSTEP_ALWAYS_AVAILABLE appears to do nothing. I can't access BS on dbl click when idle = so it is not ALWAYS available. BS MOVE_Z_WHEN_IDLE makes it available anytime, BUT it will not allow movement below Z0. Though that may seem like a good idea, it would be very difficult to get the print closer to the bed on the first layer. You would have to set a negative offset which could cause bigger problems. BABYSTEP_WITHOUT_HOMING is not enabled but I can still BS b4 homing but only raise Z so the switch does nothing..

ALSO, one must wait till compile produces an error to realize that both BABYSTEP_ALWAYS_AVAILABLE and MOVE_Z_WHEN_IDLE are not compatible -- one only.

What do I have to do to get this back the way it was? I want to access BS on dbl click ANYTIME. I want to be able to go below Z0. If you want to limit it, you can set it to 0.5mm or make it settable. IMHO, There is no use for it b4 it is homed so why even have that option?

Now I must figure out how to fix it again the way I want it. Thanks.......

ruggb commented 5 years ago

I found, by accident, what ALWAYS available means. The TUNE menu is ALWAYS available with BS on the bottom where it ALWAYS was. The hole point of DBL Click was to allow quick access. The ALWAYS function is totally useless. DBL Click doesn't function with that enabled.

Roxy-3D commented 5 years ago

@ruggb

The problem used to be the Double Click code would check if the printer was busy... And it would only do the Z-Baby Stepping if the printer was actually printing.

But now...

        #if ENABLED(BABYSTEP_ALWAYS_AVAILABLE)
          constexpr bool should_babystep = true;
        #else
          const bool should_babystep = printer_busy();
        #endif

That limitation has been removed. It sure looks to me like it should work with the 'BABYSTEP_ALWAYS_AVAILABLE' turned on.

Please let me know exactly what you are seeing... And I'll get a correct fix for the problem merged back into the v2.0.0 code base. (But it very likely will take a couple back and forths to figure this out and correct it.)

ruggb commented 5 years ago

1st there was a point when I configured the menu so BS was the first thing after the status screen. The someone, maybe you or Tannu, set up dbl click. That was even better. Then people keep thinking of ways to screw it up. Then I had to change an AND to an OR to get it to work. Now it is getting more difficult to tweak the code -- I am not a programmer. Whoever changed it, obviously, does not use it and ignores user input.

When I enabled BABYSTEP_ALWAYS_AVAILABLE, I saw no change, IE, when I dbl clicked nothing happened, I expected if it was always available, dbl clk would get me there as I had that enabled. Then I was chasing something else and went into the menu and found that the TUNE menu and of course BS was available even though I was not printing. Though, if I am not printing, there is no need to get to the BS function quickly, it is the last thing on the TUNE menu and just a PITA to get to. To me that function is useless. DBL click should get to the BS function all the time without the need to enable BABYSTEP_ALWAYS_AVAILABLE or MOVE_Z_WHEN_IDLE. What is the point??? It just adds useless code. If I enable BS and DBL CLK that should get me to BS with DBL CLK ANY time. There is really no need to put it at the end of the TUNE menu either. In fact, if BS is enabled, DBL CLK should be a default. What is the point of searching thru the menu, printing or not.

I would suggest you consider: Enable BS ==> if enabled access is via DBL CLK ==> ANY TIME. no need to set dbl clk, available all the time or move when idle. I am sure that would reduce the code base. If one has enough brains to operate a 3D printer, then they are smart enough to handle BS if they enabled it and a dbl click would not be accidental - well, it might be, but then they must adjust it.

However, lacking that bit of logic, at least a description of what it really does would be good.

ALSO, I am not sure how I feel about preventing BS from going negative Z. When trying to use it to configure my Z0 point I might need to do that, but I have not run that test yet. Previously, I thought I did that, but I may have fooled myself. I will let you know. thanks

Roxy-3D commented 5 years ago

The someone, maybe you or Tannu, set up dbl click.

Yeah... that was me. That is why I want you to be happy with easy access to Z BabyStepping.

What is the point??? It just adds useless code. If I enable BS and DBL CLK that should get me to BS with DBL CLK ANY time.

Careful here!!! Double Click access to Z-Baby Stepping was very delibertly tied to only working from the main status screen. There is an 'ergonomics' issue here. We are out of ways to alert the firmware to special user input. I added the Double Click branch of the menu tree. But I want other people to be able to use Double Click from other user interaces for specail things.

But I agree with the rest of what you said. Baby Stepping is outside of the firmware. The firmware doesn't know it is happening. Let's not clutter it with a million options.

ALSO, I am not sure how I feel about preventing BS from going negative Z.

Baby Stepping is outside of the firmware. The firmware doesn't know it is happening. The firmware should not be stopping this from happening.

ruggb commented 5 years ago

not sure what you mean, but maybe I confused YOU. I did not mean that BS should be available ANYWHERE in the menu. ANYTIME is not related to ANYWHERE in the menu. If I am not on the status screen, I don't need to dbl click to get to BS. BS should ALWAYS be available with a dbl clk from the status screen.

I thought I went below Z0 previously, but there was an offset then so I guess Marlin was seeing a positive position and allowed a neg movement. However, when I started printing I could go less than the offset, but then there was the layer height.

I need to do my recal and see if not going neg is a hindrance. That would be the only time I might need it to go neg from what Marlin THINKS is Z0. It would be a problem if printing and 0 was really 0. You could wear out a nozzle or PEI pad real fast that way!

In this version, I eliminated the Z_offset as I need to recalibrate that. Right now, if Marlin thinks it is at Z0 or does not know where it is, BS will not allow neg movement. Again, this MAY be fine.

I don't have a problem with requiring a home b4 BS will work. I can't see a reason not to have a ref position of the head when using BS.

Roxy-3D commented 5 years ago

OK! Your issue is probably easy to fix. Do you have software endstops turned on?

// Min software endstops constrain movement within minimum coordinate bounds
#define MIN_SOFTWARE_ENDSTOPS
#if ENABLED(MIN_SOFTWARE_ENDSTOPS)
  #define MIN_SOFTWARE_ENDSTOP_X
  #define MIN_SOFTWARE_ENDSTOP_Y
//#define MIN_SOFTWARE_ENDSTOP_Z
#endif

Incidentally... the mesh bed leveling systems need the Z software endstops turned off also. The reason is there might be areas of the bed that are little bit lower than the location where you home the Z-Axis. And in that case... The nozzle needs to go below 0.00mm to trace the contour of the bed.

ruggb commented 5 years ago

no - I use 3 point leveling

Roxy-3D commented 5 years ago

Do you have MIN_SOFTWARE_ENDSTOPS_Z active? If so, that will affect Z-BabyStepping.

ruggb commented 5 years ago

I do not have any software endstops enabled. Are you saying I should?

Roxy-3D commented 5 years ago

No. But turning on Software Endstops for the Z-Axis would explain why you are not able to baby step below 0.00 mm.

ruggb commented 5 years ago

I just searched for MIN_SOFTWARE_ENDSTOPS_Z and cannot find it. Of course, it is MIN_SOFTWARE_ENDSTOP_Z and those are enabled by default - WHY? So I guess, by default, I have them enabled. Which is interesting because X&Y only apply to cartesian bots and mine is a coreXY. So why are they default on? I'll try turning them all off next load. Or I guess I can try M211. Except I don't know what [Sflag] is.

ruggb commented 5 years ago

Yes, I figured it out and with them off it will go neg without a home now. Y'all snuck that in on me when I wasn't looking. Why are they default on? Got to be a logical reason.

ruggb commented 5 years ago

I left sw endstops on in Marlin. Turn off via M211 works fine if I need them off. Move Z when idle gives BS dbl clk anytime Though I don't see a need for the switch. BS should be on all the time if enabled and access should be by dbl clk on the status menu. BS anytime is a useless switch. Having BS in the tune menu is also useless. so you could eliminate 3 option switches and have full functionality for BS.

Roxy-3D commented 5 years ago

Yes, I figured it out and with them off it will go neg without a home now.

Good!

Y'all snuck that in on me when I wasn't looking. Why are they default on? Got to be a logical reason.

No... Software Endstops have been in there and turned on by default for many years. I know because when I did UBL, I couldn't figure out why the nozzle wasn't tracking the bed in certain places. It turns out, those places were where the bed went below 0.00 mm.

The reason they are on by default is because that is the safest way to have things for new machines. We really do try hard to protect people's printers from killing themselves.

Anyway... I'm glad we have that figured out!

BS anytime is a useless switch. Having BS in the tune menu is also useless.

I tend to agree. I think it ought to be available all the time and that is the end of the discussion.

But it is helpful in the tune menu. I hardly ever use it. But every once in a while, it does get used.

Good Luck!

ruggb commented 5 years ago

If it is available ALL the time with a dbl click, there is no need for it to be in the tune menu. It is redundant at that point. OK, I wasn't paying attention and just ignored sw endstops.