Allow the command queue to be cleared by commands, lcd menus

[thinkyhead]

Addressing #4042.

Background: A command (such as M190) may be in progress during an SD print. While waiting for temperature, it will call idle() in a loop. The idle() function will call lcd_update() and a user might invoke lcd_sdcard_stop() from there. "Stop print" clears the queue by setting commands_in_queue to 0 and cancels heatup, so M109 returns back to loop(), where process_next_command() was called. The next thing loop() does is commands_in_queue--. Since we cleared the command queue within process_command(), commands_in_queue gets decremented here to -1. As a result, code that uses if (commands_in_queue) { . . . } will continue to process the queue 65,535 more times — not what we want!

This PR fixes the issue by checking whether commands_in_queue is non-zero before decrementing it and moving on to the next command.

• Also use uint8_t for the command queue indices, saving 152 bytes of PROGMEM. (And hey, if this bug comes back, it will only process the queue 255 extra times.)

[Blue-Marlin]

Why not volatile?
loop() might load commands_in_queue into a register. When calling process_next_command() this register might be saved to the stack. process_next_command() might try to store a move in the planner. If the planner buffer is full it calls idle(), calling manage_inactivity(), calling get_available_commands(). That may add a a new command into the buffer - commands_in_queue is increased in memory. All called functions may return - the registers restored and loop() may have a wrong value for commands_in_queue now, what is decreased and stored back to memory.

There are so many ways you can reach idle().

A static volatile uint8_t at least can not hurt.

[Blue-Marlin]

You are damn fast.

[thinkyhead]

My understanding is that the compiler is smart enough not to presume that variables that you use within a function are "non-volatile" when calling a function in a separate execution unit. The compiler understands that any variable passed as public to the linker might be altered in the other execution unit. If the compiler didn't take this sort of thing into account, we would have to declare most variables as volatile.

In other words, the compiler should never try to optimize the second test of the value here…

if (commands_in_queue == 0) { call_lcd_function(); }
call_local_function();
if (commands_in_queue) {
  . . .
}

…because the interceding function calls are free to alter it.

I believe the intended use of volatile is when an interrupt might alter it outside the normal execution flow. Not that it might be altered by some function that you call from within the main thread.

But I will look further into when it is recommended for use!

[thinkyhead]

Sure enough, the variable (0x04A5) is treated as if it were volatile in the loop() function…

    547a:   80 91 a5 04     lds r24, 0x04A5
ExitUnknownCommand:

  // Still unknown command? Throw an error
  if (!code_is_good) unknown_command_error();

  ok_to_send();
    547e:   88 23           and r24, r24

  #if ENABLED(SDSUPPORT)
    card.checkautostart(false);
  #endif

  if (commands_in_queue) {
    5480:   81 f0           breq    .+32        ; 0x54a2 <loop+0x34>
      else
        process_next_command();

    #else

      process_next_command();
    5482:   85 dc           rcall   .-1782      ; 0x4d8e <_Z20process_next_commandv>
    5484:   80 91 a5 04     lds r24, 0x04A5

    #endif // SDSUPPORT

    // The queue may be reset by a command handler or by code invoked by idle() within a handler
    if (commands_in_queue) {
    5488:   88 23           and r24, r24
    548a:   59 f0           breq    .+22        ; 0x54a2 <loop+0x34>
    548c:   81 50           subi    r24, 0x01   ; 1
      --commands_in_queue;
    548e:   80 93 a5 04     sts 0x04A5, r24
    5492:   80 91 a7 04     lds r24, 0x04A7
      cmd_queue_index_r = (cmd_queue_index_r + 1) % BUFSIZE;
    5496:   90 e0           ldi r25, 0x00   ; 0
    5498:   01 96           adiw    r24, 0x01   ; 1
    549a:   83 70           andi    r24, 0x03   ; 3
    549c:   99 27           eor r25, r25
    549e:   80 93 a7 04     sts 0x04A7, r24

See that immediately after process_next_command() is called, commands_in_queue is reloaded from RAM: lds r24, 0x04A5. So the compiler is smartly not assuming it was unaltered.

[Blue-Marlin]

If after the above happened and the buffer is filled up to BUFSIZE-1 commands - one will become written over. However - the host will not see enough 'ok's and might stop sending. #4075?

[thinkyhead]

Contrast this with what happens if I remove the call to process_next_command… Of course it totally optimizes out the second test of commands_in_queue…

    20d8:   80 91 b5 03     lds r24, 0x03B5

  #if ENABLED(SDSUPPORT)
    card.checkautostart(false);
  #endif

  if (commands_in_queue) {
    20dc:   88 23           and r24, r24
    20de:   59 f0           breq    .+22        ; 0x20f6 <loop+0x28>
    20e0:   81 50           subi    r24, 0x01   ; 1

    // The queue may be reset by a command handler or by code invoked by idle() within a handler
    if (commands_in_queue) {
      --commands_in_queue;
    20e2:   80 93 b5 03     sts 0x03B5, r24

Insert a meaningless function call in the same execution unit and the compiler still optimizes…

    do_nothing_really(); // SERIAL_EOL;
    20f4:   c1 50           subi    r28, 0x01   ; 1
    20f6:   c0 93 b5 03     sts 0x03B5, r28

But not if the function we call alters the counter…

    do_something(); // commands_in_queue += 1
    20ee:   80 91 b5 03     lds r24, 0x03B5

…or if the function body exists someplace the compiler can't see…

    planner_do_nothing(); // SERIAL_EOL;
    20e2:   80 91 b5 03     lds r24, 0x03B5