Feature: Add hourly forecast support and migrate template sensor to automation#56
Feature: Add hourly forecast support and migrate template sensor to automation#56kiddyfurby wants to merge 7 commits intoolicooper:devfrom
Conversation
55a9f1e to
3f9ad18
Compare
olicooper
left a comment
There was a problem hiding this comment.
Thanks for the PR! I like the increased flexibility this brings, but there are a few changes I've recommended in the code review. I also spotted a usability issue after reviewing the code but I have decided to leave the individual comments on the review even though some might be outdated if the design needs to change.
The main issue is that there are two distinct paths to supplying data here; either the user supplies an entity_id for automatic weather updates, or they enable the weather service and supply data manually with a forecast type. The configuration items are different for each case..
I'm not sure the user will expect to have to supply data to the set_weather_forecast service if they choose to manually specify the forecast_type in the config, and the user might expect that the automatic weather updates will change to hourly/daily if they change the forecast_type in the config, but this would not work and instead need changing in Home Assistant.
I don't know what the updated config should look like but maybe something like this...
Default config (don't allow forecast_type to be specified):
weather:
entity_id: !secret weather_entity_idUsing weather forecast service (don't allow entity_id to be specified):
## No entity_id supplied, use set_weather_forecast service instead
weather:
## This could also be specified in the set_weather_forecast call?
forecast_type: hourlyThe example advanced config should be updated to show how to use this new functionality.
Let me know what you think. Thanks!
| if (this->weather_forecast_type_.empty()) { | ||
| this->subscribe_homeassistant_state( | ||
| &NSPanelLovelace::on_weather_forecast_update_, | ||
| this->weather_entity_id_, to_string(ha_attr_type::forecast)); | ||
| } else { | ||
| this->register_service(&NSPanelLovelace::set_weather_forecast_data, | ||
| "set_weather_forecast", {"forecast"}); | ||
| } |
There was a problem hiding this comment.
I'm not sure the user will expect to have to supply data to the set_weather_forecast service if they choose to manually specify the forecast type in the config? See notes in the main comment for more info.
| filter["forecast"][0]["datetime"] = true; | ||
| filter["forecast"][0]["condition"] = true; | ||
| filter["forecast"][0]["temperature"] = true; |
There was a problem hiding this comment.
Have you tried this on ESPHome version < 2025.7.0 because of the static 200 byte allocation in ArduinoJson::StaticJsonDocument<200> filter; - I wonder if this is now too low for the 3 additional filters?
There was a problem hiding this comment.
I have only tested on ESPHome 2025.12.
Does ESPHome has a way for custom components to specify their minimum supported ESPHome version?
Do you intend to support 2025.12 ?
There was a problem hiding this comment.
Okay, I think we can avoid the issue by bumping the required version up to 2025.7.0 (which was added in this commit 6 months ago)...
| ESPTime now; | ||
| #ifdef USE_TIME |
There was a problem hiding this comment.
Is ESPTime available when USE_TIME is false?
| if (weather_entity_is_hourly && now.is_valid()) { | ||
| tm t_filter{}; | ||
| if (iso8601_to_tm(item["datetime"].as<const char *>(), t_filter)) { | ||
| long f_time = (t_filter.tm_year + 1900) * 1000000 + | ||
| (t_filter.tm_mon + 1) * 10000 + t_filter.tm_mday * 100 + | ||
| t_filter.tm_hour; | ||
| long n_time = now.year * 1000000 + now.month * 10000 + | ||
| now.day_of_month * 100 + now.hour; | ||
| if (f_time <= n_time) | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
I am not sure about having a blanket filter without it being user configurable. What if the time source is incorrect (but valid) on the nspanel, or the weather data is slightly old because the internet connection dropped? I'd rather see some weather forecast data rather than nothing. Personally I'd turn this off by default as this is the current behavior and I don't think filtering happens in the upstream nspanel firmware. Maybe this is a task best offloaded to Home Assistant (which also has more computational power for string conversions etc.) and the nspanel trusts that the data supplied is current and valid? Then the user can choose how they want the data displayed?
Couple of additional notes:
- I can see
now.is_valid()is checked andn_timeis calculated on every loop iteration. - I believe the forecast data is always in chronological order, so to filter old forecast data you only need to check if the date is valid for the first X items? I might be wrong about that though.
| void set_weather_forecast_type(const std::string &weather_forecast_type) { | ||
| this->weather_forecast_type_ = weather_forecast_type; | ||
| } |
There was a problem hiding this comment.
I can't see a need to store a type string as it is only used for comparison. Could this be changed to something like an enum to restrict the available types to known values? It will also reduce the binary size 👍 You can still use a string in the esphome config as this is nicer for the user.
enum class weather_forecast_type : uint8_t
{
automatic = 0, // or 'default'
hourly,
daily
};|
Thank you for the helpful suggestions! I've removed the need to specify forecast_type: hourly/daily in the ESPHome config; it now relies solely on the data sent from the Home Assistant service. I’ve also moved the JSON filtering to the HA side. Because of this re-architecting, some of the issues mentioned above no longer apply—please excuse me if I haven't replied to those points individually |
There was a problem hiding this comment.
Thanks for looking at those suggestions. Firstly, good work! I do have a few more comments though, sorry!
I've tried building this but it fails because custom_services is missing from the config as seen in this esphome example. This requirement might need to be highlighted in the readme too for when the user sets the forecast_method to service (unless there is a way to automatically enable it)?
advanced-example.yaml
api:
# ## This is required when using `forecast_method: service` for weather updates
# custom_services: trueIt looks like you will need to wrap all code that refers to this feature with the USE_API_SERVICES macro. This has the added benefit of reducing binary size if it is not used too. For example (incomplete and untested)...
nspanel_lovelace.h
#ifdef USE_API_SERVICES
void set_weather_forecast_method_service(bool value) {
this->weather_forecast_method_service_ = value;
}
#endifnspanel_lovelace.cpp
#ifdef USE_API_SERVICES
if (this->weather_forecast_method_service_) {
this->register_service(&NSPanelLovelace::set_weather_forecast_data,
"set_weather_forecast_data", {"forecast"});
}
#endifI think there will be other places to add the check and it might be good to force weather_forecast_method_service_ = false if USE_API_SERVICES is not defined or simply display a clear error message and fail to build if the user hasn't added custom_services: true to their config but has set forecast_method: service.
In the future I was hoping to have weather entities that can be used on any page, but this will require storing the parsed result somehow so this functionality should be okay for now but might need reviewing in the future. I am just trying to think how the current changes might impact on future plans and make sure the config introduces minimal breaking changes.
Thanks!
| filter["forecast"][0]["datetime"] = true; | ||
| filter["forecast"][0]["condition"] = true; | ||
| filter["forecast"][0]["temperature"] = true; |
There was a problem hiding this comment.
Okay, I think we can avoid the issue by bumping the required version up to 2025.7.0 (which was added in this commit 6 months ago)...
|
Thankfully
It also looks like checking against Example init.py: if CONF_SCREENSAVER_FORECAST_METHOD in weather_config and \
weather_config[CONF_SCREENSAVER_FORECAST_METHOD] == "service":
## Maybe this could be added but it might be ignored because of `to_code` coroutine execution order?
# cg.add_define("USE_API_SERVICES")
cg.add_define("USE_NSPANEL_CUSTOM_SERVICES")
cg.add(nspanel.set_weather_forecast_method_service(True))Also, make sure to test against the various yaml config files e.g. |
# Conflicts: # components/nspanel_lovelace/nspanel_lovelace.cpp
|
Thank you. I think I have address most if not all your points. |
This pull request introduce two main improvements aimed at enhancing the user experience and flexibility of the integration:
Hourly Forecast Support: Added the ability to fetch and display hourly data, providing more granular weather insights.
Migration to Automation: I have transitioned the Home Assistant template sensor logic into an automation.
The Benefit: This change allows users to modify the weather JSON script without needing to restart Home Assistant for changes to take effect, making the configuration much more dynamic and user-friendly.
Let me know if you'd like any change.