incorrect handling of ref with target in multiple odx files

[HerrMedizins]

The inheritance of the request in ODX2 works fine. But the DOP beeing refered to in the request in ODX2 exists in both ODX files in different versions. From my understanding, and the way other odx-interpreting tools handle this situation is, that the DOP of ODX1 is used, and the DOP of ODX2 is ignored/overwritten.
Odxtools though takes the DOP from ODX2 and ignores/overwrites the DOP from ODX1, which leads to the data of the DOP in ODX1 beeing lost.

[andlaus]

Odxtools though takes the DOP from ODX2 and ignores/overwrites the DOP from ODX1, which leads to the data of the DOP in ODX1 beeing lost.

I'm not sure whether I understand your issue correctly: for ECUs of ODX2 which specify a DOP with the same name, overriding the one from ODX1 it is correct as far as I know. The original DOP still ought to be exposed in ECUs which don't specify a new one and the DOP from ODX1 can still be referenced via ODXLINK by any ECU (i.e., even the ECUs which override it), but if the ECU does not reside in ODX1, the DOCREF="ODX1" attribute needs to be specified by the DOP-REF.

But maybe I misunderstood and you mean to "extend" the data of the DOP from ODX1 with the data specified by ODX2? As far as I know, the ODX standard does not allow this, as this would easily lead to breaking the data model as some tags are mutually exclusive and there is no way of "deleting" an arbitrary inherited one.

[HerrMedizins]

I'll try another approach to explain my problem.
All ODXs im mentioning belong to the same ECU. ODX1 inherits from another ODX which in turn inherits from ODX2. So what happens is, that everything from ODX2 which is the parent ODX in this case ,is inherited, and then overwritten with the parts existing in ODX1, as expected. If i now with python parse in the pdx with all the ODXs i get one ECU Object as expected. If i now look into the data_dictionary_spec -> data_object_props i can find the DOP DOP_REF (from images) filled correctly with the data from ODX1. So far all works as expected.
Now if i got into the services -> DiagServiceXY ->parameterXY (where DiagService and Parameter are the ones visible in the ODX2 picture). The dop Object of that parameter is the DOP from ODX2, and not the DOP that ECU contains in its own data_dictionary_spec.
Problem with this is that if i try to get the valid_physical_values, i get all values from the parent (base) ODX, missing the "correct" values that are contained in ODX1, that are already correctly stored in the ECU's data_dictionary_spec.

So for me what makes sense is that the refernce in the parameter of the request should point to the DOP Object in the ECU's data_dictionary_spec, and not to the DOP of the parent Ref, whereever that is stored.

I hope that makes my problem any clearer

[andlaus]

so, I guess you have something like this

• a document fragment BASE ("ODX2") contains service Foo which uses a DOP named bar
• a document fragment DERIVED ("ODX1") inherits from BASE and also defines a DOP named bar
  • the inherited service Foo in DERIVED still uses the "bar" DOP from BASE

If this is the case, it is the correct behavior as far as I can see: if DERIVED defines objects with the same ID and short name as BASE, the references in BASE are not magically replaced by the ones from DERIVED. This is because the ODXLINK mechanism also considers the document fragment of the referenced objects, i.e., if no DOCREF attribute is specified in the respective -REF tag, it is implicitly assumed that the referenced objects resides in the same document fragment as the referencing one. (this allows to specify objects with identical short names and IDs in different document fragments, thus simplifying copy-and-paste jobs.)

To my knowledge, the only mechanism which -- in principle -- works like you seem expect are short name references: to resolve these, the full diagnostic layer needs to be used, so if DERIVED overrides an object from BASE with the same short name, the objects in BASE should -- in principle -- use the object from DERIVED. Note though, that odxtools does not support this because it would require full copies of all objects from the base layers to any derived layers and would this increase the memory consumption by a significant factor (and I regularly have to deal with files that gobble up several Gigabytes of RAM with the current version of odxtools.)

That said, do any of the commercial tools behave as you describe?

[HerrMedizins]

Indeed i have to exact scenario you described where the Objects in BASE reference the DOP through a short name reference.
So if i understand correctly, -- in principle -- the Objects in BASE should reference the DOP of DERIVED (where the DOP is referenced through a short name reference), but at least as of right know odxtools does not support that.

