search

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.34k stars 19.26k forks source link

[BUG] [FRONT/BACK/RIGHT/LEFT]_PROBE_BED_POSITION missing #15369

Closed marcio-ao closed 4 years ago

marcio-ao commented 5 years ago

The variables FRONT_PROBE_BED_POSITION, BACK_PROBE_BED_POSITION, RIGHT_PROBE_BED_POSITION and LEFT_PROBE_BED_POSITION are now missing in the configs.

We need these values in order to be able to adjust the four probe locations for our printers.

marcio-ao commented 5 years ago

@InsanityAutomation: It looks like you made this change in commit df1e51258a. How the heck are we supposed to specify our probe positions now? Each one of our FW builds carefully control the probe points so they land on our washers. This does not necessarily have any relation to the center of the bed or the endstop locations.

tgag25 commented 5 years ago

The variables FRONT_PROBE_BED_POSITION, BACK_PROBE_BED_POSITION, RIGHT_PROBE_BED_POSITION and LEFT_PROBE_BED_POSITION are now missing in the configs.

bed 200x300 offset x 65mm offset y 20

InsanityAutomation commented 5 years ago

See #15372 and #15361 for further info on whats being done around this one

Roxy-3D commented 5 years ago

It looks like you made this change in commit df1e512. How the heck are we supposed to specify our probe positions now?

@marcio-ao I do understand your frustration. But PLEASE... understand there is a lot of flux in the code base as we prepare for the v2.0.0 release. His initial commit was available for 2 weeks prior to it being merged. There was plenty of time to comment before the merge happened.

Independent of that... You are probing 4 points and not 3. So that implies very strongly you already had code changes that you were tracking and laying on top of the head of the code base.

This isn't a crisis. Both Thinky and I want you to have what you need to do your job. A nice "Please change this..." will accomplish that. Let's work as a team. I don't like everything that is happening in this Pull Request (for different reasons). But the intent is good. Let's just work together to get everything the way it needs to be so everybody is happy.

OK?

marcio-ao commented 5 years ago

@marcio-ao I do understand your frustration. But PLEASE... understand there is a lot of flux in the code base as we prepare for the v2.0.0 release.

I apologize for getting angry on this. Removing configuration options that people depend, and especially options that have existed for years, is something that needs to be done with care. In particular, it is standard practice in Marlin to issue a warning in "Conditionals_post.h" that an option has been deprecated and to have that message suggest an alternative. The fact that this message did not exist meant I had no way to get my configurations working again.

For what it is worth, @InsanityAutomation also renamed Z_PROBE_Z_OFFSET and he did add a deprecation warning, which gave me an alternative and helped me fix my configs. I assume it was a mistake that no such warnings and alternatives were made for LEFT_PROBE_BED_POSITION.

This isn't a crisis. Both Thinky and I want you to have what you need to do your job. A nice "Please change this..." will accomplish that. Let's work as a team.

I'm sorry for the way I wrote my initial post, it was uncalled for.

Independent of that... You are probing 4 points and not 3. So that implies very strongly you already had code changes that you were tracking and laying on top of the head of the code base.

The changes are small and do not affect the probing process. All it does is print a message on the last probe point -- I did not consider that anyone would really benefit from it, especially since it wasn't general enough to work with anything other than four points.

@marcio-ao I do understand your frustration. But PLEASE... understand there is a lot of flux in the code base as we prepare for the v2.0.0 release.

True, unfortunately my company has already released several products to customers based on Marlin 2.0. If I had a choice in the matter, I would say this was ill advised, but that was not my choice to make, alas.

His initial commit was available for 2 weeks prior to it being merged. There was plenty of time to comment before the merge happened.

Ah, there's a Douglas Adams quote that seems appropriate here :) Perhaps a bit of an exaggeration, but the truth of the matter is that most people (including myself) will only get a chance to test code once it has been merged.

InsanityAutomation commented 5 years ago

I apologize for getting angry on this. Removing configuration options that people depend, and especially options that have existed for years, is something that needs to be done with care. In particular, it is standard practice in Marlin to issue a warning in "Conditionals_post.h" that an option has been deprecated and to have that message suggest an alternative. The fact that this message did not exist meant I had no way to get my configurations working again.

For what it is worth, @InsanityAutomation also renamed Z_PROBE_Z_OFFSET and he did add a deprecation warning, which gave me an alternative and helped me fix my configs. I assume it was a mistake that no such warnings and alternatives were made for LEFT_PROBE_BED_POSITION.

FWIW In my PR I stated which methods I had tested and was looking for the usual Thinky refactoring and revision and sort of direction before continuing. It wasn't completely done and some of the hasty followups here are meant to lessen the impact of that.

His initial commit was available for 2 weeks prior to it being merged. There was plenty of time to comment before the merge happened.

Ah, there's a Douglas Adams quote that seems appropriate here :) Perhaps a bit of an exaggeration, but the truth of the matter is that most people (including myself) will only get a chance to test code once it has been merged.

I understand that, and would only point to the time it was available as a tone changer to the issue, not that it excludes any resolution to one.

Personally I feel what you need can be accomplished with what is set now, but as I do have hardware I will get a working config and pass it to you. If you dont agree with how its done at that point I dont oppose making more changes to suit.

InsanityAutomation commented 5 years ago

Also, I dont consider whats open now to be "complete" yet either, but it does resolve all known build issues so should go in quickly. I did not originally have tons of time set aside for this stuff this week, so im crunching on as much as I can as quickly as possible around a day job not related to printing and commitments with kids ect.

Ill get everything situated and working on one of my Taz6 machines though so at least I can make any changes as painless as possible.

marcio-ao commented 5 years ago

For me, what would be helpful would be a recipe along the lines of "to place the probe at [LEFT|RIGHT|FRONT|BACK]_PROBE_BED_POSITION, adjust the following configuration options X, Y, Z like so"

I use a python script to generate our 50 or so different configurations, so if I had equations mapping the old parameters to the new, then it would be fairly simple for me to put that into the python scripts.

InsanityAutomation commented 5 years ago

For me, what would be helpful would be a recipe along the lines of "to place the probe at [LEFT|RIGHT|FRONT|BACK]_PROBE_BED_POSITION, adjust the following configuration options X, Y, Z like so"

I use a python script to generate our 50 or so different configurations, so if I had equations mapping the old parameters to the new, then it would be fairly simple for me to put that into the python scripts.

@marcio-ao I didnt look for the python script, is it in the repo? If so ill take a look. MIN_PROBE_EDGE_LEFT/FRONT should map directly to LEFT/FRONT_PROBE_BED_POSITION where RIGHT/BACK would just be the inverse (XY_BED_SIZE - RIGHT/BACK_PROBE_BED_POSITION) and you would kill the sanity check for keeping them all on the bed for left because its negative in youre configs. The other option is shifting workspace over 3mm so its at 0, but thats probably more work than its worth to defeat 1 sanity check!

marcio-ao commented 5 years ago

@InsanityAutomation: The script is here:

https://code.alephobjects.com/diffusion/MARLIN/browse/devel/config/examples/AlephObjects/build-config.py

You'll also find all the config files in there as well.

Definitely MIN_PROBE_EDGE_LEFT/FRONT along with XY_BED_SIZE would allow us me to fix the config files, but these options don't appear to be in bugfix-2.0.x branch quite yet.

Also, just an FYI, the next printer we are working on will have a rectangular bed. I would recommend breaking up XY_BED_SIZE into X_BED_SIZE and Y_BED_SIZE.

InsanityAutomation commented 5 years ago

@InsanityAutomation: The script is here:

https://code.alephobjects.com/diffusion/MARLIN/browse/devel/config/examples/AlephObjects/build-config.py

You'll also find all the config files in there as well.

Definitely MIN_PROBE_EDGE_LEFT/FRONT along with XY_BED_SIZE would allow us me to fix the config files, but these options don't appear to be in bugfix-2.0.x branch quite yet.

