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.

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

Feature Request: add file size to M20 output #4733

Closed
Bob-the-Kuhn opened this Issue Aug 30, 2016 · 31 comments

Comments

Projects
None yet
5 participants
@Bob-the-Kuhn
Copy link
Contributor

Bob-the-Kuhn commented Aug 30, 2016

I've been having an "interesting" time uploading files to the SD card via Repetier Host. It would have been helpful to be able to see the file size.

Maybe add an option to the M20 command or make this the default option or ...

The file size is shown in the log when a "print from SD card" is started so the capability is already present.

And yes, I've RTFM.

Bob

PS - if you're rally ambitious .. I see LS flags for modify date and recursive list of subdirectories (really don't know what the last one means) ...

@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Aug 30, 2016

We have to check with host developers, who depend on us not altering the format of the M20 output, which could break their handling of SD. Perhaps we can add a flag for more detailed output. Already there is an option to get long filenames separately. So this might be a good enhancement to include both long names and file size in the M20 output, as options.

@Bob-the-Kuhn

This comment has been minimized.

Copy link
Contributor

Bob-the-Kuhn commented Aug 30, 2016

I'd be surprised if the hosts agreed on making the file size the new default to M20.

It's not worth the effort if it starts getting complicated or a lot of work.

Maybe add this to the M33 command?

@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Aug 31, 2016

the new default

I would never suggest that. It would have to be an explicit option.

M33 could be extended. No one has yet taken advantage of M33 to present a nice pretty SD file selector (that lazy @MarlinFirmware/host-software-team! 😜 ) so making it into "extended file info" probably won't disrupt anything.

@foosel

This comment has been minimized.

Copy link
Contributor

foosel commented Aug 31, 2016

No one has yet taken advantage of M33 to present a nice pretty SD file selector

I actually looked into it a couple months back and it simply is unfeasible to iterate over possibly dozens of file entries from M20, sending an M33 for each, trying to associate the output to the input through some magic (no command tracking thanks to most firmwares out there being buggy regarding acknowledgement order, and I have to work with all of them, not just the fixed versions, so I can't easily solve this) and basically stalling all other communication in the process, and that each time an M20 gets issued.

OctoPrint does already support the file size being included in the M20 output (for at least a year actually, line is split on whitespace, if there are two entries first one is considered the name, second the size in bytes, a couple of firmwares report it that way), supporting the long file name in there too would not be a problem as long as it's easily parseable and it's detectable from a simple split if a long name and/or a size are there. You would want to make that configurable though and default to off.

That's off topic for this thread somewhat but did we ever consider something like switching of functionality by the host? Eg on connect I send something like M<xyz> M20_INCL_SIZE,M20_INCL_LONG and that tells Marlin to switch on that kind of output.

@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Aug 31, 2016

file size being included in the M20 output

Nice!

@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Aug 31, 2016

…dozens of file entries from M20, sending an M33 for each…

Very true, and understood from the start. My thought with M33 was along the lines of only fetching the longname when one wanted to display it within the visible portion of a file selector. You'd start by displaying the UGLYNA~1.GCO first, then in a background thread fetch the long names that you need, replacing the short names as they arrived. But M33 doesn't provide a hook to associate the fetch with the longname, so that will need to be fixed up.

detectable from a simple split if a long name and/or a size are there.

It could work that way. You currently still need the short name for the sd file commands, but it's certainly feasible to follow a long-name version of a path if that were given.

What character would be most ideal to split on? Most characters are legal in filenames these days (even including things like tabs) but I'm sure there must be some printable characters left to choose from.

@foosel

This comment has been minimized.

Copy link
Contributor

foosel commented Aug 31, 2016

You'd start by displaying the UGLYNA~1.GCO first, then in a background thread fetch the long names that you need, replacing the short names as they arrived

"There's a bug, when I select a file from the SD to print it shows its long name, but not in the file list, there it's just squiggly-line-something! Plz fix kthxbye!"

;) Thought about that option too, but half measures have a tendency to produce a lot of noise.

What character would be most ideal to split on?

At this point you don't have much choice but using whitespace, considering that otherwise you'd become incompatible with existing implementations that already output the filesize alongside the name - and THAT would indeed produce a lot of work in the hosts.

What would be possible (and I'm sure of that because I just did a quick test implementation):

Begin file list
UGLYNA~1.GCO 2342 Uglyname with some_longer-name.gcode
End file list

Begin file list
UGLYNA~1.GCO Uglyname with some_longer-name.gcode 2342
End file list

Begin file list
UGLYNA~1.GCO 2342
End file list

The last one is what current OctoPrint already supports and tests for.

Also possible but meh is that variant:

Begin file list
UGLYNA~1.GCO Uglyname with some_longer-name.gcode
End file list

If the long name starts with a segment parseable as an integer, I have no way to distinguish it from the first variant.

My personal favorite would be <name> <size> <longname> since it basically allows me to split on whitespace twice and use the rest (if existing) as long name, and it can be distinguished from <name> <size> and <name> formats quite easily. Makes it necessary to always include the size though so that I know that I have all three pieces of info if I'm seeing more than 2 split parts.

<name> <longname> <size> would also be possible, but necessitate two splits, one left and one right (or one full split, but that would leave in the rather unfortunate situation of having the long name in parts with unknown whitespace in between which I'd then have to parse from the full line again - meh).

Also currently OctoPrint does a right-split on the line and only uses the right most entry as possible file size - I'm currently assuming that to be actually unnecessary here and that a simple limited left split would be enough since the "regular" short names can't contain whitespace (or they'll get the ~ treatment) after all, but if that assumption is wrong please say so.

Now, while we are talking about this anyhow and the OP mentioned it: the modification date is indeed available? In what format, timestamp?

@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Sep 2, 2016

How about this…? If the longname contains spaces, they will be preceded by \ and if the longname contains any \ then they will be output as \\. So, your basic escaping.

UGLYNA~1.GCO 2342 Uglyname\ with\ some_longer-name.gcode
= "Uglyname with some_longer-name.gcode"

12_THI~1.GCO 3453 12\ things\\thangs\\thongs.gcode
= "12 things\thangs\thongs.gcode"

So if a number after the short-name doesn't end the line, precede a space, or precede a \, then it's part of the long name, and no size was given. (But of course, a size can always be given with this enhancement.)

Or, probably easier, the size can just always come last. And we'll only be listing files ending in .gco or .gcode anyways, so the end of the longname will be a reliable hint.

UGLYNA~1.GCO Uglyname with some_longer-name.gcode 2342
12_THI~1.GCO 12 things\thangs\thongs.gcode 3453

Regex: ^([^ ]+)( (.*\.g(co(de))))?( +(\d+))?$    ; Grab $1, $3, and $7

Without regex, you can left-split for the short name, right-split for the size, and if the left and right splits aren't at the same point, whatever is in between is the long name. Confirm by looking for the file extension.

I'll have to see how the modification date is provided. We'll probably have to output it just as it is (probably a timestamp) because Marlin won't have the local timezone, and parsing and formatting dates in general is a nightmare unless you have access to a helpful library to do it for you.

@Bob-the-Kuhn

This comment has been minimized.

Copy link
Contributor

Bob-the-Kuhn commented Sep 2, 2016

Marlin has conversion routines for the dates.

01:12:40.443 : Size: 35074 Creation Date: 2000-1-1 Last Modify Date: 2000-1-1

The below code results in the following (using Repeteir Host).

 filesize = p.fileSize;  
 SERIAL_PROTOCOLPGM("                          "); 
 SERIAL_PROTOCOLPGM(MSG_SD_SIZE); 
 SERIAL_PROTOCOL(filesize); 

  uint16_t create_date = p.creationDate;  
  SERIAL_PROTOCOLPGM("                         Creation Date:   ");  
  SERIAL_PROTOCOL(FAT_YEAR(create_date));
  SERIAL_PROTOCOLPGM("-");
  SERIAL_PROTOCOL_F(FAT_MONTH(create_date), DEC);
  SERIAL_PROTOCOLPGM("-");
  SERIAL_PROTOCOL_F(FAT_DAY(create_date), DEC);

  uint16_t last_modify_date = p.lastWriteDate;  
  SERIAL_PROTOCOLPGM("         Last Modify Date: ");  
  SERIAL_PROTOCOL(FAT_YEAR(last_modify_date));
  SERIAL_PROTOCOLPGM("-");
  SERIAL_PROTOCOL_F(FAT_MONTH(last_modify_date), DEC);
  SERIAL_PROTOCOLPGM("-");
  SERIAL_PROTOCOL_F(FAT_DAY(last_modify_date), DEC);
@foosel

This comment has been minimized.

Copy link
Contributor

foosel commented Sep 2, 2016

How about this…? If the longname contains spaces, they will be preceded by \ and if the longname
contains any \ then they will be output as \\.

That would actually necessitate to tokenize each line instead of just splitting it once (or twice, if the size is expected at the end) on whitespace - actually more work and processing overhead.

If I'm understanding things correctly, a long name does not have to exist, right? And the 8.3 entries might also end on ".g". So a more complete example would probably be:

Begin file list
UGLYNA~1.GCO 2342 Uglyname with some_longer-name.gcode
12_THI~1.GCO 3453 12 things\thangs\thongs.gcode
AUTO.G 1234 AUTO.G
TEST.GCO 5678 TEST.GO
End file list

Regarding timestamps, if it's decided to include that too (third column, making it <name> <size> <datetime> <longname>, still allowing one whitespace split with limit 3 for trivial parsing) I'd suggest ISO8601 either without timezone (e.g. "2016-09-02T15:50:00") or just assuming UTC (e.g. "2016-09-02T15:50:00Z"):

Begin file list
UGLYNA~1.GCO 2342 2014-02-12T12:00:00Z Uglyname with some_longer-name.gcode
12_THI~1.GCO 3453 2015-11-08T13:00:00Z 12 things\thangs\thongs.gcode
AUTO.G 1234 2016-09-02T14:00:00Z AUTO.G
TEST.GCO 5678 2016-09-01T15:00:00Z TEST.GO
End file list

That would allow something like this, if the long name never goes without timestamp and size:

def parse_sd_line(line):
    parts = line.split(None, limit=3)
    if len(parts) >= 4:
        # <name> <size> <datetime> <longname>
        return parts[0], int(parts[1]), to_timestamp(parts[2]), parts[3]
    elif len(parts) == 2:
        # <name> <size>
        return parts[0], int(parts[1])
    elif len(parts) == 1:
        # <name>
        return parts[0]
    else:
        raise ValueError("Unknown format")

Any particular reasons you are in favor of putting the size at the end @thinkyhead? Asking because considering that there ARE already implementations out there that put the size after the name, I find it more intuitive to just append the long name to that format, and it makes it easier to do a "one size fits all" implementation on the host side too instead of drowning in "if" statements trying to accommodate the potential firmware variants out there.

Oh, and something else I remembered: what encoding do the filenames have? Because the serial interface here is supposed to be ASCII only...

@Bob-the-Kuhn

This comment has been minimized.

Copy link
Contributor

Bob-the-Kuhn commented Sep 2, 2016

I doubt that including the create/modify dates in this request is worth the effort.

Every time the board is reset (eg when connecting to it) the date goes back to the default of 2000-1-1. Currently there's no method to specify/set the date. Maybe that could be added as an optional parameter to the M28 (Begin write to SD card) command.

Another fine example of galloping features.

@foosel

This comment has been minimized.

Copy link
Contributor

foosel commented Sep 2, 2016

Every time the board is reset (eg when connecting to it) the date goes back to the default of 2000-1-1

Ah, it thought it was a set date inherent to the FAT entry. If the timestamp is useless anyhow, perfect:

Begin file list
UGLYNA~1.GCO 2342 Uglyname with some_longer-name.gcode
12_THI~1.GCO 3453 12 things\thangs\thongs.gcode
AUTO.G 1234 AUTO.G
TEST.GCO 5678 TEST.GO
End file list

Another fine example of galloping features.

It doesn't make much sense starting a discussion about changing the current format of a file entry record in an M20 for one field (long name), for which hosts will have to make adjustments, and then starting the same discussion two weeks later again for another field (timestamp) with another round of adjustments as the consequence. Talking about something != implementing it.

@repetier

This comment has been minimized.

Copy link

repetier commented Sep 2, 2016

Please also consider backward compatibility. Old hosts will not recognize this and do stupid things. On Repetier-Firmware we even had a option to disable file length for hosts not recognizing this.
The format comes mainly from the sd library we all use, which is why all firmwares had similar results.

So if you start changing such a important feature, give hosts a way to detect that more info could be available and make it an option to M20 so non supporting hosts still get correct answers by default. Backward compatibility is essential here since hosts also have to support older versions not having any new formats.

Regarding date, it makes no real sense as @Bob-the-Kuhn mentioned, also uploaded directly on a pc date would be correct, but firmware side uploads never.

@Bob-the-Kuhn

This comment has been minimized.

Copy link
Contributor

Bob-the-Kuhn commented Sep 2, 2016

As mentioned earlier, there’s no way that the default M20 behavior would be modified.

Your suggestions on implementing a size report would be appreciated.

I’ve cobbled together a firmware that adds an optional L parameter to the M20 command. If I output the size properly Repetier Host will grab it and put it into the SD card directory display.

FYI – another reason to NOT set the M20 default to listing the file size – the start print from SD file command also uses the same subroutines. If it sees anything beyond the file name it will error out.

@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Sep 3, 2016

For backward compatibility, M20 L1 might get a listing including long filename and size, while M20 alone would do the usual thing.

@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Sep 3, 2016

Every time the board is reset (eg when connecting to it) the date goes back to the default of 2000-1-1.

The timestamp would be for the benefit of the OS or host that can deal with it. Marlin would simply pass it through. Octoprint runs on a full-featured Linux computer, so it can make use of libraries to display the date in the local timezone. I'll produce a basic implementation soon and we can mess with it.

Any particular reasons you are in favor of putting the size at the end @thinkyhead?

You mentioned using right-split and left-split. I'm a big fan of regular expressions, personally, but if splits are your preferred method, I sought to explain how it could be done without regex in the differing cases.

a long name does not have to exist, right?

If a longname were not provided, the consistent solution would be to repeat the short-name.

I'm not trying to get every detail decided now. We're just throwing ideas around.

I find it more intuitive to just append the long name to that format

I'm amenable to whatever all you hosties prefer.

@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Sep 3, 2016

Oh, and something else I remembered: what encoding do the filenames have? Because the serial interface here is supposed to be ASCII only...

It's FAT32 which supports little-endian Unicode, so this is worth a read: http://home.teleport.com/~brainy/lfn.htm

Quoting from that page:

2-byte long characters (as opposed to ASCII characters that are 1-byte long each) that support not only traditional latin alphabet characters and arabic numbers but… also the languages of the rest of the world…. Note that, in the 2-byte unicode character, the first byte is always the character and the second byte is always the blank … For our purposes … just treat every other word as an ASCII character as long as the following byte is a blank character.

@foosel

This comment has been minimized.

Copy link
Contributor

foosel commented Sep 5, 2016

It's FAT32 which supports little-endian Unicode

In that case, forget about sending the long names over the serial line all together, at least as long as that remains defined as ASCII only (and I don't see that changing in the near future due to all the compatibility issues that would induce in hosts and potentially firmwares alike).

Size format is already defined by other implementations, I suggest just sticking to it then:

Begin file list
UGLYNA~1.GCO 2342
12_THI~1.GCO 3453
AUTO.G 1234
TEST.GCO 5678
End file list

If you depend on an additional parameter to M20 to display sizes, that would necessitate an adjustment at least in OctoPrint in order to be able to set that (I can't do this always, since I don't know what other firmware implementations might do with that). I don't know about other hosts here. Not a problem (I'd just need to find the time to add a corresponding flag), I just wanted to point it out so that we are on the same page here.

@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Sep 5, 2016

Ok, so it sounds like everyone would probably be cool with appending file size to M20 output without any additional parameter, since that seems to be the case elsewhere. And I don't mind adding a new parameter to M20 so that Unicode-savvy hosts may request long filenames. If needed, I can produce some reference code that hosts may adopt. For convenience I can do it as a PR specifically for OctoPrint. I guess Repetier-Host is now closed-source, but I am working for someone who has licensed it, and they will probably want this feature anyway.

@Bob-the-Kuhn

This comment has been minimized.

Copy link
Contributor

Bob-the-Kuhn commented Sep 5, 2016

Just in case it’s of some use to you …

Attached is my code that outputs the size if you send it “M20 L1”. Search for “bob” to find my mods.

All I did was create copies of the subroutines involved with M20 and invoke them when the L parameter is seen.

@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Apr 12, 2017

it sounds like everyone would probably be cool with appending file size to M20 output without any additional parameter, since that seems to be the case elsewhere

Let's do this one. @Bob-the-Kuhn do you have a quick-link to your M20 L code?

@Bob-the-Kuhn

This comment has been minimized.

Copy link
Contributor

Bob-the-Kuhn commented Apr 12, 2017

https://www.dropbox.com/s/g3zalu79i07fjbp/M20_L.zip?dl=0

It's pretty ugly. Do you want me to copy RCBugFix & try to merge it in?

@Bob-the-Kuhn

This comment has been minimized.

Copy link
Contributor

Bob-the-Kuhn commented Apr 12, 2017

See PR #6320 for example code.

@foosel

This comment has been minimized.

Copy link
Contributor

foosel commented Apr 12, 2017

Just a head-up from a host perspective. If I read this PR correctly, in the case of M20 L1 or M20 L3 it does output the file list like this:

Begin file list
foo.gco Size: 1234
End file list

This does not conform to how other firmwares/variants do it. That would be:

Begin file list
foo.gco 1234
End file list

as mentioned above. Please do not introduce yet another format.

@Bob-the-Kuhn

This comment has been minimized.

Copy link
Contributor

Bob-the-Kuhn commented Apr 12, 2017

Thanks for letting me know what is expected.

Is it M20 L or M20 L1?

I expect the L2 & L3 options will go away. They're left overs from 6 months ago when I first played around with this. Since there's no real time clock then the dates on files created by Marlin always have the same date.

@foosel

This comment has been minimized.

Copy link
Contributor

foosel commented Apr 12, 2017

Sorry, i don't understand the question. An L parameter is not specified in any other firmware at least I'm aware of, AFAIK that was an invention made here.

Other firmwares respond to an M20 either with just the (8.3) name or with the (8.3) name and the size in bytes, separated by a space. No prefixes of any kind.

@Bob-the-Kuhn

This comment has been minimized.

Copy link
Contributor

Bob-the-Kuhn commented Apr 12, 2017

That makes it easy. No options, no arguments - just a single space followed by the size in bytes.

@Bob-the-Kuhn

This comment has been minimized.

Copy link
Contributor

Bob-the-Kuhn commented Apr 12, 2017

Many thanks for the information.

I've updated the PR so M20 (no options or arguments) outputs the 8.3 name followed by a space and then the size in bytes.

@thinkyhead

This comment has been minimized.

Copy link
Member

thinkyhead commented Apr 12, 2017

@Bob-the-Kuhn I see your PR is ready for review. Sorry I wasn't more clear, but, yeah, we figured out that no extra parameter is required. Hosts are ready for this change.

@Bob-the-Kuhn

This comment has been minimized.

Copy link
Contributor

Bob-the-Kuhn commented Apr 12, 2017

Not one of the more complicated PRs.

Yes, it's ready for review.

@dietzm

This comment has been minimized.

Copy link

dietzm commented Oct 19, 2017

uh, I just got a bug report that this breaks GCodePrintr. Luckily it is easy to fix.
Someone should update the Marlin documentation, because it still shows the old M20 format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment