Refactor decoding, part 2 #265
Conversation
these methods are now called `decode_from_pdu()` and are the template for all other objects which can be decoded (i.e., DOPs and parameters): they take a `DecodeState` object as their lone argument and return the decoded physical value. Note that these methods are supposed to change the passed `DecodeState` object so that it is set up to decode the next object after calling `decode_from_pdu()`. This reduces the mental load required because before this, parameters, diag coded types, and DOPs all used the same name for functions which do similar but slightly different things... Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Alexander Walz <[email protected]>
…eState` if no bit position is specified/applicable, decoding continues at first bit of the byte pointed to by `cursor_position` Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Alexander Walz <[email protected]>
odxtools/decodestate.py
Outdated
| #: | ||
| #: If bit position is undefined (`None`), the object to be extracted | ||
| #: starts at bit 0. | ||
| cursor_bit_position: Optional[int] = None |
There was a problem hiding this comment.
I don't see why this can't be 0 by default
There was a problem hiding this comment.
the idea is that None means "not specified" while 0 indicates that it was explicitly set to 0 and thus code where specifying a bit position is not allowed can error out even if 0 was specified. (with this approach, code where bit positions are allowed needs to use decode_state.cursor_bit_position or 0, which is not too bad IMO...)
There was a problem hiding this comment.
Any case where None and 0 would have different logic?
There was a problem hiding this comment.
all parameters which do not accept bit positions, e.g. structures, muxes, value parameters with non-integer values, etc...
There was a problem hiding this comment.
All of those either require bit position to be non specified or 0
There was a problem hiding this comment.
point taken & code changed. I wanted to be able to check if a bit position is set to 0 where it is not allowed to be defined in the decoding logic, but I finally saw the light and concluded that this is the wrong abstraction level to check this...
odxtools/minmaxlengthtype.py
Outdated
| return value, byte_pos | ||
|
|
||
| decode_state.cursor_position = byte_pos | ||
| decode_state.cursor_bit_position = None |
There was a problem hiding this comment.
Maybe add a goto(byte_pos, bit_pos = 0) function to decode_state ?
There was a problem hiding this comment.
I've tried this as a prototype, but I was not really happy with it because it obfuscated what was happening too much. That said, how do you interpret the following sentence from section 7.3.5.2 of the ASAM version of the spec:
The parameter starts then at the byte edge next to the end of the last parameter.
does "last parameter" mean
a) the currently decoded parameter in the list of parameters
b) the parameter which encompasses the byte of the PDU furthest to the back which has been seen so far
c) the parameter specified last in the list
IMO, (c) does not make sense, we currently use interpretation (b), but the more I think about it, (a) makes more sense (and, as a bonus, is easier to implement)...
There was a problem hiding this comment.
Both A and B are valid interpretations given how vague the spec is, and would give identical results given sane enough PDX file that does order the parameters by desired position.
That being said, the specs didn't use the term "previous parameter" which is more common, making option B more likely.
There was a problem hiding this comment.
okay. would you support switching the interpretation to option (a) in the future anyway? (it simplifies things considerably...)
There was a problem hiding this comment.
I have not seen a PDX so far, where choosing A or B matters, if I ran into issues I will let you know.
| browse.run(browse_args) | ||
| with self.assertRaises(SystemError): | ||
| # browse can only be run interactively | ||
| browse.run(browse_args) |
There was a problem hiding this comment.
Maybe mock the interactive part for testing?
There was a problem hiding this comment.
good idea. I tested to implement this, but the assumption that sys.stdout is a tty is burried too deeply within PyInquirer. (there are plenty of asserts and probably quite a few implicit assumptions...)
That said, I've noticed that this test stopped to ask for user input when PyInquirer is available and it is run in "bare metal mode" (without using pytest), so I mocked sys.stdout.isatty() to make it reliably bail out...
this should make it clear what kind of position is meant. thanks to [at]kayoub5 for the suggestion. Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Alexander Walz <[email protected]>
andlaus
force-pushed
the
refactor_decoding2
branch
2 times, most recently
from
c93ae9e
to
88bee93
Compare
if something went wrong while loading a CLI tool, the backtrace can be accessed via `odxtools $TOOL --help`. Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Alexander Walz <[email protected]>
rather "work" (mind the quotes): If pyinquirer is working and this was called "bare metal" (i.e., not via `pytest`), the `browse` tool stopped to ask the user for input. Since this is undesireable for a unit test, we now mock `sys.stdout.isatty()` to always return `False`. Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Alexander Walz <[email protected]>
andlaus
force-pushed
the
refactor_decoding2
branch
from
88bee93
to
9c7f239
Compare
i.e., it is now always an integer, never `None`. This looses us the ability of distinguishing between "no bit position defined" and "bit position set to 0" in the decoding machinery, but this is not too relevant as the ODX spec says that, the bit position ought to be assumed 0 if it is not defined and policing if it is allowed to be defined or not ought not should not be part of the decoding logic. thanks to [at]kayoub5 for his persistence! Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Alexander Walz <[email protected]>
It seems like PyInquirer does not work more often than not (e.g., importing the module fails). Since InquirerPy describes itself as `a re-implementation of the PyInquirer project, with bug fixes of known issues, new prompts, backward compatible APIs as well as more customisation options.`, let's use it instead. Signed-off-by: Andreas Lauser <[email protected]> Signed-off-by: Alexander Walz <[email protected]>
The approach taken in this PR will serve as the template for all other objects which can be decoded (i.e., DOPs and parameters): decoding is done by a
decode_from_pdu()method which takes aDecodeStateobject as their lone argument and returns the decoded physical value and the passedDecodeStateis supposed to be changed so that it is set up to decode the next object after callingdecode_from_pdu().IMO, this substantially reduces the mental load required to understand the decoding logic, because before this, parameters, diag coded types, and DOPs all used identical names for the decoding functions but they behaved slightly different in terms of the parameters which they took and the types they returned...
Andreas Lauser <[email protected]>, on behalf of MBition GmbH.
Provider Information