tbird20d / grabserial

Grabserial - python-based serial dump and timing program - good for embedded Linux development
GNU General Public License v2.0
195 stars 77 forks source link

Corrected -o behavior and fixed compatibility with -R. #43

Closed Jeroen6 closed 3 years ago

Jeroen6 commented 3 years ago

@AlphaSierraHotel Based on your comments on the previous merge request I propose this.
It works without any format string. It works with "%%" default string. It works with custom format strings. It fixes -R by using loading out_pattern to re-generate the uri with timestamp later on.

Your proposal of using == ""%" is not correct in my opinion as you then lose the option to provide paths and extensions.

I'm not sure what should happen when -R and -o are given without time format string...

Regarding the 'nt' issue and making it agnostic, you will break compatibility when doing that.

AlphaSierraHotel commented 3 years ago

Respectfully, paths always worked before with one exception - you could not use a path and specify the shortcut pattern "%" to get a full timestamp with a path. It seems that's what you're trying to do/add/fix. Prior to the recent changes, you could use a path but you would need to specify the pattern for it to would work. Ie. -o "C:\logs\%H-%M-%S.log" or to emulate the full pattern, use -o "C:\logs\%Y-%m-%dT%H-%M-%S" (with dashes, so that it's valid on Windows).

I interpreted Tim's use of the "%" shortcut as just that, a shortcut for when he's running it from a command line and just wants to capture output quickly to his current working folder. I prefer to specify a full pattern with my paths, especially those run from scripts. Currently, that's broken.

Let's give @tbird20d some time to review all this (I'm sure he's busy) and see what he says.

Edit: punctuation

Jeroen6 commented 3 years ago

That sounds plausible. It may even be possible then to revert and only add it as a remark in the doc/help.

There is no rush.

AlphaSierraHotel commented 3 years ago

@Jeroen6, if you want to give it try, clone the output-filenaming branch of my grabserial fork https://github.com/AlphaSierraHotel/grabserial/tree/output-filenaming and give it spin with the various file naming patterns we've discussed. It's basically reverting the recent changes but I did change the default '%' shortcut pattern to be platform agnostic with just dashes, no colons. If it works for you, we can collaborate on documentation updates to go along with it to better explain the file naming patterns on how to use them.

Jeroen6 commented 3 years ago

Yeah, that branch looks alright. Also works with -R. Maybe make "%" the default no argument -o option, so there is no ambiguity that it doesn't work with extensions or paths.

PS C:\git\grabserial> python grabserial -v -d COM10 -B 9600 -T -a -R 10s -o "c:\log\%Y-%m-%dT%H-%M-%S.txt"
Saving data to 'c:\log\2020-07-13T08-02-09.txt'

PS C:\git\grabserial> python grabserial -v -d COM10 -B 9600 -T -a -R 10s -o "%"
Saving data to '2020-07-13-08-02-01'

PS C:\git\grabserial> python grabserial -v -d COM10 -B 9600 -T -a -R 10s -o "%.txt"
Traceback (most recent call last):
  File "grabserial", line 949, in <module>
    restart_requested = grab(sys.argv[1:])
  File "grabserial", line 497, in grab
    out_filename = datetime.datetime.now().strftime(out_pattern)
ValueError: Invalid format string

Sorry for wasting everyone's time with my ""fix"".

Jeroen6 commented 3 years ago

Let's take it over here. https://github.com/AlphaSierraHotel/grabserial/tree/output-filenaming