[1.1.x] Make G26 work with other meshes too #8522

Roxy-3D · 2017-11-22T22:22:03Z

Example Configuration.h files are not updated yet. You need to cross
your settings over to the default Configuration.h file in the \Marlin
directory. (UBL_G26_MESH_VALIDATION enablement has moved to a new
location in the file.)

Most of the changes involved pulling the G26 stuff out of the UBL class so it was available under other bed leveling systems. (For example, the G26 variables and support functions that set, test and clear flags in large bit maps.)

I'm hoping we can use this new functionality to resolve: #8513

@thinkyhead I'll wait to merge so you can warm over what ever needs to be changed. Once we get this Pull Request into conformance, I'll do a similar one for bugfix-v2.0.0.

thinkyhead · 2017-11-22T22:41:49Z

If this needs a rebase so it applies to the current bugfix-1.1.x, I can do that on your behalf and push the new commits straight to your fork, overwriting your remote bugfix-1.1.x branch. Then you would use this to update your local copy:

git checkout bugfix-1.1.x
git fetch origin
git reset --hard origin/bugfix-1.1.x

Or, if you'd like to get some rebase practice…

git fetch MarlinFirmware
git checkout bugfix-1.1.x
git rebase MarlinFirmware/bugfix-1.1.x

Roxy-3D · 2017-11-22T22:43:22Z

No... I'm in over my head... I'll try to give you contributor rights to my fork so you can push things there.

thinkyhead · 2017-11-22T22:43:30Z

… the default Configuration.h file in the \Marlin directory …

Be sure to remove your personal customizations from that file.

thinkyhead · 2017-11-22T22:44:30Z

I don't need any extra rights to push to Roxy-3D:bugfix-1.1.x because you've made a PR with it.

Roxy-3D · 2017-11-22T22:48:05Z

Be sure to remove your personal customizations from that file.

I will.... But let's get everything up to snuff first. And then I'll make a pass and get all of the Configuration.h files changed to what ever format they are going to be in.

The reason I didn't do any of them is because the G26 stuff got moved outside of the Auto Bed Leveling #ifdef's. It is kind of by itself. But short of duplicating those options in three areas, it kind of had to be that way. Once I know we have agreement on where it is... I can proceed with getting the Configuration.h files correct.

#endif // BED_LEVELING

/**
 * Enable the G26 Mesh Validation Pattern tool.
 * Requires AUTO_BED_LEVELING_UBL, AUTO_BED_LEVELING_BILINEAR or MESH_BED_LEVELING
 */
#define UBL_G26_MESH_VALIDATION   // Enable G26 mesh validation
#if ENABLED(UBL_G26_MESH_VALIDATION)
  #define MESH_TEST_NOZZLE_SIZE     0.4   // (mm) Diameter of primary nozzle.
  #define MESH_TEST_LAYER_HEIGHT    0.2   // (mm) Default layer height for the G26 Mesh Validation Tool.
  #define MESH_TEST_HOTEND_TEMP   205.0   // (°C) Default nozzle temperature for the G26 Mesh Validation Tool.
  #define MESH_TEST_BED_TEMP       60.0   // (°C) Default bed temperature for the G26 Mesh Validation Tool.
#endif

thinkyhead · 2017-11-22T22:50:26Z

The rebase resulted in no conflicts. If I push the rebased branch to your fork, you will not be able to use Github Desktop's fetch/sync button until after you've fetched the new copy of the branch. The simplest way to update your local copy is:

git checkout bugfix-1.1.x            ;# The branch we're working on
git fetch origin                     ;# All changes from remote fork
git reset --hard origin/bugfix-1.1.x ;# Move to the latest commit

Roxy-3D · 2017-11-22T22:55:59Z

Go ahead and push the changes... I'll try those three lines. If they don't work, I can just use the web pages to edit things. But I don't understand these Travis errors. I only have the Z_Servo probe defined. And same goes for the only one bed leveling system is allowed to be defined.

Initializing packages...
Preparing boards...
Verifying...
In file included from sketch/MarlinConfig.h:39:0,
                 from sketch/G26_Mesh_Validation_Tool.cpp:27:
SanityCheck.h:588: error: static assertion failed: Please enable only one probe option: PROBE_MANUALLY, FIX_MOUNTED_PROBE, BLTOUCH, SOLENOID_PROBE, Z_PROBE_ALLEN_KEY, Z_PROBE_SLED, or Z Servo.
 static_assert(1 >= 0
 ^
SanityCheck.h:696: error: static assertion failed: Select only one of: MESH_BED_LEVELING, AUTO_BED_LEVELING_LINEAR, AUTO_BED_LEVELING_3POINT, AUTO_BED_LEVELING_BILINEAR or AUTO_BED_LEVELING_UBL.
 static_assert(1 >= 0
 ^

thinkyhead · 2017-11-23T01:04:02Z

But I don't understand these Travis errors.

Travis enables various options for testing. If you already have options set, then Travis will end up testing whatever you've set plus whatever it enables for testing. This is why you should never edit the default configurations when submitting PRs.

If you want to have Travis CI test only the single configuration that you've edited, you can edit Travis CI and remove everything after the first build_marlin command. The original full Travis CI must be restored before the PR gets merged.

You can of course make a copy of the branch, push it up to your own fork, and then visit your fork's Commits Page to see the Travis CI results there.

Roxy-3D · 2017-11-23T01:10:03Z

Oh!!!! OK...

thinkyhead · 2017-11-23T01:20:46Z

Marlin/ultralcd.cpp

@@ -57,6 +57,10 @@
  extern void mesh_probing_done();
 #endif

+#if ENABLED(AUTO_BED_LEVELING_UBL) ||  ENABLED(UBL_G26_MESH_VALIDATION)
+  bool has_control_of_lcd_panel;


Since the name was predicated on having ubl. in front of it, a better new name for this flag might be lcd_under_external_control.

Agreed... We don't have to do it now. But it should probably be in a more 'neutral' place and not in G26 then.

thinkyhead · 2017-11-23T01:22:06Z

Marlin/ubl.h

@@ -189,7 +142,7 @@
                                MESH_MIN_Y + 14 * (MESH_Y_DIST), MESH_MIN_Y + 15 * (MESH_Y_DIST)
                              };

-      static bool g26_debug_flag, has_control_of_lcd_panel;
+      static bool has_control_of_lcd_panel;


Since it's now being defined as a global in ultralcd.cpp — just remove this.

thinkyhead · 2017-11-23T01:22:35Z

Marlin/ubl.cpp

@@ -70,8 +60,7 @@
  constexpr float unified_bed_leveling::_mesh_index_to_xpos[16],
                  unified_bed_leveling::_mesh_index_to_ypos[16];

-  bool unified_bed_leveling::g26_debug_flag = false,
-       unified_bed_leveling::has_control_of_lcd_panel = false;
+  bool unified_bed_leveling::has_control_of_lcd_panel = false;


Being moved to ultralcd.cpp as a global? Then remove this here.

thinkyhead · 2017-11-23T01:23:38Z

Marlin/Marlin_main.cpp

@@ -7567,12 +7567,13 @@ inline void gcode_M42() {

 #endif // Z_MIN_PROBE_REPEATABILITY_TEST

-#if ENABLED(AUTO_BED_LEVELING_UBL) && ENABLED(UBL_G26_MESH_VALIDATION)
+#if ENABLED(UBL_G26_MESH_VALIDATION)


I guess this will just be called G26_MESH_VALIDATION after this PR.

Yeah... Agreed...

thinkyhead · 2017-11-23T01:25:17Z

For this failing test, Travis is enabling FIX_MOUNTED_PROBE…

The command "opt_enable PIDTEMPBED FIX_MOUNTED_PROBE Z_SAFE_HOMING ARC_P_CIRCLES CNC_WORKSPACE_PLANES CNC_COORDINATE_SYSTEMS" exited with 0.

. . .

In file included from sketch/MarlinConfig.h:39:0,
                 from sketch/G26_Mesh_Validation_Tool.cpp:27:
SanityCheck.h:588: error: static assertion failed: Please enable only one probe option: PROBE_MANUALLY, FIX_MOUNTED_PROBE, BLTOUCH, SOLENOID_PROBE, Z_PROBE_ALLEN_KEY, Z_PROBE_SLED, or Z Servo.
 static_assert(1 >= 0

thinkyhead · 2017-11-23T01:32:26Z

To speed up Travis CI and skip all extra tests, removed everything after the base config test. Safe to fetch and merge from origin.

thinkyhead · 2017-11-23T01:34:42Z

Marlin/ubl_motion.cpp

+  #if ENABLED(UBL_G26_MESH_VALIDATION)
+    extern bool g26_debug_flag;
+  #else
+    bool g26_debug_flag=false;


constexpr bool g26_debug_flag = false;?

The UBL_motion.cpp file still uses g26_debug_flag to spit out extra information about the mesh cells and where the nozzle is passing. So in a sense, it is still slightly UBL related. That is why it needs a place to be defined even if G26 is turned off.

This is a temporary work around... Maybe it should be called detailed_movement_debug_flag, or something like that. That would make it more natural to find a home for it. And maybe leverage in other movement functions...

thinkyhead · 2017-11-23T01:53:30Z

Used global search-and-replace to put new options in all configs.
Global search-replace and some edits to rename option to G26_MESH_VALIDATION.

Roxy-3D · 2017-11-23T02:44:13Z

It maybe we should add a sanity check that G26 can't be turned on without 1 of the 3 Mesh systems turned on??? That would eliminate the nested #if's. One strange thing I noticed is I could not do a single line

#if ENABLED(UBL_G26_MESH_VALIDATION) && (ENABLED(MESH_BED_LEVELING) || ENABLED(AUTO_BED_LEVELING_UBL) || ENABLED(AUTO_BED_LEVELING_BILINEAR))
It seems like that should work. But it doesn't.

+      #if ENABLED(UBL_G26_MESH_VALIDATION)
 +        #if ENABLED(MESH_BED_LEVELING) || ENABLED(AUTO_BED_LEVELING_UBL) || ENABLED(AUTO_BED_LEVELING_BILINEAR)
 +          void gcode_G26();
 +          case 26: // G26: Mesh Validation Pattern generation
 +            gcode_G26();
 +            break;

Should I go ahead and get the example Configuration.h files in this format?

thinkyhead · 2017-11-23T03:07:26Z

It's not needed, because the UBL_G26_MESH_VALIDATION option is nested under those conditions in the configurations now. But, I should add a sanity check too.

#if ENABLED(MESH_BED_LEVELING) || ENABLED(AUTO_BED_LEVELING_BILINEAR) || ENABLED(AUTO_BED_LEVELING_UBL)
  // Gradually reduce leveling correction until a set height is reached,
  // at which point movement will be level to the machine's XY plane.
  // The height can be set with M420 Z<height>
  #define ENABLE_LEVELING_FADE_HEIGHT

  /**
   * Enable the G26 Mesh Validation Pattern tool.
   */
  //#define G26_MESH_VALIDATION   // Enable G26 mesh validation
  #if ENABLED(G26_MESH_VALIDATION)
    #define MESH_TEST_NOZZLE_SIZE     0.4   // (mm) Diameter of primary nozzle.
    #define MESH_TEST_LAYER_HEIGHT    0.2   // (mm) Default layer height for the G26 Mesh Validation Tool.
    #define MESH_TEST_HOTEND_TEMP   205.0   // (°C) Default nozzle temperature for the G26 Mesh Validation Tool.
    #define MESH_TEST_BED_TEMP       60.0   // (°C) Default bed temperature for the G26 Mesh Validation Tool.
  #endif

#endif

Roxy-3D · 2017-11-23T03:11:29Z

Can you explain why the one line #if doesn't work?

#if ENABLED(UBL_G26_MESH_VALIDATION) && (ENABLED(MESH_BED_LEVELING) || ENABLED(AUTO_BED_LEVELING_UBL) || ENABLED(AUTO_BED_LEVELING_BILINEAR))

(It always fails... Even with G26 turned on and any of the bed leveling systems active.)

thinkyhead · 2017-11-23T03:15:30Z

If options are set up with encompassing conditions in the configurations, and sanity check is done to ensure that options aren't enabled without the things they depend on, then the rest of the code can simply refer to the one condition. So…

#if ENABLED(UBL_G26_MESH_VALIDATION)

…is all you need.

Roxy-3D · 2017-11-23T03:17:20Z

Yes... But originally... Instead of two nested #if's I tried that single line #if. It never worked. And it sure seems like it should have.

thinkyhead · 2017-11-23T03:21:22Z

It definitely should've. Try it again and see.

BTW, the conditionals file defines a shorthand for use anywhere. HAS_MESH is true for those three forms of bed leveling.

#define HAS_MESH (ENABLED(AUTO_BED_LEVELING_BILINEAR) \
               || ENABLED(AUTO_BED_LEVELING_UBL) \
               || ENABLED(MESH_BED_LEVELING))

So…

#if !HAS_MESH && ENABLED(G26_MESH_LEVELING)
  #error "No!"
#endif

Roxy-3D · 2017-11-23T03:29:45Z

Should we also resolve this duplication prior to merge?

+  /**
 +   * These definations for _GET_MESH_X/Y() are identical to the ones in Marlin_main.cpp
 +   * Probably these should be moved to a common place.
 +   */
 +  #if ENABLED(AUTO_BED_LEVELING_BILINEAR)
 +    #define _GET_MESH_X(I) bilinear_start[X_AXIS] + I * bilinear_grid_spacing[X_AXIS]
 +    #define _GET_MESH_Y(J) bilinear_start[Y_AXIS] + J * bilinear_grid_spacing[Y_AXIS]
 +  #elif ENABLED(AUTO_BED_LEVELING_UBL)
 +    #define _GET_MESH_X(I) ubl.mesh_index_to_xpos(I)
 +    #define _GET_MESH_Y(J) ubl.mesh_index_to_ypos(J)
 +  #elif ENABLED(MESH_BED_LEVELING)
 +    #define _GET_MESH_X(I) mbl.index_to_xpos[I]
 +    #define _GET_MESH_Y(J) mbl.index_to_ypos[J]
 +  #endif
 +

When you merge... I'll start a Pull Request for v2.0.0 with identical changes...

thinkyhead · 2017-11-23T18:10:22Z

Should we also resolve this duplication prior to merge?

Totally.

Roxy-3D · 2017-11-23T20:59:17Z

Both Marlin_main.cpp and G26_Mesh_Validation_Tool.cpp need those definitions. The problem is, those lines are not really enough to justify a whole new file. It isn't obvious to me where the lines should end up.

One answer might be to define _GET_MESH_X() and _GET_MESH_Y() in each bed leveling system's .h file. The problem with doing that is Bi-Linear doesn't have a .h file. Maybe that is the answer? Maybe Bi-Linear should have a small .h file for its stuff? If so, then it becomes obvious how to eliminate the redundancy.

thinkyhead · 2017-11-23T22:33:37Z

For 1.1.x (for now, anyway) they should just go into Marlin.h which is parent to the core auto bed leveling code in Marlin_main.cpp.

The 2.0 branch moves those to a main bedlevel.h file, with common bed leveling code in a bedlevel.cpp file. After this passes Travis and is tested and merged, I can do a followup PR that separates out core bed leveling for the 1.1.x branch into bedlevel.* files too. It's a bigger job than I want to get into in this PR.

Roxy-3D · 2017-11-23T22:45:20Z

After this passes Travis and is tested and merged, I can do a followup PR that separates out core bed leveling for the 1.1.x branch into bedlevel.* files too.

OK. I'll wait until that happens to start making the bugfix-v2.0.0 Pull Request. At that point it should be a lot more straight forward to do the v2.0.0 Pull Request.

UPDATE: Those magic 3 git lines worked. I was going to move the _GET_MESH_X() and _GET_MESH_Y() lines to Marlin.h but it looks like that has already been done. Are we ready to merge? As it turns out, there is already some 'testing' of these changes at #8513 (comment) Those are pictures of somebody with Bi-Linear using the G26 tool to figure out why he is getting bad adhesion along the edges and corners.... So... The basic functionality is working with Bi-Linear now.

thinkyhead · 2017-11-24T03:20:36Z

Those magic 3 git lines worked.

Great! You can use them again to fetch the latest changes and bring your working copy up to date. I think this PR is pretty solid at this point, and probably ready to merge.

git checkout bugfix-1.1.x            ;# This branch's working copy
git fetch origin                     ;# Get @th's changes from github
git reset --hard origin/bugfix-1.1.x ;# Point working copy at now

I also went through the process of putting together G26_MESH_VALIDATION for the 2.0.x branch in #8535. It was no walk in the park, but it's passing Travis now, and if —as I found— the changes to G26 are surprisingly easy and it's ready to go, then we can merge both of these anytime.

Example Configuration.h files are not updated yet. You need to cross your settings over to the default Configuration.h file in the \Marlin directory. (UBL_G26_MESH_VALIDATION enablement has moved to a new location in the file.)

Roxy-3D · 2017-11-24T03:50:52Z

Those magic 3 git lines worked.

Great! You can use them again to fetch the latest changes and bring your working copy up to date. I think this PR is pretty solid at this point, and probably ready to merge.

My fork is already Rebased now. Shouldn't GitHub Desktop Sync work now?

thinkyhead · 2017-11-24T03:53:24Z

If you do another of those git fetch-and-reset command-line operations, then you'll be caught up. But I'm patching and squashing as I go, until it passes Travis, so you may want to wait a little longer.

thinkyhead · 2017-11-24T05:19:43Z

Merged, haha! So I guess now you can…

git checkout bugfix-1.1.x                    ;# This branch's working copy
git fetch MarlinFirmware                     ;# Get the newest bugfix from main fork
git reset --hard MarlinFirmware/bugfix-1.1.x ;# Point working copy at now
git push -f origin                           ;# Update your fork on github

Roxy-3D mentioned this pull request Nov 22, 2017

[1.1.6] Billinear bed leveling not precise correcting #8513

Closed

thinkyhead changed the title ~~1st pass at making G26 work with Bi-Linear Leveling.~~ [1.1.x] 1st pass at making G26 work with Bi-Linear Leveling. Nov 22, 2017

thinkyhead added PR: Improvement F: Calibration labels Nov 22, 2017

thinkyhead reviewed Nov 23, 2017

View reviewed changes

thinkyhead added Needs: Work More work is needed S: Don't Merge Work in progress or under discussion. labels Nov 23, 2017

Roxy-3D added Status: Ready to Merge S: Experimental and removed S: Don't Merge Work in progress or under discussion. labels Nov 23, 2017

thinkyhead changed the title ~~[1.1.x] 1st pass at making G26 work with Bi-Linear Leveling.~~ [1.1.x] Make G26 work with other meshes too Nov 23, 2017

thinkyhead mentioned this pull request Nov 24, 2017

[2.0.x] Adapt G26 to work for all meshes #8535

Merged

Roxy-3D and others added 2 commits November 23, 2017 21:41

Make G26 work with all mesh leveling.

8282d73

Example Configuration.h files are not updated yet. You need to cross your settings over to the default Configuration.h file in the \Marlin directory. (UBL_G26_MESH_VALIDATION enablement has moved to a new location in the file.)

Apply G26 for meshes to all configs

e5b43d4

Roxy-3D removed the Status: Ready to Merge label Nov 24, 2017

Arrange G26 functions in dependency order

3b431f2

thinkyhead merged commit 5c08772 into MarlinFirmware:bugfix-1.1.x Nov 24, 2017

benlye mentioned this pull request Nov 24, 2017

BUG: encoder button BTN_ENC broken in bugfix-1.1.x #8537

Closed

[1.1.x] Make G26 work with other meshes too #8522

[1.1.x] Make G26 work with other meshes too #8522

Conversation

Roxy-3D commented Nov 22, 2017 • edited

thinkyhead commented Nov 22, 2017

Roxy-3D commented Nov 22, 2017

thinkyhead commented Nov 22, 2017

thinkyhead commented Nov 22, 2017 • edited by Roxy-3D

Roxy-3D commented Nov 22, 2017 • edited

thinkyhead commented Nov 22, 2017 • edited

Roxy-3D commented Nov 22, 2017 • edited by thinkyhead

thinkyhead commented Nov 23, 2017 • edited

Roxy-3D commented Nov 23, 2017

thinkyhead Nov 23, 2017

Choose a reason for hiding this comment

Roxy-3D Nov 23, 2017 • edited

Choose a reason for hiding this comment

thinkyhead Nov 23, 2017

Choose a reason for hiding this comment

thinkyhead Nov 23, 2017

Choose a reason for hiding this comment

thinkyhead Nov 23, 2017

Choose a reason for hiding this comment

Roxy-3D Nov 23, 2017

Choose a reason for hiding this comment

thinkyhead commented Nov 23, 2017 • edited

thinkyhead commented Nov 23, 2017 • edited

thinkyhead Nov 23, 2017

Choose a reason for hiding this comment

Roxy-3D Nov 23, 2017

Choose a reason for hiding this comment

thinkyhead commented Nov 23, 2017

Roxy-3D commented Nov 23, 2017 • edited

thinkyhead commented Nov 23, 2017 • edited

Roxy-3D commented Nov 23, 2017 • edited

thinkyhead commented Nov 23, 2017

Roxy-3D commented Nov 23, 2017

thinkyhead commented Nov 23, 2017 • edited

Roxy-3D commented Nov 23, 2017 • edited

thinkyhead commented Nov 23, 2017

Roxy-3D commented Nov 23, 2017 • edited

thinkyhead commented Nov 23, 2017 • edited

Roxy-3D commented Nov 23, 2017 • edited

thinkyhead commented Nov 24, 2017 • edited

Roxy-3D commented Nov 24, 2017 • edited

thinkyhead commented Nov 24, 2017

thinkyhead commented Nov 24, 2017 • edited

Roxy-3D commented Nov 22, 2017 •

edited

thinkyhead commented Nov 22, 2017 •

edited by Roxy-3D

Roxy-3D commented Nov 22, 2017 •

edited

thinkyhead commented Nov 22, 2017 •

edited

Roxy-3D commented Nov 22, 2017 •

edited by thinkyhead

thinkyhead commented Nov 23, 2017 •

edited

Roxy-3D Nov 23, 2017 •

edited

thinkyhead commented Nov 23, 2017 •

edited

thinkyhead commented Nov 23, 2017 •

edited

Roxy-3D commented Nov 23, 2017 •

edited

thinkyhead commented Nov 23, 2017 •

edited

Roxy-3D commented Nov 23, 2017 •

edited

thinkyhead commented Nov 23, 2017 •

edited

Roxy-3D commented Nov 23, 2017 •

edited

Roxy-3D commented Nov 23, 2017 •

edited

thinkyhead commented Nov 23, 2017 •

edited

Roxy-3D commented Nov 23, 2017 •

edited

thinkyhead commented Nov 24, 2017 •

edited

Roxy-3D commented Nov 24, 2017 •

edited

thinkyhead commented Nov 24, 2017 •

edited