Support payload argument separation similar to DltViewer #10

ChrisRBe · 2021-01-08T22:06:55Z

This is hopefully a fitting implementation for #9.

Adds a config item to specify if payload args should be separated or not; defaults to false.
Slight interface change required imho to support this payload separation.

Please note: I haven't installed a local build environment, so .. it could work. Probably not. First time typescripting...

mbehr1 · 2021-01-09T08:48:24Z

Thx! :-)
I'll review the PR. Need to be extremely careful with any performance or memory impacts on processing huge dlt files.
I like the config switch but I'd prefer if with the new switch the output is 1:1 compliant with dltviewer. I guess there are more differences than just the missing space, or?
Does dltviewer add a space between all args?
Does it add a space after each arg or only if some more args are following?
Does it add a space even after string args?
...?

mbehr1 · 2021-01-09T08:51:58Z

README.md

@@ -95,6 +95,7 @@ This extension contributes the following settings:
 * `dlt-logs.fileExtensions`: Specifies the file extensions to use for file open dialog. Defaults to .dlt|.DLT.
 * `dlt-logs.maxNumberLogs`: Specified the maximum number of DLT logs that get displayed in one page. If more logs exist - considering the active filters - a paging mechanism is in place that starts rendering a new page at 4/5th of the page boundary. Searching is limited to the visible page. Defaults to 0.4mio logs. Depending on your machines performance/RAM you might reduce/increase this. Best case is to find a limit/config where all logs fit into that range (use filter!).
 * `dlt-logs.reReadTimeout`: Specified the timeout in ms after opening the file before starting to parse the dlt file. If the file doesn't open, increase this to e.g. 5s.
+* `dlt-logs.separatePayloadArgs`: Separate payload arguments with a space (' ') character. Default is false to not break existing filters.


How about reversing the logic? The target should be same output as dltviewer.
The problem are existing users that have filters with payload. We could introduce the flag as
"legacyPayloadDecoding", default false, at startup check whether there are existing filters with payload and ask/inform user to check his filters or enable the option?

In lights of your later comments regarding memory consumption etc. this is probably the better solution.

One question remains: How do you recognize "legacy" filters? It would be complete guesswork as especially the payload filter is in the end only a string regex. What would be the identifier?
Of the top of my head? Add a completely redundant version field to either the filter section or each filter. If the field is not available, show a message stating that there are maybe incompatible filters?

Yep, as soon as there is any filter with a payload/payloadregex I'd show the message and ask the user. Filters with ecu/apid/ctid,... are not impacted.

src/dltDocument.ts

src/dltParser.ts

mbehr1 · 2021-01-09T08:58:03Z

src/dltParser.ts

@@ -292,7 +295,7 @@ export class DltMsg {
                                    argOffset += 2;
                                    const rawd = this._payloadData.slice(argOffset, argOffset + lenRaw);
                                    this._payloadArgs.push({ type: Buffer, v: Buffer.from(rawd) }); // we make a copy here to avoid referencing the payloadData that we want to release/gc afterwards
-                                    this._payloadText += rawd.toString("hex");
+                                    this._payloadText += rawd.toString("hex") + this.payloadArgSeparator;


I think it's not the only place where the space is missing?
And: does dltviewer add a space at the end?

Hard to tell.
Do you have an easy to use DLT example that uses all different DLT_LOG functions to write messages? Plus, I'm not really that good at reading the dltviewer code to answer that clearly.

What I would say: the current implementation, or rather, simply adding a space at this position did not seem to add additional spaces at the end in vscode.

Sadly I miss good examples. Could write a small test app that creates those.
I might have some code to remove whitespace at end ;-) (will take a look)
I can check the dltviewer code and try to understand the rules implemented there.

mbehr1

see inline comments.

mbehr1 · 2021-01-09T08:59:51Z

btw: I could configure the GitHub workflow to build a .vsix install package for PRs as well (to avoid the need to build locally)?

ChrisRBe · 2021-01-09T10:14:33Z

Thx! :-)
I'll review the PR. Need to be extremely careful with any performance or memory impacts on processing huge dlt files.
I like the config switch but I'd prefer if with the new switch the output is 1:1 compliant with dltviewer. I guess there are more differences than just the missing space, or?

uff. good question. You don't really see the formatting in dltviewer easily and I'm not good at reading the dltviewer source code to easily point to any possible differences.

