set name option for multiplexer signal #579

hyihua · 2023-06-12T16:08:18Z

The function _dump_message uses multiplexer signal id as the signal name. However, it might be incompatible in some platforms, such as the screenshot shown.

In addition, using the ID as the name of the multiplexer signal is not clear enough for people to understand the meaning of this multiplexer. For this issue, I'd suggest that if the multiplexer signal has an original name, use the signal name rather than use the signal id.

If the multiplexer signal has a name, use the signal name rather than use the signal id.

change the expected value from from `MultiplexedSig` to `MultiplexorSig`

cantools/database/can/formats/sym.py

andlaus · 2023-06-12T19:53:11Z

would be nice if a test case actually triggered that condition... (i.e., empty name for the multiplexer selector signal)

update Co-authored-by: Andreas Lauser <github@poware.org>

coveralls · 2023-06-12T20:07:19Z

Pull Request Test Coverage Report for Build 5360296379

8 of 8 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.005%) to 93.903%

Totals
Change from base Build 5186159054:	0.005%
Covered Lines:	7254
Relevant Lines:	7725

💛 - Coveralls

update the test case to rename a multiplexer signal to empty to triggered renaming condition

The GitHub action checks failed because the code didn't use the right variable in `test_multiplex_sym_dump`

cantools/database/can/formats/sym.py

andlaus · 2023-06-12T21:22:57Z

tests/test_database.py


-        # Note: cantools database cannot support multiple multiplexer signal names, so SYM file names the multiplexer
-        # signal after the multiplexer id (Hence, 2A, not MultiplexerSig)


@samc1213: I don't get that comment. Do you remember what you meant to say here? (In my understanding, assigning more than one multiplexer selector signal to any multiplexed signal does not make sense anyway?)

The relevant pull request where this change was added.

@andlaus Are there any updates for this? Is there anything we can do to get this move forward?

I am just confused why this comment was made in the first place, but I think that it was spurious. (so everything is alright here as far as I can see...) I'll merge the PR once the special treatments for empty signal names (and the corresponding test) are removed...

The files are already updated. Please let me know if there are any other issues. We really hope that this feature can be merged and it's an important feature for our product implementation. Thanks @andlaus

I'm sorry for the late reply. I can't recall why I thought SYM multiplexer and cantools database representations didn't match, and decided to use the multiplexer value as the signal name. I thought during parsing we would lose the name of each multiplexer, so we had to use the value or something. I tried to reproduce but couldn't.

* Report an error when meeting an empty signal name rather than set name to signal value. * Generate a test case for testing exception

…ex_sym_dump`

tests/files/sym/test_multiplex_dump.sym

tests/test_database.py

…_multiplex_sym_with_empty_signal_name_dump`

andlaus · 2023-06-23T18:19:05Z

once the CI is happy (seems like there is a typing mistake somewhere), I'll merge this.

hyihua · 2023-06-23T19:30:16Z

once the CI is happy (seems like there is a typing mistake somewhere), I'll merge this.

But it seems like the error was not introduced by my changes.

andlaus · 2023-06-23T19:42:17Z

hm, this is what the github action logs say:

@zariiii9003: Any idea what's going on here and how to fix it?

andlaus · 2023-06-23T19:46:09Z

okay: the linter is not run on the tests, so

ruff check --fix ./cantools

fixes the issue. (it is unrelated to this PR...)

hyihua · 2023-06-23T20:36:20Z

okay: the linter is not run on the tests, so
ruff check --fix ./cantools
fixes the issue. (it is unrelated to this PR...)

I run this command locally before, but just feel like it's not related to this pull request and then I should not push the changes to this pull request. But anyway, we should fix the issue. Made a new commit for that.

andlaus · 2023-06-23T20:40:30Z

I think it would have been slightly better if you put the fixes ruff made to the tests into a separate PR, but since the CI is happy and IMO they are all janitorial changes which we would want to have at some point anyway, I'll merge. thanks for the contribution!

hyihua · 2023-06-23T20:45:08Z

I think it would have been slightly better if you put the fixes ruff made to the tests into a separate PR, but since the CI is happy and IMO they are all janitorial changes which we would want to have at some point anyway, I'll merge. thanks for the contribution!

That's a great suggestion. Thanks for that.

hyihua · 2023-06-23T20:58:06Z

@andlaus, may I ask about when you will release a new version of cantools?

hyihua added 3 commits June 12, 2023 11:59

set option for multiplexer signal

eb441ee

