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 api v7 #65

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Vaelatern
Copy link

I'll be slowly working on this as I need it. https://www.reddit.com/r/reolinkcam/comments/xpcxq6/reolink_camera_api_user_guide_v7_update_in_sept/

Please consider merging, or telling me what to improve so my work can be merged.

Have a great day!

Just a print so the login failure reason is recorded
These come disabled by default. The arg is 1 or 0, so we take a boolean
and coerce it to integer, for the API's sake.

Further we'll check the error code on the response
Copy link
Member

@Benehiko Benehiko left a comment

Choose a reason for hiding this comment

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

Hey @Vaelatern

Thank you for the contribution! I just have some questions about the conversion from bool to int since the requests library should take care of this for you.

Comment on lines +21 to 53
if http_enable:
http_enable = 1
else:
http_enable = 0
if https_enable:
https_enable = 1
else:
https_enable = 0
if onvif_enable:
onvif_enable = 1
else:
onvif_enable = 0
if rtmp_enable:
rtmp_enable = 1
else:
rtmp_enable = 0
if rtsp_enable:
rtsp_enable = 1
else:
rtsp_enable = 0

body = [{"cmd": "SetNetPort", "action": 0, "param": {"NetPort": {
"httpEnable": http_enable,
"httpPort": http_port,
"httpsEnable": https_enable,
"httpsPort": https_port,
"mediaPort": media_port,
"onvifEnable": onvif_enable,
"onvifPort": onvif_port,
"rtmpEnable": rtmp_enable,
"rtmpPort": rtmp_port,
"rtspEnable": rtsp_enable,
"rtspPort": rtsp_port
Copy link
Member

Choose a reason for hiding this comment

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

We could rather use int() to convert the boolean instead of writing an if statement to map the boolean values to an integer.

~ ⟩ python3
Python 3.11.2 (main, Feb  7 2023, 13:52:42) [GCC 11.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> print(int(True))
1
>>> print(int(False))
0

Copy link
Member

Choose a reason for hiding this comment

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

probably don't even need to convert from bool to int so the variable can be passed along directly

@@ -41,14 +41,14 @@ def set_osd(self, bg_color: bool = 0, channel: float = 0, osd_channel_enabled: b
body = [{"cmd": "SetOsd", "action": 1,
"param": {
"Osd": {
"bgcolor": bg_color,
"bgcolor": int(bg_color),
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if one needs to convert bool explicitly here to int since requests will convert this for us when sending the request.

"channel": channel,
"osdChannel": {
"enable": osd_channel_enabled, "name": osd_channel_name,
"enable": int(osd_channel_enabled), "name": osd_channel_name,
Copy link
Member

Choose a reason for hiding this comment

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

same here

Comment on lines +50 to +51
"osdTime": {"enable": int(osd_time_enabled), "pos": osd_time_pos},
"watermark": int(osd_watermark_enabled),
Copy link
Member

Choose a reason for hiding this comment

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

and same here

@@ -51,7 +51,7 @@ def set_recording_encoding(self,
"action": 0,
"param": {
"Enc": {
"audio": audio,
"audio": int(audio),
Copy link
Member

Choose a reason for hiding this comment

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

same here

@Vaelatern
Copy link
Author

I think I did conversions to int() because of a bug I ran into.

I forget.

I'm happy to make those changes though.

It's worth noting this doesn't support all of v7, just the parts I needed.

If you think that's fine, I'm willing to apply the requested changes and see what happens.

@Benehiko
Copy link
Member

I think I did conversions to int() because of a bug I ran into.

I forget.

I'm happy to make those changes though.

It's worth noting this doesn't support all of v7, just the parts I needed.

If you think that's fine, I'm willing to apply the requested changes and see what happens.

Yes, could you verify that there is indeed an error if you do not convert to int?

Also please update the PR title using conventional commits as it is good practice and be more specific than api v7.
To me this is more of a cleanup of some of the function parameter types and also you fix the modify user function to accept the old password. Here is an example: fix: cleanup function parameter types to conform to api v7 specs

https://www.conventionalcommits.org/en/v1.0.0/#summary

@Vaelatern
Copy link
Author

Vaelatern commented Apr 14, 2023

I'm still going to update this

Please note the PR title is disconnected from the commits, which are correctly named. The PR title you can edit to describe as you like.

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

2 participants