knutwurst / Marlin-2-0-x-Anycubic-i3-MEGA-S

Marlin 2.0.x Version for Anycubic i3 MEGA M/S/P/X/CHIRON and 4MAX with Anycubic TFT or the "new" DGUS Clone TFT - Now also with BLTouch!
GNU General Public License v3.0
782 stars 183 forks source link

[BUG] WIFI ESP3D file sd file upload MIssing #393

Closed wes1993 closed 1 year ago

wes1993 commented 1 year ago

Upload file from web (ESP3D) Expected behavior: file uploaded on the SD

Actual behavior: it's not possible to load files with extension .gco if I use .gco it's not visible on display of the printer

Same as https://github.com/knutwurst/Marlin-2-0-x-Anycubic-i3-MEGA-S/issues/330 but also with 1.5 beta firmware the problem persist...

Can someone tell me how can i download and use the Marlin bug fix updates

P.S. I have this display: Anycubic 1.0 display

Bye Stefano

wes1993 commented 1 year ago

I have added this fix in the configuration.h but i have this error: https://github.com/MarlinFirmware/Marlin/pull/24752

error "The ANYCUBIC LCD requires LCD_SERIAL_PORT to be defined."

stklcode commented 1 year ago

The referenced pull request has nothing to do with the Knutwurst firmware at all. That's kind of a backport Marlin's default LCD implementation. And it only enables display of special menu entries and does not affect file loading.

The LCD firmware also filters entries by extension. Unfortunately this filter cannot be modified by the Marlin firmware. By default ".gcode" and on Mega P display also ".bmp" are displayed. ".GCO" is just the shorthand for the legacy 8.3 names, so "longfilename.gcode" is represented as "LONGFI~1.GCO". Some displays do not filter, but at least the DGUS clone on the Mega P does and it does not show ".gco".

wes1993 commented 1 year ago

@stklcode, Thanks a lot for your reply. I know that the pull request is from another project but it's Marlin so i think share some stuff.

From what i remember with old firmware 1.3.1 i can see and print .gco with this new fimrware the printer won't see .gco anymore.

stklcode commented 1 year ago

No, the code is 100% irrelevant for this project. The Knutwurwt Firmware has its own Anycubic LCD implementation. The Marlin PR (which was made by myself, so let’s assume I know what it does) ports the Knutwurst behavior back to the Marlin core, so the Marlin implementation supports the Mega P display. And, as already said, the change does not affect the files that are displayed. It’s only about the special menu pseudo-elements.

Which printer or which display are you using? I cannot reproduce it myself, as my display has a built-in filter and never supported .gco files.

Is there any reason why you want to use .gco files and not .gcode ? Pretty uncommon I’d say…

stklcode commented 1 year ago

Only filter for SD card file display is here (this is unmodified Marlin code): https://github.com/knutwurst/Marlin-2-0-x-Anycubic-i3-MEGA-S/blob/ac7c27240382b9b3912cebc31fe0031bfa000a06/Marlin/src/sd/cardreader.cpp#L224-L229

This has slightly changed since 1.3.1, but the relevant part is still the same: https://github.com/knutwurst/Marlin-2-0-x-Anycubic-i3-MEGA-S/blob/21d062c285e37d0397c3926a36fadea8f04f8615/Marlin/src/sd/cardreader.cpp#L173-L177

So from that I’d say the .gco extension should be fine from the Marlin side…

Additional question: Are the files actually copied to the SD card and just not displayed on the printer or are they not copied at all?

wes1993 commented 1 year ago

Thanks a lot again for your reply!! sorry for my delay… very very busy days at work… :(

The files are copied to SD card from Wi-Fi by the ESP01-S but for this I must use the 8.3 format…

I have also copied file from my PC to my SD card directly from PC in 8.3 format but unfortunately the same problem happen… The display of my printer can’t see the files in .gco format… :(

leonel85 commented 1 year ago

The problem is that .gco file il filtered by the display. If you have a long file name you should see it because the extension is override by:

       if (fileNameWasCut) {
              outputString[fileNameLen - 7] = '~';
              outputString[fileNameLen - 6] = '.';
              outputString[fileNameLen - 5] = 'g';
              outputString[fileNameLen - 4] = 'c';
              outputString[fileNameLen - 3] = 'o';
              outputString[fileNameLen - 2] = 'd';
              outputString[fileNameLen - 1] = 'e';
            }

So I fix it doing more ore less the same thing `

if ENABLED(KNUTWURST_DGUS2_TFT)

          if (fileNameLen >= MAX_PRINTABLE_FILENAME_LEN) {
            fileNameWasCut = true;
            fileNameLen    = MAX_PRINTABLE_FILENAME_LEN;
          }

          //LEO ADD
          //could be a GCO file and never be long file 
          if (fileNameLen == 0)
          {
            isGCOFile = (strcasecmp_P(strrchr(card.filename, '.'), PSTR(".GCO")) == 0) ;
             fileNameLen = strlen(card.filename) + 2;
          }
          //END LEO

        #endif

        char outputString[fileNameLen];

        // Bugfix for non-printable special characters
        // which are now replaced by underscores.
        if(isGCOFile)
        {
          for (unsigned char i = 0; i <= fileNameLen; i++) {
            outputString[i] = card.filename[i];
            if (!isPrintable(outputString[i]))
              outputString[i] = '_';             
          }
        }
        else
        {
          for (unsigned char i = 0; i <= fileNameLen; i++) {
            if (i >= fileNameLen) {
              outputString[i] = ' ';
            }
            else {
              outputString[i] = card.longFilename[i];
              if (!isPrintable(outputString[i]))
                outputString[i] = '_';
            }
          }
        }

        // I know, it's ugly, but it's faster than a string lib
        if (fileNameWasCut  || isGCOFile) {
          outputString[fileNameLen - 7] = '~';
          outputString[fileNameLen - 6] = '.';
          outputString[fileNameLen - 5] = 'g';
          outputString[fileNameLen - 4] = 'c';
          outputString[fileNameLen - 3] = 'o';
          outputString[fileNameLen - 2] = 'd';
          outputString[fileNameLen - 1] = 'e';
        }

        outputString[fileNameLen] = '\0';

`

stklcode commented 1 year ago

Wouldn’t it be simpler to copy the filename to a local variable first place, append “de“ if it ends with “.gco“ and keep the sanitization routine in place as it is? (yes, that comes with some memory overhead)

The solution is not perfect though, think of two files named “test.gco“ and “test.gcode“. But maybe still better than not displaying the files. Same can be applied to <longname>123~.gcode and <longname>1234.gcode yet, because of the truncation and apparently we can live with that fuzziness.

I personally don’t get the problem at all, is there any reason beside “i want to“ to not just use the supported .gcode extension? Any software that can only generate .gco or something?


Edit: I see the issue with filename length 0.

leonel85 commented 1 year ago

Wouldn’t it be simpler to copy the filename to a local variable first place, append “de“ if it ends with “.gco“ and keep the sanitization routine in place as it is? (yes, that comes with some memory overhead)

Yes I can do and instead of append "de" its best to ovveride alse GCO just to avoid to have something like: "filename.GCOde"

The solution is not perfect though, think of two files named “test.gco“ and “test.gcode“. But maybe still better than not displaying the files. Same can be applied to 123~.gcode and 1234.gcode yet, because of the truncation and apparently we can live with that fuzziness. This is why I prefer to trunc one letter and add ~ just to show that theris some manipolation on the file name

I personally don’t get the problem at all, is there any reason beside “i want to“ to not just use the supported .gcode extension? Any software that can only generate .gco or something?

yes, if you use ESP3D the file must be 8.3 format (i don't know if it's the same on last version)

It's possible to write better. I just test if it works because there was also an error in the original code:

       for (unsigned char i = 0; i <= fileNameLen; i++) {
            if (i >= fileNameLen) {
              outputString[i] = ' ';
            }
            else {
              outputString[i] = card.longFilename[i];
              if (!isPrintable(outputString[i]))
                outputString[i] = '_';
            }
          }
 if (i >= fileNameLen) {
              outputString[i] = ' ';
            }

this will never happen because for (unsigned char i = 0; i <= fileNameLen; i++)

Anyway it's possible to write it better and I think its better to see the files with manipolated name instead of wite line on the display.

stklcode commented 1 year ago

if you use ESP3D the file must be 8.3 format (i don't know if it's the same on last version)

OK, wasn't aware of that restriction, never used that combination.

Could you check whether the issue is still present with the latest master (since #398 has been merged now)? My Mega P does show a "beep.gco" file again, but I cannot test on any other printer (and don't know which pinter(s) you are actually using)

leonel85 commented 1 year ago

Sorry I take a look on: So it's possible to see .GCO file on the screen I will try to just use the correct fileName. If card.longFilename size == 0 just use card.filename. It will not be necessary to manipolate it becasuse if it's long it works also with .GCO

I will try it later

stklcode commented 1 year ago

I will try to just use the correct fileName. If card.longFilename size == 0 just use card.filename.

The same idea is used in card.longest_filename(), likely for the same reason (first use was some Ultra LCD implementation): https://github.com/knutwurst/Marlin-2-0-x-Anycubic-i3-MEGA-S/blob/8877ad534dc2f601fc20efca77d4febfdd61a5a9/Marlin/src/sd/cardreader.h#L146

So we now use this display filename to start off, too.

I will try it later

Thanks.

leonel85 commented 1 year ago

I can not test now but I think theris something to fix

        char* fileName  = card.longest_filename();
            #if ENABLED(KNUTWURST_DGUS2_TFT)
              if (fileNameLen >= MAX_PRINTABLE_FILENAME_LEN) {
                fileNameWasCut = true;
                fileNameLen    = MAX_PRINTABLE_FILENAME_LEN;
              }
            #endif
   in this case you must resize fileName  and you don't need fileNameWasCut  anymore because you can do this inside:
           outputString[fileNameLen-7] = '~';
            outputString[fileNameLen-6] = '.';
            outputString[fileNameLen-5] = 'g';
            outputString[fileNameLen-4] = 'c';
            outputString[fileNameLen-3] = 'o';
            outputString[fileNameLen-2] = 'd';
            outputString[fileNameLen-1] = 'e';

And last remove this code :

  if (i >= fileNameLen) {
                outputString[i] = ' ';
              }
      I think that's all and it should work
stklcode commented 1 year ago

No idea what you're talking about, sorry....

leonel85 commented 1 year ago

In the last commit I see this: ` // THe longname may not be filed, so we use the built-in fallback here. char* fileName = card.longest_filename(); int fileNameLen = strlen(fileName); bool fileNameWasCut = false;

        // Cut off too long filenames.
        // They don't fit on the screen anyway.
        #if ENABLED(KNUTWURST_DGUS2_TFT)
          if (fileNameLen >= MAX_PRINTABLE_FILENAME_LEN) {
            fileNameWasCut = true;
            fileNameLen    = MAX_PRINTABLE_FILENAME_LEN;
          }
        #endif

        char outputString[fileNameLen];

        // Bugfix for non-printable special characters
        // which are now replaced by underscores.
        for (unsigned char i = 0; i <= fileNameLen; i++) {
          if (i >= fileNameLen) {
            outputString[i] = ' ';
          }
          else {
            outputString[i] = fileName[i];
            if (!isPrintable(outputString[i]))
              outputString[i] = '_';
          }
        }

        // I know, it's ugly, but it's faster than a string lib
        if (fileNameWasCut) {
          outputString[fileNameLen - 7] = '~';
          outputString[fileNameLen - 6] = '.';
          outputString[fileNameLen - 5] = 'g';
          outputString[fileNameLen - 4] = 'c';
          outputString[fileNameLen - 3] = 'o';
          outputString[fileNameLen - 2] = 'd';
          outputString[fileNameLen - 1] = 'e';
        }

        outputString[fileNameLen] = '\0';`

at line 1494 you have this:

char* fileName = card.longest_filename();

Then at line 1203 you change the fileNameLen

fileNameLen = MAX_PRINTABLE_FILENAME_LEN;

so this

for (unsigned char i = 0; i <= fileNameLen; i++) { if (i >= fileNameLen) { outputString[i] = ' '; } else { outputString[i] = fileName[i]; if (!isPrintable(outputString[i])) outputString[i] = '_'; } }

should be

for (unsigned char i = 0; i <= ORIGINAL_FILE_SIZE; i++) {

ORIGINAL_FILE_SIZE is > fileNameLen

Otherwise i will never be >= fileNameLen because the loop is until <=fileNameLen if (i >= fileNameLen) { outputString[i] = ' '; }

I will write the code that is best

leonel85 commented 1 year ago

See the code. I hope it's clear if i'm not wrong

` // The longname may not be filed, so we use the built-in fallback here. char* fileName = card.longest_filename(); int fileNameLen = strlen(fileName); bool fileNameWasCut = false;

    // Cut off too long filenames.
    // They don't fit on the screen anyway.
    #if ENABLED(KNUTWURST_DGUS2_TFT)
      if (fileNameLen >= MAX_PRINTABLE_FILENAME_LEN) {
        fileNameLen    = MAX_PRINTABLE_FILENAME_LEN;
        fileNameWasCut = true;
      }
    #endif

    char outputString[fileNameLen];

    // Bugfix for non-printable special characters
    // which are now replaced by underscores.
    //strlen of original size strlen(card.longest_filename()
    for (unsigned char i = 0; i <= strlen(card.longest_filename()); i++) {
      if (i >= fileNameLen) {
        outputString[i] = ' ';
      }
      else {
        outputString[i] = fileName[i];
        if (!isPrintable(outputString[i]))
          outputString[i] = '_';
      }
    }

    // I know, it's ugly, but it's faster than a string lib
    if (fileNameWasCut) {
      outputString[fileNameLen - 7] = '~';
      outputString[fileNameLen - 6] = '.';
      outputString[fileNameLen - 5] = 'g';
      outputString[fileNameLen - 4] = 'c';
      outputString[fileNameLen - 3] = 'o';
      outputString[fileNameLen - 2] = 'd';
      outputString[fileNameLen - 1] = 'e';
    }

    outputString[fileNameLen] = '\0';

`

stklcode commented 1 year ago

Please stop posting full quotes of the code. It's really hard to get the single change from these answers.

That can't be 100% correct either.

We initialize:

 char outputString[fileNameLen];

Using the proposed loop:

    for (unsigned char i = 0; i <= strlen(card.longest_filename()); i++) {
      if (i >= fileNameLen) {
        outputString[i] = ' ';

We might overfill the character array.

Imagine fileName = "1234567890". So we have fileNameLength = 10. We not truncate the name, so we have fileNameLength = 5 and initialize the output as an array of 5 characters: outputString[5]

The loop then iterates over the array and fills


PS: I built a small demo here: https://www.sololearn.com/compiler-playground/c5a0mbpxc6xV

With NULL-Termination the output is exactly the same, because we terminate the string after fileNameLen anyway, so there is no need to fill anything up with spaces here. However, we must not write data out of the array bounds, to not corrupt whatever is stored in the memory beyond the array memory.

leonel85 commented 1 year ago

Ok That's true but i will never be >= fileNameLen The example works perfect but there's unecessary code

char outputString[10];
for (unsigned char i = 0; i <= fileNameLen; i++) {
    outputString[i] = fileName[i];
}
stklcode commented 1 year ago

Agree. The minimal loop without unnecessary if-clauses should be correct and sufficient here.

Also i <= fileNameLen should be i < fileNameLen, because we set outputString[fileNameLen] = '\0'; after the loop, no need to copy the original NULL-Byte.

leonel85 commented 1 year ago

That is what I meaning from the begining. Sorry if was not clear

leonel85 commented 1 year ago

I change the for cycle and test it on anycubic mega x. It works

for (unsigned char i = 0; i <= fileNameLen; i++) {
    outputString[i] = fileName[i];
}
wes1993 commented 1 year ago

Hello, thanks a lot to everyone for the help, unfortunately i'm really busy at work this days, as soon as i can i will test the firmware in my printer :-D

Best Regards Stefano

wes1993 commented 1 year ago

Hello to everyone!!! I have updated the fimrware and work perfectly again now!!!

Thanks a lot!!! Stefano

github-actions[bot] commented 1 year ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.