If the multiplexer signal has a name, use the signal name rather than use the signal id.

update test cases related to multiplexer signal name

f0b1373

update the assertion argument in test_multiplex_sym_dump

20fd91c

change the expected value from from `MultiplexedSig` to `MultiplexorSig`

andlaus reviewed Jun 12, 2023

View reviewed changes

cantools/database/can/formats/sym.py Outdated Show resolved Hide resolved

Update multiplexer signal naming, flipping the option logic

1def44f

update Co-authored-by: Andreas Lauser <github@poware.org>

update the test case for multiplex_sym_dump

57eaf16

update the test case to rename a multiplexer signal to empty to triggered renaming condition

hyihua changed the title ~~set option for multiplexer signal~~ set name option for multiplexer signal Jun 12, 2023

fix the issue due to using wrong variable

b0c0ae9

The GitHub action checks failed because the code didn't use the right variable in `test_multiplex_sym_dump`

andlaus reviewed Jun 12, 2023

View reviewed changes

cantools/database/can/formats/sym.py Show resolved Hide resolved

andlaus reviewed Jun 12, 2023

View reviewed changes

hyihua added 3 commits June 21, 2023 11:52

update the treatment for empty signal name

89d537c

* Report an error when meeting an empty signal name rather than set name to signal value. * Generate a test case for testing exception

add code to call dump function in test

6bbd622

call as_sym_string() rather than as_dbc_string() in `test_multipl…

86a0201

…ex_sym_dump`

andlaus reviewed Jun 22, 2023

View reviewed changes

tests/files/sym/test_multiplex_dump.sym Outdated Show resolved Hide resolved

andlaus reviewed Jun 22, 2023

View reviewed changes

tests/test_database.py Outdated Show resolved Hide resolved

revert changes on test_multiplex_dump.sym and update function `test…

2972721

…_multiplex_sym_with_empty_signal_name_dump`

hyihua requested a review from andlaus June 22, 2023 20:23

run ruff check for fixing error when running GH action linter

0193209

andlaus merged commit 10eb7bb into cantools:master Jun 23, 2023
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set name option for multiplexer signal #579

set name option for multiplexer signal #579

hyihua commented Jun 12, 2023 •

edited

andlaus commented Jun 12, 2023

coveralls commented Jun 12, 2023 •

edited

andlaus Jun 12, 2023

hyihua Jun 12, 2023 •

edited

hyihua Jun 20, 2023 •

edited

andlaus Jun 21, 2023

hyihua Jun 23, 2023

samc1213 Jul 6, 2023

andlaus commented Jun 23, 2023

hyihua commented Jun 23, 2023 •

edited

andlaus commented Jun 23, 2023

andlaus commented Jun 23, 2023

hyihua commented Jun 23, 2023 •

edited

andlaus commented Jun 23, 2023

hyihua commented Jun 23, 2023

hyihua commented Jun 23, 2023


		# Note: cantools database cannot support multiple multiplexer signal names, so SYM file names the multiplexer
		# signal after the multiplexer id (Hence, 2A, not MultiplexerSig)

set name option for multiplexer signal #579

set name option for multiplexer signal #579

Conversation

hyihua commented Jun 12, 2023 • edited

andlaus commented Jun 12, 2023

coveralls commented Jun 12, 2023 • edited

Pull Request Test Coverage Report for Build 5360296379

💛 - Coveralls

andlaus Jun 12, 2023

Choose a reason for hiding this comment

hyihua Jun 12, 2023 • edited

Choose a reason for hiding this comment

hyihua Jun 20, 2023 • edited

Choose a reason for hiding this comment

andlaus Jun 21, 2023

Choose a reason for hiding this comment

hyihua Jun 23, 2023

Choose a reason for hiding this comment

samc1213 Jul 6, 2023

Choose a reason for hiding this comment

andlaus commented Jun 23, 2023

hyihua commented Jun 23, 2023 • edited

andlaus commented Jun 23, 2023

andlaus commented Jun 23, 2023

hyihua commented Jun 23, 2023 • edited

andlaus commented Jun 23, 2023

hyihua commented Jun 23, 2023

hyihua commented Jun 23, 2023

hyihua commented Jun 12, 2023 •

edited

coveralls commented Jun 12, 2023 •

edited

hyihua Jun 12, 2023 •

edited

hyihua Jun 20, 2023 •

edited

hyihua commented Jun 23, 2023 •

edited

hyihua commented Jun 23, 2023 •

edited