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

[ipcamera] Fix Hikvision cameras stay offline when a 401 reply is given with no www-authenticate header #15613

Merged
merged 4 commits into from
Dec 17, 2023

Conversation

t2000
Copy link
Contributor

@t2000 t2000 commented Sep 17, 2023

It might happen that a IP camera (Hikvision) replies with HTTP 401 but does not add a WWW-Authenticate header for our reconnect logic.

This fix reconnects the ThingHandler to the camera in such cases.

Fixes #15413

@t2000 t2000 requested a review from Skinah as a code owner September 17, 2023 15:50
@lolodomo
Copy link
Contributor

@Skinah : I let you check if that is fine for you.

@lolodomo lolodomo added the bug An unexpected problem or unintended behavior of an add-on label Sep 23, 2023
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

lgtm, especially as cameras often behave buggy and thus retrying is the best option.

@jlaur
Copy link
Contributor

jlaur commented Oct 6, 2023

FYI, there are some recent discussions/investigations in the linked issue.

@kaikreuzer
Copy link
Member

Ah, thanks @jlaur, I missed that discussion.

@t2000
Copy link
Contributor Author

t2000 commented Dec 9, 2023

I have updated the MR according to the discussion here: #15413 (comment)

@lolodomo
Copy link
Contributor

@Skinah : please have a look.

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@Skinah
Copy link
Contributor

Skinah commented Dec 11, 2023

Please do not merge until this is discussed.
In the discussion of the issue thread, I did state that this can be automated by asking the camera if it supports inputs and then removing the channels if the camera replies that it does not. I feel this is a better way to go because it does not cause a breaking change and it is automatic. It also has the benefit of a channel that does nothing is removed from being seen by the user that can cause confusion.

This is what I would like to discuss before this is merged, is there a reason why another config is added that will cause a breaking change? I thank @t2000 very much for their testing and time taken making two lots of changes to address this, please do not feel disheartened by this comment as I appreciate your work, it is just that I would like to discuss this direction taken first before agreeing to the merge if there was a reason.

@t2000
Copy link
Contributor Author

t2000 commented Dec 11, 2023

In the discussion of the issue thread, I did state that this can be automated by asking the camera if it supports inputs and then removing the channels if the camera replies that it does not.

Automatic behavior is nice and I am a fan of it if it works ;-)
In theory we can detect if a camera supports external inputs, but from the community thread we already saw that cameras might respond that they support external I/Os while in reality they don't. For such cases we still would need a configuration to override the automatic behavior :(

This is what I would like to discuss before this is merged, is there a reason why another config is added that will cause a breaking change?

It is certainly not breaking because of the new config parameter, as there is a default for that in case it is not set.

However, I see the point that the default should probably be set to true, so everything stays as is, so for those users who are not having trouble, nothing changes. And if you do NOT have external alarm inputs you have to explicitly configure this new config parameter to false.

Would that be a solution (you could add the automatic detection later anyhow if you like and keep this override for faulty cameras)?

please do not feel disheartened by this comment as I appreciate your work

I absolutely don't feel like that. I am always in favor for the "right" solution. :-)

@Skinah
Copy link
Contributor

Skinah commented Dec 12, 2023

Make a call to /ISAPI/System/IO/capabilities

If it is zero like this one or it is missing, you know the camera has no IO outputs.

<IOOutputPortNums>0</IOOutputPortNums>

See
https://community.openhab.org/t/ipcamera-binding-doing-erroneous-isapi-request-to-hikvision-camera/147345/3

It is confusing as they have also posted the output of the status api call.

It might happen that a IP camera (Hikvision) replies with HTTP 401 but
does not add a WWW-Authenticate header for our reconnect logic while we
request the status of it's I/O ports.

This fix offers an advanced config option to deactivate requesting the
I/O ports.

Fixes openhab#15413

Signed-off-by: Stefan Triller <github@stefantriller.de>
Signed-off-by: Stefan Triller <github@stefantriller.de>
Signed-off-by: Stefan Triller <github@stefantriller.de>
Signed-off-by: Stefan Triller <github@stefantriller.de>
@t2000 t2000 force-pushed the fixIpCameraOffline branch from e515a0b to 8343017 Compare December 15, 2023 15:38
@t2000
Copy link
Contributor Author

t2000 commented Dec 15, 2023

Reverted to reconnect, see discussion.

@lolodomo
Copy link
Contributor

Waiting for @Skinah approval to merge, we are still in time for 4.1...

@Skinah
Copy link
Contributor

Skinah commented Dec 17, 2023

LGTM

@Skinah Skinah changed the title [ipcamera] Fix camera staying offline when it replies with HTTP close [ipcamera] Fix Hikvision cameras stay offline when a 401 reply is given with no www-authenticate header Dec 17, 2023
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

Thank you

@lolodomo lolodomo merged commit e4c4d03 into openhab:main Dec 17, 2023
3 checks passed
@lolodomo lolodomo added this to the 4.1 milestone Dec 17, 2023
@t2000 t2000 deleted the fixIpCameraOffline branch December 17, 2023 10:40
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
…en with no www-authenticate header (openhab#15613)

Signed-off-by: Stefan Triller <github@stefantriller.de>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
…en with no www-authenticate header (openhab#15613)

Signed-off-by: Stefan Triller <github@stefantriller.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ipcamera] CONFIGURATION_ERROR Camera gave no WWW-Authenticate: Your login details must be wrong.
5 participants