Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Fix M23 select long filename for print #25540

Merged
merged 7 commits into from
Mar 27, 2023
Merged

Fix M23 select long filename for print #25540

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0804dc4
Fix selecting file for print with M23 by LFN
Mar 19, 2023
1f9c4b9
Fix utf8 conversion call only when UTF_FILENAME_SUPPORT enabled
Mar 19, 2023
2ec1ccc
Fix LFN lookup - reset tmp LFN when resetting LFN name checksum
Mar 20, 2023
8dd95fa
cleanup
thinkyhead Mar 23, 2023
217039b
Merge branch 'bugfix-2.1.x' into pr/25540
thinkyhead Mar 23, 2023
90cfb2a
const
thinkyhead Mar 23, 2023
1371950
tweaks
thinkyhead Mar 26, 2023
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
77 changes: 46 additions & 31 deletions Marlin/src/sd/SdBaseFile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,14 +703,20 @@ bool SdBaseFile::open(SdBaseFile *dirFile, const uint8_t dname[11]
// Get VFat dir entry
pvFat = (vfat_t *) p;
// Get checksum from the last entry of the sequence
if (pvFat->sequenceNumber & 0x40) lfnChecksum = pvFat->checksum;
if (pvFat->sequenceNumber & 0x40) {
lfnChecksum = pvFat->checksum;
memset(lfnName, '\0', sizeof(lfnName));
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to clear the whole buffer here to ensure a null terminator later, or can we get away with just lnfName[0] = '\0'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I guess it wouldn't work: lfnName is filled from the end of the actual LFN found in VFAT sequences (which appear in reverse order; see method getLFNName), which means just using lfnName[0] = '\0' wouldn't suffice.
However, the code below

if (!strncasecmp((char*)dlname, (char*)lfnName, lfnNameLength)) lfnFileFound = true;

compares these chararrays up until lfnNameLength, which is initialized as

lfnNameLength = useLFN ? strlen((char*)dlname) : 0,

above, which means we should compare these strings up until requested part length. Which means, yes, we probably don't even need this line at all and it should work OK for comparison.

Copy link
Contributor Author

@eduard-sukharev eduard-sukharev Mar 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Although, on second thought, If we remove the memset, then we're probably prone to error cases.
Let's assume we have files:

/my_cool_model.gcode
/important.log
/important.log.gcode

then, in certain conditions, attempting to open /important.log.gcode will actually open /important.log file.

Copy link
Member

Choose a reason for hiding this comment

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

Generally, when copying strings into buffers we should append a nul after writing the other bytes. This avoids the need to clear the whole buffer before the copy. The memset might use some extra electrons, but it's minor thing in practical terms. Only the code size ultimately matters, not the performance, since this is not a bottleneck.

}
// Get LFN sequence number
lfnSequenceNumber = pvFat->sequenceNumber & 0x1F;
if WITHIN(lfnSequenceNumber, 1, reqEntriesNum) {
// Check checksum for all other entries with the starting checksum fetched before
if (lfnChecksum == pvFat->checksum) {
// Set chunk of LFN from VFAT entry into lfnName
getLFNName(pvFat, (char *)lfnName, lfnSequenceNumber);
#if ENABLED(UTF_FILENAME_SUPPORT)
convertUtf16ToUtf8((char *)lfnName);
#endif
// LFN found?
if (!strncasecmp((char*)dlname, (char*)lfnName, lfnNameLength)) lfnFileFound = true;
}
Expand Down Expand Up @@ -1506,44 +1512,53 @@ int8_t SdBaseFile::readDir(dir_t *dir, char *longFilename) {
// Post-process normal file or subdirectory longname, if any
if (DIR_IS_FILE_OR_SUBDIR(dir)) {
#if ENABLED(UTF_FILENAME_SUPPORT)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although this is the original behavior, I guess UCS-2 to UTF-8 conversion should be done always, since VFat always stores long names as UCS-2 (UTF-16) in LFN sequences, while we always handle 8bit string names.

#if LONG_FILENAME_CHARSIZE > 2
// Add warning for developers for unsupported 3-byte cases.
// (Converting 2-byte codepoints to 3-byte in-place would break the rest of filename.)
#error "Currently filename re-encoding is done in-place. It may break the remaining chars to use 3-byte codepoints."
#endif

// Is there a long filename to decode?
if (longFilename) {
// Reset n to the start of the long name
n = 0;
for (uint16_t idx = 0; idx < (LONG_FILENAME_LENGTH); idx += 2) { // idx is fixed since FAT LFN always contains UTF-16LE encoding
const uint16_t utf16_ch = longFilename[idx] | (longFilename[idx + 1] << 8);
if (0xD800 == (utf16_ch & 0xF800)) // Surrogate pair - encode as '_'
longFilename[n++] = '_';
else if (0 == (utf16_ch & 0xFF80)) // Encode as 1-byte UTF-8 char
longFilename[n++] = utf16_ch & 0x007F;
else if (0 == (utf16_ch & 0xF800)) { // Encode as 2-byte UTF-8 char
longFilename[n++] = 0xC0 | ((utf16_ch >> 6) & 0x1F);
longFilename[n++] = 0x80 | ( utf16_ch & 0x3F);
}
else {
#if LONG_FILENAME_CHARSIZE > 2 // Encode as 3-byte UTF-8 char
longFilename[n++] = 0xE0 | ((utf16_ch >> 12) & 0x0F);
longFilename[n++] = 0xC0 | ((utf16_ch >> 6) & 0x3F);
longFilename[n++] = 0xC0 | ( utf16_ch & 0x3F);
#else // Encode as '_'
longFilename[n++] = '_';
#endif
}
if (0 == utf16_ch) break; // End of filename
} // idx
} // longFilename
n = convertUtf16ToUtf8(longFilename);
}
#endif
return n;
} // DIR_IS_FILE_OR_SUBDIR
}
}

#if ENABLED(UTF_FILENAME_SUPPORT)
uint8_t SdBaseFile::convertUtf16ToUtf8(char *longFilename) {
#if LONG_FILENAME_CHARSIZE > 2
// Add warning for developers for unsupported 3-byte cases.
// (Converting 2-byte codepoints to 3-byte in-place would break the rest of filename.)
#error "Currently filename re-encoding is done in-place. It may break the remaining chars to use 3-byte codepoints."
#endif

int16_t n;
// Reset n to the start of the long name
n = 0;
for (uint16_t idx = 0; idx < (LONG_FILENAME_LENGTH); idx += 2) { // idx is fixed since FAT LFN always contains UTF-16LE encoding
const uint16_t utf16_ch = longFilename[idx] | (longFilename[idx + 1] << 8);
if (0xD800 == (utf16_ch & 0xF800)) // Surrogate pair - encode as '_'
longFilename[n++] = '_';
else if (0 == (utf16_ch & 0xFF80)) // Encode as 1-byte UTF-8 char
longFilename[n++] = utf16_ch & 0x007F;
else if (0 == (utf16_ch & 0xF800)) { // Encode as 2-byte UTF-8 char
longFilename[n++] = 0xC0 | ((utf16_ch >> 6) & 0x1F);
longFilename[n++] = 0x80 | ( utf16_ch & 0x3F);
}
else {
#if LONG_FILENAME_CHARSIZE > 2 // Encode as 3-byte UTF-8 char
longFilename[n++] = 0xE0 | ((utf16_ch >> 12) & 0x0F);
longFilename[n++] = 0xC0 | ((utf16_ch >> 6) & 0x3F);
longFilename[n++] = 0xC0 | ( utf16_ch & 0x3F);
#else // Encode as '_'
longFilename[n++] = '_';
#endif
}
if (0 == utf16_ch) break; // End of filename
} // idx

return n;
}
#endif // UTF_FILENAME_SUPPORT

// Read next directory entry into the cache
// Assumes file is correctly positioned
dir_t* SdBaseFile::readDirCache() {
Expand Down
6 changes: 5 additions & 1 deletion Marlin/src/sd/SdBaseFile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,10 @@ class SdBaseFile {
bool openCachedEntry(uint8_t cacheIndex, uint8_t oflags);
dir_t* readDirCache();

#if ENABLED(UTF_FILENAME_SUPPORT)
uint8_t convertUtf16ToUtf8(char *longFilename);
#endif // UTF_FILENAME_SUPPORT

// Long Filename create/write support
#if ENABLED(LONG_FILENAME_WRITE_SUPPORT)
static bool isDirLFN(const dir_t* dir);
Expand All @@ -403,5 +407,5 @@ class SdBaseFile {
static inline uint8_t getLFNEntriesNum(const char *lname) { return (strlen(lname) + 12) / 13; }
static void getLFNName(vfat_t *vFatDir, char *lname, uint8_t startOffset);
static void setLFNName(vfat_t *vFatDir, char *lname, uint8_t lfnSequenceNumber);
#endif
#endif // LONG_FILENAME_WRITE_SUPPORT
};