Marlin: Unable to use M421 (Set Mesh Level Matrix) commands due to usage conflict

[bdowling]

I think I should have posted this over here, sorry. Realized when I couldn't find the RCBugFix branch in Dev.

https://github.com/MarlinFirmware/MarlinDev/issues/421

(Love the irony of getting it issue number 421 there though)

---

M421 are unable to be set due to a conflict in the code regarding what X and Y values are to be passed. Well, technically, I can set the X0, Y0 ;)

https://github.com/MarlinFirmware/Marlin/blob/RCBugFix/Marlin/Marlin_main.cpp#L5855

The test of the parameters are checking that an index is being passed, but then those values are being handed off to the mapping functions mbl.select_x_index(x) and respective y rather than being used as an index.

I would like to propose that the interface be modified to offer consistency such that the relevant output from M503 can be pasted back into the serial to be able to set/reload these parameters if so needed. Currently M503 outputs the absolute X and Y coordinates. It could go either way, change the M503, or change M421 to accept the physical coordinates. I'm not sure how widely this is used. I think I must be the first to try. ;)

Ref: http://reprap.org/wiki/G-code#M421:_Set_a_Mesh_Bed_Leveling_Z_coordinate

Willing to submit a PR on this, just wanted to open the discussion for the above.

[epatel]

I tried to raise this issue a year ago. To no result. I consider it broken because it uses a method from mbl that was created for a totally different use.

https://github.com/MarlinFirmware/Marlin/commit/72bf6acb5b32a295616db02b96e4b1adabeac87a

[bdowling]

FYI, M503 is rounding the mesh output numbers, where as G29 shows them in more precision. So the cut & paste I proposed above actually introduces slight variances.

G29
....
Recv: Measured points:
Recv:   2.82500  2.55000  2.25000
Recv:   2.82500  2.87500  2.57500
Recv:   2.50000  2.35000  2.20000

M503
.....
Recv: echo:  M421 X5.00 Y5.00 Z2.83
Recv: echo:  M421 X130.00 Y5.00 Z2.55
Recv: echo:  M421 X255.00 Y5.00 Z2.25
Recv: echo:  M421 X5.00 Y100.00 Z2.83
Recv: echo:  M421 X130.00 Y100.00 Z2.88
Recv: echo:  M421 X255.00 Y100.00 Z2.58
Recv: echo:  M421 X5.00 Y195.00 Z2.50
Recv: echo:  M421 X130.00 Y195.00 Z2.35
Recv: echo:  M421 X255.00 Y195.00 Z2.20

[jbrazio]

@epatel in your opinion what would be the best way to fix this ?

[thinkyhead]

I tried to raise this issue a year ago. To no result.

You can see clearly at the link https://github.com/MarlinFirmware/Marlin/commit/72bf6acb5b32a295616db02b96e4b1adabeac87a that I raised two points in two quick responses, but received _no answer_. If there was "no result" it's because the conversation was never taken up and the issue was forgotten. That's just what happens when an issue is not pressed.

So… If there is a bug in the way that these positions are being matched to grid indices, then we should fix that. I can certainly fix the output of M503. But I still believe that it's "most compatible" to have M421 use nearest-coordinates instead of grid-point indices.

[thinkyhead]

For getting proper indices to "match" a given X or Y coordinate, I suggest following this approach:

int select_x_index(float x) {
  for (uint8_t i = 0; i < MESH_NUM_X_POINTS; i++)
    if (fabs(x - get_x(i)) <= (MESH_X_DIST) / 2)
      return i;
  return -1;
}

int select_y_index(float y) {
  for (uint8_t i = 0; i < MESH_NUM_Y_POINTS; i++)
    if (fabs(y - get_y(i)) <= (MESH_Y_DIST) / 2)
      return i;
  return -1;
}

If the given coordinate is close enough to a grid point (a wide margin of half-the-space-between is given), then the nearest index is returned. For a near-matching grid this will work great. If the grid is changed or moved before re-flashing the board, code can look for a returned index of -1 to indicate that the grid is probably obsolete, and to either throw it away or synthesize a new grid.

[Roxy-3D]

But I still believe that it's "most compatible" to have M421 use nearest-coordinates instead of grid-point indices.

There is another method that might be worth discussing. I don't know if this really makes sense but I do envision people moving the nozzle to a random location where they are having problems getting the first level adhesion. This point would not necessarily be at a point on the mesh. What if instead of finding the nearest mesh point to apply the correction, we do a bi-linear correction in both X & Y of the four mesh points around the specified correction? And of course, the closer that correction is to a mesh point, the more weighting it would be given.

[bdowling]

Why does M421 need to accept non grid points? I feel like this over complicates this function and provides more confusion for the user.

What if the user has two or more flaws in their bed inside one grid region and they input them all? This is not going to give them the result they expect by allowing non-edge points. That's for finding the closest by half or the bi-linear correction. (They could up the grid size to 7x7 or It's time to get a new bed if they need that much correction! !)

Coordinates passed should match what M503 outputs. If we want to let them skip decimals, give a +-1 mm reach.

Also, can someone help me understand the overlap functionality with G29 S3? They are working on the same mbl grid, are they not?

In any case I'd like to see this simplified. When we get better at automatic mesh leveling we can do more magical things. ;)

[Roxy-3D]

Why does M421 need to accept non grid points?

I think one of the answers to this question is the fact that an imperfection in the bed is most likely to show up between a couple of mesh points and not right at a mesh point. So if you have a high (or low) spot on your bed, you will be able to see that as the first layer is going down. It is fairly straight forward to pause the print and move the nozzle to the location where the flaw is apparent. Probably, this is not at a mesh point.

But now that you know where the flaw is, what do you do about it? Probably you want to push or pull the mesh at that location.

[bdowling]

If the problem is that localised, isn't this likely to make matters worse at the local grid point and others because you are just pushing or pulling the whole plane? Possibly even hitting the bed at grid corners if Z you want to interpolate was a low spot. Depending on how far off from corners the problem is.

It might be better if this is a supported use case to increase the grid size to support smaller quadrants.

[epatel]

@thinkyhead What I meant was that I raised the question of how good that implementation was and if you look at the PR I refer to your didn't even care to compile the code before submitting it to the repo. I still think it no good to try to do this, even with a nearest neighbour. I do not want to tangle up in fruitless discussion with you yet again. _(My) Life is to short_. Back then I presented the facts, and really I didn't expect anyone to try it, and it took until now for someone to actually do that. Let them show you I though, you never believe me anyway. Reasons for not doing it like that is basically why re-sampling is bad, you know the Nyquist theorem etc? (apart from the broken implementation) I think we are talking about so small values and few samples that there is a need have _true_ number and not re-sampled. I believe they could actually be re-sampled off so the effect could get a magnitude worse. Safe would maybe go from 5 to 3 or some matching steps, pruning points. I have not and I will not use this feature so I leave this discussion here.

[epatel]

@jbrazio I would ask what the feature is for, and then implements accordingly. I imagine the use case is for saving to file a grid measurement and then be able to load it again, so I would lean to using indices and be fine with that. Changing number of measuring points (re-flash printer) or changing between ABL and MBL (also re-flash) is for me totally expected times for re-probing the bed (I re-probe mine like once a month anyway). Maybe because room temperature (season change) change it topology or screws/springs holding it or what ever.

[thinkyhead]

I do not want to tangle up in fruitless discussion with you yet again. (My) Life is to short.

Jesus, so you didn't get what you wanted, or didn't get something in time… and now I'm just a big old baddie, am I? And yet I have done so much work to help improve this feature, apparently unappreciated.

Half the time I don't even know what you're talking about. I really can't understand what you are referring to in your comments above. You will have to start over, because I haven't been carrying around this topic in my brain – I need a refresher at the very least to understand what the heck you are talking about. I do not live and breathe MBL.

Starting over: What is your problem with the way that M421 is implemented? Did I not _make perfectly clear_ why we should prefer coordinates instead of grid indices? It's so that M421 can be used, and be forward-compatible, for any bed leveling system, including those without a regular grid, such as 3-point. Trying to think of _others_ here, not giving special privilege to MBL.

Sorry, but if it's such a problem to have a bloody discussion, then I will stay away from MBL from now on. You all work it out and submit new code when it's ready. Just leave me out of it, if I'm such a terrible barrier to success.

[thinkyhead]

Why does M421 need to accept non grid points?

It doesn't. The point of using the actual points instead of grid indices is for broader compatibility with current and future bed leveling options. When specifying XY in M421 you absolutely should endeavor to match the actual grid points. The only reason the M421 GCode was created in the first place was to provide a GCode to correspond to points stored in EEPROM. It was added during a period when not all EEPROM storage had corresponding GCodes.

Why did I choose coordinates instead of indices? Because coordinates can be stored in any order. Because coordinates make sense for 3-point leveling, or others that don't have a regular grid. It's because M421 is not specific to Mesh Bed Leveling.

[thinkyhead]

I have merged #3760 which fixes the bug reported by this issue, properly implements M421, updates the MBL coordinate-to-index functions to use nearest-neighbor, and produces 5 digits of precision in the output from M503.

Think of M421 as corresponding roughly to G30 but without the probing. For MBL it will store a "grid point" whereas in future, with other forms of leveling it may store a "corner point", a "perimeter point", etc.

To go along with the M421 command (which is still quite new, and originally created only for EEPROM correspondence) we should have a command like "Recompute Leveling" (e.g., M422), which you would use after setting enough points with M421.

None of that should be intimately tied to MBL. These functions should be _considerate_ of other forms of leveling.

[Blue-Marlin]

How about a compromise? Coordinates with X and Y - indices with I and J.

[epatel]

You think I focus too much on MBL? I created MBL so I try to follow what is happening with it. Shouldn't I do that? I could start investigating more pieces of Marlin, but I rather not do that.

I remember this M421 thing from the commit https://github.com/MarlinFirmware/Marlin/commit/72bf6acb5b32a295616db02b96e4b1adabeac87a "Fix MBL compile error"

1. You had broke a PR for me back then (https://github.com/MarlinFirmware/Marlin/pull/1938), and after a month I gave up fixing the issues you introduces in all your changes (you obviously submitted code you didn't even compile once!)
2. Apart from being broken, it looked weird, using XY values instead of indices and will probably cause re-sample problems I tried to raise above. Also you are trying to solve an imaginary future problem, which we still after a year have not reached. @bdowling seem to be the first person to actually use M421, so maybe listen to him (_not over complicates things_)
3. You didn't even acknowledge my comment that the way you tried to solve it would never reach the last index for XY, and it was left like that since then.

It's because M421 is not specific to Mesh Bed Leveling.

Well, right now it says. M421 - Set a single Z coordinate in the Mesh Leveling grid. X<mm> Y<mm> Z<mm> https://github.com/MarlinFirmware/Marlin/blob/RC/Marlin/Marlin_main.cpp#L207

But fine, let it be like this. Put in stuff that will cause people to complain about MBL when the root cause in this case will be re-sampled points in M421

[epatel]

I just took a look at this commit https://github.com/MarlinFirmware/Marlin/pull/3760/files

...and frankly I think the way you implemented select_x/y_index() doesn't seem to care about how they are used in get_z() where the actual bi-linear calculation is made. But I imagine you tested this before you submitted it? No, doesn't look like it.

The lines
https://github.com/thinkyhead/Marlin/blob/13175ce7da7a1cc260e923df3bf81f801fb6438d/Marlin/mesh_bed_leveling.h#L80-L88
Does a number of index + 1 which require the x/y values to be _above_ the index/point...which is the reason you could not reach the last index (that I have been saying again and again) this is by design for the bi-linear calculation.

I believe you actually broke MBL right now, please prove me wrong.

For reference those lines are the three line-interpolation steps for bi-linear calculation see

From the numbers in above image, where columns is X and rows is Y (after select_x_index) we would get
x0=14.5
y0=20.2
get_x(select_x_index(x0))=14
get_y(select_y_index(y0))=20
z1=150.5=get_z(x0=14.5, 14, 91, 15, 210)
z2=128.5=get_z(x0=14.5, 14, 162, 15, 95)
z0=139.5=get_z(y0=20.2, 20, z1=150.5, 21, z2=128.5)

[thinkyhead]

Coordinates with X and Y - indices with I and J.

My thought exactly!

[epatel]

I think that change will also screw up the line splitting function, which also uses select_x/y_index(). See https://github.com/MarlinFirmware/Marlin/blob/dee6322bf012c117f4fbb434aaf775ddaffc3334/Marlin/Marlin_main.cpp#L7206-L7209

I have not tested it myself but I can imagine it either split lines and calculate the split point wrong or even go into an infinite loop (not very likely though because I used a bitfield to make sure that does not happen).

But I guess we will know soon enough.

[thinkyhead]

...and frankly I think the way you implemented select_x/y_index() doesn't seem to care about how they are used in get_z() where the actual bi-linear calculation is made. But I imagine you tested this before you submitted it? No, doesn't look like it.

Indeed. I am on the verge of homeless, so I really need your eyes on this, and less ridicule, thanks. If you see an issue that turns out to be a shortsighted bug, submit that patch as a bug-fix quickly, and get that bug fixed. The power of open source is awesome for mitigating human error, but unfortunately not yet perfect enough to prevent it.

A thousand apologies for allowing bugs to slip through. More sleep is needed, clearly.

[thinkyhead]

You didn't even acknowledge my comment that the way you tried to solve it would never reach the last index for XY, and it was left like that since then.

I must have missed that. Sorry! I didn't ignore it _on purpose_. As it happens I suddenly bailed on Marlin for a while because it was getting stupid around here, with too much bitching in the issue queue, and all kinds of nonsense. I needed to get away from it and do other things. I only came back a short time ago to help out with integrating the pileup.

[bdowling]

All.. I know I'm a recent interested party here, but I have already sensed some of the history and struggles here...

As with any large open source project though, we have to keep mindful of the huge effort it does take to manage, curate and protect the users of the end product.

Most of those involved are largely unpaid volunteers who just have a stake in the project to help make it better. Nobody is perfect, and absolutely everything done is a balancing act, that's one of the only constants. Time, resources, compatibility, skills, needs, desires, personality and mission are often in conflict.

So what's my point, why am I rambling? I guess I just want to see us come together with the goal of helping each of us contribute where we can, solve the bugs as elegantly as we can, integrate the new features with the best intent and move on to the next one, because there always will be more. The past can't be changed, but we can make the future whatever we want.

Respectfully,
B

[thinkyhead]

Indeed. Finger-pointing is … pointless. Meanwhile, the open source process —like it or not— involves sometimes needing to educate fools and idiots, like myself, on your pet subject, and often you have to work against the tide to sell a position. And in the end, maybe you cannot sell your position in spite of its obvious (to oneself) superiority. It's useless then to keep piling on complaints. Work harder. Provide concrete examples and scientific proof that your idea is better. Or come up with some reasonable compromise, if possible.

Moreover, to take a non-answer as an "intentional snub" is to miss the fact that things do get lost in the shuffle, as other issues or life-in-general may steal away attention. "The squeaky wheel gets the grease," as they say. If someone disappears or dies or something, don't give up, keep pressing the issue and someone else will take it up.

Thank you for your time.

[epatel]

Oh, I feel so much better now, I want to hug and kiss everyone. No, frankly, submitting untested code is a no-no for me. And though I was pointing that out, you still did it again (and I have seen you do it many many times before). When I created MBL I actually wrote the bulk part of it outside of Marlin just to make sure the math etc was correct. This, Marlin, actually feels like _Animal Farm_, where some can do what ever they want (without even compiling), and others struggle to get a small thing in. Very unbalanced and chaotic. I would love to see a better organization and maybe some board to see who is working on what. Github issues feel to chaotic and hard to follow, they require more discipline than people give them right now. Trello require even more, so maybe JIRA could be an option to track what is worked on. Rough development that break stuff should not be allowed! Sure it is hard to test things, but for that I suggest that a reporter automatically signs up as a volunteer to help test things before they are put in a "public" branch. And maybe we could get a few volunteers to be on standby. Right now this feels like a playground for a few selected.

[emartinez167]

I plan to return to my tester duties as soon as I can get my machine back
up running in a few weeks so count me in!
On Tue, 17 May 2016 at 15:58, Edward Patel [email protected] wrote:

Oh, I feel so much better now, I want to hug and kiss everyone. No,
frankly, submitting untested code is a no-no for me. And though I was
pointing that out, you still did it again (and I have seen you do it many
many times before). When I created MBL I actually wrote the bulk part of it
outside of Marlin just to make sure the math etc was correct. This, Marlin,
actually feels like _Animal Farm_, where some can do what ever they want
(without even compiling), and others struggle to get a small thing in. Very
unbalanced and chaotic. I would love to see a better organization and maybe
some board to see who is working on what. Github issues feel to chaotic and
hard to follow, they require more discipline than people give them right
now. Trello require even more, so maybe JIRA could be an option to track
what is worked on. Rough development that break stuff should not be
allowed! Sure it is hard to test things, but for that I suggest that a
reporter automatically signs up as a v olunteer to help test things before
they are put in a "public" branch. And maybe we could get a few volunteers
to be on standby. Right now this feels like a playground for a few selected.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
https://github.com/MarlinFirmware/Marlin/issues/3745#issuecomment-219645958

[jbrazio]

@epatel from "old Marlin" there are only a few left, new blood has come in (I am an example), old blood is obviously trying to make things right.. you should give us a chance and come back on board.

Marlin is a bit fat right now, there are some cleanup tasks being done for this RC cycle, for now we live with the tools we got. When the cycle finishes we should decide what new tools we will bring on board.

Concerning untested code, sometimes we have no way around, what I mean is, as Marlin is heavily hardware dependent sometimes you have to submit code which compiles but is untested because you don't have the specific hardware to do it.. this is because we are lacking [active] devs and/or testers per area/feature/whatever, like you for the MBL.

[epatel]

Concerning untested code, sometimes we have no way around, what I mean is, as Marlin is heavily hardware dependent sometimes you have to submit code which compiles but is untested because you don't have the specific hardware to do it.. this is because we are lacking [active] devs and/or testers per area/feature/whatever, like you for the MBL

I did test MBL extensively before I made the initial PR, and I didn't (and couldn't wouldn't) merge the code directly into the "public" branch. Obviously other people do not do that, test or even refrain from merging untested things. They didn't even test it them self!

And my suggestion was only to not merge code until at least some/one/oneself test it, it can still be living in any branch anywhere, but force people to be guinea pigs for untested code is not a good thing in my eyes. Thats why I am suggesting a tester "guild" for those task a developer can not test him/her self. A tester could either be an issue reporter (being part of the solution, also being initially interested in a change/fix) or maybe others like @emartinez167 being volunteers. Pushing untested code should really be a last resort, but then, if it can't be tested is it necessary? Are you just not opening a failure up the road, way later when you do not even understand why things break.

[epatel]

Right now we have a _Release Candidate_, or do we? Its like things are being pushed even faster into the branch than ever. This is upside down. Shouldn't focus be on identifying bugs and fix them for a Release, and not change more code? Code Freeze anyone? How many RC will there be? RC7, RC8, RC9? Doesn't seem to be any change from a normal dev period, rather the opposite...huzzle to push more broken stuff into it.

[jbrazio]

I agree with you, my proposal is "stable, unstable and development" for the branches.

People fail to understand that some of Marlin was "broken" and what may seem new features are just rework of broken concepts. All new features introduced on RC are contained in their own conditional clauses. What lead to the "broken" state I cannot answer for because I was not here..

PS: this is not an universal truth but a principle we try to follow.

[jbrazio]

I just went trough the first page of the commit log for RCBugFix..

Commits on May 16, 2016

• Add stowing process for MECHANICAL_PROBE 95f30529a68621f5f1735afaacea090f8cb50996 - bugfix
• Separate Z_PROBE_ALLEN_KEY from MECHANICAL_PROBE e2b87f6c85ae30d1d980902e013895c9a6a55f65 - bugfix

Commits on May 15, 2016

• Bugfix: iteration invokes undefined behavior ecd490ed49e2d6d5e1754fda4cccf7f19cda8e70 - bugfix
• Follow-up the PR #3720 and #3759 6d722716f7c9976a6670c23c7b8832a9ec9375da - bugfix
• Remove the hotbed icon when HAS_TEMP_BED is false 451000387dc75e91ef9089c1f6d6817c80ff865e - bugfix
• Fix error checking in M421 13175ce7da7a1cc260e923df3bf81f801fb6438d - bugfix
• More robust MBL index / point conversion bc5a547d55722a87d41d2e4ee6ad32ed9db736ba - feature/bugfix ?
• More precision in M503 output for MBL's M421 a3520b6f01f402989cdb22fa958f9c1f5740dd6e - bugfix

Commits on May 14, 2016

• Adds an option to disable print job timer auto start 8a18c52002737e71ff142a93e78aae26fe7614ea - feature
• M78 now allows stats reset using the S78 argument a79267217b2840edc367660c3dd10fb046770996 - feature
• Bugfix: Multiple M77 no longer increment the print counter - b660f1bdb8bcd5465c6347b9986001fe28da9f93 - bugfix
• Tweaks to some verbiage ddf3e1e22dc3cbb2377808909112f9a1e69639ae - bugfix
• Don't try to enable unused auto fans 4d6bb52b2654680de3f6e688e9c8f17173abe089 - bugfix

[thinkyhead]

playground for a few selected

Not so much a "playground," but certainly some changes are being floated to see how well they might be received, and to garner feedback. Some things may be reverted, reformed, improved, polished, according to the building of consensus. Keep that feedback coming!

We are aiming to get the code into more solid shape, bugs fixed, behaviors stabilized, little more at this point… pressing forward for the release. With so many changes and bugs being patched at a high rate of speed, some things may be missed. Sometimes in haste and without sleep some errors have slipped through. Most of the time these things are caught pretty quickly. Those keener minds and eyes that provide fast fixes are appreciated. Keep those patches coming!

Nothing untested or broken should end up in a release, naturally, and nothing shall be considered "truly tested" until multiple setups have had a chance to try it out. Bugs, typos, and logical holes are being caught through this process, by virtue of the involvement of more individuals and machines, by never trusting blindly, because mistakes, logical and otherwise, are always possible where humans are involved.

Thanks to everyone for keeping this process always lively and challenging!