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

Reolink privacy mode #135947

Closed
wants to merge 15 commits into from
Closed

Conversation

starkillerOG
Copy link
Contributor

@starkillerOG starkillerOG commented Jan 18, 2025

Breaking change

Proposed change

Reolink recently added a "Privacy mode" to the firmware of the Reolink E1 Zoom.
When the user activates this mode, the camera streams, HTTP API, ONVIF, RTMP, RTSP ports are all shut down.
Basically the only two things the camera can do at this point is disabling the privacy mode or rebooting (only options left in the Reolink app also).
This integration uses those protocols so this causes a whole bunch of errors and problems.

This version bump of the upstream library paired with some changes to the Home Assistant code resolves this issue: The integration will always be able to setup.

During runtime, privacy mode will not cause any errors anymore, all entities of this integration will be marked as unavailable.
As soon as privacy mode is turned off all entities will become available again (push update, so almost instant).

I have a follow up PR ready that will add a switch entity that can enable/disable this privacy mode. This entity will stay available even if privacy mode is turned ON. The user will be able to turn off privacy mode through this entity to get all entities available again.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

@starkillerOG starkillerOG changed the title Bump reolink-aio to 0.11.7 fixing privacy pode Bump reolink-aio to 0.11.7 fixing privacy mode Jan 18, 2025
@starkillerOG starkillerOG added this to the 2025.1.3 milestone Jan 18, 2025
@bdraco
Copy link
Member

bdraco commented Jan 18, 2025

        enable_onvif = kwargs.get("NetPort", {}).get("onvifEnable")
        enable_rtmp = kwargs.get("NetPort", {}).get("rtmpEnable")
        enable_rtsp = kwargs.get("NetPort", {}).get("rtspEnable")

It would be nicer to rewrite that as

        net_port = kwargs.get("NetPort", {})
        enable_onvif = net_port.get("onvifEnable")
        enable_rtmp = net_port.get("rtmpEnable")
        enable_rtsp = net_port.get("rtspEnable")

@starkillerOG
Copy link
Contributor Author

        enable_onvif = kwargs.get("NetPort", {}).get("onvifEnable")
        enable_rtmp = kwargs.get("NetPort", {}).get("rtmpEnable")
        enable_rtsp = kwargs.get("NetPort", {}).get("rtspEnable")

It would be nicer to rewrite that as

        net_port = kwargs.get("NetPort", {})
        enable_onvif = net_port.get("onvifEnable")
        enable_rtmp = net_port.get("rtmpEnable")
        enable_rtsp = net_port.get("rtspEnable")

Thanks for the suggestion, just changed it in the upstream lib: starkillerOG/reolink_aio@9d398a5

@home-assistant home-assistant bot marked this pull request as draft January 18, 2025 23:54
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@starkillerOG starkillerOG marked this pull request as ready for review January 20, 2025 12:20
@home-assistant home-assistant bot requested a review from joostlek January 20, 2025 12:20
Copy link
Contributor

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

If privacy mode is turned ON when starting, the integration will disable privacy mode for about 5-10 seconds to get the needed information to be able to start the integration (mainly the capabalities of the camera). Then the Privacy mode will be enabled again.

I'm against enabling it even for a short period as the user enabled it for a certain reason. The integration could store the needed information to setup in a storage file, so have a cache for already know devices.
If a user adds a new camera with the privacy mode enabled, the integration should offer in my opinion only a restart and privacy mode entity

@home-assistant home-assistant bot marked this pull request as draft January 20, 2025 12:52
@joostlek
Copy link
Member

I also think that we should not do this. As in, Reolink now seem like the good choice for local devices. I think if the devices now provide a privacy mode, and with Home Assistant we break that mode, its not really that privacy after all. I think it the beauty and challenge here is to make sure that we can do without that information. This way the device and the mode does what it promises.

I think for adding a new device I think its a sensible question to ask if the user wants to disable privacy mode, so we can set up the camera correctly.

@starkillerOG
Copy link
Contributor Author

Well this is going to be a challange.
I will see what I can do.

Do you have an example of how to storage a large amount of text in a data file?

Probably the most visable approach would be to cache the complete reponse to all get_host_data commands, and load that in when Privacy mode is enabled.
I am mostly woried what happens when the response changes but it was not retrieved during startup. It contains many one-time values like the preset-position points, limit values for zoom, tracking, and any other number entity, and the options for the select entities. For instance quick replies, that can be added in the Reolink app.
Or after a firmware update, the capabilities of the camera often change.

That info will not be retrieved again, which could lead to some very curious user issues:
for instance the camera no longer supports a feature after a firmware update (This happens, for instance pet detection can be replaced by animal detection which is more advanced, but the pet sensors will not work anymore).
If the user has privacy mode always enabled when at home, and at home he restarts homeassistant and makes his diagnostics files, I will see incorrect capabalities and will have a hard time understanding why things don't work.
Of course this can all be cercumvented with more advanced code, but its going to be a deep rabbit hole I think....

@joostlek
Copy link
Member

Isn't it possible just to compile a list of "this camera supports x, x, x"? Because the entities being set up are either unknown or unavailable, so we should not store enough to know all info, but we should know enough to know which entities to set up.

@starkillerOG
Copy link
Contributor Author

But then how do I get all the other info I need during statup:
UID (unique ids), MAC, models, hardware version (device info).
I also need to figure out which command versions I need (for instance getEvents vs GetAIstate and GetMdState or GetRecV20 vs GetRec etc.) For some the only option is to try with fallbacks during get_host_data to figure out what to use.
But also retrieving the streaming details like h264 encoding vs 265 encoding, I need to check those, you need a diffrent RTSP url depending on the encoding and not all camera's are reporting the encoding correctly (depending on firmware versions) so there is a whole process with multiple diffrent commands to know for sure which RTSP url to use.
Besides all the limits (min/max) and option lists are also only set 1x during startup.

The checking of the RTSP URL will in fact be another thing I will need to take into account, I should also store that URL, did not think of that yet.

@joostlek
Copy link
Member

For the time being I think it would be okay to say, let's just retry if its in privacy mode

@edenhaus
Copy link
Contributor

I would only store data, which is really required for setting up the entities.

But also retrieving the streaming details like h264 encoding vs 265 encoding, I need to check those, you need a diffrent RTSP url depending on the encoding

In my opinion, you don't need to know this kind of information as the camera entity will unavailable and therefore you don't need the difference between the RTSP urls

@frenck frenck removed this from the 2025.1.3 milestone Jan 20, 2025
@starkillerOG
Copy link
Contributor Author

@joostlek @edenhaus my appologies for beeing a bit grumpy about the needed changes.
In the end it was not that hard to get it to work and the result is much better: privacy mode stays on as its supposed to.

There are still some things to do:

  • write and adjust the tests
  • publish a new version of reolink-aio
  • catch the error in the config flow and make a extra step requesting the user if he wants to disable privacy mode for the setup.

However could you take a look at the way I am now saving and loading the required data to a file?
Before I continue finishing this up, I would like to get some feedback if this approach would be accepted like this.

I know you would prefer to save the minimum amount of needed info, but I think it will be a lot more robust towards the future if I save the whole host data json. This way the upstream libary follows the same process as if the camera just responded with data. So I don't need to make exceptions everywhere in case data is not know yet. In the future I would need to remember to add those exception each time I add some new stuff...

@starkillerOG starkillerOG changed the title Bump reolink-aio to 0.11.7 fixing privacy mode Reolink privacy mode Jan 25, 2025
@starkillerOG
Copy link
Contributor Author

starkillerOG commented Jan 25, 2025

This PR has grown too big, therefore I am splitting it up:

@github-actions github-actions bot locked and limited conversation to collaborators Jan 26, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants