Implement support for container and CAN-FD frames

[AlexMicWalz]

• implement support for container-frames,
• implement support for CAN-FD frames,
• adapt output message for loading files.

Alexander Walz <[email protected]>, on behalf of MBition GmbH.
Provider Information

[AlexMicWalz]

Hello, meanwhile I've also signed the Eclipse Contributor Agreement, hope this helps.
Kind regards, Alex

[lukasmittag]

to get the CI clear you maybe need to rebase and force push or add another commit

[andlaus]

is there a way to fix these pesky formatting errors automatically? (something like ruff --fix, but flake8 does not seem to have such a feature...)

[SebastianSchildt]

is there a way to fix these pesky formatting errors automatically? (something like ruff --fix, but flake8 does not seem to have such a feature...)

yes there is!

You do pip install pre-commit  in the working directory, so then when you commit, it will check, and fail if something is wrong. You can also run pre-commit manually in the terminal in your working directory. it will then modify the files so they match style. Then you just need to git add them to accept that, and commit will work, and also the CI check after push.

[andlaus]

I did this for the HEAD of this PR, but flake8 still complains:

kuksa.val.feeders/dbc2val|7a0f8a2... > pre-commit && flake8 .
Trim Trailing Whitespace.............................(no files to check)Skipped
Fix End of Files.....................................(no files to check)Skipped
Check Yaml...........................................(no files to check)Skipped
Check for added large files..........................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
./dbcfeederlib/canreader.py:35:110: E231 missing whitespace after ':'
./dbcfeederlib/canreader.py:35:115: E252 missing whitespace around parameter equals
./dbcfeederlib/canreader.py:35:116: E252 missing whitespace around parameter equals
./dbcfeederlib/canreader.py:35:121: E501 line too long (122 > 120 characters)
./dbcfeederlib/canreader.py:47:121: E501 line too long (132 > 120 characters)
./dbcfeederlib/canreader.py:90:13: F841 local variable 'e' is assigned to but never used
./dbcfeederlib/canreader.py:103:41: E231 missing whitespace after ','
./dbcfeederlib/canreader.py:108:5: E303 too many blank lines (2)
./dbcfeederlib/dbcparser.py:121:1: W293 blank line contains whitespace
./dbcfeederlib/dbcreader.py:35:121: E501 line too long (131 > 120 characters)

the various sub-commands for pre-commit (e.g., pre-commit run) also do not seem to make a difference. what am I missing?

[andlaus]

(note: runninng black makes flake8 happy, bit it leads to a ton of irrelevant formatting changes. I suppose you're not fond of these?)

[SebastianSchildt]

Ahh, I think the issue is, you committed already, hence when you run pe-commit it always determines (no files to check)Skipped.

I guess @erikbosch knows more about the pre-commit hook, but as an ugly way try this

• Touch you files again, i.e. add a space somehwere
• git add them
• run pre-commit
• it should have modified your files, check whether you like it
• git add again
• commit

[andlaus]

okay, tried that: unfortunately, it didn't change anything. That said, what formatter tool is pre-commit supposed to call in order to fix the coding style? (it could be any of yapf, black, autopep8, ...)

[SebastianSchildt]

For the config see https://github.com/eclipse/kuksa.val.feeders/blob/main/.pre-commit-config.yaml and https://github.com/eclipse/kuksa.val.feeders/blob/main/.flake8

I am currently not sure whether the pre-commit flake part will also modify the source. The whitespace checker definitely does. If in doubt I think @erikbosch might support by adding a commit on top of your work

Apart from "style and beauty", functionality wise @erikbosch might have a say and @sophokles73 as your are an active user of this provider, you might want to check if this still work for your use cases

[sophokles73]

FMPOV a comment regarding the underlying condition that you are checking here would make this easier to understand
Also, there is a space character missing after the comma ...

[sophokles73]

this new parameter should probably also be documented in the README

[erikbosch]

Yes, and we should better discuss if we think this one need to be controllable by environment variable and config file as well, similar to how canport is managed.

if args.canport:
        canport = args.canport
    elif os.environ.get("CAN_PORT"):
        canport = os.environ.get("CAN_PORT")
    else:
        canport = config.get(CONFIG_SECTION_CAN, CONFIG_OPTION_PORT, fallback=None)

[sophokles73]

@AlexMicWalz this looks good to me :-) It would be great if you could also add a unit test checking the FD frame parsing/handling ...

[erikbosch]

The current status of the pre-commit checks is that we have not prescribed/proposed any tool to fix the findings. Any suggestion on a good candidate is welcome, as we then could discuss if we should make that one the "recommended" formatting tool. But that I assume would include work on agreeing/defining settings for formatting, preferably in a standardized format understandable by multiple IDEs/tools.

Until now I have fixed findings manually in my PRs. As long as you change files that have been touched recently that is typically not too cumbersome as it only concerns the lines you have changed. If you touch files that has not been modified since pre-commit was introduced it may mean more work, as we have used a lazy approach for the pre-commit checks and have not checked/fixed all files, but instead do it when touching files.

[erikbosch]

I did some smoketest using the same logfile as we use for release testing and did not observe anything odd. I think we shall add support for defining this in at least config file as we have said in VSS_project that all settings should be available in config file, but command line arg shall have precedence.

[andlaus]

I did some smoketest using the same logfile as we use for release testing and did not observe anything odd

testing for container frames is quite a bit of effort because DBC does not support this feature (you need to use an ARXML file). We'll look into providing a unit test...

[andlaus]

The current status of the pre-commit checks is that we have not prescribed/proposed any tool to fix the findings. Any suggestion on a good candidate is welcome, as we then could discuss if we should make that one the "recommended" formatting tool.

the approach used by odxtools is to run YAPF as part of the CI and bail out if any discrepancy is detected. To fix such issues a small shell script is provided...

[sophokles73]

testing for container frames is quite a bit of effort because DBC does not support this feature (you need to use an ARXML file). We'll look into providing a unit test...

Maybe you can use a similar approach as in https://github.com/eclipse/kuksa.val.feeders/blob/91c3091a8918eed7d79500892ba8d894930b7a67/dbc2val/test/test_readers/test_readers.py#L72

without the need for a DBC file.

[AlexMicWalz]

I have fixed the findings of flake8.
Unit-test is in the making.

[erikbosch]

We are migrating CAN Provider (dbcfeeder) to a new repo. Discussion may continue in this PR, but there is no point in updating this PR, if so rather create a new PR in the new repository.

[SebastianSchildt]

@erikbosch what is the state of this, would be a pity to let it rot. Could you or maybe the @andlaus make an effort to create move the PR to the new location? I feel it was already (almost?) good enough content wise

[andlaus]

as far as I remember the main showstopper here was the fact that there was no unit test. I promised one, but when I did so, I assumed that there was a unit test for the (non-container aware) DBC feeder. seems like this assumption was not correct and after bouncing my head against making one from scratch I gave up. (the problem is how to check that a given CAN signal gets exposed on the Kuksa side without having to set up a metric ton of servers, databases, etc.)

I came to the conclusion that it is best to tackle the implementation of end to end unit tests for the feeders in another PR sometime in the future. (I'll go over this PR to make it merge-ready tomorrow or early next week.)

[andlaus]

transferred the PR to the new repository: eclipse-kuksa/kuksa-can-provider#6

this PR can be closed AFAICS. (I don't have permission to do so...)