[2.0.x] Advanced G29 probe fail recovery and retry

[marcio-ao]

This PR adds the G29_ADVANCED_RECOVERY_AND_RETRY option to "Configuration_adv.h" that allows the user to define a recovery and retry behavior for "G29".

By specifying "G29_MAX_RETRIES", the user can control how many times the G29 will repeat itself when one of the probes fails.

The user can declare a custom GCODE script to attempt to clear the probe failure condition prior to a retry, such as by:

• Re-levelling the X axis by running the Z axis into endstops
• Wipe the nozzle clean on a pad

In total, the user can declare a custom GCODE scripts to execute on the following conditions:

• When a probe in G29 fails and before a retry (G29_RECOVER_COMMANDS)
• When the G29 executes successfully (G29_SUCCESS_COMMANDS)
• When all retries are completed in failure (G29_FAILURE_COMMANDS)

The user may also declare action keywords which are used to notify the host software of exhausted retried (G29_ACTION_ON_FAILURE) and of the initiation of the recovery script (G29_ACTION_ON_RECOVER).

Furthermore, G29_HALT_ON_FAILURE will cause the printer to halt completely if the G29 fails. This keeps the printer from proceeding to print on an un-leveled bed.

[thinkyhead]

Here's a problem. When you execute other G-code commands, their parameters replace the ones in the parser. So the original parameters to G29 get lost, and if the last command to get executed before this call has parameters of its own, they will be sent to and get interpreted by G29().

So you'll need to save the original G29 command string and call parser.parse on it before re-entering G29.

char cmd[strlen(parser.command_ptr) + 1];
strcpy(cmd, parser.command_ptr);
set_bed_leveling_enabled(false);
for (uint8_t i = G29_MAX_RETRIES; i--;) {
  parser.parse(cmd);
  G29();
  . . .
}

[marcio-ao]

@thinkyhead: Ah, good catch. I put a big warning in the comments to execute_commands_immediate_P() about the parser state being modified, and yet I walked right into the trap that I was warning others about. Oh the irony.

Saving the command string is easily doable in this particular instance, so I'll make the fix. Thanks for catching that.

I haven't looked into the parsing code at all, but do you have any notions as to whether its global state could be made into class variables? This would allow a new parser to be created where needed, and this would allow execute_commands_immediate_P() to create an new instance of the parser for its own use and thus make it safe to use anywhere.

[marcio-ao]

@thinkyhead: How about I put the changes you suggested in execute_commands_immediate_P() itself? I.e:

void GcodeSuite::execute_commands_immediate_P(const char *pgcode) {
  // Save the parser state
  char saved_cmd[strlen(parser.command_ptr) + 1];
  strcpy(saved_cmd, parser.command_ptr);

  // Process individual commands in string
  char cmd[30];
  while (pgm_read_byte_near(pgcode) != '\0') {
    const char *delim = strchr_P(pgcode, '\n');
    size_t len = delim ? delim - pgcode : strlen_P(pgcode);
    strncpy_P(cmd, pgcode, len);
    cmd[len] = '\0';
    pgcode += len;
    if (delim) pgcode++;

   // Parse the next command in the string
    parser.parse(cmd);
    process_parsed_command(true);
  }

  // Restore the parser state
  parser.parse(saved_cmd);
}

This would make execute_commands_immediate_P safe to use anywhere.

[thinkyhead]

do you have any notions as to whether its global state could be made into class variables?

We prefer static classes for their economical linkage.
A string is the cheapest storage for a G-code command and/or its parameters.

[thinkyhead]

How about I put the changes you suggested in execute_commands_immediate_P() itself?

That works, if the intent is to always use the function for sub-commands. In which case, maybe it should be renamed to execute_subcommands_P().

[marcio-ao]

That works, if the intent is to always use the function for sub-commands. In which case, maybe it should be renamed to execute_subcommands_P().

@thinkyhead : The use cases I have in mind are:

• When a GCODE has a need for an user-defined script, defined in Configuration.h or Configuration_adv.h, to modify the GCODE behavior, such as in the case of G29.
• When a third-party developer wants to modify the functionality of a GCODE, knows the GCODE moves they want to insert and where, but does not necessarily know the required C++ calls to do the same moves.
• When someone wants to test a quick modification to a GCODE to see how it behaves, without necessarily having to write a lot of C++ code.

I suspect these two cases are what you are calling "subcommands", and I think the name change is appropriate, although I'd need examples of what other situations may not be "subcommands", to know whether that is necessary.

That said, aside from a small performance hit, it should not hurt to save the parser state even in situations where it may not be needed. I would prefer execute_subcommands_P() to be a convenience function that is easy and safe to use without many restrictions, rather than something that is optimized for performance or memory use. If someone really wants performance, they can always implement what they need in C++, as has been the usual case in Marlin up to now. I guess I see "execute_subcommands_P()" as an "interpreted language" for Marlin, as opposed to "compiled" code, with the exact same trade-offs.

I'll go ahead with the function name change, as it makes sense to me, and I also put in the "execute_subcommands()" that operates on RAM, as @Roxy-3D has suggested.

[marcio-ao]

@thinkyhead : Fixed the parser state problem based on your feedback.

Also, used a variable sized buffer for cmd[], like you used for saved_cmd[]. I guess somehow I had the misconception that array sizes needed to be constant, but I now see that they don't need to be. I've just learned something new.

[thinkyhead]

I had the misconception that array sizes needed to be constant

Sometimes they do, but not always! (Local autos can.) It's a handy way to do a "malloc," but on the stack.