Update LongNamesConverter on dbc.py

[simonefilippucci]

The function to create short names is based on the smallest text (cut_name), but the check is done on longer text (short_name).
When two signals differ only after the short_name, the index is resetted and two identical short_name are created creating a conflict.

[coveralls]

Pull Request Test Coverage Report for Build 8643915571

Details

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 93.552%

---

Totals
Change from base Build 8024886298:	0.0%
Covered Lines:	7254
Relevant Lines:	7754

---

💛 - Coveralls

[zariiii9003]

Could you provide some code example? i cannot reproduce your issue.

[simonefilippucci]

Could you provide some code example? i cannot reproduce your issue.

You can run this script:

class Test:
    def __init__(self):
        self._short_names = set()
        self._next_index_per_cut_name = {}

    def convert(self, name):
        short_name = None

        if len(name) == 32:
            self._short_names.add(name)
        elif len(name) > 32:
            cut_name = name[:27]
            short_name = name[:32]

            if short_name in self._short_names:
                index = self._next_index_per_cut_name[cut_name]
                self._next_index_per_cut_name[cut_name] += 1
                short_name = f'{name[:27]}_{index:04d}'
            else:
                self._next_index_per_cut_name[cut_name] = 0
                self._short_names.add(short_name)

        return short_name
    
    def proposed_convert(self, name):
        short_name = None

        if len(name) == 32:
            self._short_names.add(name)
        elif len(name) > 32:
            cut_name = name[:27]
            short_name = name[:32]

            if cut_name in self._next_index_per_cut_name:
                index = self._next_index_per_cut_name[cut_name]
                self._next_index_per_cut_name[cut_name] += 1
                short_name = '{}_{:04d}'.format(name[:27], index)
            else:
                self._next_index_per_cut_name[cut_name] = 0
                self._short_names.add(short_name)

        return short_name

def main():
    t = Test()
    signal_names =  [f"{'Name':_<30}_A_{x}" for x in range(1,6)]
    signal_names += [f"{'Name':_<30}_B_{x}" for x in range(1,6)]
    print("Current implementation:")
    for signal_name in signal_names:
        print(t.convert(signal_name))

    t = Test()
    print("\nProposed implementation:")
    for signal_name in signal_names:
        print(t.proposed_convert(signal_name))

if __name__ == "__main__":
     raise SystemExit(main())
     

This will be the output:

Current implementation:
Name___________________________A
Name________________________0000
Name________________________0001
Name________________________0002
Name________________________0003
Name___________________________B
Name________________________0000
Name________________________0001
Name________________________0002
Name________________________0003

Proposed implementation:
Name___________________________A
Name________________________0000
Name________________________0001
Name________________________0002
Name________________________0003
Name________________________0004
Name________________________0005
Name________________________0006
Name________________________0007
Name________________________0008

As you can see, when the signal name are longer than 32 characters and are equal for the first 27 characters, but different from position 27 to position 32, the output of the convert function will be the same for different signal names, for example
"Name___________________________A_2" and "Name___________________________B_2", will have the same short name: "Name________________________0000"

So in the message the translation from short name to long name will be wrong.

[zariiii9003]

Great example, thanks!
Can LongNamesConverter._database and LongNamesConverter._short_names be removed entirely? I do not see any read access.

[simonefilippucci]

Great example, thanks! Can LongNamesConverter._database and LongNamesConverter._short_names be removed entirely? I do not see any read access.

I agree, they can be removed 👍

[zariiii9003]

I agree, they can be removed 👍

I assumed you'd remove them 😅

[zariiii9003]

Closing in favor of #660