Removed null-terminated from payload

[LukeTheEngineer]

On page 53 of the Autosar FO R22-11, the specification states the data payload of type String(STRG) must be constructed without a special terminator item such as '\0'

@mbehr1 pointed out that since the length of the string is already determined, the need for a null terminator is useless in that context.

Before
case DLT_RETURN_OK: { /* Whole string will be copied */ memcpy(log->buffer + log->size, text, length); **The input string might not be null-terminated, so we're doing that by ourselves** log->buffer[log->size + length] = '\0'; log->size += arg_size; break; }

After

`
{

    /* Whole string will be copied */

    memcpy(log->buffer + log->size, text, length);

    /* The input string might not be null-terminated, so we're doing that by ourselves */

    /*The length of the string is already determined. Why need the null-terminator???*/

    //log->buffer[log->size + length] = '\0';   //*[Deprecated]

    log->size += arg_size;
    break;
}`

I would like to know your thoughts on why this was added and if there was any specific meaning to this.

[mbehr1]

I think that's not enough. line 2621 adds the null term as +1 to the strlen. That needs to be removed as well (at least)

[LukeTheEngineer]

On line 2638, size_t str_truncate_message_length = strlen(STR_TRUNCATED_MESSAGE) + 1;
also includes a null terminator +1. Does this correlate with the standard or is this different than our current issue?

I've also removed +1 from 2621.

[michael-methner]

Hello @LukeTheEngineer ,
yes, to be according to autosar specification, both places needs to be fixed otherwise long messages which will truncated will carry still NULL termination.

I fixed this at my local setup but noticed that a lot of the unit tests are failing then but I do not have the time to investigate this further now.

We had in the past some problems with NULL-termination of strings. So I am a little bit hesitant to merge such a fix. Even if it is according to spec, a lot of legacy code may nowadays rely on this NULL-termination (like the DLT unit tests). As I cannot think of any issues arising from this additional NULL-termination, I would prefer to not merging this but I am open to arguments here.

@mbehr1 , @LukeTheEngineer , @DLT-community: What's your opinion on this?

[mbehr1]

@michael-methner I stumbled exactly upon this problem on "relying" on null termination. (see mbehr1/adlt@a17c59f )
But the problem is that you cannot rely on this as there are some tools that create strings non null-terminated. So I'd better change this to always "non-null" / according to spec. than to rely on something which is not granted by spec and which not all tools do.

[LukeTheEngineer]

@michael-methner How frequently does NULL termination occur in the entire code base that affects this specific specification?

Perhaps there could be a way to perform the job of a NULL terminator without explicitly adding them? Then again, I am still relatively new to dlt-daemon, and still trying to understand how everything works.

[minminlittleshrimp]

Hello @lvklevankhanh
Please kindly have a look.

Hello @mbehr1 and @LukeTheEngineer ,
thanks for your interest in dlt topic.
To me, I agree with @michael-methner 's perspective. R22-11 quite new and we all need time to adapt.
You could easily see that huge things change after R21-11, especially the introduction of v2 protocol forcing dlt team to think about a Major release where we need to reimplement base header with HTYP2. We even do not prepare any plan for this change yet, so it will take time.
We might let this PR for a little bit later when we plan for the Major release.
Thanks for your understanding.
Minh

[mbehr1]

@minminlittleshrimp I don't agree. This change/bugfix should be done completely independent of any htyp2 support. It's a smallish bugfix.

[minminlittleshrimp]

Hello @mbehr1
Yes it is a small change, but it will affect our whole test suites which were implemented based on the old standard. Hence it requires a lot of effort to fix the whole test suites, which cannot be done in a few weeks. Plus we even do not know if there will be any further issues occur if this change is merged. Unit tests could not tell much things but some regression tests and smoketests might fail pointing to whatever legacy code inside dlt. Honestly, I think there must be some reasons that previous dlt maintainers decided to use Null like that. We will try our best to figure out and find the best way soon.
Thank you

[mbehr1]

@minminlittleshrimp I think changing the tests would be easy as well. But I just checked the old spec. So Autosar R4.0 Rev3 (spec v1.2.0) specifies:
Length of string + Unsigned 16-bit integer termination char.
Data string: Null terminated data string

So it's really a recent spec change. I wonder why this spec change was even approved. How is the review process for that?

Changing existing requirements so significantly is a mess...

So I'm fine in putting this in a new release where e.g. htyp2 will be supported as well. The trouble for v1 header will anyhow still be the same... (so tools need to handle zero-term and non-zero-term strings...)

[michael-methner]

Hello @mbehr1 ,
many thanks for that finding. I really missed that one.
The protocol v2 is not backward compatibility. So it would be something for DLT-deamon release version 3. But I do not see much interest in adapting the v2 protocol, besides it has some other nice features.

Just for completeness, here are the differences between the two specs:
R20-11 Log and Trace Protocol Specification:
[PRS_Dlt_00156] ⌈At the beginning of the Data Payload, a 16-bit unsigned integer specifies the length of the string (provided in the Data field) in byte including the termination character.
[PRS_Dlt_00373] Null terminated string (name of variable)

R21-11: Log and Trace Protocol Specification
[PRS_Dlt_00156] Note: The string end is only defined by this length information
[PRS_Dlt_00373] Data string without a special terminating item like the NUL-character (\0)

[LukeTheEngineer]

How much could the null terminators affect the user? Also, is this the only file the null-terminators are present in?

[michael-methner]

@mbehr1 , @LukeTheEngineer , @minminlittleshrimp
Thank you for working on this PR. I close this PR now as we do not implement DLTv2 protocol as of today. The current implementation is in line with the old spec R20-11.