-
Notifications
You must be signed in to change notification settings - Fork 81
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
implement de- and encoding of environment data descriptions #321
Changes from all commits
f6a1b55
3b18347
4e79723
1238282
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -130,22 +130,35 @@ def decode_from_pdu(self, decode_state: DecodeState) -> ParameterValue: | |
sdgs=[], | ||
) | ||
|
||
@override | ||
def encode_into_pdu(self, physical_value: Optional[ParameterValue], | ||
encode_state: EncodeState) -> None: | ||
if isinstance(physical_value, DiagnosticTroubleCode): | ||
trouble_code = physical_value.trouble_code | ||
elif isinstance(physical_value, int): | ||
def convert_to_numerical_trouble_code(self, dtc_value: ParameterValue) -> int: | ||
if isinstance(dtc_value, DiagnosticTroubleCode): | ||
return dtc_value.trouble_code | ||
elif isinstance(dtc_value, int): | ||
# assume that physical value is the trouble_code | ||
trouble_code = physical_value | ||
elif isinstance(physical_value, str): | ||
return dtc_value | ||
elif isinstance(dtc_value, str): | ||
# assume that physical value is the short_name | ||
dtcs = [dtc for dtc in self.dtcs if dtc.short_name == physical_value] | ||
odxassert(len(dtcs) == 1) | ||
trouble_code = dtcs[0].trouble_code | ||
dtcs = [dtc for dtc in self.dtcs if dtc.short_name == dtc_value] | ||
if len(dtcs) != 1: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could it happen that length > 1 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so: in this case, it would be impossible to decide which of the environment datas that apply for the DTC ought to be used. (I suppose that this constitutes an incorrect PDX file?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the specs say anything about that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not aware that it explicitly mentions multiple "normal" environment datas matching the same DTC, but it says that there can only be a single ALL-DATA environment data. Since multiple ALL-DATA environment datas is exactly the same problem as multiple "normal" ones for a given DTC, I infer that this is forbidden. (in which order shall these environment datas be processed anyway?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, sorry: I was in the wrong mental context for my previous two comments (interestingly they are still applicable to some extend): more than one DTC with the same short name cannot happen in valid ODX datasets because short names are required to be unique in their respective context. (if they were not, SNREFs would not work...) |
||
odxraise(f"No DTC named {dtc_value} found for DTC-DOP " | ||
f"{self.short_name}.", EncodeError) | ||
return cast(int, None) | ||
|
||
return dtcs[0].trouble_code | ||
else: | ||
raise EncodeError(f"The DTC-DOP {self.short_name} expected a" | ||
f" DiagnosticTroubleCode but got {physical_value!r}.") | ||
odxraise( | ||
f"The DTC-DOP {self.short_name} expected a" | ||
f" diagnostic trouble code but got {type(dtc_value).__name__}", EncodeError) | ||
return cast(int, None) | ||
|
||
@override | ||
def encode_into_pdu(self, physical_value: Optional[ParameterValue], | ||
encode_state: EncodeState) -> None: | ||
if physical_value is None: | ||
odxraise(f"No DTC specified", EncodeError) | ||
return | ||
|
||
trouble_code = self.convert_to_numerical_trouble_code(physical_value) | ||
|
||
internal_trouble_code = int(self.compu_method.convert_physical_to_internal(trouble_code)) | ||
|
||
|
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.
a dict would be faster for lookup
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.
we need the sequence in which the parameters were processed, i.e. that would require at least
OrderedDict
. That said, I'm not sure how such a lookup would be realized and also parameters frequently appear more than once, e.g., for fields (which are repeated structure 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.
You are using the journal with
reversed
, meaning only the last value is actually neededThere 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.
yes, but then a different name than
.journal
is also required IMO. (I think that conceptually, the concept of a "change log" for en-/decoding is quite nice.) Performance-wise I don't think that it will matter much because responses containing environment data descriptions are probable quite rare (e.g., I don't have any files which use them and so far we have not gotten any bug reports about missing en-/decoding support.)