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

Clean up/add Polar sensors #47

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

peterjeschke
Copy link
Contributor

This PR cleans up some (most?) sensors for the Polar unit and adds one sensor for the currently active feeding plan. I want to add more sensors, but that will have to wait for another PR - I don't want to push too many changes at once :D

The PR is already pretty large because it covers the following things:

Changes

Remove sensors not available in Polar units

While the API does return values for these sensors, the device does not actually support these:

  • food_low - Polar units only have a plate and don't measure how much food is in them
  • whether_in_sleep_mode - The devices (to my knowledge) do not have any sleep mode
  • temperature - Polar units actually do support temperatures (visible when spying on the MQTT communication) but the API does not report it (value is always 0°C). It only supports a "snowflake" symbol in the app, but I'm not even sure what it means.

I wanted to remove the battery data as well because my unit always reports that it's empty 🤔 But I'll spend some time investigating this because it's supposed to support this.

Add missing device/state classes

This isn't just for the Polar unit but for all sensors I could find that can have a "proper" device class.

Clean up properties in the Polar device class

Some properties were not used/duplicate:

  • available was not used and was wrong - the property device does not exist
  • device_sn is a duplicate of serial from the Device class
  • mac_address is a duplicate of mac from Device. Also wasn't used
  • temperature was already removed as a sensor because the API does not report the temperature
  • food_low and whether_in_sleep_mode also have no sensor any more

Most properties were updated to return None if they have no value. This allows HomeAssistant to signal that the value is not available instead of hardcoding something.

The feeding times were updated to return proper datetimes and the sensors now show as timestamps. This unifies the date and time fields into one and allow users to have their own locale for the fields. It also knows about the proper timezone now (so far, the time was always off for me because the timezone was wrong). This could also allow users to properly use the date/times in automations or scripts.

More notes

I did some clean up for what I feel is "better" like sensor properties returning None instead of a hardcoded default value. I also updated logging to not use f-strings because I guess it's "better practice". I can totally understand if you're not happy with these changes though and can revert this.

If you're happy with what I did in this PR, I'm more than happy to do the same clean up for other devices as well, just to keep everything in a common format. But I only have a Polar unit, so that's the first thing I worked on.

This PR obviously has some changes to some sensors (removing some, consolidating multiple into one, changing the device types and possible values). So if there are any existing automations based on these sensors, the changes will probably break something:

  • device_sn
  • next_feeding_day
  • next_feeding_time
  • next_feeding_end_time
  • food_low
  • whether_in_sleep_mode
  • temperature

@jjjonesjr33 jjjonesjr33 self-assigned this Dec 18, 2024
@jjjonesjr33 jjjonesjr33 added the In review Pending review label Dec 18, 2024
Copy link
Owner

@jjjonesjr33 jjjonesjr33 Dec 21, 2024

Choose a reason for hiding this comment

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

The @property def available is required as that is needed for binary_sensor.py as referring to the connected status for Wi-Fi.

Copy link
Owner

Choose a reason for hiding this comment

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

The @property def device_sn is required to maintain uniformity across all devices as far as displaying the serial number of each devices in sensor.py

Copy link
Owner

Choose a reason for hiding this comment

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

The @property def mac_address is required as that is used in sensor.py

Copy link
Owner

Choose a reason for hiding this comment

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

The @property

  • next_feeding_day
  • next_feeding_time
  • next_feeding_end_time

These are all used and listed in sensor.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't see any references to available (the WiFi sensor uses online) or next_feeding_day (the PR actually removes it). mac_address is also not used by the Polar sensors, mac is used instead.

next_feeding_time and next_feeding_end_time are still there, I didn't remove them

I replaced device_sn with serial because serial is already defined in the Device class. Since all devices use the same deviceSn field, it is also possible to remove device_sn from every Device and use serial everywhere. But I can revert this in this PR and revisit this later. Same thing for the mac address as well - All devices implement mac_address, but all devices except GranarySmartCameraFeeder end up using mac from Device (and even GranarySmartCameraFeeder uses the same API field)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jjjonesjr33 I restored the device_sn did you have the time to look at my other comment(s)?)

@jjjonesjr33
Copy link
Owner

Since this is a relatively a newer product, I am a bit hesitant to remove anything that is listed via their API. I'm also wondering if they will add more to their API at some point. Hopefully they will, but as to their timeframe who can really say when and if.

As far as the f-strings cleanup, I would be okay with this when we are cleaning up the whole project to implement this. My goal as of right now is to try and keep the project somewhat uniformed as possible until we can perform a full cleanup/update. Plus with us adding devices in still there's bound to be a mix-up from different developers working together and how each of us code. I would say if you wanted to undertake this you might want to wait until we get more "polished" 😄

Overall not to bad, I like where you were heading with this, just key thing to note is the devices may be listed under feeders/fountains. But a majority of their components are spread out under binary_sensor.py, button.py, number.py, sensor.py, and switch.py

Copy link
Owner

@jjjonesjr33 jjjonesjr33 left a comment

Choose a reason for hiding this comment

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

Please review comments left in pull request #47

@jjjonesjr33 jjjonesjr33 added Deferred put off (an action or event) to a later time; postpone and removed In review Pending review labels Dec 21, 2024
While the API does return values for these sensors, the device does not actually support these:

- food_low - Polar units only have a plate and don't measure how much food is in them
- whether_in_sleep_mode - The devices do not have any sleep mode

Polar units actually *do* support temperatures (visible when spying on the MQTT communication) but the API does not report it. It only supports a "snowflake" symbol in the app, but I'm not even sure what it means.
Some properties were not used/duplicate:

 - `available` was not used and was wrong - the property device does not exist
 - `device_sn` is a duplicate of `serial` from the Device class
 - `mac_address` is a duplicate of `mac` from Device. Also wasn't used
 - `temperature` was already removed as a sensor because the API does not report the temperature

Most properties were updated to return `None` if they have no value. This allows HomeAssistant to signal that the value is not available instead of hardcoding something.

The feeding times were updated to return proper datetimes and the sensors now show as timestamps. This unifies the date and time fields into one and allow users to have their own locale for the fields. It also knows about the proper timezone now. This could also allow users to properly use the date/times in automations or scripts.

Add debug logging
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deferred put off (an action or event) to a later time; postpone
Projects
Status: Deferred
Development

Successfully merging this pull request may close these issues.

2 participants