-
Notifications
You must be signed in to change notification settings - Fork 72
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
Untangle DiagLayer (2 of X) #133
Conversation
a507c0d
to
7087484
Compare
####### | ||
# <communication parameters> | ||
####### | ||
|
||
@property | ||
def communication_parameters(self) -> NamedItemList[CommunicationParameterRef]: | ||
"""All communication parameters including inherited ones.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this class is a wrapper for "Diagnostic Layer" including "inherited Diagnostic Layers", then this class is no longer a "Layer", it would be more appropriate to call it a "Diagnostic Stack" (since it includes multiple layers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while I don't disagree, the ODX spec calls the logical view on such an object -- i.e., including inheritance -- a "layer" (cf. section 7.3.2). I think we should only deviate from the spec if there is a very good reason for it...
##### | ||
@property | ||
def variant_type(self) -> DiagLayerType: | ||
return self.diag_layer_raw.variant_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use class inheritance instead of those property wrappers ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not so sure if this is a good idea. do it in a later PR? Once this saga is finished, this should be pretty easy,,,
|
||
return odxlinks | ||
return result | ||
|
||
def _resolve_references(self, odxlinks: OdxLinkDatabase) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at _resolve_references and _build_odxlinks, I am more convinced that you should just use class inheritance
continue | ||
|
||
if hasattr(obj, "odx_id"): | ||
odxlinks[obj.odx_id] = obj |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get this part, what are you doing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some objects here (erhm, diag_data_dictionary_spec
) do not have an odx_id
, but contain objects which have it, so their _build_odxlinks()
method needs to be called. This issue can be avoided if it is guaranteed that all objects add themselfs in _build_odxlinks()
if they exhibit .odx_id
. I think almost all do, but I did not have the nerves to go hunting for the ones which don't. I'll have a look...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this is a hack that needs to be fixed? maybe add TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have a look...
Okay, done. the result is pretty large and I don't think this should be part of the present PR. I'll submit that cleanup once this PR is in...
self.communication_parameters, | ||
): | ||
if obj is None or isinstance(obj, OdxLinkRef): | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would obj be an OdxLinkRef ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because .diag_comms
can now contain OdxLinkRefs
. (The specification mandates this. We currently have avoided to deal with this because the contents of the DIAG-COMMS
tag where separated into services, single-ECU jobs and references...)
7087484
to
edd8c02
Compare
…omm references this corresponds how diagnostic communications are handled in the XML. For the user's convenience, we still provide separated lists, but the filtering is now done internally by the DiagLayer class. Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Gerrit Ecke <gerrit.ecke@mbition.io>
the data part represents the information of the XML one-to-one, while the logical part takes inheritance into account (and handles retrieving the correct communication parameters, etc). Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Gerrit Ecke <gerrit.ecke@mbition.io>
instead of `NamedItemList(short_name_as_id, [])` we now use `NamedItemList(short_name_as_id)`. thanks to [at]kayoub5 for noticing this. Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Gerrit Ecke <gerrit.ecke@mbition.io>
ee14065
to
3b16238
Compare
This is the next PR in the quest to untangle the
DiagLayer
bowl of spaghetti. This time, theDiagLayer
class is separated intoDiagLayerRaw
to represent the information of the XML andDiagLayer
which provides the "logical" view on the diagnostic layer (i.e., it deals with inheritance et al) plus a few convenience functions.Andreas Lauser <andreas.lauser@mercedes-benz.com>, on behalf of MBition GmbH.
Provider Information