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

Display mixin fixes #49

Merged
merged 2 commits into from
Aug 30, 2021

Conversation

stephengolub
Copy link

Was testing some things out and realized that there was an error in the display.py code where it wasn't handling the error response keys properly.

Also added the ability to toggle the watermark on/off

@themoosman
Copy link
Contributor

Good catch, LGTM.

@Benehiko any issues?

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.

Thank you for your contribution! :)

I just have a minor comment for my own sanity, if this value key is incorrect then that would mean every mixin is referencing it incorrectly.

return True
print("Could not set OSD. Camera responded with status:", r_data["value"])
print("Could not set OSD. Camera responded with status:", r_data["error"])
Copy link
Member

Choose a reason for hiding this comment

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

Would this then not be the case for every mixin since they all reference the error with the "value" key?
Maybe this was recently added in a camera firmware update?

Copy link
Author

Choose a reason for hiding this comment

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

Possibly. Haven't dove that far in. Just addressed here since that's where the handling was already.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would this then not be the case for every mixin since they all reference the error with the "value" key?
Maybe this was recently added in a camera firmware update?

Yes, it would.

@stephengolub
Copy link
Author

stephengolub commented Aug 24, 2021 via email

@Benehiko
Copy link
Member

Yeah I'll have to implement one then, will merge this so long! Thank you @stephengolub for the contribution :)

@Benehiko Benehiko merged commit 5b782bf into ReolinkCameraAPI:master Aug 30, 2021
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.

3 participants