Does dltviewer add a space between all args?

Going out on a limb here: Yes, it does. Having an example dlt file using all possible argument combinations would be easier.

Does it add a space after each arg or only if some more args are following?
Does it add a space even after string args?

In the cases that I have seen there is a space after each arg with the last arg probably not being ended with a space.

...?

One thing comes to mind: byte representation. In the current extension version, payload containing bytes are not split on byte size (0000 vs. 00 00). Apart from that... good question.

ChrisRBe · 2021-01-09T11:49:12Z

src/dltParser.ts

@@ -93,6 +93,7 @@ export class DltMsg {
    readonly timeAsNumber: number; // time in ms. Date uses more memory!
    get timeAsDate(): Date { return new Date(this.timeAsNumber); }

+    readonly payloadArgSeparator: string;


Suggested change

readonly payloadArgSeparator: string;

static payloadArgSeparator = vscode.workspace.getConfiguration().get<boolean>('dlt-logs.legacyPayloadDecoding') ? "" : " "; // default behaviour of legacyPayloadDecoding is false, add a space character in that case.

could this work?

ChrisRBe · 2021-01-09T11:50:14Z

src/dltParser.ts

@@ -292,7 +295,7 @@ export class DltMsg {
                                    argOffset += 2;
                                    const rawd = this._payloadData.slice(argOffset, argOffset + lenRaw);
                                    this._payloadArgs.push({ type: Buffer, v: Buffer.from(rawd) }); // we make a copy here to avoid referencing the payloadData that we want to release/gc afterwards
-                                    this._payloadText += rawd.toString("hex");
+                                    this._payloadText += rawd.toString("hex") + this.payloadArgSeparator;


Suggested change

this._payloadText += rawd.toString("hex") + this.payloadArgSeparator;

this._payloadText += rawd.toString("hex") + DltMsg.payloadArgSeparator;

as the consequence?

mbehr1 reviewed Jan 9, 2021

View reviewed changes

src/dltDocument.ts Show resolved Hide resolved

mbehr1 reviewed Jan 9, 2021

View reviewed changes

src/dltParser.ts Show resolved Hide resolved

mbehr1 reviewed Jan 9, 2021

View reviewed changes

mbehr1 requested changes Jan 9, 2021

View reviewed changes

feat: support payload argument separation similar to dltviewer

79cbc04

ChrisRBe force-pushed the feature/separatePayloadArgs branch from 533a73c to 79cbc04 Compare January 9, 2021 11:37

ChrisRBe commented Jan 9, 2021

View reviewed changes

mbehr1 force-pushed the master branch 13 times, most recently from 260cfc9 to b6cf392 Compare April 23, 2022 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support payload argument separation similar to DltViewer #10

Support payload argument separation similar to DltViewer #10

ChrisRBe commented Jan 8, 2021

mbehr1 commented Jan 9, 2021

mbehr1 Jan 9, 2021

ChrisRBe Jan 9, 2021

mbehr1 Jan 9, 2021

mbehr1 Jan 9, 2021

ChrisRBe Jan 9, 2021

mbehr1 Jan 9, 2021

mbehr1 left a comment

mbehr1 commented Jan 9, 2021

ChrisRBe commented Jan 9, 2021

ChrisRBe Jan 9, 2021 •

edited

Loading

ChrisRBe Jan 9, 2021

ChrisRBe Jan 9, 2021

ChrisRBe Jan 9, 2021

	readonly payloadArgSeparator: string;
	static payloadArgSeparator = vscode.workspace.getConfiguration().get<boolean>('dlt-logs.legacyPayloadDecoding') ? "" : " "; // default behaviour of legacyPayloadDecoding is false, add a space character in that case.

	this._payloadText += rawd.toString("hex") + this.payloadArgSeparator;
	this._payloadText += rawd.toString("hex") + DltMsg.payloadArgSeparator;

Support payload argument separation similar to DltViewer #10

Are you sure you want to change the base?

Support payload argument separation similar to DltViewer #10

Conversation

ChrisRBe commented Jan 8, 2021

mbehr1 commented Jan 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbehr1 left a comment

Choose a reason for hiding this comment

mbehr1 commented Jan 9, 2021

ChrisRBe commented Jan 9, 2021

ChrisRBe Jan 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisRBe Jan 9, 2021 •

edited

Loading