Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for COMPARAM-SUBSET #92

Merged
merged 12 commits into from
Nov 16, 2022

Conversation

kayoub5
Copy link
Collaborator

@kayoub5 kayoub5 commented Nov 13, 2022

No description provided.

@kayoub5 kayoub5 requested a review from andlaus November 13, 2022 12:48
@kayoub5
Copy link
Collaborator Author

kayoub5 commented Nov 13, 2022

@andlaus I can add a test by using the sample PDX from https://github.com/sebastianwilczek/CANoe-Configurations/tree/master/Ethernet/Diagnostics/EthernetDiagnostics/ODX

Creating a full com param configuration using python sounds like an overkill.

@andlaus
Copy link
Collaborator

andlaus commented Nov 15, 2022

@andlaus I can add a test by using the sample PDX from https://github.com/sebastianwilczek/CANoe-Configurations/tree/master/Ethernet/Diagnostics/EthernetDiagnostics/ODX

if @sebastianwilczek can confirm that the file can be distributed under the MIT license, yes. If we can't get that affirmation, a second repository (odxtools-additional-tests?) could be created for that purpose. (this repo must not be affiliated with mercedes-benz, though.)

Creating a full com param configuration using python sounds like an overkill.

I agree. at least in the short and medium term ;)

instead of defining them from scratch, we simply read them from the
.odx-cs fragments shipped with the ODX standard.

Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io>
Signed-off-by: Gerrit Ecke <gerrit.ecke@mbition.io>
Signed-off-by: Alexander Walz <alexander.walz@mbition.io>
@sebastianwilczek
Copy link

Hey everyone, not sure about the context here, but the linked repository is only a modified version of the configuration files used by CANoe, developed and owned by Vector Informatik GmbH: https://www.vector.com/int/en/download/?tx_vecdownload_downloaddetail%5Bdownload%5D=92521

As far as I can tell, they do not list any form of license (in context of MIT or otherwise), so unfortunately I can not tell you whether you may use any of the files in your processes.

@andlaus
Copy link
Collaborator

andlaus commented Nov 15, 2022

I tested this PR with our internal files written by candela studio, and it seems that after this PR, the send and receive IDs (ecu.get_send_id() and ecu.get_receive_id()) are swapped...

@andlaus
Copy link
Collaborator

andlaus commented Nov 15, 2022

@sebastianwilczek: okay, I suppose we should ask vector then. I'll keep you updated.

andlaus and others added 2 commits November 15, 2022 16:18
…e is defined

... some of the comparam fragments shipped with the ODX standard only
specify a unit without any scaling. Assuming that these files are
correct, we should not complain about this either?

Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io>
Signed-off-by: Gerrit Ecke <gerrit.ecke@mbition.io>
Signed-off-by: Alexander Walz <alexander.walz@mbition.io>
odxtools/diaglayer.py Outdated Show resolved Hide resolved
odxtools/diaglayer.py Outdated Show resolved Hide resolved
@kayoub5
Copy link
Collaborator Author

kayoub5 commented Nov 15, 2022

I tested this PR with our internal files written by candela studio, and it seems that after this PR, the send and receive IDs (ecu.get_send_id() and ecu.get_receive_id()) are swapped...

should be correct now

@kayoub5
Copy link
Collaborator Author

kayoub5 commented Nov 15, 2022

@andlaus I think you forgot to commit the ISO_11898_2_DWCAN.odx-cs file, among others

thanks to [at]kayoub5 for the catch.

Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io>
Signed-off-by: Gerrit Ecke <gerrit.ecke@mbition.io>
Signed-off-by: Alexander Walz <alexander.walz@mbition.io>
@kayoub5
Copy link
Collaborator Author

kayoub5 commented Nov 15, 2022

@andlaus FYI, you push directly into a fork if the author enabled it (checked "Allow edits by maintainers")

So feel free to push changes directly to my branch.

@andlaus
Copy link
Collaborator

andlaus commented Nov 15, 2022

@andlaus I think you forgot to commit the ISO_11898_2_DWCAN.odx-cs file, among others

oops. I had some unpacked somersault.pdx file lying around in the directory where I tested this, i.e. I loaded these fragments from the incorrect location. should be fixed now: kayoub5#3

@andlaus
Copy link
Collaborator

andlaus commented Nov 15, 2022

So feel free to push changes directly to my branch.

thanks for the offer, but I think it is less confusing if I don't do this (I always get slightly dizzy if some of my PRs change without my intervention...)

@andlaus
Copy link
Collaborator

andlaus commented Nov 15, 2022

btw, I now get the following warning:

/home/and/src/odxtools/odxtools/diaglayer.py:453: OdxWarning: Communication parameter `CP_UniqueRespIdTable` specified more than once. Using first occurence.

I assume that this is caused by the fact that the ECU can be interacted with using multiple protocols (DoIP and CAN). I guess that fixing this requires a full understanding of the protocol concept and should thus be left for another day?

@andlaus
Copy link
Collaborator

andlaus commented Nov 15, 2022

ok, I've tested this with our internal files, and it seems to work without the enable_candela_workarounds hack. can you remove all traces of this blemish?

@kayoub5
Copy link
Collaborator Author

kayoub5 commented Nov 15, 2022

ok, I've tested this with our internal files, and it seems to work without the enable_candela_workarounds hack. can you remove all traces of this blemish?

We can do that in a different PR, this one is too loaded as it is

odxtools/units.py Outdated Show resolved Hide resolved
odxtools/units.py Outdated Show resolved Hide resolved
@kayoub5
Copy link
Collaborator Author

kayoub5 commented Nov 15, 2022

btw, I now get the following warning:

/home/and/src/odxtools/odxtools/diaglayer.py:453: OdxWarning: Communication parameter `CP_UniqueRespIdTable` specified more than once. Using first occurence.

I assume that this is caused by the fact that the ECU can be interacted with using multiple protocols (DoIP and CAN). I guess that fixing this requires a full understanding of the protocol concept and should thus be left for another day?

You can cross check this by checking that the different com params point to different proto stacks

It could even be different CAN buses.

@kayoub5
Copy link
Collaborator Author

kayoub5 commented Nov 16, 2022

@andlaus anything blocks merging this?

@andlaus
Copy link
Collaborator

andlaus commented Nov 16, 2022

@andlaus anything blocks merging this?

no. nice work, thanks!

@andlaus andlaus merged commit a4f7dc5 into mercedes-benz:main Nov 16, 2022
@kayoub5 kayoub5 deleted the feature/comparam_subset branch November 16, 2022 10:16
andlaus added a commit to andlaus/odxtools that referenced this pull request Nov 17, 2022
The reason for these were that CANdela uses different communication
parameter specifications than the ones specified by the `odx-cs` files
that are shipped with the ODX standard. This resulted in a different
ordering for some of the complex communication parameters (albeit with
all required fields being present). Fortunately, with mercedes-benz#92 merged, this
became unnecessary.

Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io>
Signed-off-by: Alexander Walz <alexander.walz@mbition.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants