home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
71.06k stars 29.73k forks source link

Z-Wave JS: Flickering in QR code scanner and huge error message when QR code does not get recognized #122634

Open AlCalzone opened 1 month ago

AlCalzone commented 1 month ago

The problem

See video Screencast from 26.07.2024 09:35:09.webm

This is with the latest dev branch as of now. This can be reproduced with the following QR code: qrcode

What version of Home Assistant Core has the issue?

core-2024.8.0.dev0

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant Container

Integration causing the issue

Z-Wave JS

Link to integration documentation on our website

No response

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

AlCalzone commented 1 month ago

Investigation shows that zwave-js-server does not handle the case when parseQRCodeString throws due to an invalid QR code: https://github.com/zwave-js/zwave-js-server/blob/d80597e7e97d439eaee4364072b58006cfa21bdb/src/lib/utils/message_handler.ts#L14-L17

Also, the python lib needs to be updated to handle the case where parseQRCodeString is not successful: https://github.com/home-assistant-libs/zwave-js-server-python/blob/0629fe161702386a4ed11c85cc92bfa5d378573c/zwave_js_server/model/utils.py#L21-L23

home-assistant[bot] commented 1 month ago

Hey there @home-assistant/z-wave, mind taking a look at this issue as it has been labeled with an integration (zwave_js) you are listed as a code owner for? Thanks!

Code owner commands Code owners of `zwave_js` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign zwave_js` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

(message by CodeOwnersMention)


zwave_js documentation zwave_js source (message by IssueLinks)

raman325 commented 1 month ago

Investigation shows that zwave-js-server does not handle the case when parseQRCodeString throws due to an invalid QR code: zwave-js/zwave-js-server@d80597e/src/lib/utils/message_handler.ts#L14-L17

Not sure if that's accurate, but maybe I am missing something. We wrap all commands in a generic try statement here: https://github.com/zwave-js/zwave-js-server/blob/master/src/lib/server.ts#L169

I also saw from Discord the comment about the lib's model of the QR parsing utility function not handling errors. While we don't handle it in the lib, it is handled in the integration: https://github.com/home-assistant/core/blob/70e368a57e5c3a09dd2b9546c1adac362663e19c/homeassistant/components/zwave_js/api.py#L972-L995 . Specifically the async_handle_failed_command decorator does the handling.

Are we sure there is an issue in any of the upstream systems? I'm not convinced there is

AlCalzone commented 1 month ago

Ok I didn't look at that part tbh. Since the error was generic (unknown_error) I assumed the error code had been lost along the way.