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

Fix detection of error messages. #843

Merged
merged 2 commits into from
May 20, 2023
Merged

Conversation

shypike
Copy link
Contributor

@shypike shypike commented May 3, 2023

Don't try to do exact matches, but use "in".
One issue being that the message usually looks like:
'"No registered event listener."'
(Note the extra quotes.)

@shypike
Copy link
Contributor Author

shypike commented May 3, 2023

This will make recovery of the "No registered event listener" error during local access of a Somfy box actually work.
The recovery code is correct, but the "==" matching spoils it here.

@iMicknl
Copy link
Owner

iMicknl commented May 3, 2023

Thanks @shypike, we will have a look! Interesting that they throw a slight different error locally...

How are you using the local integration? Via your own Python code or via HA?

@shypike
Copy link
Contributor Author

shypike commented May 5, 2023

I've made a crude hack in Home Assistant where I have a custom component that monkey-patches some methods of the OverkizClient class of pyoverkiz.
I have it working fine on my development machine, however on my actual home controller it often crashes with this:
aiohttp.client_exceptions.ClientConnectorError: Cannot connect to host gateway-1111-2222-3333.local:8443 ssl:default [Invalid argument]
I suspect that this is an issue with resolving local addresses in a Docker environment.
BTW: I've create the local token with some CURL statements, so no nice integration.
I've looked through your HA-component "overkiz" code, but it is intimidating. I especially see issues with multiple Somfy accounts with multiple boxes.

@shypike
Copy link
Contributor Author

shypike commented May 5, 2023

What I've seen so far, is that scenarios defined in Tahomalink don't work locally. So I had to redefine my scenarios in Home Assistant.

@iMicknl
Copy link
Owner

iMicknl commented May 5, 2023

@shypike we actually have a version of Overkiz with a local integration ready, for months, but it is not polished. If you are want to contribute, you are more than welcome. home-assistant/core#71644

See https://community.home-assistant.io/t/overkiz-integration-local-api-development-testers-topic/485264

@iMicknl
Copy link
Owner

iMicknl commented May 6, 2023

@vlebourl, @egguy do we want to merge all these changes in, or just the one that is required for this usecase?

@shypike
Copy link
Contributor Author

shypike commented May 7, 2023

@iMicknl I'll have a look, but with summer coming :)

Copy link
Owner

@iMicknl iMicknl left a comment

Choose a reason for hiding this comment

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

@shypike we would prefer to only have the messages changes where there is no exact match, for clarity. Or even add exact string matches in the if statement, instead of an in message.

Would you be able to make this change, so we can merge it in?

@shypike
Copy link
Contributor Author

shypike commented May 20, 2023

@iMicknl What is not clear to me is whether the "No registered event listener" is unique for local access. If it's valid for both local and remote, using exact string matches will not work. Unless your original code was already incorrect for remote.
Personally I'm no fan of exact strings matches because it makes it more likely to fail due to trivial changes.
I would go for an "in" for the "No registered event listener" message and no change to other strings.
BTW: my local hack (using the above changes) is rock solid has been running successfully for three weeks now. Unlike the remote version which is always hampered by the sub-par Somfy servers. I have 52 screens to control, which is the reason I bought a Tahoma instead of an Somfy-compatible RFLink transmitter (those only support 20 channels).

@iMicknl
Copy link
Owner

iMicknl commented May 20, 2023

I will try to simulate the error on cloud API to see if this is incorrect in the client library, or if this is something that is using another error message in the local API.

The reason we would like exact matches is too be as strict as possible, if Overkiz/Somfy would change the error messages, we probably would need changes anyway, thus it is good to have it fail. We rather have everything as strict as possible.

Great to hear that your local hack is working great! We still need to full implement this in HA. Can you perhaps verify if you have the same issue? Somfy-Developer/Somfy-TaHoma-Developer-Mode#102

@shypike
Copy link
Contributor Author

shypike commented May 20, 2023

@iMicknl I have mostly RTS devices and only two IO screens. The RTS devices don't give any feedback, so always have state unknown. I'll keep an eye on IO. However, I've tried to set debug mode on but nothing is logged.

logger:
  default: error
  logs:
    pyoverkiz: debug
    homeassistant.components.overkiz: debug

The above lines in the config should do the trick. What am I doing wrong?

Don't try to do exact matches, but use "in".
One issue being that the message usually looks like:
  '"No registered event listener."'
(Note the extra quotes.)
@shypike
Copy link
Contributor Author

shypike commented May 20, 2023

My last commit just strips off outer single quotes. That should care of the difference between local and remote messages.

@iMicknl iMicknl merged commit a8bbb43 into iMicknl:main May 20, 2023
3 checks passed
@shypike
Copy link
Contributor Author

shypike commented May 21, 2023

Pfff, I stripped the wrong kind of quotes. When I read my first text again, I see that "local" adds double quotes and not single quotes.
So the best solution would be:

.strip("\"'")

@iMicknl
Copy link
Owner

iMicknl commented May 28, 2023

@shypike can you save some JSON files of the error? So we can add testcases for this.

@iMicknl iMicknl mentioned this pull request May 28, 2023
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.

None yet

3 participants