Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Merged
merged 2 commits into from
Jul 5, 2016
+9 −6
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d4ff0c
Allow the queue to be cleared from within commands
thinkyhead Jul 5, 2016
b114b6a
Save 152 bytes using uint8_t for command indices
thinkyhead Jul 5, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions Marlin/Marlin_main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,11 +284,11 @@ bool axis_homed[3] = { false };

static long gcode_N, gcode_LastN, Stopped_gcode_LastN = 0;

static char* current_command, *current_command_args;
static int cmd_queue_index_r = 0;
static int cmd_queue_index_w = 0;
static int commands_in_queue = 0;
static char command_queue[BUFSIZE][MAX_CMD_SIZE];
static char* current_command, *current_command_args;
static uint8_t cmd_queue_index_r = 0,
cmd_queue_index_w = 0,
commands_in_queue = 0;
Copy link
Contributor

@Blue-Marlin Blue-Marlin Jul 5, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are damn fast.

Copy link
Member Author

@thinkyhead thinkyhead Jul 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member Author

@thinkyhead thinkyhead Jul 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Blue-Marlin Blue-Marlin Jul 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@thinkyhead thinkyhead Jul 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


#if ENABLED(INCH_MODE_SUPPORT)
float linear_unit_factor = 1.0;
Expand Down Expand Up @@ -990,8 +990,11 @@ void loop() {

#endif // SDSUPPORT

commands_in_queue--;
cmd_queue_index_r = (cmd_queue_index_r + 1) % BUFSIZE;
// 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;
cmd_queue_index_r = (cmd_queue_index_r + 1) % BUFSIZE;
}
}
endstops.report_state();
idle();
Expand Down