Wrong parameter handling with multiple odx files

[mafei1982]

I have a pdx file which contains multiple odx files. One of them define a ecu shared data which contains some diag services, and one of them define a basevariant which only contains some definitions of tables/table rows. in ecu shared data file, there is a param definition like below:

<PARAM ID="DLC.ES_UDS_Services.ESD.ES_UDS_Services.DOP.STRUCT_Service86_03_ServiceToRespondToRecord.PA.ServiceToRespondToDID"
                  xsi:type="TABLE-KEY">
                  <SHORT-NAME>ServiceToRespondToDID</SHORT-NAME>
                  <LONG-NAME>ServiceToRespondToDID</LONG-NAME>
                  <TABLE-SNREF SHORT-NAME="TABLE_Service22_DataID"/>
</PARAM>

and in this file, there is a dummy table which short name is "TABLE_Service22_DataID":

<TABLE ID="id-59bd0afb-b95a-4e18-acca-162f2ea460cb" OID="59bd0afb-b95a-4e18-acca-162f2ea460cb" SEMANTIC="DATA-READ">
              <SHORT-NAME>TABLE_Service22_DataID</SHORT-NAME>
              <LONG-NAME>TABLE Service22 DataID</LONG-NAME>
              <KEY-DOP-REF ID-REF="id-206979a7-32f8-412c-b2f8-93f8bfb74db6"/>
              <TABLE-ROW ID="id-631ab6af-fd93-43d1-911b-a3b02d50db7e" OID="13fe66c9-9f6a-4f78-8bde-fba011a51cb7">
                <SHORT-NAME>Dummy_Row</SHORT-NAME>
                <LONG-NAME>Dummy Row</LONG-NAME>
                <KEY>0</KEY>
              </TABLE-ROW>
            </TABLE>

with odxtools, this param would be parsed as a tablekey with a dummy table row in ecu shared data layer.
But in the file which contains basevariant, there is a new definition of table "TABLE_Service22_DataID" which contains real table rows. But with odxtools, the related param of base variant object is still using the dummy table.

I checked the code, and found that when resolving objects from parent, the odxtools will just get the objects from parent and will not do resolve snrefs for those objects. And as those inherited objects are the same objects as the ones of parent, doing resolve snrefs based on current context (with base variant) will also modify the objects of the parent, which shall be a new issue.

I have a workaround for it: call _resolve_snrefs on service before trying to read the details of its content based on current variant. But I still get some issues since some _resolve_snrefs lacks of resolving its children's snrefs like: BaseStructure, DiagService. So I made some code change on those files. For example, in DiagService:

def _resolve_snrefs(self, context: SnRefContext) -> None:
    context.diag_service = self

    super()._resolve_snrefs(context)

    for cpr in self.comparam_refs:
        cpr._resolve_snrefs(context)

    # The named item list of communication parameters is created
    # here because ComparamInstance.short_name is only valid after
    # reference resolution
    self._comparams = NamedItemList(self.comparam_refs)
    if self._request:
        self._request._resolve_snrefs(context)
    for response in self._positive_responses:
        response._resolve_snrefs(context)
    for response in self._negative_responses:
        response._resolve_snrefs(context)
    context.diag_service = None

With this workaround, I can get the correct result. But can I get an official fix for it or is it a known issue?

[cmydd]

I found the same issue

EV_BatteChargVaria1.odx
DIAG-SERVICE ADDRESSING="FUNCTIONAL-OR-PHYSICAL" ID="DiagnServi_ReadDataByIdentECUIdent" SEMANTIC="IDENTIFICATION">
SHORT-NAME>DiagnServi_ReadDataByIdentECUIdent/SHORT-NAME>
LONG-NAME TI="SER00010">Read Data By Identifier / ECU Identification/LONG-NAME>
REQUEST-REF ID-REF="Req_ReadDataByIdentECUIdent" DOCREF="PR_UDSOnCAN" DOCTYPE="LAYER"
DIAG-SERVICE>

PR_UDSOnCAN.odx
DIAG-SERVICE ADDRESSING="FUNCTIONAL-OR-PHYSICAL" ID="DiagnServi_ReadDataByIdentECUIdent" SEMANTIC="IDENTIFICATION">
SHORT-NAME>DiagnServi_ReadDataByIdentECUIdent/SHORT-NAME>
REQUEST-REF ID-REF="Req_ReadDataByIdentECUIdent"
POS-RESPONSE-REFS>
POS-RESPONSE-REF ID-REF="Resp_ReadDataByIdentECUIdent"
DIAG-SERVICE>

REQUEST ID="Req_ReadDataByIdentECUIdent">
SHORT-NAME>Req_ReadDataByIdentECUIdent/SHORT-NAME>
LONG-NAME>Read Data By Identifier / ECU Identification/LONG-NAME>
PARAMS>
PARAM xsi:type="VALUE" SEMANTIC="DATA-ID">
SHORT-NAME>Param_RecorDataIdent/SHORT-NAME>
LONG-NAME>Record Data Identifier/LONG-NAME>
BYTE-POSITION>1/BYTE-POSITION>
DOP-SNREF SHORT-NAME="DOP_TEXTTABLERecorDataIdentECUIdent"
PARAM>
PARAMS>
REQUEST>

