mstrens / grbl_controller_esp32

grbl controller for esp32
177 stars 81 forks source link

Parser issue, not catching wposOrMpos correctly, breaks handleLastNumericField() #16

Closed petervanderwalt closed 3 years ago

petervanderwalt commented 4 years ago

Referring to https://github.com/mstrens/grbl_controller_esp32/blob/bca2837d8c2ed7b5ebb0edefc5347c77f52dc9a7/com.cpp#L195-L201

Traced the issue though to the linked code above

If I just add Serial.print to debug, I can see why it fails: Just havent yet dug in deep enough into your code to fashion a fix (edit: see reply below)

Added my serial.println's:

case ':' :
      if ( getGrblPosState == GET_GRBL_STATUS_WPOS_HEADER ) { // separateur entre field name et value 
         getGrblPosState = GET_GRBL_STATUS_WPOS_DATA ; 
         wposOrMpos = strGrblBuf[0] ;       // save the first char of the string that should be MPos or WPos

         // lets see if it gets stored correctly
         Serial.println("");
         Serial.println(strGrblBuf);
         Serial.println("");

         strGrblIdx = 0 ;

         strGrblBuf[strGrblIdx] = 0 ;
         wposIdx = 0 ;
      }

And I can see

<Idle|WPos:

Idle

0.000,0.000,0.000|FS:0,0>

That at that point where you store wposOrMpos = strGrblBuf[0] ; that strGrblBuf still contains Idle/Jog/Hold/etc string, and not the expected wPos/mPos

Subsequently, handleLastNumericField() breaks the calculation as it checks:

 if ( wposOrMpos == 'W') {                  // we got a WPos
            wposXYZA[wposIdx] = temp ;
          } else {                                   // we got a MPos
            wposXYZA[wposIdx] = temp - wcoXYZA[wposIdx] ;
          }

But wposOrMpos = "I" (in my current Idle state) so we always hit the "else" calculation (:

Picked up that wpos/mpos data isnt coming though correctly if I use coordinate offsets, along with $10=0 (wpos reporting) With the default $10=1 it "works" mPos reporting gets handled by the "else" section of handleLastNumericField. But if reporting is not set to $10=1, then we still hit "else" even though it shouldnt. The section of code in handleLastNumericField clearly intends to handle both reporting settings, its just that earlier it gets stored the wrong character (:

petervanderwalt commented 4 years ago

Got it!

Right, so

https://github.com/mstrens/grbl_controller_esp32/blob/bca2837d8c2ed7b5ebb0edefc5347c77f52dc9a7/com.cpp#L269-L273

Should have also included || getGrblPosState == GET_GRBL_STATUS_WPOS_HEADER

ie change to

} else if ( ( (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') ) && ( getGrblPosState == GET_GRBL_STATUS_START || getGrblPosState == GET_GRBL_STATUS_HEADER ||
                    getGrblPosState == GET_GRBL_STATUS_CLOSED || getGrblPosState == GET_GRBL_STATUS_MESSAGE || getGrblPosState == GET_GRBL_STATUS_WPOS_HEADER ) ) { 
             strGrblBuf[strGrblIdx++] = c ;
             strGrblBuf[strGrblIdx] = 0 ;
          }

Now it append Mpos/Wpos to strGrblBuf correctly (though now it is IdleWPos so...

Next at https://github.com/mstrens/grbl_controller_esp32/blob/bca2837d8c2ed7b5ebb0edefc5347c77f52dc9a7/com.cpp#L176-L178 after copying the string to machineStatus, I cleaned up strGrblBuf and strGrblIdx :

if ( getGrblPosState == GET_GRBL_STATUS_START ) {
           getGrblPosState = GET_GRBL_STATUS_WPOS_HEADER ; 
           memccpy( machineStatus , strGrblBuf , '\0', 13);        
           strGrblIdx = 0;
           strGrblBuf[strGrblIdx] = 0 ;

And now wposOrMpos = strGrblBuf[0] and strGrblBuf contains Mpos or WPos correctly as expected

petervanderwalt commented 4 years ago

UTested good too!

I get a WPos feedback, with a WCO entry. And calculated MPOS correctly

<Idle|WPos:0.000,0.000,0.000|FS:0,0|WCO:92.188,0.000,0.000> WCO - X:92.19 Y:0.000.00 A:0.00 WPOS - X:0.00 Y:0.00 Z:0.00 A:0.00 MPOS - X:92.19 Y:0.00 Z:0.00 A:0.00

We can close the issue, but leaving it open as a reminder for you to apply fix too

mstrens commented 4 years ago

Thanks for finding and fixing the issue. I will include the correction in the next release (I am currently preparing a solution using sd files in order to let the user force a new touch screen calibration and changing Wifi parameter without having to reflash)

stratogk commented 4 years ago

Thanks for finding and fixing the issue. I will include the correction in the next release (I am currently preparing a solution using sd files in order to let the user force a new touch screen calibration and changing Wifi parameter without having to reflash)

Hello i will sugest for the wifi settings to set by yourself the ip address (Dhcp or fix ip) thank you

mstrens commented 3 years ago

Fix ip is supported in newer versions.