Minor cleanups #223
Minor cleanups #223
Conversation
this computation is relatively expensive... Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Alexander Walz <alexander.walz@mbition.io>
communication parameters themselfs do not provide this information and determining it from the context does not work for all instances where communication parameter references are needed (i.e. within `DiagService`) Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Alexander Walz <alexander.walz@mbition.io>
andlaus
force-pushed
the
minor_cleanups
branch
from
0509b8d to
5909e08
Compare
Why? |
mainly because DIAG-SERVICE tags may have a COMPARAM-REFS sub-element and it is completely impossible to determine if the parameter is supposed to be "functional" or not. besides that, I'm not sure what it even means for a communication parameter to be functional, and finally, there were no in-tree users or tests... |
@dmholtz was the one who introduced the |
| @@ -720,16 +717,13 @@ def get_doip_logical_ecu_address(self, protocol_name: Optional[str] = None) -> O | |||
| return int(ecu_addr) | |||
|
|
|||
| def get_doip_logical_gateway_address(self, | |||
| protocol_name: Optional[str] = None, | |||
| is_functional: Optional[bool] = False) -> Optional[int]: | |||
There was a problem hiding this comment.
Removing the is_functional parameter in any of these methods is a breaking change and should result in a new major release of odxtools if the removal is really necessary.
| @@ -41,7 +39,6 @@ def from_et(et_element: ElementTree.Element, doc_frags: List[OdxDocFragment], | |||
| else: | |||
| value = create_complex_value_from_et(odxrequire(et_element.find("COMPLEX-VALUE"))) | |||
|
|
|||
| is_functional = dl_type == DiagLayerType.FUNCTIONAL_GROUP | |||
There was a problem hiding this comment.
This is the critical line of code where the parameter is set.
I used the DiagLayer's type to determine if a communication parameter is used for functional addressing or not. The reason for this heuristic is that all of the pdx files I worked with last year provided two almost indistinguishable communication parameters, one for functional addressing and one for non-functional addressing. Apart the surrounding DiagLayer and its type there is afair no other indication to what type of addressing the parameter refers to.
I suggest not to remove this heuristic unless a reasonable alternative is found, because some of our current projects use the query methods in the DiagLayer class, which depend on this parameter.
There was a problem hiding this comment.
the problem is that DiagServices may also contain references to communication parameters and that there this heuristic is not available because diagnostic services are inherited, i.e., does the parameter change its "allegiance" if it is inherited by an ECU-VARIANT that is derived from a FUNCTIONAL-GROUP?
There was a problem hiding this comment.
@andlaus the alternative to is_functional would be to expose to what DiagLayerType the comm param belongs to.
There was a problem hiding this comment.
I suggest not to remove this heuristic unless a reasonable alternative is found, because some of our current projects use the query methods in the DiagLayer class, which depend on this parameter.
As this seems to be an issue with our PDX parameter naming conventions, I will forward this to my colleagues, who will eventually check if we can fix this internally without stopping the development of odxtools
There was a problem hiding this comment.
We have revisited our usage of this part of the API and found that there exists a fix for the behavior described above (at least) in the most recent version of odxtools. In case two communcation parameters have the same name, they can be distinguished by simply also providing the protocol name to the query.
The is_functional parameter / attribute is indeed no longer required and can be removed.
|
cool! let's get this merged then! |
This PR contains a few cleanups for stuff which I stumbled over yesterday:
is_functionalfor communication parametersDiagLayer.protocolsa cached propertyAndreas Lauser <andreas.lauser@mercedes-benz.com>, on behalf of MBition GmbH.
Provider Information