Scanner Update #210
Scanner Update #210
Conversation
malmeloo
left a comment
malmeloo
left a comment
There was a problem hiding this comment.
Thanks for your contributions! The changes look good, but I left some small notes that I think could be improved. I don't think I'll be able to test your changes today, but I'll definitely do so later this weekend.
findmy/scanner/scanner.py
Outdated
| APPLE_DEVICE_TYPE = { | ||
| 0: "Apple Device", | ||
| 1: "AirTag", | ||
| 2: "Licensed 3rd Party Find My Device", | ||
| 3: "AirPods", | ||
| } | ||
|
|
||
| BATTERY_LEVEL = {0: "Full", 1: "Medium", 2: "Low", 3: "Very Low"} |
There was a problem hiding this comment.
nitpick: maybe swap out the decimals here with their binary representation (0bXX) just for clarity
findmy/scanner/scanner.py
Outdated
| self._first_adv_key_bytes: bytes = first_adv_key_bytes | ||
|
|
||
| @property | ||
| def adv_key_bytes(self) -> bytes: |
There was a problem hiding this comment.
Not sure if I like this method... I think it could be confusing as its signature is the same as the method on e.g. a KeyPair, but unlike a keypair, the result of this method is incomplete so they aren't directly comparable. I still think it's good to expose this information though, just maybe with a different method name.
There was a problem hiding this comment.
I was trying to standardize the interface but ended up not needing to. I just changed the name so it's still exposed.
findmy/scanner/scanner.py
Outdated
| logger.info("Nearby %s - %s", self.device_type, self.mac_address) | ||
| logger.info(" Status byte: 0x%x", self.status) | ||
| logger.info(" Battery lvl: %s", self.battery_level) | ||
| logger.info(" RSSI: %s", self.rssi) | ||
| logger.info(" Extra data:") | ||
| for k, v in sorted(self.additional_data.items()): | ||
| logger.info(" %s: %s", f"{k:20}", v) | ||
| logger.info("\n") |
There was a problem hiding this comment.
This should probably just use print instead (feel free to suppress the linter.) Otherwise the result of this method will depend on whatever logger settings that are currently applied (if any). Also, maybe a file argument to give to print to allow printing to files for example?
findmy/scanner/scanner.py
Outdated
| logger.info("Separated %s - %s", self.device_type, self.mac_address) | ||
| logger.info(" Public key: %s", self.adv_key_b64) | ||
| logger.info(" Lookup key: %s", self.hashed_adv_key_b64) | ||
| logger.info(" Status byte: 0x%x", self.status) | ||
| logger.info(" Battery lvl: %s", self.battery_level) | ||
| logger.info(" Hint byte: 0x%x", self.hint) | ||
| logger.info(" RSSI: %s", self.rssi) | ||
| logger.info(" Extra data:") | ||
| for k, v in sorted(self.additional_data.items()): | ||
| logger.info(" %s: %s", f"{k:20}", v) | ||
| logger.info("\n") |
There was a problem hiding this comment.
Same here, I think using print is probably better
findmy/scanner/scanner.py
Outdated
| # Extract RSSI if it exists | ||
| rssi = None | ||
| if data.rssi is not None: | ||
| rssi = data.rssi |
There was a problem hiding this comment.
If data.rssi is already None | int, this if statement is probably redundant
findmy/scanner/scanner.py
Outdated
|
|
||
| except asyncio.TimeoutError: # timeout reached | ||
| self._device_fut = self._loop.create_future() | ||
| self.print_scanning_results(devices_seen) |
There was a problem hiding this comment.
I think whether to print results or not should be left up to the caller instead of doing it by default
|
Just found some time to test it out and it works great! Definitely a huge improvement over the current scanner. If you could take a look at my comments I'll try to merge it soon. |
|
Glad it works, thank you for the suggestions! I'll fix it up sometime this week when I get a chance. |
|
Let me know how that updated printing logic looks. I was thinking a JSONL makes it easy to append new devices, but we can do a txt or whatever instead. |
|
Ah sorry, maybe I wasn't being clear. With printing to a file I meant something more like this: def print_device(self, file: SupportsWrite[str] | None = None) -> None:
print(..., file=file)Then the user can specify a 'file' that the library will print to, but also I have some time now so I will just go ahead and modify it myself. Thanks again! |
|
Ohh makes sense, that's much cleaner. Thanks for the help! |
2 participants
Features:
self._device_fut.done()check)Please let me know what you think! I was thinking having the printing as a class method would be helpful functionality to provide, but happy to move it back it outside if we want to the user to decide. It also looks more wack because of using
logger.info.