Other commercial tools, as well as the "odx commander" VS Code extension behave the way expected.
I could offer to schedule a teams meeting to show you other tools working with that specific odxdata.

[andlaus]

So if i understand correctly, -- in principle -- the Objects in BASE should reference the DOP of DERIVED (where the DOP is referenced through a short name reference), but at least as of right know odxtools does not support that.

the problem with this is that one and the same object can behave completely differently depending on which diagnostic layer it is part of. you can work around this like this, though:

import odxtools
from copy import deepcopy
db = odxtools.load_file("my_pdx.pdx")
ecu = deepcopy(db.ecus.MyECU)
ecu.diag_layer_raw._resolve_snrefs(ecu)

(if you do this for each and every diagnostic layer of very large files, you will quickly find out that 256 GB of RAM is not that much...)

[HerrMedizins]

Those lines of code just like that had no effect. (Requests of DERIVED still have DOP of BASE).
But after additionally adding:

if self.request:
      self.request._resolve_snrefs(diag_layer)

to the _resolve_snrefs() function in diagservice.py, all snrefs seem to be correctly resolved.

That beeing said, the code from your comment is not needed if,

1. the finallize_init() function in database.py ->refresh()
   would be called in order of heritance from e.g. from BASE to DERIVED. This shouldn't change anything to worse with having multiple ecus, but for the scenario where there is only one ecu, it removes the necessity of the code in your comment, to correctly resolve the snrefs.
   or
2. in load_pdx_file.py
   adding:

for ecu in container.ecus:
        ecu.diag_layer_raw._resolve_snrefs(ecu)

again, for pdxs with a single ecu this should do the trick, for pdxs with multiple ecus, i don't think that'll help. For that case 1. might be a better approach.

[andlaus]

Those lines of code just like that had no effect. (Requests of DERIVED still have DOP of BASE).

hm, weird. maybe I misunderstood how deepcopy() works...

would be called in order of heritance from e.g. from BASE to DERIVED.

even if this would solve the problem in your specific case, the short-name references for ECUs accessed via BASE would be wrong because they would point to the ones defined in DERIVED. (also, if there are multiple derived ECUs, the SNREFs would be incorrect for all but one of them...)

again, for pdxs with a single ecu this should do the trick, for pdxs with multiple ecus, i don't think that'll help. For that case 1. might be a better approach.

Considering the fact that such short-name references must be resolved differently depending on the ECU via they are accessed, I think the only somewhat practical "comprehensive" solution I can come up with is to copy the "full" ECU before resolving the references. That's because if this is not done, resolving a SNREF in a different context will override the resolution stemming from the previous context.

The only "correct but impractical" alternative which I can see, is passing the ECU for every property that can be referenced via SNREFs, i.e.,

dop = param.dop

would become something like

dop = param.get_dop(ecu)

IMO this is too clunky, and also pretty slow because the short-name resolution would need to be done every single time the property is accessed...

[andlaus]

hm, weird. maybe I misunderstood how deepcopy() works...

I came up why it does not work: the problem is that deepcopy() replaces all referenced python objects by copies, i.e., the changes to a given object don't propagate to the same object in a different contexts anymore: e.g., changing the request objects in the diaglayer will not have any effect on the request objects of services. to get around this, the deepcopy () of the ECU can be done after updating the short name references:

import odxtools
from copy import deepcopy
db = odxtools.load_file("my_pdx.pdx")

base_ecu = db.ecus.MyBaseECU
base_ecu.diag_layer_raw._resolve_snrefs(base_ecu)
base_ecu = deepcopy(base_ecu)

derived_ecu = db.ecus.MyDerivedECU
derived_ecu.diag_layer_raw._resolve_snrefs(derived_ecu)
derived_ecu = deepcopy(derived_ecu)

[HerrMedizins]

Im not sure if we understood each other correctly, so just to be clear.
BOTH solutions of yours
1.

import odxtools
from copy import deepcopy
db = odxtools.load_file("my_pdx.pdx")
ecu = deepcopy(db.ecus.MyECU)
ecu.diag_layer_raw._resolve_snrefs(ecu)

