linux-can / can-utils

Linux-CAN / SocketCAN user space applications
2.4k stars 712 forks source link

candump support all timestamp formats in log files, and log format stdout #265

Closed richnetdesign closed 3 years ago

richnetdesign commented 3 years ago

As a user, I want the option to log zero based relative timestamp formats to a file. For instance, sometimes I don't care about absolute time, and relative is easier to read so I would like to log directly it directly to a file.

Issue Description canutils is currently unable to use timestamp type options in combination with log output -L or -l. -t <type> (timestamp: (a)bsolute/(d)elta/(z)ero/(A)bsolute w date) and -l (log CAN-frames into file. -L (use log file format on stdout)

In regards to log output format compatibility. Absolute, and zero logs formats work fine with can player. So there is no reason they shouldn't be supported. Delta log format is different as it prints time between messages, so not suited for canplayer, unless it has a delta playback mode. This is typically only used in specific diagnostic scenarios, I don't think we need to protect users from playing it back.

Example usage

candump -L -tz vcan0
(000.000000) vcan0 0667BAF6#89C76376D62ADD77
(000.200074) vcan0 05F541AB#
(000.400156) vcan0 187E0AA5#651E4230F28F
(000.600243) vcan0 0C091058#F45333551ABF4051

./candump -L -ta vcan0
(1607120998.528012) vcan0 0E7D3A28#0EE8651E732FBF64
(1607120998.728108) vcan0 00EDF658#48AE52014F

./candump -L -td vcan0
(000.000000) vcan0 1C7C20AE#77E569140F27AC41
(000.200083) vcan0 15E0F04D#EF
(000.200085) vcan0 008702CA#1B431B0CC811287B

Fix Description

Previously -l if(log) and -L if(logformt) bypassed all timestamp formatting by using goto out_fflush. I have extracted the timestamp formatting to print_timestamp so timestamp options are now available to logging workflows.

Known Issues canplayer does not support playing back -tA format logs (2020-12-04 17:25:57.869400) vcan0 02E939C4#DE18475339

hartkopp commented 3 years ago

In regards to log output format compatibility. Absolute, and zero logs formats work fine with can player. So there is no reason they shouldn't be supported. Delta log format is different as it prints time between messages, so not suited for canplayer, unless it has a delta playback mode.

As you can see at the last sentence and in 'Known issues' you discover the issues with consuming tools when changing established file formats yourself.

Your patch looks good and I can follow your arguments in most cases BUT the design principle of the log-file format is that every line stands for itself and contains either a comment or a full qualified CAN frame (real time stamp, interface, content).

There are multiple sorting and post processing tools that rely on this timestamp format and its semantics. E.g. you can ssh into a host and execute candump -L any and pipe this to canplayer on your host. IMO changing the timestamp format in the log-file format that finally only reduces(!) information is not a good idea.

Do you want this change for the log file or to look at it directly as live data representation?

Btw. are you aware of the log2long tool that was created to have the standard candump representation? You just can pipe-in the log-file format. Maybe you could extend log2long with new command line options to better fit your needs ...

richnetdesign commented 3 years ago

I understand the concern of ensuring compatibility of the log format. I also understand wanting to maximize the information stored in a log file. Some proprietary CAN diagnostic and logging tools often default to relative time, and I have been in the situations where others shared a log with me, and I wish I knew real time it was recorded.

On the flip side, I have just been in less common situations where I wanted to capture a short log, didn't care about the absolute time, and I wanted to avoid the eye / mental strain of log consumers of reading irrelevant long timestamps. I was in one of these situations, and took a quick hack at changing it, and made this patch. Example use cases

Candump defaults to a full timestamp as it should. I argue that if an engineer is recording in one of these use cases for further analysis, and they choose to use relative time; I don't think there is a reason to block them from doing this, and forcing them to do an extra step of conversion.

Thanks for letting me know about log2long. I have faint memories of it, but have yet to try it. It doesn't seem to have a high discoverability have have any help text. Perhaps there are some improvements that can be made there.

For consideration, what if we add validation to allow a user to choose zero relative timestamps (which are still valid and compatible with canplayer) but block invalid formats such as delta?

if ((log || logfrmt) && timestamp && timestamp != 'a' && timestamp != 'z' ) {
    fprintf(stderr, "Log format is not compatible with chosen timestamp format %c\n", timestamp);
    return 1;
}

Furthermore, if the log format supports comments, a comment could be injected at beginning of file to indicate the start of recording time should it later become important to know. This would allow recalculating all the relative times.

hartkopp commented 3 years ago

I would suggest to create unsigned char logtimestamp = 'a'; next to unsigned char timestamp = 0; and fill it parallel to timestamp in the option evaluation. 'a' and 'z' is something which can be considered at least as 'a' remains the default. But you had your 'logtimestamp' assignment in the hot path which is not good. Please re-spin with a global logtimestamp which only allows 'a' and 'z'.

hartkopp commented 3 years ago

Closed with commit https://github.com/linux-can/can-utils/commit/eb66451df280f95a9a12e78b151b8d867e1b78ed