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

Allow Repeatability Test from LCD to test in place #19104

Merged
merged 11 commits into from
Aug 22, 2020
Merged

Allow Repeatability Test from LCD to test in place #19104

merged 11 commits into from
Aug 22, 2020

Conversation

GMagician
Copy link
Contributor

@GMagician GMagician commented Aug 21, 2020

When probe is on nozzle left and home is at X & Y min the LCD M48 will no work because the automatic G28 will move the probe out of area.

Added a new C parameter to M48 to let it move to bed center

Do G28 O before M48 so it only homes when needed and M48 can be done at the current position.

Fix #19101

When probe is on nozzle left and home is at x & Y min the LCD M48 will no works because of automatic G28 will move the prove out of area.

Added a new C parameter to M48 to let it move to bed center
@AnHardt
Copy link
Contributor

AnHardt commented Aug 21, 2020

This makes menu selected M48 inflexible. Now it always and only probes in the center. Probing somewhere else is impossible now.

@GMagician
Copy link
Contributor Author

@AnHardt the old wasn't flexible in the same way. It always probe on homing position...and some system can't

@GMagician
Copy link
Contributor Author

@AnHardt maybe you missed the G28 in front. Maybe a better solution is not to have it, or maybe a G28 O

@thisiskeithb
Copy link
Member

Sending the error message to the LCD might be useful as well since it only sends it out over serial:

if (!probe.can_reach(probe_pos)) {
SERIAL_ECHOLNPGM("? (X,Y) out of bounds.");
return;
}

@GMagician
Copy link
Contributor Author

GMagician commented Aug 21, 2020

@thisiskeithb the problem is that with G28 done before you wil never be able to execute it, even if you know why.

Maybe maybe the best solotion might be G28 O. In this way if you have a not homed system it will behave as now. If home has been done M48 will be done in situs

@qystan
Copy link

qystan commented Aug 21, 2020

@AnHardt. This is a probe deviation test, the position on the bed where the test is done should not matter.

@GMagician
Copy link
Contributor Author

GMagician commented Aug 21, 2020

@thisiskeithb I added a lcd error message but I'm not sure it's needed when M48 is executed via LCD since now it executes on center of the bed.
Maybe with previous implementation was needed but, since previous implementation was never able to do it (only when probe is next to home side), don't know if that code has to be left as it was

@Roxy-3D
Copy link
Member

Roxy-3D commented Aug 21, 2020

@AnHardt. This is a probe deviation test, the position on the bed where the test is done should not matter.

If the mechanics of the machine were perfect... It is true the position on the bed would not matter. But then again, if the machine's mechanics were perfect, we wouldn't even need this test.

It is important to allow the probe to be in different positions for the test. Different locations will produce different results. For example, a Capacitive or inductive probe near the edge of the bed compared to the center of the bed. Or probing a corner versus the center with a probe that does not have a light touch. It is very easy to construct cases where the location changes the repeatability of the probe.

Even probing at the same location will give different results if the Star or Skizoid option is invoked due to back lash of the belts and bearings.

@qystan
Copy link

qystan commented Aug 21, 2020

That is true but then the probing test is not determining the repeatability of the probe but determining the stability of the bed or suitability of the position to perform the repeatability test.

That being the case, wouldn't choosing the center of the bed be the safest choice to test the probe?

@GMagician
Copy link
Contributor Author

GMagician commented Aug 21, 2020

@Roxy-3D I see only 2 fast solutions now:

  1. Remove G28 and force user to home and position probe where he wants
  2. Replace G28 with G28 O and in this case leave the new lcd error message

Please consider that previous code (G28 + M48) was already probing always in the same point

@qystan
Copy link

qystan commented Aug 21, 2020

My thoughts are that the menu items make useful features accessible to new users. It should work with minimal user intervention or provide guiding steps to complete the task. Error messages should be simple and provide next steps to prompt the user.

@Roxy-3D
Copy link
Member

Roxy-3D commented Aug 21, 2020

That is true but then the probing test is not determining the repeatability of the probe but determining the stability of the bed or suitability of the position to perform the repeatability test.

M48's purpose is to determine the repeatability of the probing system. Not just the probe. Perhaps the documentation needs to be scrubbed with that thought in mind.

@Roxy-3D
Copy link
Member

Roxy-3D commented Aug 21, 2020

G28 O would still allow the user to reposition the probe in a different location prior to invoking the M48. That would be my preference.

@qystan
Copy link

qystan commented Aug 21, 2020

That is true but then the probing test is not determining the repeatability of the probe but determining the stability of the bed or suitability of the position to perform the repeatability test.

M48's purpose is to determine the repeatability of the probing system. Not just the probe. Perhaps the documentation needs to be scrubbed with that thought in mind.

Wow, that is a very much bigger scope and the user should basically probe the expected/suspected weak points to determine the suitability of the probing system for use.

Yes, the documentation will need amendments to reflect and will be useful if it provides guidance on the use.

@GMagician
Copy link
Contributor Author

Ok new proposal.
G28 has been replaced with G28 O, now home is done only if required, otherwise test is done where nozzle is.
For systems with probe out of bounds, after home, the new lcd message should help to detect why nothing happens

@Roxy-3D
Copy link
Member

Roxy-3D commented Aug 21, 2020

Wow, that is a very much bigger scope and the user should basically probe the expected/suspected weak points to determine the suitability of the probing system for use.

I suggest you try the S option on M48 and see how the jitter of the X & Y axis affects the numbers you get. It maybe your machine is immune to the extra backlash those movements cause. But some number of machines do get noticeably worse repeatability of the probed values when S is used. (Cartesian machines seem less affected than Delta's and Core-XY machines).

But anyway... That extra movement (as an option) should help convince a person that M48's purpose was more than just the probe's repeatability...

It would be good if we could get some of the documentation to reflect that better. And for that matter... More comments in the M48 code might be a good idea too. If there are any volunteers willing to tackle that task, I'm all for it!

@thinkyhead thinkyhead changed the title Let LCD M48 to always work Allow Repeatability Test from LCD to test in place Aug 22, 2020
@qystan
Copy link

qystan commented Aug 22, 2020

Taking out G28 and not fixing the probe location looks like the best solution. It allows selection of probing positions to provide the flexibility of running a probe test or probe/bed/system system test to investigate bed probing results.

@thinkyhead thinkyhead merged commit 6b549e1 into MarlinFirmware:bugfix-2.0.x Aug 22, 2020
@Roxy-3D
Copy link
Member

Roxy-3D commented Aug 22, 2020

Taking out G28 and not fixing the probe location looks like the best solution. It allows selection of probing positions...

Or... It may make sense to actually do a G28 if needed... But return back to the current coordinates and continue. There is room to decide what the desired behavior is and start moving towards it. M48 first appeared in the Marlin firmware and its functionality was copied by the other 3D Printer firmwares.

If it makes sense to change M48 to make it better.... There is no reason not to migrate to the preferred behaviour.

@qystan
Copy link

qystan commented Aug 22, 2020

Taking out G28 and not fixing the probe location looks like the best solution. It allows selection of probing positions...

Or... It may make sense to actually do a G28 if needed... But return back to the current coordinates and continue. There is room to decide what the desired behavior is and start moving towards it. M48 first appeared in the Marlin firmware and its functionality was copied by the other 3D Printer firmwares.

If it makes sense to change M48 to make it better.... There is no reason not to migrate to the preferred behaviour.

Taking your thoughts and account of the probing system/ printer weakness test capability of M48, possible options.

  1. Single Point Test (position after test initiation)
    At initiation of test, do an Auto-Home (if needed), pause, put out a message to move the probe to the desired test position. If the user intends to test the printer to identify weak points, the user will have to manually position the probe at all the desired locations and manually log the results.

  2. Single Point Test (position before test initiation)
    As suggested, home to calibrate its position and return to current co-ordinates and run the test. This assumes that the user knows to position the probe first before initiating the test. So it has to pause, put out a message to position the probe at the desired position and then continue, basically back to option 1.

  3. Additional M48 implementation to bring out its potential

3.1 Retain Option 1 or 2 to provide for localised tests. This will be useful as you tweak the printer and check the results.

3.2 Provide an additional feature under another menu item (eg, PRINTER STABILITY SCAN or something) to run testing of the entire printer to identify its weakness. In the test, run the M48 at all bed levelling positions (a 5 point or a 3x3 should suffice?) and report the deviations at each point. This will give a picture of the printer and the user will be able to determine weak locations that need work. A download of the results for analysis will be required. Running this via a laptop to capture the output or writing the results to an SD card (preferred) are possibilities.

Further thoughts?

@GMagician GMagician deleted the Fix-M48-on-LCD branch August 22, 2020 06:44
@Roxy-3D
Copy link
Member

Roxy-3D commented Aug 22, 2020

It maybe useful to give M48 the ability to generate repeatability for a grid on the bed.

3 x 3 would be a good start. But it would probably make sense to allow the M48 grid code to allow the user to specify the number of points to be included in the grid. Probably 3 x 3 would be a very good default if nothing is specified by the user. I can envision some people would have a need to go fairly high (like maybe 8 x 8) when looking for problem areas on their bed.

@qystan
Copy link

qystan commented Aug 23, 2020

It maybe useful to give M48 the ability to generate repeatability for a grid on the bed.

3 x 3 would be a good start. But it would probably make sense to allow the M48 grid code to allow the user to specify the number of points to be included in the grid. Probably 3 x 3 would be a very good default if nothing is specified by the user. I can envision some people would have a need to go fairly high (like maybe 8 x 8) when looking for problem areas on their bed.

3x3 sounds like a logical start, higher density sounds iffy.

A 3x3 probes the 4 corners and mid edges where the supports would be (for contact sensors). For proximity sensors, it gives edge and presence of bed support material effect. The center gives the 'best' case as a reference for comparison against the other points.

The original issue is closed and this discussion is a separate subject, would be better to move it to a new issue?

@thisiskeithb
Copy link
Member

The original issue is closed and this discussion is a separate subject, would be better to move it to a new issue?

Yes. Please create a new Feature Request.

@Roxy-3D
Copy link
Member

Roxy-3D commented Aug 29, 2020

Yes. Please create a new Feature Request.

And please be sure to reference this thread so we know where the discussion is continuing... Or keep going here. That is OK too.

albertogg pushed a commit to albertogg/Marlin that referenced this pull request Aug 31, 2020
thinkyhead pushed a commit to thinkyhead/Marlin that referenced this pull request Sep 2, 2020
vgadreau pushed a commit to vgadreau/Marlin that referenced this pull request Dec 9, 2020
kageurufu pushed a commit to CR30-Users/Marlin-CR30 that referenced this pull request Apr 30, 2021
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