Support decode/encode signals with list type init values

[nuts4coffee]

Brief

• Please provide brief information, what this pull request provides or fixes, possibly in a list format Propose a fix for the issue https://github.com/c4deszes/ldfparser/issues/119.

Checklist

• [ ] Add relevant labels to the Pull Request
• [ ] Review test results and code coverage
  • [ ] Review snapshot test results for deviations
• [ ] Review code changes
  • [ ] Create relevant test scenarios
  • [ ] Update examples
  • [ ] Update JSON schema
• [ ] Update documentation
  • [ ] Update examples in README
• [ ] Update changelog
• [ ] Update version number

Resolves

Fixes #119

• Describe the bug or feature and link to relevant issues

Update decode/encode function to support signals with list type init values, as in BSBatt2Current: 24, {0, 0, 2}, ECU, GWM ;.

Evidence

• Analyze how the change might impact existing code

• Provide evidence that the feature is tested and covered properly Unit test has been added.

[codecov[bot]]

Codecov Report

Merging #120 (1f60afe) into master (f611870) will increase coverage by 0.44%. Report is 1 commits behind head on master. The diff coverage is 99.06%.

@@            Coverage Diff             @@
##           master     #120      +/-   ##
==========================================
+ Coverage   95.84%   96.28%   +0.44%     
==========================================
  Files          13       13              
  Lines        1371     1453      +82     
==========================================
+ Hits         1314     1399      +85     
+ Misses         57       54       -3     

Flag	Coverage Δ
3.10.0b1	96.28% <99.06%> (+0.44%)	:arrow_up:
3.6	96.28% <99.06%> (+0.44%)	:arrow_up:
3.7	96.28% <99.06%> (+0.44%)	:arrow_up:
3.8	96.28% <99.06%> (+0.44%)	:arrow_up:
3.9	96.28% <99.06%> (+0.44%)	:arrow_up:
unittests	96.28% <99.06%> (+0.44%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
ldfparser/node.py	86.44% <83.33%> (+0.72%)	:arrow_up:
ldfparser/frame.py	96.17% <100.00%> (+1.10%)	:arrow_up:
ldfparser/grammar.py	97.43% <100.00%> (+0.02%)	:arrow_up:
ldfparser/ldf.py	98.21% <100.00%> (ø)
ldfparser/lin.py	98.51% <100.00%> (+3.33%)	:arrow_up:
ldfparser/parser.py	95.32% <100.00%> (+0.15%)	:arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[nuts4coffee]

Hi @c4deszes, could you please take another look at this PR? Thank you.

[nuts4coffee]

Thank you for reviewing. I totally understand your concern. I have looked up the standard but also couldn't find much information regarding signals with array-type initial values. My first attempt was converting each element of the array individually, and the second attempt was converting the array into one integer and encoding it. Reading the spec again, based on the following statement I feel the 1st attempt makes more sense as well.

LIN 2.2A, section 9.2.3.1
The only way to describe if a signal with size 8 or 16 is a byte array with one or two elements or a scalar signal is by analyzing the init_value, i.e. the curly parenthesis are very important to distinguish between arrays and scalar values.

I could revert the changes, but I am not sure about the ask of moving the handling into the Physical/Logical encoders, as the signal encoding are still the same, and the knowledge about whether it's an array type signal is on the signal, not the Physical/Logical encoders. Could you help clarify a bit? Unfortunately I don't have a real-life example yet as we're doing this work preparing for testing in the near future. I have the following snippet from the LDF file if that helps:

Signal:

  SomeSignalName: 24, {0, 0, 2}, BCM, RCM;

Signal Encoding:

  SomeSignalName_Encoding {
    physical_value, 0, 182272, 0.00390625, -512, "A" ;
    logical_value, 16777215, "Invalid" ;
  }

[c4deszes]

Based on the LDF I sense that the way it is currently is the correct approach, I'll double check this snippet against Vector LDF Explorer, that should provide warnings if this is way out of spec. I'll also check if there's encoding/decoding support there, otherwise I'll try and spin up a Canoe environment with a virtual ECU and check what the signals are for given frame contents.

What I meant by moving support into the converters would be simply checking the incoming signal type or value then if it's an array encode or decode accordingly. After thinking about it though this would only work as long as the converter is able to handle all elements of the array, it would fail for example with ["20.0A", "Invalid"] because it's a mix of logical and physical values. There could be workarounds however in which during parsing we create wrappers around the referenced encoders for every array type signal or create/modify LinSignalEncodingType class that supports this behavior.

[nuts4coffee]

I don't have the vector tools to check myself, and I have got the following data from a colleague from Canoe which shows the decoded value is a scalar value instead of an array. So my current implementation is in-line with this. Initial value 0, 0, 2, decoded value:

Initial value 2, 0, 0, decoded value:

[nuts4coffee]

Hi @c4deszes, have you got the chance to cross-check the expected behavior for byte array type signals with Canoe or Vector LDFExplore?

I notice another issue that also applies to the raw encoding/decoding. The initial_value for the byte array is arranged in big endian order, and as I understand any signal bigger than one byte shall be transmitted in LSB with the default encoding. Shouldn't we expect the encoded value to be [2,1] in this test?

def test_frame_raw_encoding_array():
    signal1 = LinSignal('Signal_1', 16, [0, 0])
    frame = LinUnconditionalFrame(1, 'Frame_1', 2, {0: signal1})
    content = frame.raw({
        'Signal_1': [1, 2]
    })
    assert list(content) == [1, 2]  # -> [2,1]??

Copied from https://github.com/c4deszes/ldfparser/blob/master/tests/test_frame.py#L58

[c4deszes]

Did check it yesterday, and Canoe confirms what your colleague has shared with you at least when it comes to encoding/decoding in practice. If I give a raw signal value of say 0x020000 then the decoded value is 0.000A.

In LDF Explorer there's a warning for the physical and logical encoder values, basically they're way out of range, since a signal can only be 16 bit wide the maximum is 65535.

There's not much else in LDF explorer, it's possible to create an array signal in there. In the encoding settings ASCII and BCD can be selected as well, those two seem to be mutually exclusive but it doesn't have any issue if I specify ASCII encoding and provide physical encoding as well.

So overall it seems that LDF explorer simply follows the specification and doesn't have checks for array signals that this library doesn't have. Then in Canoe it looks like array encoding/decoding is not supported, when I set it to ASCII or BCD it simply showed the decimal value as the physical value.

You might be right about the byte order in the test, just like other details it's not specified or not specified well. In the third image bottom right you can see byte order "Intel" that's greyed out in LDF Explorer, the behavior is then probably controllable or controlled by for example the LIN standard used, for example ISO17987 has a setting related to this. The encoding/decoding scheme was taken initially from uCAN-LIN/LinUSBConverter but signals were added MSB first so the algorithm was modified until it worked with real LIN networks, I think in this case this is the only way to solve this problem as well.

Now seeing how Canoe behaves I sort of understand how this feature is being used, basically it's to overcome the 16bit signal size limitation. Given this I'm fine with the contents of the PR as it's only adding test cases that prove that the new scenarios work and the old ones do as well. I still don't like how vague the spec. is, but there's nothing we can do apart from analyzing production LIN networks.

[nuts4coffee]

Thank you for the investigation. If you're Ok with the contents in this PR, could you please approve it? I'm still working on figuring out the byte order, and once I confirm changes are needed will raise a separate PR.

[c4deszes]

Yes, there's just one thing remaining then, which would be adding this under the unreleased changelog entry and if you want this released as 0.21.0 then the version number has to be bumped and a new header for 0.21.0 should be created, date could be today, tomorrow doesn't affect anything.