-
Notifications
You must be signed in to change notification settings - Fork 13
Expose information about TV MAC addresses #599
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
base: main
Are you sure you want to change the base?
Conversation
thecode
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Using connection macs is too limited to what you wanted out of this information, the data is network information, I suggested changes to keep it more broad.
- You need to add the field to:
aiowebostv/aiowebostv/models.py
Lines 11 to 13 in 0cd52e5
hello: dict[str, Any] = field(default_factory=dict) system: dict[str, Any] = field(default_factory=dict) software: dict[str, Any] = field(default_factory=dict)
| TURN_ON_SCREEN = "com.webos.service.tvpower/power/turnOnScreen" | ||
| GET_CONFIGS = "config/getConfigs" | ||
| GET_MEDIA_FOREGROUND_APP_INFO = "com.webos.media/getForegroundAppInfo" | ||
| GET_CONNECTION_MACS = "com.webos.service.connectionmanager/getinfo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| GET_CONNECTION_MACS = "com.webos.service.connectionmanager/getinfo" | |
| GET_CONNECTION_INFO = "com.webos.service.connectionmanager/getinfo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used this name originally, actually, but changed it to connection_macs because of the upstream OSS documentation: https://www.webosose.org/docs/reference/ls2-api/com-webos-service-connectionmanager/#getinfo
It's not just that for me it only returns MAC addresses, it's that upstream specifically documents that the call only returns MAC addresses.
Based on that I decided to make the name less generic. While other information could potentially be merged into this attribute, that has two problems:
- A naive merge would cause the
returnValueto be eaten for all except the last merged result. Fixing this would result in an awkward at best API. - Anything else (IP address, connection status, etc.) is more of a runtime state thing, so probably belongs in
WebOsTvStateinstead. You also probably want to subscribe to these things, but the returnedsubscribedkey would have the same overloading problem asreturnValue. The problem would be worse here, actually; at leastreturnValueis almost always going to betrue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That all being said: I don't feel strongly. Consider this a strong-ISH opinion, loosely held. I'm happy to just accept your suggestions and do a smoketest to make sure it all still works.
My steelman argument for changing the name back would be, well it matches the upstream method call naming, that's not nothing!
| self.tv_info.system = await self.get_system_info() | ||
|
|
||
| self.tv_info.software = await self.get_software_info() | ||
| self.tv_info.connection_macs = await self.get_connection_macs() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self.tv_info.connection_macs = await self.get_connection_macs() | |
| self.tv_info.connection = await self.get_connection_macs() |
| async def get_connection_macs(self) -> dict[str, Any]: | ||
| """Return the MAC addresses of network interfaces.""" | ||
| return await self.request(ep.GET_CONNECTION_MACS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| async def get_connection_macs(self) -> dict[str, Any]: | |
| """Return the MAC addresses of network interfaces.""" | |
| return await self.request(ep.GET_CONNECTION_MACS) | |
| async def get_connection_info(self) -> dict[str, Any]: | |
| """Return the network connection information.""" | |
| return await self.request(ep.GET_CONNECTION_INFO) |
|
Left some online comments. Will address the model changes after we agree on the naming stuff so I can batch testing together. Looks like that's the source of the CI failure too. (Apologies, I didn't see a contributing guide and I didn't see tests, so it didn't occur to me that there might be linting I should run even if there weren't tests.) (And, thank you for your time reviewing!) |
I've tested this manually, locally, against my LG C5 OLED. Here's what
pprint(client.tv_info.connection_macs)dumps out:I opted not to create a subscription for this data even though according to the webOS OSS docs this is possible simply for efficiency reasons - my TV isn't connected to WiFi, but returns a WiFi MAC address anyway, so I assume that it's returning the static hardware MAC address, independent of whether an interface is actually up. So based on that assumption, it seems like a waste of resources on both ends to create a subscription.
I am not confident about error handling here. How do y'all handle testing on old webOS versions? I have no idea if this will be available in every version out there.
Some sleuthing to attempt to answer the compat question above: this API is not mentioned at all in the commercial/downstream LG webOS documentation: https://webostv.developer.lge.com/develop/references/connection-manager. However. According to this chart, the Connection Manager interface is available in every TV webOS version LG put out. And, the upstream OSS documentation says that the downstream-undocumented
getinfowas added in the same API level as the downstream-documentedgetStatus. So assuming that LG didn't do something absurd like remove this function, then re-add it but keep it undocumented, it seems likely that this is available everywhere? And we don't have to handle this erroring?(Side note, use case for this is getting the wake-on-LAN MAC.)