Temperature time sensors are not timestamp data type

jazzyisj commented 1 month ago

Describe the bug

The recently added time sensors for high and low temperatures are not timestamp data type.

Expected behavior

A properly instantiated datetime entity can be used to display the date or time in a human friendly format in the UI like this using the entity format parameter.

  - entity: sensor.outdoor_high_temperature_time
    name: "High Temperature Time"
    format: time

Actual behavior

The Pirateweather time state sensors are not proper timestamps and do not work with the entity format parameter in the UI.

      - entity: sensor.pirateweather_daytime_high_apparent_temperature_time_0d
        name: "High Temperature Time" 
        format: time

Home Assistant version

2024.8.0b2

Integration version

1.5.3

Other details

https://developers.home-assistant.io/docs/core/entity/datetime/

Troubleshooting steps

[X] I have updated my Home Assistant installation to the latest version.
[X] I have updated the Pirate Weather Integration to the latest version.
[X] I have gone through the documentation before opening this issue.
[X] I have searched this repository and API Repository to see if the issue has already been reported.
[X] I have restarted my Home Assistant installation.
[X] I have queried the API in my browser to confirm the issue is not with the API.
[X] I have written an informative title.

cloneofghosts commented 1 month ago

Thanks for opening this issue. What's happening here is those sensors in the API are in epoch time and not a timestamp. You'll need to convert them to a timestamp.

I'm not sure if its possible to convert them into a timestamp in the integration itself so I'll ping @alexander0042 to see if its possible

alexander0042 commented 3 weeks ago

Good catch here, and sorry for the slow reply- I was working on fixing some stability issues with the primary API, so that was the focus. The good news here is that this was an easy change, and will be out in 1.5.5 today!

cloneofghosts commented 3 weeks ago

V1.5.5 has just been released which contains this fix.

jazzyisj commented 3 weeks ago

Hate to be a pain, but the device_class of the pirate weather time sensors is still not a timestamp.

This is a properly formatted timestamp sensor from the Environment Canada integration.

This is a newly formatted Pirateweather time sensor.

The daily low temperature and forecast time sensors were completely missed.

alexander0042 commented 3 weeks ago

Thanks for mentioning the Environment Canada integration! I'll take a look at it to see what they do and go from there!

cloneofghosts commented 3 weeks ago

@alexander0042 Was testing the update in a devcontainer before creating a new release and am running into this error with the daily low sensor:

Logger: homeassistant.components.sensor
Source: helpers/entity_platform.py:598
integration: Sensor (documentation, issues)
First occurred: 10:28:22 AM (3 occurrences)
Last logged: 10:32:12 AM

Error adding entity sensor.pirateweather_daily_low_temperature_time_0d for domain sensor with platform pirateweather
Traceback (most recent call last):
  File "/home/vscode/.local/lib/python3.12/site-packages/homeassistant/components/sensor/__init__.py", line 594, in state
    if value.tzinfo is None:
       ^^^^^^^^^^^^
AttributeError: 'int' object has no attribute 'tzinfo'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/vscode/.local/lib/python3.12/site-packages/homeassistant/helpers/entity_platform.py", line 598, in _async_add_entities
    await coro
  File "/home/vscode/.local/lib/python3.12/site-packages/homeassistant/helpers/entity_platform.py", line 912, in _async_add_entity
    await entity.add_to_platform_finish()
  File "/home/vscode/.local/lib/python3.12/site-packages/homeassistant/helpers/entity.py", line 1366, in add_to_platform_finish
    self.async_write_ha_state()
  File "/home/vscode/.local/lib/python3.12/site-packages/homeassistant/helpers/entity.py", line 1005, in async_write_ha_state
    self._async_write_ha_state()
  File "/home/vscode/.local/lib/python3.12/site-packages/homeassistant/helpers/entity.py", line 1130, in _async_write_ha_state
    self.__async_calculate_state()
  File "/home/vscode/.local/lib/python3.12/site-packages/homeassistant/helpers/entity.py", line 1067, in __async_calculate_state
    state = self._stringify_state(available)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/vscode/.local/lib/python3.12/site-packages/homeassistant/helpers/entity.py", line 1011, in _stringify_state
    if (state := self.state) is None:
                 ^^^^^^^^^^
  File "/home/vscode/.local/lib/python3.12/site-packages/homeassistant/components/sensor/__init__.py", line 605, in state
    raise ValueError(
ValueError: Invalid datetime: sensor.pirateweather_daily_low_temperature_time_0d has timestamp device class but provides state 1724230800:<class 'int'> resulting in ''int' object has no attribute 'tzinfo''

alexander0042 commented 3 weeks ago

Thanks for testing this! I missed that variable since it has a different name than the others, but just pushed a commit that should fix it

cloneofghosts commented 3 weeks ago

Yup, that fixed the issue. One thing I noticed during testing was the Overnight Low Time sensor was showing the data from temperatureMinTime instead of temperatureLowTime so it shows the low as being 6h ago instead of being in 18h.

cloneofghosts commented 2 weeks ago

Other than the issue I pointed out above (maybe it's working as intended?) are we good to release a new version? Been holding off releasing a new version to make sure its stable. I've tested locally and everything works so I think we're good to release the changes?

alexander0042 commented 2 weeks ago

Yea, I was a little confused by the high/low min/max for that one as well. I initially changed it to match to description in the text, so it's pulling the high/low (Day/Night) times for both apparent and temperature. However, in the entity description it's coded as min, and I think to keep things consistent it's better not to change it, so I just pushed a change to set it back to min

cloneofghosts commented 2 weeks ago

Tested the changes again and everything is looking good. Is this good to release or do you want to look at #241 first?

alexander0042 commented 2 weeks ago

Let's leave that one open until we hear back if those sensors would be useful to have and push a new release in the meantime!

cloneofghosts commented 2 weeks ago

Alright will leave that one open. @jazzyisj I've released v1.5.8 which fixes the issues with the datetime sesnors.

jazzyisj commented 2 weeks ago

You guys are fast! Installed and tested - thanks! I'll close the issue.

Pirate-Weather / pirate-weather-ha