Add #558, #505: Feature/ Hub Siren and C420 cameras fixes #559
Conversation
|
First, thank you for the great work here! Could you please test this branch https://github.com/JurajNyiri/pytapo/tree/experiment-multiple-params-getSirenTypeList 
My cameras do not support this function at all so I cannot test. What is the response you get when you run |
JurajNyiri
changed the title
JurajNyiri
changed the title
|
Can't do the test right now, but already tested to send both msg_alarm and siren on a command and ignores the second one or fails depending on the method, I will test it with your branch and report back |
|
I'm back @JurajNyiri As you can see the camera answers more similar to the rest of the ip cameras, is the hub the one that needs different commands. |
marcosngomezi
changed the title
|
So it looks like when getSirenTypeList is used inside getMost with both arguments |
|
Is what I tried to do on the first commits of the pytapo PR with method aliases, but because the response order is not the same as the request, and doesn't says if its for siren o msg_alarm, I didnt found a trustable way to do it. Maybe a second multiple request inside getMost, but I think that is not possible on the same one. |
|
@marcosngomezi thank you for testing, I pushed a new commit in the branch https://github.com/JurajNyiri/pytapo/tree/experiment-multiple-params-getSirenTypeList . Could you please run You should see something like Of course with different values. Please let me also know if the values are as expected compared to when you call the functions (getHubSirenTypeList, getHubSirenConfig, getHubSirenStatus) directly as in your pr. |
|
I have pushed another PR which should get this to very near prod quality. Now you should get output like Of course with different values. Please let me also know if the values are as expected compared to when you call the functions (getHubSirenTypeList, getHubSirenConfig, getHubSirenStatus) directly as in your pr. |
|
I'm getting the out of order exception, but I think that I get what you are trying to do, and there should be no need to depend on the order, taking the method from the result and just returning the one (or first) that is not False |
|
That is interesting indeed! I do not get out of order exception. Does that happen only on hub or camera as well? That would block this approach unfortunately. |
|
Just pushed a possible solution without depending on order at all PR, it works for me:
The problem to solve now is for the sets how to know if should be siren or msg_alarm or send both? getConnectionType is not going to be useful because both ip cameras and hub respond to it, maybe with getSirenConfig and getAlarmConfig? |
|
Ah just realised you where trying to make a list with the multiple results, instead to just keep one, what do you think about my approach? As I said the problem is that you don't know which method was the successful and need to depend on another way to know which values should use for sets |
|
Looks like only the hub has the order problem, but the camera has a lot of missing result methods: Camera:
Hub response is also not deterministic with the order, it looks like a newer version doing things in parallel that ensures the method but not the order. |
|
Thats a great idea, I would say instead of throwing the exception, we could convert it to a list instead. That way HA can then go into the list and find the correct property it is looking for. I posted a comment on the PR. |
|
I am going to accept PR and do the changes above, then I will push the code for you to test again. |
|
not sure about the list, there is not going to be a way to know which one is first and second anyway, because can be in different order, and it would force tapo-control to always check if it is a dictionary or a list, are you sure that is worth making it a list? for what we have seen there are no devices that respond to both, if some day appears, there should be some extra research to understand why it responds to both, its an strange case to have two sirens on the same device |
|
I have pushed a new commit making everything a list to keep it consistent and removed the exception. Could you please test? As for tapo integration I can refactor it so that it can accept these values as a list and look for the property it needs in the loop and as soon as it finds it break the loop. |
|
Actually give me a second, I will try to make it more robust so that the number of requests always equals the number of responses, even if response fails... |
|
@marcosngomezi pushed another commit. Let me know if it works please. |
|
As for this PR with the new codebase of pytapo (assuming it will work). What I think we can afford to do is inside As for the 2 that are there twice, both of these are not in there yet in the main branch, so for these what we can do is create a for loop and find the first expected value for them and save accordingly. |
|
Its working: |
|
Would you use same siren and config components for both types of sirens or keep it as siren and hub siren components in the integration? makes sense to have just one now I suppose |
|
I think we can use the same, as the response is the same from the point of HA there is no difference, both have key "siren_type_list" |
|
@marcosngomezi I have just pushed new version to pytapo https://pypi.org/manage/project/pytapo/release/3.3.20/ |
|
Let me know if you are OK with updating the rest like described above for breaking changes, if not I can do the changes on main. We should also now use the types reported by camera instead of hardcoding them inside the code. |
|
Please make sure to test also if the integration uses the cached values, so that we do not re-request data that have been already retrieved via getMost. You should see a lot of "Found cached capability {check_function}, creating {cls.name}" |
| else: | ||
| self._attr_state = camData["hubSiren"]["volume"] | ||
|
|
||
| #TODO max value for c420 is 300 while on hub is 599, to validate on other cameras |
There was a problem hiding this comment.
could you validate the max value on your camera/s?
method: "setAlarmConfig"
params: {"msg_alarm": { "siren_duration": 5 }}
There was a problem hiding this comment.
What happens if we set 599 on camera?
There was a problem hiding this comment.
I am a bit confused, there is no siren_duration anywhere in the code of pytapo.
I ran it anyways on C200 and C520WS:
print(tapo.executeFunction("setAlarmConfig", {"msg_alarm": {"siren_duration": 5}}))
Traceback (most recent call last):
File "/Users/jnyiri/repos/pytapo/test2.py", line 21, in <module>
print(tapo.executeFunction("setAlarmConfig", {"msg_alarm": {"siren_duration": 5}}))
File "/Users/jnyiri/repos/pytapo/pytapo/__init__.py", line 510, in executeFunction
raise Exception(
Exception: Error: Function not supported, Response: {"method": "setAlarmConfig", "result": {}, "error_code": -40210}
There was a problem hiding this comment.
As for the value, we could set this to self._attr_native_max_value = 300 if hass.data[DOMAIN][entry.entry_id]["isParent"] is False.
There was a problem hiding this comment.
On the getAlarmConfig I get the next response from cameras:
[{'enabled': 'off', 'alarm_mode': ['siren'], 'siren_type': 'Siren', 'light_type': 'flicker', 'siren_volume': 'normal', 'siren_duration': 300}]
And I can change any value with the setAlarmConfig, you dont have siren_duration in your cameras?
There was a problem hiding this comment.
from another comment I see that is probably alarm_duration in some cameras, in that case could you test max value for print(tapo.executeFunction("setAlarmConfig", {"msg_alarm": {"alarm_duration": 5}}))
| @@ -349,6 +356,7 @@ async def async_select_option(self, option: str) -> None: | |||
| await self._coordinator.async_request_refresh() | |||
|
|
|||
|
|
|||
| # TODO c420 alarm_mode has alarm_mode siren, check compatibility of method | |||
There was a problem hiding this comment.
I saw that siren was considered on pytapo but not here, probably not working correctly, to check
There was a problem hiding this comment.
Could you elaborate? I am not sure what you mean. How can I help?
There was a problem hiding this comment.
at the C420 is used the string siren in place of sound, in this select updateTapo method is not being considered the "siren" value, I will check on my camera and came back
|
Hi there @JurajNyiri not yet finished, numbers are remaining and some TODOs, but some questions and need of validation on other cameras rised. |
|
Hey @marcosngomezi thank you for all your updates. Let me know if I can help in any way with this PR. I will test it out once you say its ready and go through the code in detail. |
| self._controller.performRequest, | ||
| { | ||
| "method": "multipleRequest", | ||
| "params": { | ||
| "requests": [ | ||
| { | ||
| "method": "do", | ||
| "params": { | ||
| "msg_alarm": { | ||
| "manual_msg_alarm": {"action": "start"} | ||
| } | ||
| }, | ||
| }, | ||
| { | ||
| "method": "setSirenStatus", | ||
| "params": {"msg_alarm": {"status": "on"}}, | ||
| }, | ||
| ] | ||
| }, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
We should move this to pytapo so that the lib does not call performRequest directly anywhere.
| #TODO validate: getLastAlarmInfo returns last alarm config that is not necessarily last config seted, sound like little bug, but cant test on c420 or hub because they dont support getLastAlarmInfo | ||
|
|
||
|
|
||
| try: | ||
| alarmData = data["getLastAlarmInfo"]["msg_alarm"]["chn1_msg_alarm_info"] | ||
| alarmData = data["getAlarmConfig"][0] |
There was a problem hiding this comment.
C520WS / C200
pytapo.getAlarm:
C520WS:
{'enabled': 'off', 'light_alarm_enabled': 'on', 'alarm_mode': ['sound', 'light'], 'alarm_type': '0', 'light_type': '1', 'sound_alarm_enabled': 'on', 'alarm_volume': 'high', 'alarm_duration': '0'}
C200:
{'.name': 'chn1_msg_alarm_info', '.type': 'info', 'enabled': 'off', 'alarm_type': '0', 'light_type': '1', 'alarm_mode': ['sound', 'light'], 'sound_alarm_enabled': 'off', 'light_alarm_enabled': 'off'}
pytapo.getAlarmConfig:
[{'method': 'getAlarmConfig', 'result': {}, 'error_code': -40210}, {'method': 'getAlarmPlan', 'result': {}, 'error_code': -40210}, {'method': 'getSirenTypeList', 'result': {}, 'error_code': -40210}, {'method': 'getLightTypeList', 'result': {}, 'error_code': -40210}, {'method': 'getSirenStatus', 'result': {}, 'error_code': -40210}]
This change will break cameras.
There was a problem hiding this comment.
Ok then we could get the getLastAlarmInfo one as third option if previous ones didnt work, because is the least accurate but for compatibility with those cases, currently is the first option to be considered
| alarmData = data["getAlarmConfig"] | ||
| alarm = alarmData["enabled"] | ||
| alarm_mode = alarmData["alarm_mode"] | ||
| alarm = data["getSirenStatus"][0]["status"] |
There was a problem hiding this comment.
This change will break cameras, see above.
It will need to be handled separately.
|
Hi @marcosngomezi I had to push a new update to fix unrelated issue, please rebase this if you get the chance and see my comments above. |
marcosngomezi
force-pushed
the
feature/hub-siren-and-c420fixes
branch
from
01ee477
to
206bf69
Compare
|
Hi there @JurajNyiri , sorry for the delay, trying to finish this PR, I think is ready for a review, also have tested it and is working as expected.
|
|
Thank you!
I saw some users with this issue but have never been able to track it down or replicate it. Usually I told users to remove all entries in jsons for tapo_control and that solved it. Might be some misconfiguration, somehow, via some edge case...
Some cameras are slower, some of mine take a rather long time to respond, not much we can do. |
| result = await self._hass.async_add_executor_job( | ||
| self._controller.executeFunction, | ||
| "multipleRequest", | ||
| { | ||
| "requests": [ | ||
| { | ||
| "method": "do", | ||
| "params": { | ||
| "msg_alarm": { | ||
| "manual_msg_alarm": {"action": "stop"} | ||
| } | ||
| }, | ||
| }, | ||
| { | ||
| "method": "setSirenStatus", | ||
| "params": {"msg_alarm": {"status": "off"}}, | ||
| }, | ||
| ] | ||
| }, | ||
| ) |
There was a problem hiding this comment.
I will focus on testing this, ideally we should move it to pytapo function so that we do not need executeFunction call directly.
There was a problem hiding this comment.
Thank you for your updates!
Comments below in review addressed in aae7b35
Please merge, clean up and let me know if it works, if all works as expected I will do final testing with C200 for all the features.
| and "chn1_msg_alarm_info" in data["getLastAlarmInfo"][0]["msg_alarm"] | ||
| and data["getLastAlarmInfo"][0]["msg_alarm"]["chn1_msg_alarm_info"] != False | ||
| ): | ||
| alarmData = data["getLastAlarmInfo"]["msg_alarm"]["chn1_msg_alarm_info"] |
There was a problem hiding this comment.
2024-06-28 15:13:14.298 ERROR (MainThread) [custom_components.tapo_control] getLastAlarmInfo unexpected error err=TypeError('list indices must be integers or slices, not str'), type(err)=<class 'TypeError'>
There was a problem hiding this comment.
alarmData = data["getLastAlarmInfo"][0]["msg_alarm"]["chn1_msg_alarm_info"]
| LOGGER.error(f"getLastAlarmInfo unexpected error {err=}, {type(err)=}") | ||
|
|
||
| try: | ||
| alarmStatus = data["getSirenStatus"][0]["status"] |
There was a problem hiding this comment.
This can be (and is on my camera) [False, False].
This results in
2024-06-28 15:20:31.453 ERROR (MainThread) [custom_components.tapo_control] getSirenStatus unexpected error err=TypeError("'bool' object is not subscriptable"), type(err)=<class 'TypeError'>
There was a problem hiding this comment.
if (
data["getSirenStatus"][0] is not False
and "status" in data["getSirenStatus"][0]
):
alarmStatus = data["getSirenStatus"][0]["status"]
There was a problem hiding this comment.
it should be initialized as false then
There was a problem hiding this comment.
The issue is that data["getSirenStatus"][0] itself is False, so when we try to read status off of it it fails as boolean does not have dict entries
There was a problem hiding this comment.
yes, I mean in the cases that is missing, i will default the status to false, as we wouldnt know, but probably is false
|
|
||
| if alarmConfig != None: |
There was a problem hiding this comment.
I dont understand the comment, if we got alarmConfig from some of the previous sources, we search for typelist, but if none of them worked makes no sense to search it
There was a problem hiding this comment.
Check my commit, it's linter complaining, just a better practice, instead of != do if alarmConfig is not None:
| if alarmConfig != None: | ||
| try: | ||
| alarmSirenTypeList = data["getSirenTypeList"][0]["siren_type_list"] | ||
| except Exception: |
There was a problem hiding this comment.
Unable to connect to Tapo: Cameras Control controller: cannot access local variable 'err' where it is not associated with a value
There was a problem hiding this comment.
After fixing the err var
2024-06-28 15:26:49.357 ERROR (MainThread) [custom_components.tapo_control] getSirenTypeList unexpected error err=TypeError("'bool' object is not subscriptable"), type(err)=<class 'TypeError'>
There was a problem hiding this comment.
if (
data["getSirenTypeList"][0] is not False
and "siren_type_list" in data["getSirenTypeList"][0]
):
| entry, hass, TapoSiren, "getAlarm", config_entry | ||
| entry, hass, TapoSiren, "getSirenTypeList", config_entry | ||
| ) |
There was a problem hiding this comment.
Siren became unavailable after fixes described in review and this PR on my camera
There was a problem hiding this comment.
I understanded that getSirenTypeList was the common factor between cameras with siren, getAlarm is no available on C420s, we have to do some kind of OR for both methods then
There was a problem hiding this comment.
I see, we might need a custom logic then... or possibly just call it twice, once for getAlarm and once for getSirenTypeList. It might create 2 entities for some cameras though we do not know of, so maybe if getAlarm failed to create, do getSirenTypeList
| result = await self._hass.async_add_executor_job( | ||
| self._controller.startManualAlarm, | ||
| ) |
There was a problem hiding this comment.
After changing it back to getAlarm and triggering, it no longer sounds without this.
There was a problem hiding this comment.
weird, the first command in the multipleRequest, is the same that is being sended in the startManualAlarm method https://github.com/JurajNyiri/pytapo/blob/bd5c6d579d618d1e47a9049873b7e57c9d74ffb0/pytapo/__init__.py#L1628
Could you confirm that
performRequest(
{
"method": "do",
"msg_alarm": {"manual_msg_alarm": {"action": "start"}},
})
works, but
performRequest,(
{
"method": "multipleRequest",
"params": {
"requests": [
{
"method": "do",
"params": {
"msg_alarm": {
"manual_msg_alarm": {"action": "start"}
}
},
},
]
},
})
dont?
There was a problem hiding this comment.
Correct, the one via multipleRequest does not work. This call was found long time ago from other chinese cameras - not Tapo cameras, so it might be something they copied in but did not officially support and it was never supported via the app in the beginning when it was found. Might explain the inconsistencies we are facing here.
| async def async_turn_off(self, **kwargs) -> None: | ||
| result = await self._hass.async_add_executor_job( | ||
| self._controller.stopManualAlarm, | ||
| ) |
There was a problem hiding this comment.
Stop also no longer works with my camera with these changes
There was a problem hiding this comment.
same thing to check than with the start
There was a problem hiding this comment.
Same.
tapo.performRequest(
{
"method": "do",
"msg_alarm": {"manual_msg_alarm": {"action": "stop"}},
}
)
works.
tapo.performRequest(
{
"method": "multipleRequest",
"params": {
"requests": [
{
"method": "do",
"msg_alarm": {"manual_msg_alarm": {"action": "stop"}},
},
]
},
}
)
doesn't.
| result = await self._hass.async_add_executor_job( | ||
| self._controller.stopManualAlarm, | ||
| ) | ||
| async def async_turn_off(self, send: bool | None = None, **kwargs) -> None: |
There was a problem hiding this comment.
For me, the send is always None, so this never executes. Why was it added?
There was a problem hiding this comment.
Its added because my cameras return the time_left param on the response(line126 and 84)and there is no need to send the stop command to the camera, is just for updating the HA status, maybe there is a better way to do, I only saw it being called on the _turn_off_after method but now I get that it is part of the entity interface, if send is None it enters to the else right?, there should be no error
There was a problem hiding this comment.
When user manually switches off the siren via the siren switch it always goes to else (at least for my camera) which does not execute any command on the camera therefore the siren is not stopped.
|
@marcosngomezi if you haven't yet, check my commit and that branch, just in case you missed my main comment on the review, might save you some time as I addressed most of the above, we just now need to make it work on both cameras as we discussed in the comments now. |
|
Yes I saw it, lets resolve the pending comments and I will do the merge, remaining fixs and testing |
Related to pytapo/issues/105
Addresses #558, #505
Requires pytapo/pull/106
Changes: