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

Restore camera battery_voltage and wifi_strength #877

Merged
merged 7 commits into from
Feb 4, 2024

Conversation

dashrb
Copy link
Contributor

@dashrb dashrb commented Jan 30, 2024

Description:

Full cameras (outdoor XT, XT2, and indoor) lost their battery voltage and wifi_strength fields in release v0.22.5. Due to a logic bug, these cameras no longer had their REST API queried for their full set of information; only the "base" information contained on the "home screen" was being stored.
This should restore the REST API call, and those fields should appear again.

Related issue: fixes #865

Alas, a number of "downstream" changes were made in v0.22.6 which possibly need to be reverted, now that the "full camera attributes" are available again.
Consider looking at changes made in #835, #837, and #867.

Checklist:

  • Local tests with tox run successfully PR cannot be meged unless tests pass
  • Changes tested locally to ensure platform still works as intended
  • Tests added to verify new code works

Note that the "full" cameras have their own URL API which must be queried for their attributes.
the wifi_strength key is no longer missing
Copy link
Contributor

@mkmer mkmer left a comment

Choose a reason for hiding this comment

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

Should we also add a property for battery voltage? (I think so)

@dashrb
Copy link
Contributor Author

dashrb commented Jan 30, 2024

Should we also add a property for battery voltage? (I think so)
I did that, right? camera.py lines 33, 63, 251... Or do I misunderstand?

That said, the raw JSON returned from the camera REST call is pretty lengthy (about 175 fields), and I don't know what many of the fields even mean. In particular, there are a number of fields which have a lfr_ prefix. Not sure what that is but might speculate that it refers to the sync module that "owns" the camera.

@mkmer
Copy link
Contributor

mkmer commented Jan 30, 2024

A property like this:

@property
def battery(self):
"""Return battery as string."""
return self.battery_state
except returning the self._battery_voltage. and call the property battery_voltage....
This allows an application to get the value without getting the whole JSON - a bit redundant, but we have properties for the other values HA uses.
We should also have and _ in the self version so that it's not confused with the property version...

@mkmer
Copy link
Contributor

mkmer commented Jan 30, 2024

From what I've been able to "decode" - LFR_ is the network the blink cameras use to talk to the sync module (it's not wifi). My doorbells report the LFR signal strength.

@dashrb
Copy link
Contributor Author

dashrb commented Jan 30, 2024

The lfr_ thing seems like a good assessment. From my indoor camera, the lfr_ and wifi_ fields are:

      "lfr_sync_interval": 8,
      "lfr_strength": -43,
      "last_lfr_alert": "",
      "lfr_alert_count": 0,
      "wifi_timeout": 30,
      "wifi_strength": -27,
      "last_wifi_alert": "",
      "wifi_alert_count": 0,
      ....
      "last_connect": {
        "lfr_strength": -40,
        "lfr_108_wakeups": 2,
        "lfr_tb_wakeups": 65282,
        "wifi_strength": -38,
        "wifi_connect_failure_count": 0,
      .....

Copy link
Contributor

@mkmer mkmer left a comment

Choose a reason for hiding this comment

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

Suggestions...

blinkpy/camera.py Outdated Show resolved Hide resolved
blinkpy/camera.py Outdated Show resolved Hide resolved
blinkpy/camera.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bb47175) 99.79% compared to head (575e37a) 99.79%.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #877   +/-   ##
=======================================
  Coverage   99.79%   99.79%           
=======================================
  Files           8        8           
  Lines        1482     1486    +4     
=======================================
+ Hits         1479     1483    +4     
  Misses          3        3           
Flag Coverage Δ
unittests 99.79% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mkmer
Copy link
Contributor

mkmer commented Jan 30, 2024

Well... They all should have properties at some point, we have the json as a fallback. At this point, we keep moving forward slowly :)
Thanks for adding it.

@fronzbot fronzbot merged commit 0882c5f into fronzbot:dev Feb 4, 2024
8 checks passed
@dashrb
Copy link
Contributor Author

dashrb commented Mar 9, 2024

Note to self: the aforementioned reversions in #835, and #837 were both implemented in this ticket; furthermore #867 appears to be perfectly fine, as the temperature in Fahrenheit is seemingly located both in temperature and also in signals.temp. No further actions are needed, AFAIK.

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.

Camera battery_level no longer reflects voltage
3 participants