Attention is currently required from: Hoernchen.
dexter has posted comments on this change by Hoernchen. ( https://gerrit.osmocom.org/c/python/pyosmocom/+/43171?usp=email )
Change subject: tlv: preserve the comprehension bit ......................................................................
Patch Set 1: Code-Review+1
(5 comments)
Patchset:
PS1: I have looked through your patch and I think it is plausible, however it introduces a state that other TLV classes don't have. Maybe this needs some explanation.
Commit Message:
https://gerrit.osmocom.org/c/python/pyosmocom/+/43171/comment/fec56f4a_fb906... : PS1, Line 35: instead of setting the flag, currently tag=0x8123 -> 0x81a3. I would split this in a separate patch (only if you think this would speed things up, as I think this part would be ready at least.)
File src/osmocom/tlv.py:
https://gerrit.osmocom.org/c/python/pyosmocom/+/43171/comment/4228f038_52c1d... : PS1, Line 658: # True/False: as received Maybe it makes sense to explain this property in greater detail? This is an additional state an API user would expect right away. I understand this as a state that flips to the state of the last parsed CR bit when from_tlv() has been used. This basically means that when I have used from_tlv() once, the API user cannot rely on the CR bit set as originally defined.
This is probably a corner case as the IE objects are usually short lived and probably not wildly mixed. Decode, change something, Re-Encode is probably the most exciting usecase to expect but I still think we should have an explanatory comment here.
File tests/test_tlv.py:
https://gerrit.osmocom.org/c/python/pyosmocom/+/43171/comment/873ef7ec_0ebdc... : PS1, Line 83: I would add a comment that explains that the first vector has the CR bit set, but the second has not. And both must come out the same when they are decoded and re-encoded.
https://gerrit.osmocom.org/c/python/pyosmocom/+/43171/comment/81a1304c_23ed1... : PS1, Line 86: ie.from_tlv(encoded) maybe add an assert that checks that ie.comprension has changed state as expected?