[baylanger]

Add get_image_channels which lists the channel IDs a device accepts (ImageChannelInfo: id, enabled, input_port) from:

/ISAPI/Image/channels (camera)
/ISAPI/ContentMgmt/ImageProxy/channels (NVR)

input_port is read from inputPort (NVR) or videoInputID (camera)

An NVR serves the image settings of its attached cameras at /ISAPI/ContentMgmt/ImageProxy/channels/ (documented in Hikvision's NVR ISAPI guide) using the same ImageChannel document a camera serves.

Add a proxy option to the image methods (get_image_settings, get_image_values, set_image_setting, set_image_linked_setting, put_image_settings) that addresses that path instead of /ISAPI/Image/channels/.

An NVR may list a channel whose camera it cannot proxy; reading it raises ISAPINotSupportedError.

Note: this is written based on Hikvision NVR ISAPI guide, someone will need to test this on a real NVR.

#140 PR4

[mezz64]

Thanks @baylanger. These changes look fine, but I'd like it to be tested on a real device before merging.

@ptarjan Any chance you can test this change? I believe you've tested on a Hik NVR in the past.

[baylanger]

@ptarjan check out #154 - I used this script in the past to test against my camera, might be helpful for use.

[ptarjan]

Tested on a real NVR: Hikvision DS-7608NI-EV2/8P, firmware V3.4.96 (build 181107), 8 attached cameras. Read-only (GET) calls only, at ad4c736.

• get_image_channels(proxy=True) → 8 channels (id 1-8, input_port 1-8). enabled is None, because this NVR has no <enabled> element.
• get_image_settings(ch, proxy=True) and get_image_values(ch, proxy=True) work on all 8 channels with no exceptions. Each returns one setting (enableImageLossDetection, switch, false) and the values {id, inputPort, enableImageLossDetection}. This NVR's proxy <Color> element is empty, so it exposes no brightness or contrast.
• HTTP: ImageProxy/channels and ImageProxy/channels/<id>[/capabilities] return 200. A nonexistent id (9999) returns 503 serviceUnavailable, not 404.
• Non-proxy on the same NVR: /ISAPI/Image/channels returns 503 serviceUnavailable and /ISAPI/Image/channels/1 returns 400 badXmlContent. Both surface as a plain ISAPIError, not ISAPINotSupportedError, which matters if a caller falls back on "not supported".
• This NVR reports deviceType = IPC in deviceInfo, so the NVR/DVR auto-detection in tools/live_image_check.py (Add module check image settings on a real device #154) won't turn proxy on by itself here. --proxy is needed.
• The XML namespace (hikvision.com/ver20) and element names match what the parser expects.

Not tested: writes.

[baylanger]

@ptarjan

I made a change to live_image_check.py , if you have time let me know if that works better.

For your other comments, I'll try to figure out something.

Thanx for the follow up.

[baylanger]

Thanks for testing on that "IPC" NVR. Maybe we'll need to test w/ a real NVR/DVR?

Replying to the 2 most concening points:

A nonexistent id (9999) returns 503 serviceUnavailable, not 404.

That is what the NVR (IPC) sends. pyhik reports it unchanged as ISAPIError
(status_code=503, sub_status="serviceUnavailable").

Callers should use channel IDs from get_image_channels(), they shouldn't
normally hit this.

If you'd rather have it raised as ISAPINotFoundError, I can change that.

Non-proxy on the same NVR: /ISAPI/Image/channels returns 503 serviceUnavailable
and /ISAPI/Image/channels/1 returns 400 badXmlContent. Both surface as a plain
ISAPIError, not ISAPINotSupportedError, which matters if a caller falls back on
"not supported".

I kept ISAPINotSupportedError for 404, 501 and notSupport only. 503 usually
means the device is busy and 400 means it rejected the request, so a caller that
treats them as "not supported" can draw the wrong conclusion.

A caller that wants to fallback can check status_code / sub_status, or try
proxy=True when the direct listing fails. tools/live_image_check.py now does the
second one because this NVR reports deviceType IPC ... the type alone isn't
enough.

I can add a note about this to the README. Is this good enough?

[ptarjan]

Thanks, that all sounds right to me.

• It is a real NVR: the DS-7608NI-EV2/8P is an 8-channel PoE NVR. It just reports deviceType IPC in deviceInfo, which is why type-based detection isn't enough. So devices in the wild will misreport this.
• 503/400 staying a plain ISAPIError: agreed. ISAPINotSupportedError should mean "not supported", not "busy" or "rejected". No need for ISAPINotFoundError either, since callers should use the IDs from get_image_channels(). A short README note on the fallback (try proxy=True when the direct listing fails, or check status_code/sub_status) would be good.
• I read the new detect_proxy() in Add module check image settings on a real device #154 (0a556ca). On this NVR the camera endpoints fail with 503/400 ISAPIError and ImageProxy/channels lists 8 channels, so it should pick the proxy path by itself. I haven't run the new version on the device yet, and I'll follow up once I have.

[baylanger]

Fantastic. I just push a change to the README.

[ptarjan]

Ran the updated live_image_check.py from #154 (0a556ca) against the same NVR (DS-7608NI-EV2/8P, V3.4.96), with pyhik from this PR (ad4c736). Read-only checks only.

No flags (auto-detect): fails

device: model=? (ISAPIError), type=? (ISAPIError), firmware=? (ISAPIError)
proxy endpoints: no (could not read the device type: ISAPIError)
1. channels
request failed: ISAPIError: Request failed with status 503 (status=503, sub_status=serviceUnavailable)
exit=2

This time the device-info read failed as well (the last run got deviceType IPC from it), so auto-detect had nothing to go on and fell back to the camera endpoints, which return 503 here.

--proxy: passes

PASS  channels (8 listed)
PASS  settings (1 found)   # enableImageLossDetection only
PASS  values (3 read)
overall: ok

So the proxy path is solid. Auto-detect fails on this NVR when device info is unreadable. I'd suggest that a 503 serviceUnavailable / 400 badXmlContent from /ISAPI/Image/channels falls through to get_image_channels(proxy=True) before giving up, which is what the new README tells callers to do. Then detection wouldn't depend on device info at all. Thanks for the README update, it reads well.

[baylanger]

Thanks for re-running it. Surely that was a bug in the script, I’ve changed it to what you suggested.

The device type is now only a shortcut for NVR and DVR. Otherwise the script tries the camera endpoints and, if they fail w/ a device err such as 503 or 400, it tries proxy=True before giving up. A 401 or a connection error still counts as a camera.

On your NVR a read-only run should now print "device type: unknown ... SAPIError"; then the camera image endpoints failed; trying the proxy endpoints, then proxy "endpoints: yes".

Could you re-run it when you have a moment?

[ptarjan]

Re-ran it read-only on the same NVR (DS-7608NI-EV2/8P, firmware V3.4.96) with the script at cd02ca2. Both runs pass now:

• No flags (auto-detect): device type: unknown (could not read it: ISAPIError), then the camera image endpoints failed with 503 serviceUnavailable, it tried the proxy endpoints, and got proxy endpoints: yes. Then 8 channels listed, 1 setting found (enableImageLossDetection), 3 values read. overall: ok
• --proxy: the same results. overall: ok

Thanks for the quick fix. The detection works on this recorder now.

[baylanger]

I just updated the README, replaced long paragraph 🙅 with a bullet list. Much easier to read.

Is the last step before merging to write to a NVR? Who could do that ;)

I'm going to create a new PR, not ready yet. It queries a camera or NVR, and save 3 documents (capabilities, channels, values) (per camera if it's a NVR) saving exactly what is returned from the device. End users can use this "dump" to troubleshot, open an issue, and can contribute their files in tests/fixtures/ ; A follow up PR will add the logic to the test_* to go over each files in the fixtures directory, thus possibly helping to identify any issues for future releases. If you like this idea or don't think this is need, let me know.

