[BUG] ADVANCED_OK returns additional info

[CRCinAU]

Board is a SKR Mini E3 v2.0

Configs:
https://github.com/MarlinFirmware/Configurations/tree/import-2.0.x/config/examples/Creality/Ender-3%20Pro/BigTreeTech%20SKR%20Mini%20E3%202.0

Enable ADVANCED_OK and compile.

Build is from bugfix-2.0.x on Jan 28 2021.

Output from USB Serial:

Send: N609 G1 X81.579 Y150.534 E0.02365*91
Recv: ok WN\x856m0g9p P0 B1
Send: N610 G1 X81.579 Y153.156 E0.08224*94
Recv: ok WN\x856m1h0g P0 B3
Send: N611 G1 X82.334 Y153.156 E0.02365*93
Recv: ok WN\x856m1h1h P0 B3
Send: N612 G1 X82.334 Y147.117 E0.18936*89
Recv: ok WN\x856m1h2i P0 B3
Send: N613 G1 X83.088 Y146.469 E0.03118*94
Recv: ok WN\x856m1h3j P0 B3
Send: N614 G1 X83.088 Y153.156 E0.20968*90
Recv: ok WN\x856m1h4k P0 B3

I'm not sure what the WN\x856m1h3j etc parts are.... Probably a bug...

[logiclrd]

Just a curious bystander here, I looked up the code that actually outputs the advanced OK line:

  #if ENABLED(ADVANCED_OK)
    char* p = command_buffer[index_r];
    if (*p == 'N') {
      SERIAL_ECHO(' ');
      SERIAL_ECHO(*p++);
      while (NUMERIC_SIGNED(*p))
        SERIAL_ECHO(*p++);
    }
    SERIAL_ECHOPAIR_P(SP_P_STR, int(planner.moves_free()),
                      SP_B_STR, int(BUFSIZE - length));
  #endif

I can't see how that could possibly produce that output, definitely weird! The W character seems to be coming from the second line inside that if, at which point *p should be N, so where is the W coming from?! Definitely weird.

[CRCinAU]

I have a feeling it may be a buffer overflow....

[logiclrd]

Looking closer, it seems like every character is prefixed by another unrelated character:

  MN\x856m1h3j
  ?N????6?1?3?
   N    6 1 3 

That would correspond with it outputting line number N613.

[logiclrd]

(Or perhaps suffixed -- the W may be the suffix to the space character preceding the line number.)

[CRCinAU]

@X-Ryl669 - This wouldn't be related to the recent serial refactor by chance, would it?

[logiclrd]

I dug further and am pretty sure this is a bug introduced by 3f01b22. It refactors serial functionality, and the new functionality for outputting a single character (print(char c, int base = 0)) seems to be missing an else clause:

  NO_INLINE void print(char c, int base = 0)               { print((long)c, base); }

  void print(long c, int base = DEC)
  {
    if (!base)
      write(c);
    // should be `else {` here?
      write((const uint8_t*)"-", c < 0);
      printNumber(c < 0 ? -c : c, base);
    // should be `}` here?
  }

Basically, it says, "if there's no base defined, just output the character", and then it also says "output the character as a number in the specified base", but it doesn't say "only do the second one if not the first one".

I'm actually surprised this isn't causing hard crashes, because printNumber assumes base will be non-zero and divides by it. Those weird extra characters are the result of division by 0!

[logiclrd]

Here's the pre-refactor code:

template<typename Cfg>
void MarlinSerial<Cfg>::print(long n, int base) {
  if (base == 0) write(n);
  else if (base == 10) {
    if (n < 0) { print('-'); n = -n; }
    printNumber(n, 10);
  }
  else
    printNumber(n, base);
}

[logiclrd]

It's not my codebase, but in my opinion, the new code is being far too "clever" for no real gains. I mean, write((const uint8_t*)"-", c < 0); really doesn't scream what it is actually doing. The old way was an extra line of code, and probably compiled to an additional byte or two, but was way more readable and maintainable. ¯\_(ツ)_/¯

[logiclrd]

Yep, that PR is definitely bundling a fix for this bug with the actual build issue fix.

[logiclrd]

[X-Ryl669]

Yes. I've forgotten to write else statement in the code. The PR #20932 is currently collecting the issues like these and providing a fix for all of them. You can try it, it's a work in progress and should solve each issue one after the other.
write((const uint8_t*)"-", c < 0); was in the initial code base, through IIRC.

[logiclrd]

I searched the codebase prior to the refactor and found a more comprehensive and readable implementation. I posted it earlier in this discussion. I think it is much more sensible code personally :-)

[logiclrd]

I see all the discussion and follow-up commits on the other PR, I do think that the more fully-written-out form is better code in a number of ways. :-)

[X-Ryl669]

Please test with latest bugfix, it should be resolved. If so, please close the issue. Thanks!

[github-actions]

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.