dop_snref Temporary plan #283

QWander · 2024-03-31T16:08:33Z

In section 7.3.2.5 "Importing and referencing of cobjects",the ability to cross-reference shortnames using "dop_snref" is
odefined.
dop_snref can reference the content of other files, but the all_data_object_properties here do not define the entire content, which may lead to errors.
The change here temporarily solves this problem (skipping the use of dop_snref that references other files). This is necessary, otherwise it will trigger odxraise.
Long term change direction: I tend to add the content of the import file when generating all_data_object_properties, rather than processing it in the resolve_snrefs section.

…object_properties here do not define the entire content, which may lead to errors. The change here temporarily solves this problem (skipping using dop_snref that references other files)

CLA-Mercedes-Benz · 2024-03-31T16:08:38Z

All committers have signed the CLA.

andlaus · 2024-04-01T07:58:54Z

To be honest, I don't really get what the issue is here: Shouldn't the DiagDataDictionarySpec.all_data_object_properties be extended to include the missing objects?

on a different note: the semantics of short name references are pretty awful because the objects these references point to depend on the calling object, i.e., the only way to implement them "correctly" would be do do short name lookups each and every time an object referenced like this is accessed. since this is slow, and hopefully not very common, odxtools currently does pre-lookups and relies some hacks to bend the whole database towards the ECU of interest if this is an issue, cf. #200.

QWander · 2024-04-01T11:22:59Z

You are correct that modifying DiagDataDictionarySpec.all_data_object_properties is indeed the best option, as I have also mentioned in the "Long term change direction". However, such modifications involve changes to the DiagDataDictionarySpec.from_et() section, which would require a significant overhaul of the entire from_et framework. I have not yet figured out how to handle this part, but I will try to make the necessary changes later on.

The temporary changes I have submitted can allow me to temporarily bypass this issue and address the practical problems I have encountered in my current work, which perhaps others have encountered as well. I have confirmed in my local code that such changes are feasible.

andlaus · 2024-04-02T08:48:21Z

Even after reading section 7.3.5.2, I still don't get it: is the issue that your files use IMPORT-REF in addition to PARENT-REF to define the hierarchy of ECUs? (IMPORT-REF is currently not implemented, but I can have a look.)

QWander · 2024-04-02T14:10:03Z

My understanding of :
Through IMPORT-REFS, common diagnostic data definitions (such as ADMIN-DATA, COMPANY-DATAS, DATA-OBJECT-PROPS, etc.) can be placed in one shared ODX files. This allows other ODX files to reference these shared definitions via IMPORT-REFS, without the need to redefine the same content in each file.
Example:
Currently, there are ECUs: A, B, C
I define the common parts of A-C in the D file
Files A, B, and C declare references to the in the D file through .
In this way, I can directly reference the content of the D file in the A file through .

QWander · 2024-04-02T14:12:26Z

I have two ideas to solve the problem:
1:
After the initialization of DiagDataDictionarySpec._all_data_object_properties is complete, add a step to append properties to DiagDataDictionarySpec._all_data_object_properties from the target file when the ECU using IMPORT-REF. Analogously, ADMIN-DATA, COMPANY-DATAS, etc., may need to be updated accordingly.
2:
Enhance all _resolve_snrefs() functions by adding Database.diag_layer_containers as an input parameter to handle ECUs defined using appropriately.

andlaus · 2024-04-02T14:54:05Z

Solution 1:
After the initialization of DiagDataDictionarySpec._all_data_object_properties, add a step to append the DiagDataDictionarySpec.all_data_object_properties from the target file for ECUs defined using . Analogously, ADMIN-DATA, COMPANY-DATAS, etc., may need to be updated accordingly.

I'm pretty sure that all ODX files that are contained within PDX archive files are already loaded. maybe there are some bugs, but IMO these should be fixed then. The only thing which I can come up with that could be wrong in your case is your files using the IMPORT-REF mechanism (which is a massive pain, because if I understood this correctly it is not a "normal" inheritance mechanism but only imports those objects from the base layer that get referenced by the derived layer...). Would be nice if you could check this...

… another instance;add __contains__ magic methods for use <in>

… dop_snref.section 7.3.2.5 'Importing and referencing of cobjects'

QWander · 2024-04-05T14:06:07Z

Apart from changes to the coding standard and test cases,my local test have already passed.Please help me check if it is feasible. @andlaus

andlaus · 2024-04-05T14:13:00Z

