fix en- and decoding of environment data descriptions #379
Conversation
it turns out that the specification only mandates that the base data type of the DOP used by the referenced parameter must be A_UINT32, but the DOP does not need to be a DTC-DOP. Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Katja Köhler <katja.koehler@mercedes-benz.com>
andlaus
force-pushed
the
fix_env_data_desc_coding
branch
from
983ca39 to
41494f9
Compare
| if not isinstance(prev_dop, DtcDop): | ||
| odxraise(f"The DOP of the parameter referenced by environment data " | ||
| f"descriptions must be a DTC-DOP (is '{type(prev_dop).__name__}')") | ||
| if not isinstance(prev_param, ValueParameter): |
There was a problem hiding this comment.
it could be a constant parameter? no?
There was a problem hiding this comment.
I don't think so: environment data descriptions are a multiplexer mechanism to attach additional information to a DTC depending on the value of the trouble code (basically this data describes the circumstances under which the DTC was encountered). mandating the key of the multiplexer to be a constant defies the purpose? (haven't checked the spec about this, though..
There was a problem hiding this comment.
if the dtc have only one environment data, then it could be in theory a constant.
the main idea, is that the check you are doing either needs to backed by a specification clause, or to prevent an error for something not supported by the rest of the code.
the check here does not seem to fit any of those conditions.
There was a problem hiding this comment.
if the dtc have only one environment data, then it could be in theory a constant.
yes, but then you don't need the multiplexer because you can simply put the parameters of the environment data into the parameters for the response. Anyway, since the spec probably does not explicitly forbid such atrocities, I fixed it: 672ff47 (plus, I found a better solution for the copy-and-paste issue...)
| odxraise() | ||
| return | ||
|
|
||
| numerical_dtc_value = prev_dop.convert_to_numerical_trouble_code( |
There was a problem hiding this comment.
this block feels copy-pasted
There was a problem hiding this comment.
I noticed this too, and tried to move it one indentation level up. this turned out to be surprisingly difficult because not all DOPs exhibit .diag_coded_type which would have necessitated an to make it conditional on isinstance(prev_dop, (DtcDop, DataObjectProperty)). given that isinstance() is relatively expensive and the resulting code was IMO not being much nicer, I decided to live with the copy-and-paste...
There was a problem hiding this comment.
Introduce a function, that takes prev_dop and prev_param_value as input and return numerical_dtc_value
There was a problem hiding this comment.
good point. Done: a9a791f
…ters for the referenced parameter IMO both do not make any sense, but they probably are not explicitly forbidden by the spec, so let's cover our bases. thanks to [at]kayoub5 for the nudge. Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io>
Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io>
…alue of the DTC parameter to an integer into a method this is surprisingly complicated and led to quite a bit of error-prone copy and pasted code. thanks to [at]kayoub5 for the suggestion! Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io>
(only a `DiagCodedType` and the value already is specified using the internal representation.) Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io>
andlaus
force-pushed
the
fix_env_data_desc_coding
branch
from
c8189b2 to
ede0fed
Compare
| break | ||
|
|
||
| if dtc_param is None: | ||
| if numerical_dtc_value is None: |
There was a problem hiding this comment.
Check could be moved inside _get_numerical_dtc_from_parameter, since it's done for each call
There was a problem hiding this comment.
the check here is only triggered if no parameter with the specified short name has been found (_get_numerical_dtc_from_parameter() always returns an integer)
| odxraise(f"The data type used by the DOP of the parameter referenced " | ||
| f"by environment data descriptions must be A_UINT32 " | ||
| f"(encountered '{dop.diag_coded_type.base_data_type.value}')") | ||
| return |
There was a problem hiding this comment.
The check is only to be spec compliant, no need to halt here as far as code goes
| return |
There was a problem hiding this comment.
if we don't return here, it is basically assured that a different exception like AttributeError will be raised by the remainder of the method...
| odxraise(f"The data type used by the parameter referenced " | ||
| f"by environment data descriptions must be A_UINT32 " | ||
| f"(encountered '{param.diag_coded_type.base_data_type.value}')") | ||
| return |
There was a problem hiding this comment.
The check is only to be spec compliant, no need to halt here as far as code goes
| return |
…rom_parameter()` also, be less strict about the kind of DOP used by the parameter: any simple DOP is fine, it does not need to be a `StandardLengthType`... Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io>
it turns out that the specification only mandates that the base data type of the DOP used by the referenced parameter must be A_UINT32, but the DOP does not need to be a DTC-DOP.
Andreas Lauser <andreas.lauser@mercedes-benz.com>, on behalf of MBition GmbH.
Provider Information