Also, just an FYI, the next printer we are working on will have a rectangular bed. I would recommend breaking up XY_BED_SIZE into X_BED_SIZE and Y_BED_SIZE.

I meant to put a / between them as they are separate and exist now! :) The open PR (#15372 ) has the values referenced for the probe as well as the last bit of compilation fixes done.

Thats where I found the config file and must have skipped right past the script in my beeline to it!

tgag25 commented 5 years ago

OK i understand

define MANUAL_X_HOME_POS -65

define MANUAL_Y_HOME_POS -20

define NOZZLE_TO_PROBE_OFFSET { 8, 64, -1.8 }

define Z_SAFE_HOMING

if ENABLED(Z_SAFE_HOMING)

define Z_SAFE_HOMING_X_POINT ((X_BED_SIZE) / 2) // X point for Z homing when homing all axes (G28).

define Z_SAFE_HOMING_Y_POINT ((Y_BED_SIZE) / 2) // Y point for Z homing when homing all axes (G28).

endif

my printer is an DIY printer glass bed 200x300mm zmax 320mm cartesian

thanks for this nice and good progran

InsanityAutomation commented 5 years ago

OK i understand

define MANUAL_X_HOME_POS -65

define MANUAL_Y_HOME_POS -20

These will automatically get set to the MIN position for the axis if not set, so no need to use them unless home isnt really an end (eg a side flag sensor).

define X_MIN_POS -65

define Y_MIN_POS -20

This is more appropriate.

tgag25 commented 5 years ago

ok it is a very nice and good program

thanks

ZS1FJK commented 5 years ago

Hello Just thought id chime in and say i am a bit stuck now and don't know how to proceed with getting the new firmware to work without these commands. `#define LEFT_PROBE_BED_POSITION

define RIGHT_PROBE_BED_POSITION

define FRONT_PROBE_BED_POSITION

define BACK_PROBE_BED_POSITION `

I have a Lulzbot Taz 6.

InsanityAutomation commented 5 years ago

Hello Just thought id chime in and say i am a bit stuck now and don't know how to proceed with getting the new firmware to work without these commands. #define LEFT_PROBE_BED_POSITION #define RIGHT_PROBE_BED_POSITION #define FRONT_PROBE_BED_POSITION #define BACK_PROBE_BED_POSITION I have a Lulzbot Taz 6.

That's the specific example we've been discussing. See the open pull request referenced above, as well as formula to convert old setting to new.

marcio-ao commented 5 years ago

@InsanityAutomation: Can you repost the most up-to-date information here? There are too many other places with info and it's very confusing. This is probably the ticket people will be looking at when they run into the problem so it would help to have a summary right here.

marcio-ao commented 5 years ago

I'll try figuring out to fix our config generator script on Monday once the dust has settled on this :)

tgag25 commented 5 years ago

http://marlinfw.org/meta/download/

InsanityAutomation commented 5 years ago

@marcio-ao yeah, with any luck that PR will get merged tonight with or without sanity checks and tomorrow I'll be able to simply highlight the conversion discussion above!

Really not trying to make you turn to tomato farming here!

tgag25 commented 5 years ago

It is an affair of specialist y dont give any response

ZS1FJK commented 5 years ago

Hello Just thought id chime in and say i am a bit stuck now and don't know how to proceed with getting the new firmware to work without these commands. #define LEFT_PROBE_BED_POSITION #define RIGHT_PROBE_BED_POSITION #define FRONT_PROBE_BED_POSITION #define BACK_PROBE_BED_POSITION I have a Lulzbot Taz 6.

That's the specific example we've been discussing. See the open pull request referenced above, as well as formula to convert old setting to new.

Thank you

tgag25 commented 5 years ago

http://marlinfw.org/meta/download/bugfix-2.0.x.zip

tgag25 commented 5 years ago

http://marlinfw.org/meta/download/

bugfix-2.0.x.zip

InsanityAutomation commented 5 years ago

@tgag25 I have no idea what you are looking for and getting at. If you need support and are not reporting a software bug, please see the resources normally reserved for support :

This Issue Queue is for Marlin bug reports and development-related issues, and we prefer not to handle user-support questions here. (As noted on this page.) For best results getting help with configuration and troubleshooting, please use the following resources MarlinFW.org Marlin Documentation RepRap.org Marlin Forum Tom's 3D Forums Facebook Group "Marlin Firmware" Facebook Group "Marlin Firmware for 3D Printers" Marlin Configuration on YouTube Marlin Discord server. Join link: https://discord.gg/n5NJ59y

After seeking help from the community, if the consensus points to to a bug in Marlin, then you should post a bug report.

InsanityAutomation commented 5 years ago

@marcio-ao at a glance, if you updated the lines at 1370 - 1386 following the formula mentioned above, you could then change the MARLIN var name on lines 1409-1412 and 1436/1442 and remove lines 1416-1422

if MINI_BED:
  STANDARD_LEFT_PROBE_BED_POSITION             = -3
  STANDARD_RIGHT_PROBE_BED_POSITION            = 163
  STANDARD_BACK_PROBE_BED_POSITION             = 168
  STANDARD_FRONT_PROBE_BED_POSITION            = -4
elif TAZ_BED:
  STANDARD_LEFT_PROBE_BED_POSITION             = -10
  STANDARD_RIGHT_PROBE_BED_POSITION            = 288
  STANDARD_BACK_PROBE_BED_POSITION             = 291
  STANDARD_FRONT_PROBE_BED_POSITION            = -9

Would become

if MINI_BED:
  STANDARD_MIN_PROBE_EDGE_LEFT             = -3
  STANDARD_MIN_PROBE_EDGE_RIGHT           = (STANDARD_X_BED_SIZE + 8)
  STANDARD_MIN_PROBE_EDGE_BACK             = (STANDARD_Y_BED_SIZE + 13)
  STANDARD_MIN_PROBE_EDGE_FRONT            = -4
elif TAZ_BED:
  STANDARD_MIN_PROBE_EDGE_LEFT             = -10
  STANDARD_MIN_PROBE_EDGE_RIGHT            = (STANDARD_X_BED_SIZE + 8)
  STANDARD_MIN_PROBE_EDGE_BACK             = (STANDARD_Y_BED_SIZE + 11)
  STANDARD_MIN_PROBE_EDGE_FRONT            = -9
marcio-ao commented 5 years ago

@InsanityAutomation: Humm, so is it correct to say you merely renamed "[SIDE]_PROBE_BED_POSITION" to "MIN_PROBEEDGE[SIDE]"?

marcio-ao commented 5 years ago

@InsanityAutomation: In "Configuration.h", on "bugfix-2.0.x", this is how MIN_PROBE_EDGE is defined:

// Certain types of probes need to stay away from edges
#define MIN_PROBE_EDGE 10

So, it is a distance from the edge of the bed. From you example, it looks like you may have changed the meaning of "EDGE" to mean an absolute position, which is exactly what "PROBE_BED_POSITION" used to be.

So, either your formula above is incorrect, or the names of the variables are now incorrect and unclear.

InsanityAutomation commented 5 years ago

It may seem like it's the same on a system with no offset for the probe where you are using the nozzle but in actuality once you start the finding probe offsets it functions quite a bit differently. Your offsets will all come out to be zero so it will turn into an absolute position. If you have a probe that has an offset of 30 mm for example on the right hand side, that value of -3 for the left edge would end up being ignored as the probe offset will not allow it to get past x 7. With the old values being a fixed position you would get probing aborted as it could not reach the point.

This comes back to the comment I made a while ago about as a cleanup and memory saver adding a toggle for fixed nozzle probing that disable setting X and Y offsets and a few more descriptions around setups like yours to make sense with these values.

marcio-ao commented 5 years ago

@InsanityAutomation: I think you are misunderstanding my concern. I am referring to these lines:

if MINI_BED:
  STANDARD_MIN_PROBE_EDGE_LEFT             = -3
  STANDARD_MIN_PROBE_EDGE_RIGHT           = (STANDARD_X_BED_SIZE + 8)
  STANDARD_MIN_PROBE_EDGE_BACK             = (STANDARD_Y_BED_SIZE + 13)
  STANDARD_MIN_PROBE_EDGE_FRONT            = -4

Since STANDARD_X_BED_SIZE and STANDARD_Y_BED_SIZE are both 155, this becomes:

if MINI_BED:
  STANDARD_MIN_PROBE_EDGE_LEFT             = -3
  STANDARD_MIN_PROBE_EDGE_RIGHT           = 163
  STANDARD_MIN_PROBE_EDGE_BACK             = 168
  STANDARD_MIN_PROBE_EDGE_FRONT            = -4

So this is saying the right probe happens 163 mm left from the right edge of bed, which isn't correct.

InsanityAutomation commented 5 years ago

@marcio-ao I see, my mistake! I stopped a step short last night when putting that together.... I mustve been half asleep by then...

if MINI_BED:
  STANDARD_MIN_PROBE_EDGE_LEFT             = -3
  STANDARD_MIN_PROBE_EDGE_RIGHT           = -8
  STANDARD_MIN_PROBE_EDGE_BACK             = -13
  STANDARD_MIN_PROBE_EDGE_FRONT            = -4
elif TAZ_BED:
  STANDARD_MIN_PROBE_EDGE_LEFT             = -10
  STANDARD_MIN_PROBE_EDGE_RIGHT            = -8
  STANDARD_MIN_PROBE_EDGE_BACK             = -11
  STANDARD_MIN_PROBE_EDGE_FRONT            = -9
marcio-ao commented 5 years ago

@InsanityAutomation: So, I think this is the required voodoo:

if MINI_BED:
  STANDARD_LEFT_PROBE_BED_POSITION             = -3
  STANDARD_RIGHT_PROBE_BED_POSITION            = 163
  STANDARD_BACK_PROBE_BED_POSITION             = 168
  STANDARD_FRONT_PROBE_BED_POSITION            = -4
  STANDARD_X_BED_SIZE                          = 155
  STANDARD_Y_BED_SIZE                          = 155
elif TAZ_BED:
  STANDARD_LEFT_PROBE_BED_POSITION             = -10
  STANDARD_RIGHT_PROBE_BED_POSITION            = 288
  STANDARD_BACK_PROBE_BED_POSITION             = 291
  STANDARD_FRONT_PROBE_BED_POSITION            = -9
  STANDARD_X_BED_SIZE                          = 280
  STANDARD_Y_BED_SIZE                          = 280

  MIN_PROBE_EDGE_LEFT             = STANDARD_LEFT_PROBE_BED_POSITION
  MIN_PROBE_EDGE_RIGHT            = STANDARD_X_BED_SIZE - STANDARD_RIGHT_PROBE_BED_POSITION
  MIN_PROBE_EDGE_BACK             = STANDARD_Y_BED_SIZE - STANDARD_BACK_PROBE_BED_POSITION
  MIN_PROBE_EDGE_FRONT            = STANDARD_FRONT_PROBE_BED_POSITION

So, here is a suggestion. Instead of having users (or my script) deal with all this jiggery pokery, why not add something to "Conditionals_post.h" so that if [LEFT|RIGHT|BACK|FRONT]_PROBE_BED_POSITION is defined, it automatically applies these formulas to compute MIN_PROBE_EDGE[LEFT|RIGHT|BACK|FRONT]?

InsanityAutomation commented 5 years ago

Because it implies that they're static, when the only time they will be is with the very uncommon nozzle as probe or piezo electric sensors. In any other situation theye dynamic following the probe offsets as configured with M851XY.

marcio-ao commented 5 years ago

@InsanityAutomation: I see. Okay, I guess I can live with this compromise of putting the formulas in my Python script then.

InsanityAutomation commented 5 years ago

Yeah, kindve tricky I agree. Just with the drastic change in function becoming dynamic it needed to be something adaptive. Leaving the values the same with the notably different function I felt would cause mass confusion when they change the offset and suddenly it moves when they assumed it was fixed.

I know most of what you work with is the fixed nozzle probe, but in the rest of the industry theyre just rare so not the first and foremost implementation when developing. Just trying to put that in perspective.

That said, though I like the nozzle as probe setups in concept, I have seen very few other machines implement them, and I have not seen another implementation besides yours I can say I like. Its a fine line between hot enough to soften any filament (PC takes alot!) and not burning holes in the bed and the washers are fairly unique.

Im sure we will run into more of this stuff as the future moves to parity with purely gcode configured systems (RRF for example). Obviously I think some things dont make sense to change at run time (bed size), but others will im sure.

marcio-ao commented 5 years ago

I know most of what you work with is the fixed nozzle probe, but in the rest of the industry theyre just rare so not the first and foremost implementation when developing.

That's funny. My first 3D printer used the nozzle for probing (using force sensitive resistors under the bed) and then I came to Lulzbot and all our printers use the nozzle for probing (conductive). So I've never experienced anything other than a fixed nozzle :)

InsanityAutomation commented 5 years ago

My first one was wood and a probe would have seemed space age to it! Lol Nowadays aside from yours, I have machines from Creality, Raise3d, Formbot (Vivedino), Anet, Mamarubot (Hieha), Tronxy, Artillery (Evnovo), and maybe one or two more I'm forgetting lol

boelle commented 5 years ago

i had the same problem, my bed has 4 probepoints and i spend a lot of time setting up bi-linear so that the probe hits the points. its the mk42 bed and the probe points are not centered on the bed

i ended up adding a command in the code so i dont forget it

define GRID_MAX_POINTS_X 3

define GRID_MAX_POINTS_Y GRID_MAX_POINTS_X

//G29 X3 Y3 L35 R238 F7 B203 to probe bed

// Probe along the Y axis, advancing X after each column //#define PROBE_Y_FIRST

but back to this one, since the config has been removed deliberate, whatever it was a smart move or not can be up for debate, but does it not make this one "not a bug" ?

ZS1FJK commented 5 years ago

Hello I Have set the following and it dose not seem to change the probing position.

#define MIN_PROBE_EDGE -20

#define AUTO_BED_LEVELING_LINEAR

` #define MIN_PROBE_EDGE_LEFT -9

define MIN_PROBE_EDGE_RIGHT 9

define MIN_PROBE_EDGE_FRONT -9

define MIN_PROBE_EDGE_BACK 9`

ZS1FJK commented 5 years ago

Negative values do not work. It wont probe outside bed perimeter .

boelle commented 5 years ago

@marcio-ao do you think there is anything we can do about this one?

InsanityAutomation commented 5 years ago

As the lulzbot code will likely end up getting all master branch functional references soon, we will probably make a toggle that will override the sanity checks and limits on it for the rare machines in that configuration.

marciot commented 5 years ago

I am no longer employed by LulzBot. I set up a branch of LulzBot Marlin called "drunken-octopus-marlin" but I can only afford to work on it an hour or two each month, not really enough to troubleshoot anything of this complexity. Right now I am doing some UI consulting work for Cocoa Press and I need to focus my energies on that. If the probing issue gets resolved upstream, I can do a pull and incorporate the changes, but other than that, I am afraid I might be of limited assistance from now on.

marciot commented 5 years ago

I just confirmed that this is still broken in the latest Marlin checkout. Please fix this. This is a pretty major regression in code that was working fine in Marlin before.

boelle commented 5 years ago

@shitcreek would you not agree with @marciot ?

kurtisane commented 5 years ago

Any temporary Solution for this issue ?

InsanityAutomation commented 5 years ago

This will be towards the top of my list when the day job stops being 7 days a week...

InsanityAutomation commented 5 years ago

@kurtisane a PR is open with a code change for it. #15929 Give that a shot if you get a chance

boelle commented 4 years ago

@kurtisane did you try #15929 ?

boelle commented 4 years ago

Lack of Activity This issue is being closed due to lack of activity. If you have solved the issue, please let us know how you solved it. If you haven't, please tell us what else you've tried in the meantime, and possibly this issue will be reopened.

ALSO: the mentioned config options are not missing but they where removed