c4deszes / ldfparser

LIN Description File parser written in Python
https://c4deszes.github.io/ldfparser/
MIT License
64 stars 26 forks source link

Error encode/decode frame with list value signals #119

Closed nuts4coffee closed 1 year ago

nuts4coffee commented 1 year ago

Describe the bug Unable to encode/decode frame with signal of list type.

Signal in LDF file:

  BSBatt2Current: 24, {0, 0, 2}, BMS2, GWM ;

To Reproduce Steps to reproduce the behavior:

The issue can be reproduced from running the following 2 unit tests.

def test_encode_array():
    signal = LinSignal('BattCurr', 24, [0, 0, 2])
    encoding_type = LinSignalEncodingType(
        "BattCurrCoding",
        [PhysicalValue(0, 182272, 0.00390625, -512, "A")]
    )

    signal.encoding_type = encoding_type
    frame = LinUnconditionalFrame(0x20, "LinStatus", 3, {0: signal})

    encoded = frame.encode({"BattCurr": [-511.99609375, -511.99609375, -511.99609375]})

def test_decode_array():
    signal = LinSignal('BattCurr', 24, [0, 0, 2])
    encoding_type = LinSignalEncodingType(
        "BattCurrCoding",
        [PhysicalValue(0, 182272, 0.00390625, -512, "A")]
    )

    signal.encoding_type = encoding_type
    frame = LinUnconditionalFrame(0x20, "LinStatus", 3, {0: signal})

    decoded = frame.decode(frame.encode_raw({"BattCurr": [1, 1, 1]}))

Expected behavior Encode/decode should work without errors.

Stacktrace/Code If applicable, add stacktrace or code segments to help explain your problem.

ldfparser\frame.py:216: in decode
    converted[signal_name] = signal.encoding_type.decode(value, signal, keep_unit)
ldfparser\encoding.py:180: in decode
    return decoder.decode(value, signal, keep_unit)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <ldfparser.encoding.PhysicalValue object at 0x0000017FF8A9B848>, value = [1, 1, 1]
signal = <ldfparser.signal.LinSignal object at 0x0000017FF8A9B188>, keep_unit = False

    def decode(self, value: int, signal: 'LinSignal', keep_unit: bool = False) -> float:
>       if value < self.phy_min or value > self.phy_max:
E       TypeError: '<' not supported between instances of 'list' and 'int'

ldfparser\encoding.py:65: TypeError

Environment:

Optionally include the output of 'pipdeptree --warn silence -p ldfparser'

Additional context Add any other context about the problem here.

c4deszes commented 1 year ago

In your examples you're providing scalar (float) values when encoding array signals, in the standard the initial value provided to array type signals must be an integer array and all the examples there point to this limitation.

Do you have a productive example of scalars encoded/decoded into array signals? Is this something that's allowed in ISO 17987 or SAE J2602?

nuts4coffee commented 1 year ago

In the example, the initial value is an integer array: [0, 0, 2]. The scalar values are the physical value. I think this one can better demo the issue. If I encode a frame with default values, I cannot decode it back.

def test_encode_array():
    signal = LinSignal('BattCurr', 24, [0, 0, 2])
    encoding_type = LinSignalEncodingType(
        "BattCurrCoding",
        [PhysicalValue(0, 182272, 0.00390625, -512, "A")]
    )

    signal.encoding_type = encoding_type
    frame = LinUnconditionalFrame(0x20, "LinStatus", 3, {0: signal})

    encoded = frame.encode({})    # encoded = bytearray(b'\x00\x00\x02')
    frame.decode(encoded)           # Error
c4deszes commented 1 year ago

If you do provide a value in the encode step you would also encounter errors because the PhysicalValue encoder cannot encode list types. So even a line like this encoded = frame.encode({"BattCurr": [0, 1, 2]}) will lead to an exception.

I'd be hesitant to merge #120 since we don't have an example LDF for it, the standard doesn't mention anything about arrays being encoded into scalars, only ASCII and BCD converter examples are available. There's a slight hint at it in the NCF example.

I'll check the ISO standard tomorrow to see if there's anything in there that would confirm this use case. If it does I would prefer the approach where support for array signals is done in the value converters, rather than the frame encoding algorithm.

c4deszes commented 1 year ago

The ISO standard that I read didn't mention anything new regarding array signals. If this is something you know exists in your LDF files then we can add support but as previously mentioned it should done in the converter that way we won't introduce any breaking changes.

nuts4coffee commented 1 year ago

Thank you for pushing back. I realize my original implementation is quite wrong. I've proposed a new solution, please review and let me know if this one is ok.