Fix: current_command_args contains command

[CONSULitAS]

@thinkyhead
Hi Scott,
your code from a0f362c@thinkyhead is great, but you forgot to skip the
command.
Symptom M117 Testshows M117 Test on LCD instead of Test.

see also
a0f362c735401ebbcd95de3f
6f8e3c2f17ecc770 lines 941, 2851 and so on

Greetings and welcome back

Jochen

[thinkyhead]

@CONSULitAS OMG you're right, haha! Thanks for this fix. It should go into MarlinDev too.

By the way, have you looked at the way GCode is interpreted in Matt Roberts' RepRap firmware? It's really quite smart. I might try to use his approach for a future revision.

And… it's good to be back! So much to catch up on…

[thinkyhead]

The recent GCode parser changes were intended to make it more space-tolerant, so even a command as ugly as this should still work:

• N1G 1 X100.5Y 99.1 *44

So I think actually this needs to be changed further. From this:

current_command_args = current_command + 2; // skip first letter and following character
while (*current_command_args >= '0' && *current_command_args <= '9') ++current_command_args;
while (*current_command_args == ' ') ++current_command_args;

…to this…

current_command_args = current_command + 1; // skip the code letter
while (*current_command_args == ' ' || (*current_command_args >= '0' && *current_command_args <= '9'))
  ++current_command_args;

It only skips one character because the command could be something like N1 M with nothing else — although actually the code_is_good flag should catch this.

Agh, actually I see the code_is_good evaluation is not space-tolerant either. A command like N1 M 100 will not be interpreted with the current code. So I'll need to revamp this a little further….

[CONSULitAS]

@thinkyhead You are welcome. :-)

Could you please explain what to do with GitHub to migrate this to MarlinDev? Thanks

Your suggestion for the parser is interesting. We should indeed make no assumptions about the GCODE we get. And I like code which is error tolerant specially for foreign input data.

But I think, the condition is hardly readable and not easy to maintain. Additionally we have other similar code parts on other places in Marlin_main. So we should make it clearer and easier to maintain.

From your:

while (*current_command_args == ' ' || (*current_command_args >= '0' && *current_command_args <= '9'))
  ++current_command_args;

to

  #define IS_DIGIT(thechar) ((thechar) - '0' + 0U <= 9U)
  #define IS_SPACE(thechar) ((thechar) == ' ')
  #define IS_NOTNULL(thechar) ((thechar) != '\0')
  while (IS_NOTNULL(*current_command_args) && (IS_SPACE(*current_command_args) || IS_DIGIT(*current_command_args))) ++current_command_args;

but even better

  #define IS_DIGIT(thechar) ((thechar) - '0' + 0U <= 9U)
  #define IS_SPACE(thechar) ((thechar) == ' ')
  #define IS_NOTNULL(thechar) ((thechar) != '\0')
  #define SKIPCHARS_WHILE(thestringpointer, thecondition) \
    while( IS_NOTNULL(thestringpointer) && (thecondition) ) \
      (++thestringpointer)

  // skip spaces and digits [ 0-9]*
  SKIPCHARS_WHILE( current_command_args, (IS_SPACE(*current_command_args) || IS_DIGIT(*current_command_args) ) );

with this while (*current_command == ' ') ++current_command;would become SKIPCHARS_WHILE(current_command, IS_SPACE(*current_command)); // skip [ ]*

The Macros don't change the size of the code except IS_DIGIT, which even saves some bytes and is safe for side effects by ++ too (see http://ptspts.blogspot.de/2009/03/ascii-isdigit-isalpha-and-isxdigit.html).

BTW: Inspiration for this is partly from arduino/Arduino@cd9a6ec which has been merged to Arduino two weeks ago. Great code!

[thinkyhead]

Well, you know how I love macros. 😁 I always wonder if the compiler optimizer is smart enough to tell that if (thing != 0 && thing == ' ') can be reduced to if (thing == ' '). If it is, then such "pseudo-code" can be helpful. We don't do huge amounts of parsing in Marlin, but string macros can always come in handy.

Some processing may be obviated by having a parser that works a bit differently – collecting all the argument values into an array up-front, avoiding calling any library functions to parse numbers, and avoiding floats. (Using code like v = v * 10 + c - '0' as digits arrive, etc.). The current method where each argument is scanned-for in the string ends up being wasteful especially for G1.

We can explore that further in the next iteration. Meanwhile the macros may help…

As for migrating fixes to MarlinDev, well I've got this one. See MarlinFirmware/MarlinDev#352. Basically, I just keep a working copy of this and of MarlinDev, so I can always create a new branch, apply changes, and submit a PR pretty quickly. I just keep my dev branch up-to-date and use git checkout dev -b my_fixup_branch to start any changes. I'm able to compare the text of different branches side-by-side and apply changes either way, so I use that to move changes over to my dev working copy, commit them, and make a new PR against dev.

[Blue-Marlin]

As far as i know, there is no general optimisation for this. The compiler is not smart enough to see that if (thing == ' ') is completely sufficient here.
But it can be optimised by ordering the expressions, if we know about the distribution of the tested cases. (At least in K&R C)
in ( a && b ) b is not evaluated if a is false, because we always get a false.
In ( a || b) b is not evaluated if a is true, because we already know the result.

So in general testing a == ' ' positive is less likely than testing a != 0. (several ' ' in a string but only one end (all but one not 0))
So if (thing == ' ' && thing != 0) should be faster than if (thing != 0 && thing == ' ').
!= 0 is almost always true -> you have to test the second condition.
== ' ' is almost always false -> you can omit the second test.