and 2.

import odxtools
from copy import deepcopy
db = odxtools.load_file("my_pdx.pdx")

base_ecu = db.ecus.MyBaseECU
base_ecu.diag_layer_raw._resolve_snrefs(base_ecu)
base_ecu = deepcopy(base_ecu)

derived_ecu = db.ecus.MyDerivedECU
derived_ecu.diag_layer_raw._resolve_snrefs(derived_ecu)
derived_ecu = deepcopy(derived_ecu)

DON'T work themselves. Just resolving the snref like that, either before of after copying has NO effect.

At least not until adding

if self.request:
      self.request._resolve_snrefs(diag_layer)

to the _resolve_snrefs() function in your Diagservice.py
After adding those lines of code into your diagservice.py, BOTH of your solutions work, so atleast in my case it doesn't matter whether to copy the ecu before or after resolving the snrefs.

Again, i can only talk about the case of a single ECU in a pdx file. Still ,even with a single ECU the additional lines of code in the Diagservice.py are NECCESARY to see any changes to the resolving of snrefs. (In my case now seemingly correctly resolving them)

[andlaus]

At least not until adding to the _resolve_snrefs() function in your Diagservice.py

okay, probably the SNREFs in the base variant need to be determined using the ECU. try this:

import odxtools
from copy import deepcopy

db = odxtools.load_file("my_pdx.pdx")

base_variant = db.diag_layers.MyBaseVariant
ecu = db.ecus.MyECU

base_variant.diag_layer_raw._resolve_snrefs(ecu)
ecu.diag_layer_raw._resolve_snrefs(ecu)

ecu = deepcopy(ecu)

Alternatively the issue could be caused if there are at least two services which use the same request object and resolving the SNREFs is dependent on the service object and the service which you are interested in requires to use the result of the first service, and finally there might be an obscure bug in the reference resolution code, but I can't think of a way of finding the exact reason without having access to some file where this happens...

Again, i can only talk about the case of a single ECU in a pdx file. Still ,even with a single ECU the additional lines of code in the Diagservice.py are NECCESARY to see any changes to the resolving of snrefs. (In my case now seemingly correctly resolving them)

the problem is that your solution likely is not "more correct" than the current state; it is just different, but the different version is what fits you needs. (for others this might be the other way round...)

[HerrMedizins]

Again the code you posted has no effect. (Parameter of Request of DERIVED still have DOP of BASE)
That beeing said, adding the 2 lines of Code in the Diagservice.py, makes it work again.
So same scenario as before. Without the adjustment in Diagservice.py, nothing works.
With the adjustment in Diagservice.py Each of your 3 Solutions works.

Alternatively the issue could be caused if there are at least two services which use the same request object <

I'm not 100% sure, but i think this is not the cause in my pdx. Im going to check it again just to be sure.

and finally there might be an obscure bug in the reference resolution code, but I can't think of a way of finding the exact reason without having access to some file where this happens...<

I'm not able to provide you the pdx i have. I can only offer to schedule a Teams meeting, to show you the pdx, maybe you can build a dummy one with the same structure to reproduce it then.

[andlaus]

Again the code you posted has no effect.

can you verify the following:

• the request used by the ECU is inherited from the base variant, not e.g. a protocol
• the request is only used by the service which you're interested in, not others
• the DOP is referenced by via a SNREF
• the DOP is explicitly mentioned by the NOT-INHERITED-DOPS of the ECU diag layer's PARENT-REF. (if this is not the case, the current SNREF resolution mechanism does not work properly, and also it constitutes an violation of the ODX specification because short names must be unique within their respective domain.)

That beeing said, adding the 2 lines of Code in the Diagservice.py, makes it work again.

yes, but that is conceptually incorrect: the "owner" of all requests is the diagnostic layer (i.e., protocol, Base variant, ECU, ...), not the service objects. For this reason reference resolution must be triggered from the diagnostic layer or fun things like infinite recursions may happen quite quickly.

[HerrMedizins]

the request used by the ECU is inherited from the base variant, not e.g. a protocol

The Request-Ref existing in DERIVED points to the Request in: DIAG_LAYER_CONTAINER -> PROTOCOL->REQUESTS . so im guessing NO

the request is only used by the service which you're interested in, not others

YES

the DOP is referenced by via a SNREF

YES

he DOP is explicitly mentioned by the NOT-INHERITED-DOPS of the ECU diag layer's PARENT-REF.

NO

[andlaus]

The Request-Ref existing in DERIVED points to the Request in: DIAG_LAYER_CONTAINER -> PROTOCOL->REQUESTS . so im guessing NO

okay, that explains it. the code above does not work because the request is defined by the protocol instead of the base variant. (IMO it is a bit questionable to do this from the logical point of view, BTW.) Anyway, if the request is defined in the protocol, this should work:

import odxtools
from copy import deepcopy

db = odxtools.load_file("my_pdx.pdx")

protocol = db.diag_layers.MyProtocol
ecu = db.ecus.MyECU

protocol.diag_layer_raw._resolve_snrefs(ecu)
ecu.diag_layer_raw._resolve_snrefs(ecu)

ecu = deepcopy(ecu)

If this still does not work, try calling .diag_layer_raw._resolve_snrefs(ecu) for every single diagnostic layer the ECU inherits from before doing the deepcopy().

[HerrMedizins]

Now this code itself works.
I'm assuming the order in which im calling the resolve_snrefs function on all diagnostic layers does not matter right?

Edit:
Now the Requests that were not correctly resolved are working and beeing correctly resolved. In turn as it seems now other request are no longer correctly resolved. or better said their parameters have a dopsnref, but their dop Object is None....

Edit2: and loading in a different pdx (that has been working before) throws an exception

Traceback (most recent call last):
File "/home/pi/Odxparser/main.py", line 614, in
diaglayer.diag_layer_raw._resolve_snrefs(i)
File "/home/pi/.local/lib/python3.9/site-packages/odxtools/diaglayerraw.py", line 277, in _resolve_snrefs
obj._resolve_snrefs(diag_layer)
File "/home/pi/.local/lib/python3.9/site-packages/odxtools/diagdatadictionaryspec.py", line 171, in _resolve_snrefs
obj._resolve_snrefs(diag_layer)
File "/home/pi/.local/lib/python3.9/site-packages/odxtools/basicstructure.py", line 293, in _resolve_snrefs
p._resolve_snrefs(diag_layer)
File "/home/pi/.local/lib/python3.9/site-packages/odxtools/parameters/valueparameter.py", line 40, in _resolve_snrefs
dop = odxrequire(self.dop)
File "/home/pi/.local/lib/python3.9/site-packages/odxtools/exceptions.py", line 76, in odxrequire
odxraise(message)
File "/home/pi/.local/lib/python3.9/site-packages/odxtools/exceptions.py", line 39, in odxraise
raise error_type()
odxtools.exceptions.OdxError

Edit3: With the pdx that atleast is parseable with odxtools, the problem now is that a parameter of a request in the PROTOCOL has a dop_snref. But now since im resolving the Protocols snrefs with the ECU, that as it seems did not inherit any structures from the Protocol, the parameter is not able to resolve its snref, because its only checking for that snref in the DiagLayer. In that Case if the DOP beeing referenced does not exist in the DiagLayer im resolving with, what DOP should it take. Then just take the DOP from the source DiagLayer?

My Temporary fix, so that im able to atlease parse all ODXs, and that resolves some Issues with the Problem Mentioned in Edit3 is, replacing following code in parameterwithdop.py _resolve_snrefs() function

self._dop = (
                    spec.data_object_props.get(self.dop_snref) or spec.structures.get(self.dop_snref))

with

if spec.data_object_props.get(self.dop_snref) or spec.structures.get(self.dop_snref):
                self._dop = (
                    spec.data_object_props.get(self.dop_snref) or spec.structures.get(self.dop_snref))

Since the Problem was that the correct dop that has been resolved by the protocol, with itself as diaglayer, was overwritten with None by resolving snrefs of protocol with the ECU diag layer (cause the dop only exists in the protocol and is not inherited by the ECU), i now only update the DOP via resolve_snrefs if the function can successfully resolve a Dop.
I know that this is probably not correct behaviour and lead to different problems. But not beeing able to resolve a snref locally, shouldn't just overwrite it with None either i asume, and insted try to resolve it in its source Container, or in order of inheritance or something. At least that's what i would guess.

[andlaus]

I'm assuming the order in which im calling the resolve_snrefs function on all diagnostic layers does not matter right?

I think that's correct: As far as I can see you "only" need to call _resolve_snrefs() for every diagnostic layer which is part of the inheritance hierarchy of the one which you're interested in...

their dop Object is None....

this is probably a known incompleteness of the current implementation: the DOP is almost certainly not yet implemented, for example STATIC-FIELD. Patches welcome ;)

But now since im resolving the Protocols snrefs with the ECU, that as it seems did not inherit any structures from the Protocol, the parameter is not able to resolve its snref, because its only checking for that snref in the DiagLayer.

this might be a bug in odxtools, structures should be inherited

My Temporary fix, so that im able to atlease parse all ODXs, and that resolves some Issues with the Problem Mentioned in Edit3 is, replacing following code in parameterwithdop.py _resolve_snrefs() function

looks like you're not using the latest git version of odxtools: @kayoub5 recently extended this short name lookup considerably. I'll make a release, so this fix is available via pypi

Since the Problem was that the correct dop that has been resolved by the protocol, with itself as diaglayer, was overwritten with None by resolving snrefs of protocol with the ECU diag layer (cause the dop only exists in the protocol and is not inherited by the ECU),

I wonder why they are not inherited in your case. the code seems to suggest that they are. Would be nice if you had a look...

[andlaus]

(cause the dop only exists in the protocol and is not inherited by the ECU)

how comes? is it explicitly excluded by some NOT-INHERITED-$FOO tag in a PARENT-REF?

[HerrMedizins]

I think that's correct: As far as I can see you "only" need to call _resolve_snrefs() for every diagnostic layer which is part of the inheritance hierarchy of the one which you're interested in...

Sounds good to me

this is probably a known incompleteness of the current implementation: the DOP is almost certainly not yet implemented, for example STATIC-FIELD. Patches welcome ;)

Yes and no, i have some parameters where that is the case, and some parameters where the DOP is a Structure, where the Problem might be:

this might be a bug in odxtools, structures [should be inherited]

---

looks like you're not using the latest git version of odxtools: @kayoub5 recently extended this [short name lookup] considerably. I'll make a release, so this fix is available via pypi

Going through the Codebase yesterday i noticed, thanks for the new Tag :)

I wonder why they are not inherited in your case. the code seems to suggest that they are. Would be nice if you had a look...

how comes? is it explicitly excluded by some NOT-INHERITED-$FOO tag in a PARENT-REF?

I will be checking to see if i find any reason for the structures not beeing inherited and give out an update asap

EDIT 1: Im not entirely sure why, but now with the 5.3.1 instead of the 5.3.0 my Ecu object correctly inherited all structrure Objects from BASE

[HerrMedizins]

So, with Odxtools version 5.3.1 the Structure objects are correctly and completly inherited now.
to correctly resolve snrefs i use

for diaglayer in db.diag_layers:        
        diaglayer.diag_layer_raw._resolve_snrefs(ecu)

For 2 of 3 pdx files this works, and as far as i can see snrefs are correctly resolved.
But for the last pdx again i get the Exception

Traceback (most recent call last):
File "/home/pi/Odxparser/main.py", line 682, in
diaglayer.diag_layer_raw._resolve_snrefs(i)
File "/home/pi/.local/lib/python3.9/site-packages/odxtools/diaglayerraw.py", line 277, in _resolve_snrefs
obj._resolve_snrefs(diag_layer)
File "/home/pi/.local/lib/python3.9/site-packages/odxtools/diagdatadictionaryspec.py", line 181, in _resolve_snrefs
obj._resolve_snrefs(diag_layer)
File "/home/pi/.local/lib/python3.9/site-packages/odxtools/basicstructure.py", line 293, in _resolve_snrefs
p._resolve_snrefs(diag_layer)
File "/home/pi/.local/lib/python3.9/site-packages/odxtools/parameters/valueparameter.py", line 40, in _resolve_snrefs
dop = odxrequire(self.dop)
File "/home/pi/.local/lib/python3.9/site-packages/odxtools/exceptions.py", line 76, in odxrequire
odxraise(message)
File "/home/pi/.local/lib/python3.9/site-packages/odxtools/exceptions.py", line 39, in odxraise
raise error_type()
odxtools.exceptions.OdxError

