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 75 forks source link

Change interpretation of "last parameter" #309

Closed andlaus closed 4 months ago

andlaus commented 4 months ago

The specification states that if the position of a parameter is not explicitly specified using BYTE-POSITION, "The parameter starts then at the byte edge next to the end of the last parameter". This PR changes the interpretation of "last parameter" from "parameter currently closest to the end of the PDU" to "most recently handled parameter". The reason is that IMO it is more likely that this is what is meant by the specification, and that it is slightly simpler to implement. That said, I consider it to be likely that no data set encountered in the wild is affected by this.

Besides this, a bug in dynamic endmarker fields is fixed by this PR: it turns out that the data occupied by the endmarker of these fields ought not to be considered consumed (why???), i.e., one needs to explicitly add a ReservedParameter after DynamicEndmarker fields...

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

kayoub5 commented 4 months ago

@andlaus if "most recently handled parameter" was intended, it would have been called "previous parameter", not "last parameter"

kayoub5 commented 4 months ago

Besides this, a bug in dynamic endmarker fields is fixed by this PR: it turns out that the data occupied by the endmarker of these fields ought not to be considered consumed

Got spec reference for this?

andlaus commented 4 months ago

@andlaus if "most recently handled parameter" was intended, it would have been called "previous parameter", not "last parameter"

I'm not so sure about that; the language of the MCD-2 spec is sometimes quite strange (in the sense that it clearly was not written by an English native speaker) and the most literal interpretation of "last parameter" is the final parameter in the respective list, but that does not make any sense. I guess that somebody implemented their en-/decoding routines before writing that paragraph, and given that it is more obvious and simple to just continue where you currently are instead of moving the cursor, I guess that this is what was intended. That said, if there is concrete evidence to the contrary, I'm happy to revert this assessment...

Got spec reference for this?

that would be section 7.3.6.10.5 of the ASAM MCD-2 specification.