Auto_Bed_Leveling: Enhanced G29 command

[Roxy]

Why 3 for the servos when I only have one?

Changes made to the config as shown.

This single error might explain the flakey behavior of your system. Let's dig in:

In Configuration.h you have the number of servo's defined as 1 in a statement like this:

#define NUM_SERVOS 1 // Servo index starts with 0 for M280 command

Let's check the data structures in Marlin_main.cpp. At roughly 400 line into the file you will see a declaration:

#if NUM_SERVOS > 0
  Servo servos[NUM_SERVOS];
#endif

This is where storage to manage the servo's happens. You allocated storage for 1 servo. Nothing more! But lets go look how this storage is used. It doesn't matter where you look, but let's check out the engage_z_probe() function as an example:

static void engage_z_probe() {
    // Engage Z Servo endstop if enabled
    #ifdef SERVO_ENDSTOPS
    if (servo_endstops[Z_AXIS] > -1) {
#if defined (ENABLE_AUTO_BED_LEVELING) && (PROBE_SERVO_DEACTIVATION_DELAY > 0)
        servos[servo_endstops[Z_AXIS]].attach(0);
#endif
        servos[servo_endstops[Z_AXIS]].write(servo_endstop_angles[Z_AXIS * 2]);
#if defined (ENABLE_AUTO_BED_LEVELING) && (PROBE_SERVO_DEACTIVATION_DELAY > 0)
        delay(PROBE_SERVO_DEACTIVATION_DELAY);
        servos[servo_endstops[Z_AXIS]].detach();
#endif
    }
    #endif
}

Check out the three lines in that function. You can trace them into the C++ code in servos.cpp if like but they write to and use the servos[] array (of size 1). But Z_AXIS is defined as 2 in Marlin.h:

enum AxisEnum {X_AXIS=0, Y_AXIS=1, Z_AXIS=2, E_AXIS=3};

        servos[servo_endstops[Z_AXIS]].attach(0);
        servos[servo_endstops[Z_AXIS]].write(servo_endstop_angles[Z_AXIS * 2]);
        servos[servo_endstops[Z_AXIS]].detach();

By doing this, you are writing (and reading) off the end of the array. This is a VERY bad thing to do because you are clobbering what ever memory is in the wrong spot. Here is the stuff right after the servos[] array:

bool CooldownNoWait = true;
bool target_direction;

This is super bad if this is being allocated next in memory. It is the next thing but it is declared as external so maybe you are not corrupting this. If this gets corrupted, all of the malloc() and free() of memory get sick and do bad things.

extern "C"{
  extern unsigned int __bss_end;
  extern unsigned int __heap_start;
  extern void *__brkval;

  int freeMemory()

[DarkAlchemist]

So, it is an issue within the ABL code itself then because you are defining how many servos you really have, in my case 1, but the code is not able, or possibly not able, to handle only a declaration of 1 servo.

If the above statement is true it would seem wise to remove the declaration to me or to fix the code (sure it up) so it can actually handle the real number of servos you actually have attached.

[Roxy]

I just did a compare of the Bdac Configuration.h file and yours... You probably want to add this line to your stuff:

#define ENABLE_AUTO_BED_LEVELING // Delete the comment to enable (remove // at the start of the line)
#define Z_PROBE_REPEATABILITY_TEST  // If not commented out, Z-Probe Repeatability test will be included if Auto Bed Leveling is Enabled.

#ifdef ENABLE_AUTO_BED_LEVELING

The good news is I don't see anything too scary. You might be able to just replace his Configuration.h file with yours and compile.

[DarkAlchemist]

#define Z_PROBE_REPEATABILITY_TEST

What is that used for in the new code?

[Roxy]

So, it is an issue within the ABL code itself then because you are defining how many servos you really have, in my case 1, but the code is not able, or possibly not able, to handle only a declaration of 1 servo.

If the above statement is true it would seem wise to remove the declaration to me or to fix the code (sure it up) so it can actually handle the real number of servos you actually have attached.

Agreed... But remember, this is Open Source. The person that did the original Auto Bed Leveling code grabbed somebody else's Servo code. And that is how the servo code was setup. This really should be cleaned up because a lot of people are making that mistake. But once again, this is Open Source... So anybody that wants it changed bad enough can make the change. Just be Thankful you have the source so these types of issues can be resolved.

#define Z_PROBE_REPEATABILITY_TEST

What is that used for in the new code?

This is the M48 command I wrote. I actually went through the process to get this merged back into the main code base. It will let us run some numbers once everything else is working to see how repeatable your Z-Probe is (and as a result, to see how well the Auto Bed Leveling is going to work on your system.)

[DarkAlchemist]

Then how do we get to the new fork author(s) and let them know this needs to be cleaned up because this is a bad one imo. No need for OS to have this sort of stuff continue once someone finds an issue if someone is actively maintaining it, right?

[Roxy]

Then how do we get to the new fork author(s) and let them know this needs to be cleaned up because this is a bad one imo. No need for OS to have this sort of stuff continue once someone finds an issue if someone is actively maintaining it, right?

You would make a branch of Erik Zalm's Marlin code and fix the issue. I just checked a frozen version of the code from September 15th (rather than get the current stuff). That #define sets the number to 3, so it is correct. Probably 'fixing' this issue just involves making the comments (the documentation) much more clear. Or, you could get more clever and just make that array big enough that this problem never happens.

Once you have the code correct, you issue a 'Pull Request' to Erik Zalm and eventually he will take a look at what you did and if he likes it, he will fold it back into the main branch of the tree. It requires effort and so far nobody has done the required effort.

With that said... In another thread Dacb is supporting changes to the Marlin coded base for MakerFarm printers. (That is the code base we are going to wake up on your printer). I'm thinking I'll suggest a better set of comments for that file and if Dacb agrees, he will fold them into his Configuration.h file. Then later, when ever we (he) get around to it, the better documentation will be there when his fork gets merged back into the main Marlin tree.

UPDATE: Actually... I posted the stuff in the other thread so Dacb could add it to his fork, but I just deleted that post. The reason is I think this is OK. There is a table that gets used and it indexes into the 0 slot of that array. So declaring 1 servo is OK.

This line is used:
int servo_endstops[] = SERVO_ENDSTOPS

with SERVO_ENDOSTOPS defined as:
#define SERVO_ENDSTOPS {-1, -1, 0} // Servo index for X, Y, Z. Disable with -1

That one line isn't causing your flakey behaviour....

[DarkAlchemist]

I have compiled it and it did compile fine with all of these changes.

[DarkAlchemist]

Well, what do I do now? It compiled but I did not flash.

[Roxy]

Well, what do I do now? It compiled but I did not flash.

Go ahead and flash it... Let's see if it moves in the right direction and detects the end stops and such. If so, give it a G28 followed by a G29...