Skip to content

Add current_keys()/current_mac_addresses() to RollingKeyPairSource - #262

Open
ubrt wants to merge 5 commits into
malmeloo:mainfrom
ubrt:feature/current-mac-candidates
Open

Add current_keys()/current_mac_addresses() to RollingKeyPairSource#262
ubrt wants to merge 5 commits into
malmeloo:mainfrom
ubrt:feature/current-mac-candidates

Conversation

@ubrt

@ubrt ubrt commented Aug 22, 2026

Copy link
Copy Markdown

Refs #88.

Two convenience methods on RollingKeyPairSource, for answering "which BLE MAC address should this accessory be advertising right now" — the piece you need to recognise an accessory's own advertisement in a scan, e.g. to ring it directly over GATT.

Almost all of this already existed; the PR is mostly a name for a thing callers were otherwise assembling themselves:

def current_keys(self, now: datetime | None = None) -> set[KeyPair]:
    if now is None:
        now = datetime.now(timezone.utc)
    return {key for _, key in self.keys_between(now, now)}

def current_mac_addresses(self, now: datetime | None = None) -> set[str]:
    return {key.mac_address for key in self.current_keys(now)}

It spans the full get_min_index()get_max_index() range for now rather than a single index, because rollover since the last observed alignment is uncertain — so this returns a set of candidates, not one address. That is deliberate and is why the naming is plural.

Works for both RollingKeyPairSource implementations: FindMyAccessory (derived rolling keys) and FixedRollingKeyPairAccessory (a fixed key list, where the "current" set is simply all of them).

Three tests in tests/test_accessory.py: that a fixed-key accessory returns all its keys and well-formed MACs, that at the alignment date current_keys() agrees with keys_at() at the alignment index, and that the now-defaulting path works.