my main issue with this PR is that I cannot see how it is connected to what's described in the ODX specification, in particular that you do a lot of stuff concerned with SNREFs, while the spec explicitly diallows this (cf. section 7.2.3.5, bullet point 2: [...] It is not possible to import a data object by a SHORT-NAME reference only.). I've implemented my interstanding of the spec in #285. would be nice if you gave it a whirl...

QWander · 2024-04-05T14:18:33Z

Okay, I will try it out soon, thank you.

QWander · 2024-04-07T03:43:31Z

The solution you provided was not successful. @andlaus
odxraise.log
odxtools\parameters\tablestructparameter.py:68: OdxWarning: Table keys cannot yet be defined using SNREFs in TableStructParameters. warnings.warn( Traceback (most recent call last): File "odxedit.py", line 158, in <module> b = PdxTest(pdx) File "odxedit.py", line 33, in __init__ self.db = odxtools.load_pdx_file(pax_path) File "odxtools\load_pdx_file.py", line 8, in load_pdx_file return Database(pdx_zip=ZipFile(pdx_file)) File "odxtools\database.py", line 91, in __init__ self.refresh() File "odxtools\database.py", line 128, in refresh dlc._finalize_init(self._odxlinks) File "odxtools\diaglayercontainer.py", line 129, in _finalize_init ecu_shared_data._finalize_init(odxlinks) File "odxtools\diaglayer.py", line 273, in _finalize_init self.diag_layer_raw._resolve_snrefs(self) File "odxtools\diaglayerraw.py", line 291, in _resolve_snrefs self.diag_data_dictionary_spec._resolve_snrefs(diag_layer) File "odxtools\diagdatadictionaryspec.py", line 232, in _resolve_snrefs structure._resolve_snrefs(diag_layer) File "odxtools\basicstructure.py", line 319, in _resolve_snrefs param._resolve_snrefs(diag_layer) File "odxtools\parameters\valueparameter.py", line 54, in _resolve_snrefs super()._resolve_snrefs(diag_layer) File "odxtools\parameters\parameterwithdop.py", line 69, in _resolve_snrefs self._dop = odxrequire(ddds.all_data_object_properties.get(self.dop_snref)) File "odxtools\exceptions.py", line 76, in odxrequire odxraise(message) File "odxtools\exceptions.py", line 39, in odxraise raise error_type() odxtools.exceptions.OdxError

QWander · 2024-04-07T03:45:54Z

I recommend my solution again:
An attribute named ecu_shared_datas has been added to the Database, which is used to define the common parts of the ODX implementation, such as when being referenced by ;
In the DiagLayer._finalize_init function, Database.ecu_shared_datas has been added as an input parameter. When using , the corresponding ecu_shared_datas are imported into the reference file. In this way, I have implemented the addition of the imported file's part in DiagDataDictionarySpec._all_data_object_properties;
Thus, during the _resolve_snrefs process, all_data_object_properties represents both the properties of the ECU file and the properties of the referenced part. This effectively implements .

andlaus · 2024-04-07T17:54:41Z

odxtools\parameters\tablestructparameter.py:68: OdxWarning: Table keys cannot yet be defined using SNREFs in TableStructParameters.

your problem actually is completely unrelated to the IMPORT-REF mechanism: As the exception says, your file tries to specify a table key for a table struct parameter via a short name reference. I will have a look, in the meantime you can probably use the non-strict mode as long as you do not try to en- or decode a request or response which contains that parameter...

QWander · 2024-04-08T11:19:22Z

odxtools\parameters\tablestructparameter.py:68: OdxWarning: Table keys cannot yet be defined using SNREFs in TableStructParameters.

your problem actually is completely unrelated to the IMPORT-REF mechanism: As the exception says, your file tries to specify a table key for a table struct parameter via a short name reference. I will have a look, in the meantime you can probably use the non-strict mode as long as you do not try to en- or decode a request or response which contains that parameter...

File A references file B through the IMPORT-REF method, and then using the short name from file B in file A will cause problems.

andlaus · 2024-04-08T15:50:38Z

odxtools/diaglayer.py

+                doc_type = next(iter(imp.ref_docs)).doc_type
+                # Delete the “DLC_” name prefix
+                if doc_type == 'CONTAINER':
+                    doc_name = doc_name[4:]


the DLC_ prefix in short names of diag layer containers is a convention of the PDX files you are interested in. we cannot hard-code this here...

andlaus · 2024-04-08T15:53:03Z

odxtools/nameditemlist.py

@@ -55,6 +55,19 @@ def append(self, item: T) -> None:

        super().append(item)

+    def append_list(self, item_list) -> None:


what's the difference of this and the .extend() method?! Also, there's no type annotation for item_list.

andlaus · 2024-04-08T16:00:36Z

odxtools/parameters/parameterwithdop.py

@@ -66,7 +66,10 @@ def _resolve_snrefs(self, diag_layer: "DiagLayer") -> None:

        if self.dop_snref:
            ddds = diag_layer.diag_data_dictionary_spec
-            self._dop = odxrequire(ddds.all_data_object_properties.get(self.dop_snref))
+            if snref_shortname := ddds.all_data_object_properties.get(self.dop_snref):


That if statment is equivalent to simply

self._dop = ddds.all_data_object_properties.get(self.dop_snref)

that said, I think the odxrequire() should stay because the reference to the DOP must be resolved. If it isn't (most probably because the referenced DOP is not yet implemented, e.g., DYNAMIC-ENDMARKER-FIELD, one should use the non-strict mode for now. This is because quite a bit of code assumes that ParameterWithDop objects provide a DOP...)

andlaus · 2024-04-08T16:04:49Z

odxtools/nameditemlist.py

@@ -152,6 +165,9 @@ def __getattr__(self, key: str) -> T:

        return self._item_dict[key]

+    def __contains__(self, key) -> bool:
+        return key in self._item_dict


this method should be unnecessary: ItemAttributeList is a list, so the x in x in my_named_item_list should be an item. maybe this function can be renamed to .contains_name()?

if x in my_named_item_list: print("x in my_named_item_list") else: print("x not in my_named_item_list")
This method returns a Boolean value (True or False), indicating whether the key exists in the _item_dict dictionary, not the item.

all code：return True if key in self._item_dict else False

sure. the point is that this class primarily is a list, not a dict, and lists check for the item values, not the keys like dictionaries...

andlaus · 2024-04-08T16:08:44Z

odxtools/diaglayer.py

@@ -591,6 +594,27 @@ def not_inherited_fn(parent_ref: ParentRef) -> List[str]:

        return self._compute_available_objects(get_local_objects_fn, not_inherited_fn)

+    def _add_import_ref(self, ecu_shared_datas) ->  None:


I guess that after #285, the IMPORT-REF mechanism is implemented. What can/should be added to Database and DiagLayerContainer though, are wrapper attributes for the different kinds of DiagLayers (analogos to .ecus or .protocols)

andlaus · 2024-04-08T16:09:59Z

odxtools/database.py


    @property
    def odxlinks(self) -> OdxLinkDatabase:
        """A map from odx_id to object"""
        return self._odxlinks

+    @property
+    def ecu_shared_datas(self) -> OdxLinkDatabase:


that should return NamedItemList[DiagLayer] like the .ecus and .protocols properties. (on line 95 you define self._ecu_shared_data this way...)

andlaus · 2024-04-08T16:12:38Z

odxtools/database.py

@@ -125,13 +128,20 @@ def refresh(self) -> None:
        # let the diaglayers sort out the inherited objects and the
        # short name references
        for dlc in self.diag_layer_containers:
-            dlc._finalize_init(self._odxlinks)
+            dlc._finalize_init(self._odxlinks, self._ecu_shared_datas)


if you need to add such an argument, make it a Database object, i.e., this line should become dlc._finalize_init(self._odxlinks, self). I somehow doubt that this is necessary, though...

andlaus · 2024-04-08T16:16:49Z

odxtools/diaglayercontainer.py

        for ecu_shared_data in self.ecu_shared_datas:
-            ecu_shared_data._finalize_init(odxlinks)
+            ecu_shared_data._finalize_init(odxlinks, ecu_shared_datas)


as mentioned above, it is better pass the whole Database object here (if this is really necessary.)

andlaus · 2024-04-08T19:43:25Z

File A references file B through the IMPORT-REF method, and then using the short name from file B in file A will cause problems.

at least as far as described by the specification, this should work: only DIAG-COMM-PROXY, DTC-PROXY, DIAG-VARIABLE-PROXY, and ROW-WRAPPER elements can be imported via this mechanism. DIAG-VARIABLEs are not yet implemented and the ODXLINK references of the remaining proxy objects should be resolved before short names are dealt with. (modulo bugs.) The exception message which you posted in #285 also points into a different direction, cf. #288 ...

andlaus · 2024-07-12T20:18:42Z

@QWander: can you please check if your issue still exists with the latest version of odxtools?

andlaus · 2024-07-16T08:28:35Z

okay, I'll close this now: I'm pretty sure this issue goes away if all files belonging to your database are loaded (e.g., using something like

db = odxtools.load_files("protocols.odx-d", "base_variants.odx-d", "ecu_variants.odx-d", "comparams.odx-cs")

) and even if not, the non-strict mode will probably fix it.

dop_snref can reference the content of other files, but the all_data_…

60c3265

…object_properties here do not define the entire content, which may lead to errors. The change here temporarily solves this problem (skipping using dop_snref that references other files)

andlaus mentioned this pull request Apr 5, 2024

Implement the IMPORT-REF mechanism #285

Merged

QWander added 6 commits April 5, 2024 20:43

add ecu shared dates property

d23c536

Modified DiagLayerContainer._finalize_init function input parameter

c434b0e

Add append_list function:An instance of a itemAttributeList class,add…

53fa94a

… another instance;add __contains__ magic methods for use <in>

Add a step to include ecu shared datas when using import_ref.

e5ee641

add: Implement cross-reference reference support using Import-ref and…

e71e384

… dop_snref.section 7.3.2.5 'Importing and referencing of cobjects'

add function description

d10641f

andlaus mentioned this pull request Apr 8, 2024

TableStructParameter: allow the table key to be specified via SNREF #288

Merged

andlaus requested changes Apr 8, 2024

View reviewed changes

andlaus mentioned this pull request May 24, 2024

Improve the file loading mechansims #307

Merged

andlaus closed this Jul 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dop_snref Temporary plan #283

dop_snref Temporary plan #283

QWander commented Mar 31, 2024

CLA-Mercedes-Benz commented Mar 31, 2024 •

edited

Loading

andlaus commented Apr 1, 2024 •

edited

Loading

QWander commented Apr 1, 2024

andlaus commented Apr 2, 2024

QWander commented Apr 2, 2024

QWander commented Apr 2, 2024 •

edited

Loading

andlaus commented Apr 2, 2024

QWander commented Apr 5, 2024

andlaus commented Apr 5, 2024

QWander commented Apr 5, 2024

QWander commented Apr 7, 2024 •

edited

Loading

QWander commented Apr 7, 2024

andlaus commented Apr 7, 2024 •

edited

Loading

QWander commented Apr 8, 2024

andlaus Apr 8, 2024

andlaus Apr 8, 2024

andlaus Apr 8, 2024

andlaus Apr 8, 2024 •

edited

Loading

QWander Apr 9, 2024

QWander Apr 9, 2024

andlaus Apr 9, 2024

andlaus Apr 8, 2024

andlaus Apr 8, 2024

andlaus Apr 8, 2024

andlaus Apr 8, 2024

andlaus commented Apr 8, 2024

andlaus commented Jul 12, 2024

andlaus commented Jul 16, 2024

		@@ -55,6 +55,19 @@ def append(self, item: T) -> None:

		super().append(item)

		def append_list(self, item_list) -> None:

		@@ -591,6 +594,27 @@ def not_inherited_fn(parent_ref: ParentRef) -> List[str]:

		return self._compute_available_objects(get_local_objects_fn, not_inherited_fn)

		def _add_import_ref(self, ecu_shared_datas) -> None:

dop_snref Temporary plan #283

dop_snref Temporary plan #283

Conversation

QWander commented Mar 31, 2024

CLA-Mercedes-Benz commented Mar 31, 2024 • edited Loading

andlaus commented Apr 1, 2024 • edited Loading

QWander commented Apr 1, 2024

andlaus commented Apr 2, 2024

QWander commented Apr 2, 2024

QWander commented Apr 2, 2024 • edited Loading

andlaus commented Apr 2, 2024

QWander commented Apr 5, 2024

andlaus commented Apr 5, 2024

QWander commented Apr 5, 2024

QWander commented Apr 7, 2024 • edited Loading

QWander commented Apr 7, 2024

andlaus commented Apr 7, 2024 • edited Loading

QWander commented Apr 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andlaus Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andlaus commented Apr 8, 2024

andlaus commented Jul 12, 2024

andlaus commented Jul 16, 2024

CLA-Mercedes-Benz commented Mar 31, 2024 •

edited

Loading

andlaus commented Apr 1, 2024 •

edited

Loading

QWander commented Apr 2, 2024 •

edited

Loading

QWander commented Apr 7, 2024 •

edited

Loading

andlaus commented Apr 7, 2024 •

edited

Loading

andlaus Apr 8, 2024 •

edited

Loading