From 23c75da727c913dbf745dcb4d736c2b4c35f66ba Mon Sep 17 00:00:00 2001
From: Yuri D'Elia <wavexx@thregr.org>
Date: Sun, 18 Apr 2021 00:06:56 +0200
Subject: [PATCH] Fix Crash/PP recovery position on instructions with comments

PR #2967 altered the way ``sdpos_atomic`` was set, causing issues in the
crashdetect/powerpanic recovery offset if the instruction being
recovered happens to contain a comment.

Previously ``sdpos`` was assumed to be a single byte prior to the last
read character. sdpos+1 would thus position the index to the next
instruction. With gcode-filtering in place, sdpos is left just before
the comment, while the actual read position is at the newline. This
causes to parser to resume in the middle of the comment.

Change the value returned by cardreader::get_sdpos() to always return
the last read position, as everybody expects (!!).

This avoids the +1, and correctly sets the resume position to the next
valid instruction without overhead.
---
 Firmware/cardreader.h | 9 ++++++---
 Firmware/cmdqueue.cpp | 4 ++--
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/Firmware/cardreader.h b/Firmware/cardreader.h
index 715d82fa..47343e23 100644
--- a/Firmware/cardreader.h
+++ b/Firmware/cardreader.h
@@ -59,9 +59,12 @@ public:
 
   FORCE_INLINE bool isFileOpen() { return file.isOpen(); }
   bool eof() { return sdpos>=filesize; }
-  // There may be a potential performance problem - when the comment reading fails, sdpos points to the last correctly read character.
-  // However, repeated reading (e.g. after power panic) the comment will be read again - it should survive correctly, it will just take a few moments to skip
-  FORCE_INLINE int16_t getFilteredGcodeChar() {  sdpos = file.curPosition();return (int16_t)file.readFilteredGcode();};
+  FORCE_INLINE int16_t getFilteredGcodeChar()
+  {
+      int16_t c = (int16_t)file.readFilteredGcode();
+      sdpos = file.curPosition();
+      return c;
+  };
   void setIndex(long index) {sdpos = index;file.seekSetFilteredGcode(index);};
   FORCE_INLINE uint8_t percentDone(){if(!isFileOpen()) return 0; if(filesize) return sdpos/((filesize+99)/100); else return 0;};
   FORCE_INLINE char* getWorkDirName(){workDir.getFilename(filename);return filename;};
diff --git a/Firmware/cmdqueue.cpp b/Firmware/cmdqueue.cpp
index a400a4e1..3bde3336 100755
--- a/Firmware/cmdqueue.cpp
+++ b/Firmware/cmdqueue.cpp
@@ -600,7 +600,7 @@ void get_command()
       }
       // The new command buffer could be updated non-atomically, because it is not yet considered
       // to be inside the active queue.
-      sd_count.value = (card.get_sdpos()+1) - sdpos_atomic;
+      sd_count.value = card.get_sdpos() - sdpos_atomic;
       cmdbuffer[bufindw] = CMDBUFFER_CURRENT_TYPE_SDCARD;
       cmdbuffer[bufindw+1] = sd_count.lohi.lo;
       cmdbuffer[bufindw+2] = sd_count.lohi.hi;
@@ -625,7 +625,7 @@ void get_command()
       // or a 115200 Bd serial line receive interrupt, which will not trigger faster than 12kHz.
       ++ buflen;
       bufindw += len;
-      sdpos_atomic = card.get_sdpos()+1;
+      sdpos_atomic = card.get_sdpos();
       if (bufindw == sizeof(cmdbuffer))
           bufindw = 0;
       sei();