get the EV_BatteChargVaria1.Req_ReadDataByIdentECUIdent.DOP_TEXTTABLERecorDataIdentECUIdent is PR_UDSOnCAN.Req_ReadDataByIdentECUIdent.DOP_TEXTTABLERecorDataIdentECUIdent

if EV_BatteChargVaria1 have DOP_TEXTTABLERecorDataIdentECUIdent, It should be called his own DOP_TEXTTABLERecorDataIdentECUIdent, not the PR_UDSOnCAN DOP_TEXTTABLERecorDataIdentECUIdent
Because is DOP-SNREF

[andlaus]

this sounds awfully like #200: the problem seems to be that SNREFs depend on the context which they were called from, not just the object which contains them. As a workaround you can re-target the snrefs of your whole database to the diag layer which you are interested in:

ecu = db.diag_layers.MyNiceEcu
context = SnRefContext(database=db, diag_layer=ecu)
for diaglayer in db.diag_layers:
        diaglayer._resolve_snrefs(context)

There is no good solution to this issue: In the worst case the whole database needs to be copied for each and every ECU since every ECU might resolve its SNREFs differently even if these references are in inherited objects. (assuming that loading your database currently requires about 1000MB of RAM and that it contains 50 diag layers, you would need about 50GB of RAM just to get around this issue...)

[mafei1982]

this sounds awfully like #200: the problem seems to be that SNREFs depend on the context which they were called from, not just the object which contains them. As a workaround you can re-target the snrefs of your whole database to the diag layer which you are interested in:

ecu = db.diag_layers.MyNiceEcu
context = SnRefContext(database=db, diag_layer=ecu)
for diaglayer in db.diag_layers:
        diaglayer._resolve_snrefs(context)

There is no good solution to this issue: In the worst case the whole database needs to be copied for each and every ECU since every ECU might resolve its SNREFs differently even if these references are in inherited objects. (assuming that loading your database currently requires about 1000MB of RAM and that it contains 50 diag layers, you would need about 50GB of RAM just to get around this issue...)

Yeah, it is a similar issue. But without adding code to resolve snrefs for child items like parameters of base structure, resolving snrefs based on current context will not work for those child items.
So I think the conclusion now is that, we need to resolve snrefs for each diag layer before we using it.

[andlaus]

[...] we need to resolve snrefs for each diag layer before we using it.

yes, that is the ambiguity of the standard I've mentioned above: In the standard it is not explicitly stated anywhere that all references must be resolvable, but reading the document I get the impression that this is heavily implied. Other implementations (softing's?) might have come to different conclusions.

Anyway if you have a not too intrusive way of fixing this issue, I'm happy to review and merge the patch...

[Ch4rg3r]

I think #373 won't solve the issue. From my understanding of the standard, SN-REFs follow the hierarchy. Therefore they should be resolved iterative per layer. This should also apply to imported data from ecu shared datas.

[andlaus]

From my understanding of the standard, SN-REFs follow the hierarchy. Therefore they should be resolved iterative per layer.

it can be even worse: in principle, SNREFS contexts can be even more fine-grained, e.g. environment data descriptions specify an SNREF to a parameter. This results the reference to be resolved potentially differently depending on the request, response, or structure it is featured in. (That said, it is true that in practice at least 95% of the problem is due to the relevant diagnostic layer.)

[Ch4rg3r]

SNREFS contexts can be even more fine-grained, e.g. environment data descriptions specify an SNREF to a parameter. This results the reference to be resolved potentially differently depending on the request, response, or structure it is featured in.

Yes, maybe layer was the wrong definition here. It would be dependent on the highest possible level in ODX, so all types deriving from OdxCategory. Such an iterative approach in addition must handle circular references, like Import-refs and parent-refs correctly. Wouldn't this require a SnRefContext, that handles inheritance?

[andlaus]

Wouldn't this require a SnRefContext, that handles inheritance?

As far as I can see, the current SnRefContext class would work just fine for that. What cannot be done in a comprehensive way is to resolve SNREFs ahead of time (i.e., as part of the database creation procedure). This would basically mean that a SNREF context would need to be passed to any function which potentially involves objects that are specified using a SNREF (i.e., basically everything).

Since I consider this "magic SNREF switcheroo" to be pretty much a niche issue and I also have neither the time nor the motivation for such a "90% rewrite" of odxtools (which would also necessitate major refactorings of essentially all current code that uses odxtools), people who run into this issue should for now sidestep it by either resolving SNREFs manually, or by using something like the "database retargeting hack" outlined above...

[andlaus]

[...] or by using something like the "database retargeting hack" outlined above...

Making this more convenient and bullet-proof, a .retarget_snrefs() method could be added to DiagLayer in order to retarget the snrefs of a diag layer and its parents to a given diaglayer. I'll have a look...