Catch extra 'ok' message sent on manual z-axis move

[BarbourSmith]

The firmware was sending an extra 'ok' status when paused prompting the machine to gain one extra line in the buffer with each z-axis adjustment eventually overflowing the buffer.

The solution is to remove the extra command which does not seem to be needed for operation with our without the z-axis.

The pause command does not seem to be effected.

Fixes #366

[krkeegan]

Wait, I don't think this is wrong.

I think the firmware should be sending an 'ok' to the ~ command.

What looks like the issue is that in SerialPortThread.py we don't handle an 'ok' to a quick_queue command.

[BarbourSmith]

Ohhhh I like that a lot! @krkeegan

[krkeegan]

It is a bit odd, but even if a gcode line is sent first and then a quick_code command is sent second, GC should always treat the immediate 'ok' after a quick_code as applying to the receipt of the QC and not the gcode command.

Not very FIFO, but the quick_codes should only be safety commands ~,! and whatever else we add.

[BarbourSmith]

When I'm looking at the code I don't see how the quick_queue is treated differently than the regular commands. They are both sent with the write() method, right?

def _write (self, message):
        #message = message + 'L' + str(len(message) + 1 + 2 + len(str(len(message))) )

        taken = time.time() - self.lastWriteTime
        if taken < self.MINTimePerLine:  # wait between sends
            # self.data.logger.writeToLog("Sleeping: " + str( taken ) + "\n")
            time.sleep (self.MINTimePerLine) # could use (taken - MINTimePerLine)

        message = message.encode()
        print "Sending: " + str(message)

        message = message + '\n'

        self.bufferSpace       = self.bufferSpace - len(message)
        self.lengthOfLastLineStack.put(len(message))
        self.machineIsReadyForData = False

        message = message.encode()
        try:
            self.serialInstance.write(message)
            self.data.logger.writeToLog("Sent: " + str(message))
        except:
            print("write issue")
            self.data.logger.writeToLog("Send FAILED: " + str(message))

        self.lastWriteTime = time.time()

[krkeegan]

It is down in getmessage() around line 115 of SerialPortThread.py

I would add a counter to the SerialPortThread class around line 18. Call it quickQueueCounter. Increment it by one whenever you send a quick_queue command around line 127 and decrement it by one whenever an 'ok' is received around 118 if it is greater than zero. Only set the machineIsReadyForData if the quickQueueCounter cannot be decremented (because it is at zero).

[BarbourSmith]

Ok, I can do that but I don't fully understand why it is wrong now

Wouldn't sending the '~' command add an entry of length 1 (or probably length 2 with '\n') to the lengthOfLastLineStack which would then be pulled off with the next 'ok' command

or at worst the order could be swapped and so the next 'ok' command would pull off the previous line and then the 'ok' after that would pull the '~' off so it might be wrong for a second but it wouldn't lead to the accumulated error we are seeing.

Thanks for talking through this with me because now I don't understand why my fix works at all...

Why is the code I changed even being called...no '~' is sent

Gcode.cpp ~50

            }
            else if (c == '~'){
                bit_false(sys.pause, PAUSE_FLAG_USER_PAUSE);
            }
            else{

Pause is being called directly from within the G1 function around line 636

if (abs(currentZPos - zgoto) > threshold){
            Serial.print(F("Message: Please adjust Z-Axis to a depth of "));
            if (zgoto > 0){
                Serial.print(F("+"));
            }
            Serial.print(zgoto/sys.inchesToMMConversion);
            if (sys.inchesToMMConversion == INCHES){
                Serial.println(F(" in"));
            }
            else{
                Serial.println(F(" mm"));
            }
            pause(); //Wait until the z-axis is adjusted

            zAxis.set(zgoto);
        }

[davidelang]

currently, if you send 100 characters, then a ~, GC thinks that the OK goes with the 100 character line, so thinks there's more space available than there really is (because the firmware has processed the ~, but not the 100 character line)

[krkeegan]

Yeah, the lengthOfLastLineStack is a bit of an issue. That shouldn't get reset either.

Hmm, maybe we need to split _write into a regular and quick_command writes? Alternatively, we could not add the quick_commands to the buffer and then not clear them either.

Not sure what the most logical method is here.

[BarbourSmith]

@davidelang summarized the issue really well, but I would expect that issue to give us a temporary mismatch between what GC thinks and how much space is actually in the buffer, but what we see is a growing disagreement between the two which seems like a separate issue to me

[BarbourSmith]

Another clue to ponder:

When using the pause button repeatedly the issue does not happen, however when the machine pauses itself and requests a z-axis depth change the issue does. The difference being that in the first case two ~ are sent to pause and unpause the machine while in the second case only a single ~ is sent to unpause it because the Z-.1 will trigger a pause automatically

So to answer my own question:

Why is the code I changed even being called...no '~' is sent

The ~ IS being sent, but it is being sent when the 'continue' button is pressed in the z-axis popup

[BarbourSmith]

I am not sure I understand how these work:

// These are nifty functions from Grbl
#define bit_true(x,mask) (x) |= (mask)
#define bit_false(x,mask) (x) &= ~(mask)
#define bit_istrue(x,mask) ((x & mask) != 0)
#define bit_isfalse(x,mask) ((x & mask) == 0)

from nutsandbolts.h

[BarbourSmith]

The difference being that in the first case two ~ are sent to pause and unpause the machine while in the second case only a single ~ is sent to unpause it because the Z-.1 will trigger a pause automatically

This is actually wrong also. The pause button doesn't send any commands to the machine, it just stops sending new lines to be executed. The ~ command from the grbl spec should only be sent on a resume from pause which is what we are doing.

[BarbourSmith]

I am going to close this pull request in favor of a more correct fix upcoming