Skip to content
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

opack: implement dates #2466

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thiccaxe
Copy link
Contributor

I'd also like to add support for negative integers into opack, if possible. If anyone has a mac, I'd ask if you could test this piece of code https://github.com/fabianfreyer/opack-tools/ to see whether or not opack supports negative integers in the first place.

My suspicion is that OPACK does not, in fact, have a data type for signed integers, and attempting to encode negative integer will be treated as an unsigned integer internally. (e.g. OPACKDecode(OPACKEncode(NSNumber(-15))) = NSNumber(4294967281), at a high level)

@postlund
Copy link
Owner

postlund commented Aug 1, 2024

I can confirm that integer data types in OPACK are signed integers. I used the method I've described here:

https://pyatv.dev/documentation/protocols/#reference-decoding

To decode 0x30FF, which corresponds to an integer with one byte payload. So 0xFE is a signed integer (-2), which the application confirms:

postlund@Pierre ~ % echo 30FE | xxd -r -p | ./decode
2024-08-01 19:32:57.967 decode[8415:554621] Decoded: -2

I can also confirm that this case is broken in the OPACK implementation of pyatv:

>>> opack.pack(-2)
b'\x06'

It is because of this check:

if data < 0x28 and not size_hint:

Integers in the range of [0, 39] are encoded as [value + 8] and the check above catches negative values for this case. Integers less than or equal to -2 (as -1 has its own value of 0x07) must follow the same rules are we check for positive integers right now (as those are signed). Typically add tests that opack.pack(-39) and opack.pack(-40) works as intended (both for packing and unpacking). You are more than welcome to send a PR for that!

@@ -15,6 +14,8 @@

_SIZED_INT_TYPES: Dict[int, Type] = {}

_OPACK_TIME_OFFSET: float = 978307200.0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment (and possibly rename the constant) making it clear that this is a conversion from offset between unix time and mach absolute time? This is a very magic constant.

@@ -36,8 +36,7 @@ def test_pack_uuid():


def test_pack_absolute_time():
with pytest.raises(NotImplementedError):
pack(datetime.now())
assert pack(datetime.fromtimestamp(978307200.0)) == b"\x06" + (b"\x00" * 8)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with the tests, I don't really like magic constants. I have used mach time in MRP IIRC, so you could add some kind of helper method to convert datetime objects mach time (and possibly pack) if you like.

@thiccaxe
Copy link
Contributor Author

thiccaxe commented Aug 1, 2024

I'll add in a conversion function, and use datetime.timedelta, as plistlib does

-----
After wondering why dates weren't being encoded as dates for about 30 minutes, it hit me like a train that I was looking at bplist decoding and not opack decoding. And not only that, while the field is semantically a timestamp, it is encoded as a float in the bplist. Well, the opack conversion should still be accurate, though there might be a mixup between seconds and milliseconds.

@postlund
Copy link
Owner

postlund commented Nov 2, 2024

I'm not sure where we are with this right now. Can we clean it up and merge it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants