-
Notifications
You must be signed in to change notification settings - Fork 65
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
DiagLayer: refactor the value inheritance mechanism #146
Conversation
with this, all objects that the spec says shall be subject to the value inheritance mechanism should now covered (modulo bugs), and the mechanism is IMO implemented in a more straight forward way with less code duplication... Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Gerrit Ecke <gerrit.ecke@mbition.io>
def __gather_local_services( | ||
self, odxlinks: OdxLinkDatabase) -> List[Union[DiagService, SingleEcuJob]]: | ||
diagcomms_by_name: Dict[str, Union[DiagService, SingleEcuJob]] = {} | ||
def _compute_available_objects( |
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.
_compute_available_objects not used to simplify protocols and _compute_available_commmunication_parameters ?
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.
protocols are not inherited, but in some sense they are the inheritance hierarchy. Regarding the comparams, for some arcane reason, they use a slightly different inheritance mechanism: https://github.com/andlaus/odxtools/blob/untangle_diaglayer_7/odxtools/diaglayer.py#L486 (cf section 7.3.6 of the spec). It's very cool that you notice such things, I probably wouldn't ;)
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.
Could you put this as comments? so that the next guy does not try to "simplify" the code and break logic?
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.
Other than this, LGTM
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.
good point. done: 8400304
thanks to [at]kayoub5 for bringing this up. Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Gerrit Ecke <gerrit.ecke@mbition.io>
seems like it was released today and produced one new error... Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Gerrit Ecke <gerrit.ecke@mbition.io>
6e40d69
to
6de1fc4
Compare
…bject to value inheritance thanks to [at]kayoub5 for the proposal. Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Gerrit Ecke <gerrit.ecke@mbition.io>
alright. I guess I should tag a release... |
This is the final PR in the long quest to refactor
DiagLayer
: An overhaul of the code that implements value inheritance. Now, all objects that the spec says shall be subject to the value inheritance mechanism should be covered (modulo bugs), and the mechanism is IMO implemented in a more straight forward way (i.e., with less code duplication as well as all value-inheritance related code being kept in theDiagLayer
class).Andreas Lauser <andreas.lauser@mercedes-benz.com>, on behalf of MBition GmbH.
Provider Information