RC2 --- M665 Support is Incomplete

[Roxy-3D]

M665 is used to set the Delta values. It allows the setting of delta_diagonal_rod_trim_tower values to fine tune the flatness of the Effector travel. The problem is, it is not fully supported by the EEPROM's save, load, restore_to_default and print_current_value functions.

I have my RC2 altered to provide that support. I'll include these changes in the Pull Request I generate. Nobody needs to do anything on this unless they need this functionality now because it should get merged eventually (next couple of weeks). If you need the functionality immediately, post a request in this thread and I'll send you all of the changes.

#if ENABLED(DELTA)
  /**
   * M665: Set delta configurations
   *
   *    L = diagonal rod
   *    R = delta radius
   *    S = segments per second
   *    A = Alpha (Tower 1) diagonal rod trim
   *    B = Beta (Tower 2) diagonal rod trim
   *    C = Gamma (Tower 3) diagonal rod trim
   */
  inline void gcode_M665() {
    if (code_seen('L')) delta_diagonal_rod = code_value();
    if (code_seen('R')) delta_radius = code_value();
    if (code_seen('S')) delta_segments_per_second = code_value();
    if (code_seen('A')) delta_diagonal_rod_trim_tower_1 = code_value();
    if (code_seen('B')) delta_diagonal_rod_trim_tower_2 = code_value();
    if (code_seen('C')) delta_diagonal_rod_trim_tower_3 = code_value();
    recalc_delta_settings(delta_radius, delta_diagonal_rod);
}

[Wackerbarth]

Roxy,

In addition, the functionality is incomplete. In addition to being able to trim the rod lengths at run time, we also need to be able to trim the tower radius values.

However, adding functionality (including storing the presently available parameters in EEPROM) is not a "bug fix", but rather an additional (compatible) feature. You really should to switch over to MarlinDev to present the additions that you are proposing.

Hopefully we can get 1.1.0 RELEASED so that we can again open the source code to additions and, if necessary, back-port some additional features.

But, given that the author's of much of the code in question are not presently active, I don't know if we will ever be able to actually release 1.1.0.

[Roxy-3D]

I understand the argument that this isn't really a bug. I kind of debated where to post the message. What is there, does work. But given the fact the 'L', 'R' and 'S' values get reported by M503 (and also saved, restored and set to default) I decided it was a bug to not have the 'A', 'B' and 'C' values do the same.

It took me a while to figure out why I wasn't really able to use that functionality. When I discovered the support wasn't symmetrical for all of the M665 parameters, that pushed me over the edge and thought it was best treated as a Release Candidate bug.

[Wackerbarth]

My real concern about the M665 command is that, as written, it doesn't handle all of the parameters that should be configurable. But adding more "ad hoc" parameters in this fashion just makes Marlin's "collection of warts" even greater. I would really like to see a "designed" approach to handling these, and the parameters for other kinematic designs, that allows a common implementation rather than having special cases for each variant.

[AnHardt]

Stopping improvements, until we have a feature, where we currently don't have a concept for, can't be the way to go.

[Wackerbarth]

I disagree. Particularly with respect to the external interface (the M-codes, etc.), it doesn't make sense to add something which is known to address only part of a problem. Doing so introduces yet another case of "historical baggage" for which there will be pressure to maintain "backward compatibility".

It is much better to require that the proposed addition at least attempt to provide the whole functionality that we know that we need.

That doesn't mean that, once you have a design, you necessarily have to implement all of it at once. But, by first having a design, you, at least, are not introducing additional incompatible warts.

[Roxy-3D]

I suspect it would not be difficult to add 3 more parameters to trim each tower's radius. And we could add 3 more parameters to trim each tower's angle. But that might be too much complexity. Most of the setup and configuration guides I have seen make some simplifying assumptions. The guides and videos I've looked at assume the towers are evenly spaced around the circle at a fixed radius.

But I can tell you I'm seeing value in being able to trim each diagonal rod independently from each other. I'm glad that incomplete feature made it into RC2. And I already have the code to round out the full support of that feature for future code bases.

[Wackerbarth]

Roxy, There are some descriptions of the tower positions that allow for individual variance in both tower radius and the angle. However, mathematically, having both creates too many degrees of freedom. To provide both kinds of adjustment creates a condition whereby there is no unique description of the positions.

Therefore, we should not provide both. Computationally, it is easier if we maintain fixed angles and vary only the radius. Just as there are some implementations that assume that all of the arms are of the same length, but you have found that having the ability to have different values permits a better fit to the observed probe readings, the same applies to the ability to trim the radius for each tower to obtain a better fit.

So, we need to be prepared to provide those 2 additional degrees of freedom. It is likely that, for the user's ease, you will provide 3 trim values and eliminate one degree of freedom by placing a constraint on how the trim values and the fundamental length are related when the resulting lengths are expressed in canonical form.