Which again can be fixed by adding the if statement to resolve_snrefs in parameterwithdop, to only update the DOP object if it is able to resolve a DOP. (Correct behaviour?)

[andlaus]

Which again can be fixed by adding the if statement to resolve_snrefs in parameterwithdop, to only update the DOP object if it is able to resolve a DOP.

To me, this smells a bit like an incorrectness in that PDX file, namely trying to reference an object via SNREF which does not exist in a given context. if you do not really need the service which uses that DOP, you can also try to use odxtools' non-strict mode.

(Correct behaviour?)

I believe that both, the current behaviour of odxtools and the behaviour discussed above can be seen as conforming to the words of the ODX specification. (Whether these are "correct" is a different discussion IMO, in particular in the light all the semantic weaknesses of short-name references which lead to the discussion here...)

[HerrMedizins]

To me, this smells a bit like an incorrectness in that PDX file, namely trying to reference an object via SNREF which does not exist in a given context. if you do not really need the service which uses that DOP, you can also try to use odxtools' non-strict mode.

Since i need that service, non-strict mode won't be an option. About the incorecctness of the pxd file i myself don't know pdxs well enough to judge whether that is the case or not. I can only reference to the VS Code extension: ODX Commander, and Vectors ODX Studio, which both are able to resolve all references.

I believe that both, the current behaviour of odxtools and the behaviour discussed above can be seen as conforming to the words of the ODX specification. (Whether these are "correct" is a different discussion IMO, in particular in the light all the semantic weaknesses of short-name references which lead to the discussion here...)

What a great specification...
I got multiple smaller errors, i'll check if the root of them are Short-Name-References as well. If not i'll open new issues for them.
Regarding Short-Name-References, for now, i'll just go with the behaviour discussed above and see where that leads me.

Thanks for all the help and explainations you provided. :)

[HispidaBytes]

so, I guess you have something like this

* a document fragment BASE ("ODX2") contains service `Foo` which uses a DOP named `bar`

* a document fragment DERIVED ("ODX1")  inherits from BASE and also defines a DOP named `bar`
  
  * the inherited service `Foo` in DERIVED still uses the "bar" DOP from BASE

If this is the case, it is the correct behavior as far as I can see

Hi andlaus this is not the correct behaviour.
The ODX standard specifically says that SNREF should be resolved by taking consideration of overriden objects.
Please have a look at figure 32 of the ISO22901 standard.

So to recap:

• a document fragment BASE ("ODX2") contains:
  • service Foo that has a SNREF to a DOP named bar
  • a DOP named bar
• a document fragment DERIVED ("ODX1") inherits from BASE and contains:
  • a DOP named bar

The correct behaviour is that if I “look” from the LAYER "ODX1" I see the inherited service Foo but this one must resolve the DOP-SNREF with the DOP bar from ODX1!

[andlaus]

that's possible. that said, I consider this to be pretty much an edge case and it is next to impossible to implement that behavior without falling back to either extensive copying or using "lazy" SHORT-NAME lookups.

For now, if you have a case like this, you can get around this using the snippet above:

ecu = db.diag_layers.MyNiceEcu
for diaglayer in db.diag_layers:
        diaglayer.diag_layer_raw._resolve_snrefs(ecu)

(if you need more than one ECU object, you need to load a fresh database for each of them, though.)

On the other hand, maybe there is a simple solution for this issue which I haven't seen yet. patches welcome.

[andlaus]

I came to the conclusion that the only way to implement calling context dependent SNREF object resolution properly ahead of time (i.e., not twisting the whole database towards a single diaglayer) is to implement a custom version of deepcopy() which copies all objects that exhibit SNREFs or which contain subobjects with SNREFs. That said, even if this can be implemented in a not too hacky way, I don't think that it would be a good idea to go this route, because I'm pretty sure that a lot of people who use machines not featuring obscene amounts of RAM would run out of memory pretty quickly if this was implemented...