[baylanger]

@ptarjan when you have time, I have another request to hep me decide how to finalize the "dump" feature.

Can you download and install dump.py and run python -m pyhik.dump --with-events

It asks for:

• Hostname or IP addr
• Port: 80 (default) HTTP; 443 HTTPS; for any other port it asks if the device uses HTTPS
• Username
• Password

It doesn't write anything to the camera, this is read-only. It will create a directory named from your "IPC IP address + date + time" and inside that directory it will have multiple files. I'm interested to know if the video_channels*.xml file holds for cameras attached to the NVR, each camera model and firmware version. I have tested on a camera and works fine. If you don't have time, tomorrow late or Monday I'll open a PR with uddated README and tests. I'll just repost this comment in that PR.

[baylanger]

😆😆😆😆😆 ... I just realized you are the actual maintainers for the HA hikvision integration. I thought I'd have to deal with other people.

[ptarjan]

Ha, yes, I'm a codeowner of the hikvision integration in HA (the HA maintainers own HA itself). Nice to work with you directly.

Ran python -m pyhik.dump --with-events read-only on the same NVR (DS-7608NI-EV2/8P, V3.4.96). Exit 0, 20 files plus the summary.

Your question: no, video_channels-*.xml has no camera model or firmware here. It came from /ISAPI/ContentMgmt/InputProxy/channels (/ISAPI/System/Video/inputs/channels gave 403 notSupport). Each InputProxyChannel only has id, name, sourceInputPortDescriptor (proxyProtocol, addressingFormatType, ipAddress, managePortNo, srcInputPort, userName, streamType, deviceID) and connMode. On this NVR all 8 cameras are non-Hikvision, attached over ONVIF or vendor RTSP. That's probably why the NVR knows nothing about them, and why ImageProxy only exposes enableImageLossDetection. A recorder with Hikvision cameras attached over the Hikvision protocol may report more. For those, /ISAPI/ContentMgmt/InputProxy/channels/status or the per-channel .../InputProxy/channels/<id>/deviceInfo might be worth trying, if you want model and firmware for each camera.

Other notes from the run:

• deviceInfo answered this time with deviceType IPC on this NVR, so your fallback from the camera endpoints (503) to the proxy endpoints is what got it right.
• motionDetection on channel 1: 403 notSupport, noted as not offered. 👍
• Redaction looked right in the event and video channel files (addresses, user names, device IDs and names replaced).

Write test: I don't think it's possible on this NVR. The only proxied setting is the enableImageLossDetection switch, and --write changes a numeric setting, so there's nothing for it to change. Someone with Hikvision cameras behind an NVR would need to run that one.

Dump/fixtures idea: 👍. Real device documents in tests/fixtures/ would catch model-specific parsing regressions much better than hand-written XML. If @mezz64 is on board, it'd be worth keeping the redaction for anything that ends up in fixtures, and maybe add a check that rejects fixture files that still contain IPs, MACs or serials.

[baylanger]

I added to #154 writes to select and switches. As with numbers, if switch it will flip the current value (true to false or false to true) and flip it back. For select, it picks the first option in the device’s list that isn’t the current value, write once and then puts the original value.

When you have time, give this a try : live_image_check.py --channel 1 --write --path enableImageLossDetection

For the check the rejects fixtures files that has IP address, MAC, serials... it could run locally as a pre commit or a part of the githup workflow. I guess the pre-commit is best. You have a better choice?

[ptarjan]

Ran the write test on the NVR with #154 at 7bef63b: live_image_check.py --proxy --channel 1 --write --path enableImageLossDetection. Passes:

4. change a setting and put it back
   enableImageLossDetection (switch): false -> true
   result: ImageWriteResult(success=True, status_code=200, sub_status='ok', also_changed={})
   -> PASS: read back OK
   restore enableImageLossDetection -> false: restored
overall: ok

So a write through ImageProxy works on this NVR, and the read-back and the restore both check out. Small side note: deviceInfo failed with ISAPIError again on this run, but pyhik.dump read it fine yesterday (type IPC), so this NVR's deviceInfo is flaky. (Edit: wrong, it was my venv missing xmltodict. See below.) Good thing detection doesn't depend on it.

Fixture check: up to you and @mezz64. If it helps, a test in the GitHub workflow (or a pytest test over tests/fixtures/) would catch every contributor, while pre-commit only catches people who installed the hook. A pre-commit hook running the same check could be a nice extra. Either way, it's worth reusing the redaction patterns from dump.py so the two stay in sync.

[baylanger]

this NVR's deviceInfo is flaky

If you run ~ 2-3 times is the deviceInfo always unknown? If not, could always add 3 retries and give up after.

[ptarjan]

If you run ~ 2-3 times is the deviceInfo always unknown?

Mostly, yes. Here is what I have from this NVR (DS-7608NI-EV2/8P, V3.4.96) over three days:

when	tool	deviceInfo
11-07, 2 runs (both modes)	live_image_check @ #154	failed (ISAPIError, a ≥400 status other than 401/403/404)
11-07, 2 more runs	same	failed
11-08	dump.py	answered (type IPC)
11-09	live_image_check (write test)	failed

So 1 success out of 6 runs. Also, inside each failing run it was really tried 3 times back to back: get_device_info() doesn't cache a failure, so get_device_model(), get_device_type() and get_firmware_version() each made their own request, and all three failed. A tight 3-retry loop would have made no difference here. Whatever the NVR is doing seems to last minutes, not milliseconds.

(Edit: this turned out to be my test setup missing xmltodict, not the NVR. See below.)

For what it's worth, I don't think retries would buy much here, but that's your and @mezz64's call. What #154 already does (treat a deviceInfo error like an unknown type and fall back to trying the proxy endpoints) is what made every one of those runs pass anyway. Even when deviceInfo does answer, it says IPC, so the type alone was never going to pick the proxy path on this box. One idea, if you think it's worth it: cache the failure for the session so a broken deviceInfo costs one request instead of three.

I didn't record the exact status code. If it would help, I can capture it on the next run.

[baylanger]

Good idea caching the device_info and the script now prints the actual error text.

[ptarjan]

Thanks, the error text found it. I need to correct what I said earlier: the NVR's deviceInfo isn't flaky. The problem was my test environment.

I re-ran read-only on tools/live_image_check.py @ 34310bd with pyhik @ f37aa38, in both modes:

device: model=? (ISAPIError: xmltodict is required for ISAPI client. Install it with: pip install xmltodict), ...
device type: unknown (could not read it: ISAPIError)
camera image endpoints failed (ISAPIError: Request failed with status 503 (status=503, sub_status=serviceUnavailable)); trying the proxy endpoints
proxy endpoints: yes (the proxy endpoints work)

All checks passed (8 channels, settings, values).

My venv was installed without the isapi extra, so xmltodict was missing. Every "deviceInfo failed" in my earlier runs was this, never the NVR. dump.py parses with ElementTree, and that's why it was the one run where deviceInfo answered (DS-7608NI-EV2/8P, type IPC, V3.4.96). The 1-in-6 table I posted is wrong, sorry about that.

Two things worth taking from it:

• The fallback passed a real test. With deviceInfo completely unreadable, auto-detect still got the camera endpoints' 503 and switched to ImageProxy on its own.
• The dependency split looks like a trap. image.py works without xmltodict (ElementTree), but ISAPIClient.request() won't parse any response at all without it, deviceInfo included. That means an install without pyhik[isapi] lists and edits image settings fine but can't identify the device. If the HA integration ends up going through ISAPIClient, its manifest would probably need xmltodict (or pyhik[isapi]). Inside pyhik, the options I can see are making it a hard dependency or moving the client's parsing onto ElementTree so there's only one XML path. That's for you and @mezz64 to decide; I'm just flagging what I saw while testing.

[baylanger]

I was slightly suspicious on the deviceInfo being flaky, it didn't seem right. Glad we have the real root cause.

ISAPIClient.request() parses every response with xmltodict which was only installed with the isapi extra. Without it, image settings still worked but get_device_info() and the other request() methods failed. q

I noticed xmltodict was an optional package when ISAPIClient was added, so event-stream-only users did not get a new dependency. Now that ISAPIClient is a main feature of the library, the half-working install costs more than one small package does.

I opt to make xmltodict a required dependency. Rewriting the client on ElementTree (cleaner but it touches ~ 27 callers), I propose we go with this and in the future we can work to drop xmltodict for ElementTree; If that's ok with both of you?

[ptarjan]

No objection from me as the hikvision integration's codeowner, but it's @mezz64's call as the maintainer. If it does become required, I'd expect nothing extra in the integration's manifest, since pip pulls it in with pyhik, and HA core already pins xmltodict==1.0.4, so there's no conflict with a >=0.13.0 floor.

Moving to ElementTree later as a separate PR sounds reasonable to me, if @mezz64 wants to go that way.

[baylanger]

Fantastic. Unless there's anything else to change here or for #153 , once both PRs are merged and there's a new release, I can start doing the HA ones.

For #154 perhaps a module is a better option than a script?

For the "dump" feature, I'll try to finish the PR this week.

[ptarjan]

Nothing else from me on #151. I also read #153 again and have no concerns there, but of course both are up to @mezz64.

On #154, module vs script: that's your and @mezz64's call. For what it's worth, a module would help with bug reports: anyone with pyhik installed (including inside an HA venv or container) could run something like python -m pyhik.check --host … without cloning the repo, and it would always match their installed version. A few ideas if you go that way, take or leave them:

• Keeping it stdlib-only on top of pyhik's own deps, and not importing it from pyhik/__init__.py, so it costs nothing when unused.
• Keeping the password out of argv (env var or prompt), as it does today.
• Possibly having dump share the same entry point later (python -m pyhik check|dump), so a bug report needs one command.

Looking forward to the HA PRs after the release. Ping me on them and I'll review as the integration's codeowner. The HA maintainers have the final say there, of course.

[mezz64]

Thank you both for all the testing on this, i'll get it merged. @baylanger feel free to open up a new PR to add the xmltodict requirement, i'm ok with that.