For example, you might choose to require that the sum of the trims is zero. Alternately, you could require that the smallest of the three trims be zero. Or you could require that the fundamental length be an integer and the smallest trim be between zero and one. (It doesn't matter which canonical form is chosen as long as you end up with a unique set of values for any given configuration)

What we need to do now is to define the syntax that will be used to provide these values, even if some of the trim computations are not presently evaluated.

[Roxy-3D]

For example, you might choose to require that the sum of the trims is zero. Alternately, you could require that the smallest of the three trims be zero. Or you could require that the fundamental length be an integer and the smallest trim be between zero and one. (It doesn't matter which canonical form is chosen as long as you end up with a unique set of values for any given configuration)

I have not really dug into the math. But if my diagonal rod trim values do not sum to zero, is the math still going to work? Right now, the code generates values regardless of what the trim values are set at. I guess what I'm really asking is "Why do we need to put any constraints on the diagonal rod trim values?" If the math works, does it really matter?

Looking at it from another perspective, we have the bulk of the diagonal rod length nailed down as the same for each tower. But we add or subtract a little to it for each tower to make it right. One idea would be to only let the user define 2 of the 3 trim values. And have the 3rd one calculated from the first 2.

[Wackerbarth]

Note that I was referring only to the canonical form. Having a single canonical form becomes important when you look at the inverse function (the one computed as a part of "bed leveling").

In the forward direction, you add two (or, I would suggest three) values to obtain the effective value which gets applied. It doesn't really matter how many terms you add together in that direction because you can choose any set of values which sum to the desired value.

But, in the reverse direction, you need to be able to have a unique value for each component of the sum.

Assume that we define length as length = base + trim. Now we might pick base = 10 and trim = 0.1 to get a length of 10.1. We could also pick base = 9 and trim = 1.1 to get the same result.

But, without some other constraint, if we experimentally determine that the length should be 10.1, we have no way to determine the correct value for trim. If might be 0.1. Or it might be 1.1. Or, any other value. (all depending on the value that we select for base).

Under those conditions, any "curve fitting" algorithm will be unstable because there are multiple answers that are all equally "correct".

[Roxy-3D]

In addition, the functionality is incomplete. In addition to being able to trim the rod lengths at run time, we also need to be able to trim the tower radius values.

It turns out you were very right! :) Or... Maybe I should say, I followed your suggestion pretty closely. I added code to alter, store, recall, set to hardware defaults and print the trim on each Tower's Delta Radius.

I only needed a tiny amount of trim on the Alpha tower. But it really helped me get the Effector to travel flat.

  /**
   * M665: Set delta configurations
   *
   *    L = diagonal rod
   *    R = delta radius
   *    S = segments per second
   *    A = Alpha (Tower 1) diagonal rod trim
   *    B = Beta (Tower 2) diagonal rod trim
   *    C = Gamma (Tower 3) diagonal rod trim
   *    I = Alpha (Tower 1) radius trim
   *    J = Beta (Tower 2) radius trim
   *    K = Gamma (Tower 3) radius trim
   **/
  inline void gcode_M665() {
    if (code_seen('L')) delta_diagonal_rod = code_value();
    if (code_seen('R')) delta_radius = code_value();
    if (code_seen('S')) delta_segments_per_second = code_value();
    if (code_seen('A')) delta_diagonal_rod_trim_tower_1 = code_value();
    if (code_seen('B')) delta_diagonal_rod_trim_tower_2 = code_value();
    if (code_seen('C')) delta_diagonal_rod_trim_tower_3 = code_value();
    if (code_seen('I')) delta_radius_trim_tower_1 = code_value();
    if (code_seen('J')) delta_radius_trim_tower_2 = code_value();
    if (code_seen('K')) delta_radius_trim_tower_3 = code_value();
    recalc_delta_settings(delta_radius, delta_diagonal_rod);
  }

[thinkyhead]

Since this code is now in the Jan10 branch, perhaps this issue can be closed…? There's some code for M666 in another issue. Maybe that can be added as well.

[Roxy-3D]

There's some code for M666 in another issue. Maybe that can be added as well.

Can you provide an issue # ? My guess is it could be added.

[HedronUser]

@thinkyhead This is still an issue for me and here is why. An older version of Marlin, before RC, could trim z height (H), individual tower positions (A B C), individual delta radius values (I J K), and individual diagonal rod (U V W) using M666. Why should M665 not do something similar in RC? I'm REALLY missing the additional delta geometry support old Marlin provided and I'm getting ready to try and implement a fix in RC. Any help would be AWESOME!

[AnHardt]

@HedronUser To be exact. You are not talking about an old version of our Marlin. You are talking about an old fork of an even older Marlin (jcrocholl? RichCattell? ). That means, this extension has never been on our Marlin.

[HedronUser]

Yes you are correct. I believe It was the richchattel branch (used for andycart's cherry pi delta) now that you referenced and refreshed my memory. Thank you. So perhaps those features in his fork will be added someday. In the meantime I switched back to repetier firmware because it has the advanced delta config settings I like, ( as well as functional sd card code, and eeprom support (via repetier host)). Cheers!

On Feb 28, 2016, at 2:57 PM, AnHardt notifications@github.com wrote:

@HedronUser To be exact. You are not talking about an old version of our Marlin. You are talking about an old fork of an even older Marlin (jcrocholl? RichCattell? ). That means, this extension has never been on our Marlin.

— Reply to this email directly or view it on GitHub.

[thinkyhead]

In the meantime I switched back to repetier firmware

It is a good firmware!

[thinkyhead]

Can you provide an issue # ? My guess is it could be added.

@Roxy-3DPrintBoard #2695

[bloodstrike]

Will M503's output be updated so that the values of M665 A, B, and C (and possibly I, J, and K) be listed?

[Roxy-3D]

Will M503's output be updated so that the values of M665 A, B, and C (and possibly I, J, and K) be listed?

I just looked at the RCBugFix. Indeed they are not printed there. They are printed in my version.

@thinkyhead This is a pretty minor and low impact bug. But it is also very low risk to fix. It's your call what we should do. If you agree it is very low risk, the code is below ready to Cut & Paste.

I commented out the I, J & K just because RCBugFix does not allow those to be set and it really doesn't make sense to add those at this point in time. The code looks like this:

  #if ENABLED(DELTA)
    CONFIG_ECHO_START;
    if (!forReplay) {
      SERIAL_ECHOLNPGM("Endstop adjustment (mm):");
      CONFIG_ECHO_START;
    }
    SERIAL_ECHOPAIR("  M666 X", endstop_adj[X_AXIS]);
    SERIAL_ECHOPAIR(" Y", endstop_adj[Y_AXIS]);
    SERIAL_ECHOPAIR(" Z", endstop_adj[Z_AXIS]);
    SERIAL_EOL;
    CONFIG_ECHO_START;
    SERIAL_ECHOLNPGM("Delta settings: L=delta_diagonal_rod, R=delta_radius, S=delta_segments_per_second");
    CONFIG_ECHO_START;
    SERIAL_ECHOPAIR("  M665 L", delta_diagonal_rod);
    SERIAL_ECHOPAIR(" R", delta_radius);
    SERIAL_ECHOPAIR(" S", delta_segments_per_second);
    SERIAL_ECHOPAIR(" A", delta_diagonal_rod_trim_tower_1);
    SERIAL_ECHOPAIR(" B", delta_diagonal_rod_trim_tower_2);
    SERIAL_ECHOPAIR(" C", delta_diagonal_rod_trim_tower_3);
//    SERIAL_ECHOPAIR(" I", delta_radius_trim_tower_1 );
//    SERIAL_ECHOPAIR(" J", delta_radius_trim_tower_2 );
//    SERIAL_ECHOPAIR(" K", delta_radius_trim_tower_3 );
    SERIAL_EOL;

  #elif ENABLED(Z_DUAL_ENDSTOPS)

[thinkyhead]

Good point! I will add this shortly.

[bloodstrike]

@Roxy-3DPrintBoard - Thanks! I tried adding the SERIAL_ECHOPAIR lines, but without more changes it won't compile:

'delta_diagonal_rod_trim_tower_1' was not declared in this scope

Hopefully @thinkyhead is on it and it will be added to the RCBugFix branch soon.

[Roxy-3D]

I can send you a .ZIP file of the working code base with that. But that is kind of why I commented out the I, J & K. We are trying to stabilize the Release Candidate, not add extra features right now. But the fact that 'Supported' variables are not being reported with a M503 is a bug. And the code to enable (and fix the bug) is so small, simple and low risk.... Maybe we should add that to the release candidate. But not the full I, J & K support.

I'll .ZIP up a full working code base with I, J & K if you want to finish the extraction...

[thinkyhead]

3177 has no missing symbol issues and patches up the EEPROM with aplomb.

[thinkyhead]

@HedronUser By the way, sorry to have missed the opening fandango last Friday. I got invited to some kind of poetry-comedy slam thing. But I hope to get out to Hedron makerspace soon! If you were planning to attend the March 30th PDX 3D Printing Lab monthly meetup, I will be doing a presentation on Marlin Firmware, covering all the new and exciting things we've been doing lately. It should be a real hoot!

[thinkyhead]

3177 merged.

[HedronUser]

@thinkyhead You rock, we're actually having another party this Friday night, no pressure. I am looking forward to your presentation.

Jesse Jenkins Instructor of Digital Fabrication at Hedron 503-720-9897 jesse@hedron.technology

Hedron http://hedron.technology Learn | Share | Make 503.610.0718

Slack @hedrontech Tweet @hedrontech Flickr @hedrontech

-

On Sat, Mar 19, 2016 at 6:03 AM, Scott Lahteine notifications@github.com wrote:

@HedronUser https://github.com/HedronUser By the way sorry to have missed the opening fandango on Friday. I got invited to some kind of poetry-comedy slam thing. But I hope to get out to Hedron soon. If you will be at the March 30th PDX 3D Print group thingy, I will be doing a presentation on Marlin Firmware. It should be a real hoot!

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/MarlinFirmware/Marlin/issues/2773#issuecomment-198698913

[sns5400]

I look for the possibility to use M665 with parameter "H" who represent the Delta Hight for the space between nozzle and touching the endstop, but this seems no possible, anybody who now wy ? I use 1.1 Marlin RC 8 and can not give Height, when i adapt Manual_Z_Home_pos no result even afther start from scratch.

[github-actions[bot]]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.