Context: this came out of adding a "play sound" button to OpenTagViewer (parawanderer/OpenTagViewer#17), where the Android side does the scan and the GATT write natively and only needs this library to say what to look for. You mentioned in #88 that it'd be useful for hass-FindMy too.

Convenience helpers to get the key(s)/BLE MAC address(es) an accessory
might currently be advertising, spanning the get_min_index()..get_max_index()
range for rollover uncertainty. Useful for recognizing an owned accessory's
own advertisement in a BLE scan (e.g. to trigger it directly).
`int.to_bytes()` only gained its default arguments in 3.11, so on 3.10 the
call in mac_address raises `TypeError: to_bytes() missing required argument
'byteorder'`. The package declares `requires-python = ">=3.10"`, so this is
a supported version where a public property simply does not work.

It went unnoticed because nothing called `mac_address` from the test suite;
the tests added in this branch are the first, which is how it surfaced.

Verified on a real 3.10 interpreter both ways: 103 passed with this change,
and the three new tests fail without it.
@ubrt

ubrt commented Aug 22, 2026

Copy link
Copy Markdown
Author

CI caught something on 3.10, and it turned out not to be the new code:

findmy/keys.py:101: in mac_address
    first_byte = (self.adv_key_bytes[0] | 0b11000000).to_bytes(1)
TypeError: to_bytes() missing required argument 'byteorder' (pos 2)

int.to_bytes() only gained its default arguments in 3.11, so on 3.10 KeyPair.mac_address raises. Since requires-python is >=3.10, that is a supported version where a public property does not work at all. Nothing in the suite called mac_address before, which is why it never showed up; the tests here are the first to touch it.

I have pushed a second commit fixing it (to_bytes(1, "big")). Verified on a real 3.10 interpreter in both directions: 103 passed with the fix, and the three new tests fail without it, so they do genuinely cover the property rather than passing by accident.

Happy to split that into its own PR if you would rather keep this one to the new methods.

get_min_index returns the alignment index itself for any time at or after
the alignment date, so an accessory whose true index has drifted below where
alignment believes it is falls outside the range entirely. It is then never
matched, and nothing raises: it simply never appears nearby.

Observed on real hardware rather than reasoned about. Two tags lying beside
the scanner, both separated and both reported by the network minutes
earlier: one matched, the other advertised steadily at -49 dBm for over a
minute while absent from its own 69-address candidate set.

The optional margin widens the range on both sides.
NearbyOfflineFindingDevice.is_from already takes the same precaution, with
12 hours, which is the value that fixed it here. Omitting it is unchanged
behaviour, and pinned as such.
@parawanderer

Copy link
Copy Markdown

Not a maintainer — this change is also open against my fork as parawanderer#1, and I reviewed it there in full: the review. The diffs are byte-identical and accessory.py/keys.py are identical between this branch's base and mine, so everything in it applies here unchanged. Applied and run: 103 pass, ruff and basedpyright clean.

Rather than repeat it, the three findings in one line each, and what's different about deciding them here.

1. test_a_margin_widens_the_candidate_set_both_ways only tests one way. Every FindMyAccessory test uses paired_at=now, so alignment_index == 0, get_min_index(now - 12h) is -48, and keys_at returns set() below zero — the widening comes entirely from the forward side. Delete the backwards half (keys_between(now, now + margin)) and all six still pass. Since the backwards half is what the docstring says the margin is for, it's worth a test that reaches it:

accessory.update_alignment(now, 2880)   # a month old, as a real one would be

behind = {k.adv_key_bytes for k in accessory.keys_at(2879)}
assert behind <= {k.adv_key_bytes for k in accessory.current_keys(now, timedelta(hours=12))}

Passes as written; fails with the backwards half removed. (@ubrt — this is the check your to_bytes comment above already applies, just not to the margin commit.)

2. current_keys returns set[KeyPair] with no index, so a caller matching an advertisement can't tell which index matched and can't call update_alignment — which is what collapses the range from 100 keys and 42 ms to 3 keys and 0.4 ms. keys_between yields (ind, key) and the index is dropped one line later. Measurements in the linked review.

3. margin defaults to timedelta(0), i.e. to the failure the docstring describes, while citing is_from's 12 hours as precedent.

What's different here rather than on the fork: 2 and 3 are API decisions, and this is the only place they can actually be made — my fork carries this change only until it can rebase onto a release containing it, so whatever shape lands here is the shape everyone gets. Same for a thing I raised there and would not ask of @ubrt: a memo on _AccessoryKeyGenerator._get_keypair takes the repeated 12-hour call from 42 ms to 6.7 ms and roughly halves the first, at the cost of retaining a KeyPair per index per accessory. Maintainers' call, separate from this PR.

ubrt and others added 2 commits August 23, 2026 12:22
Three things from review.

**The margin test only exercised the forward side.** Every fixture here
pairs at `now` with no alignment update, so `alignment_index == 0`,
`get_min_index(now - 12h)` is -48, and `keys_at` yields nothing for a
negative index. Confirmed by deleting the backwards half: the old test still
passed. Replaced with one aligned at index 2880, which fails without it, and
which asserts the forward half alone does *not* cover the case so it cannot
quietly stop testing anything.

**current_keys hid the cheap path.** The documented use is a scanning loop,
and a loop that matches an advertisement has to report which index matched
or it can never call update_alignment - so every later call pays for the
wide range again. Measured on a 30-day-old accessory aligned at 2880: a 12
hour margin derives 100 keys against 3 for a fresh alignment, about 100x the
cost. Both methods now return a mapping to the index. A dict answers `in`
and iterates like the set did, so simple callers read the same.

For a secondary key the index is a lower bound, since one covers 96 primary
indices and keys_between yields a key's first occurrence in the range. Said
so in the docstring, and safe regardless: update_alignment ignores an index
below the one it holds.

**Left the default at no margin.** Defaulting to 12 hours would make every
call ~100x more expensive to cover a case that does not arise while
alignment is kept fresh, and with the index now returnable, keeping it fresh
is something a caller can actually do. The docstring says plainly that a
bare call assumes trustworthy alignment and describes the two working
together instead.

Rebased onto 4f94015.
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