search

mercedes-benz / odxtools

odxtools is a collection of utilities to interact with the diagnostic functionality of automotive electronic control units using python
MIT License
180 stars 76 forks source link

Add `PositionableParam` #278

Closed andlaus closed 8 months ago

andlaus commented 8 months ago

This is the base class of all parameters for which a position within the PDU can be defined. Interestingly, the XSD does not seem to feature any parameters that are not positionable, but let's reflect the class hierarchy implied by the ODX specification...

Andreas Lauser <andreas.lauser@mercedes-benz.com>, on behalf of MBition GmbH. Provider Information

kayoub5 commented 8 months ago

I simply do not see the benefit of this PR, if non-positioned parameters simply don't exist, why introduce the complexity of a non-necessary abstraction layer?

andlaus commented 8 months ago

I simply do not see the benefit of this PR, if non-positioned parameters simply don't exist, why introduce the complexity of a non-necessary abstraction layer?

mainly to make it easier to look at the XSD and find the corresponding odxtools code. But I agree, we should probably solve this "by documentation" (i.e., add a comment that Parameter implements the POSITIONABLE-PARAM XSD group), and/or simply rename Parameter to PositionableParameter?

kayoub5 commented 8 months ago

I simply do not see the benefit of this PR, if non-positioned parameters simply don't exist, why introduce the complexity of a non-necessary abstraction layer?

mainly to make it easier to look at the XSD and find the corresponding odxtools code. But I agree, we should probably solve this "by documentation" (i.e., add a comment that Parameter implements the POSITIONABLE-PARAM XSD group), and/or simply rename Parameter to PositionableParameter?

A documentation change should be more than enough

andlaus commented 8 months ago

okay. I moved all the "meat" of this PR into Parameter. (I'd like to keep the centralized position handling for the decoding routines.)