-
-
Notifications
You must be signed in to change notification settings - Fork 35.8k
Add energy price calendar platform to Teslemetry #145848
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
base: dev
Are you sure you want to change the base?
Conversation
cc48592 to
25287fa
Compare
25287fa to
56c4bdb
Compare
|
I am wondering how well a calendar works for this. As in, people generally need this for raw data, while a calendar is used for things that happen during a day |
|
Right I agree that it shows, but I would argue that it's not a calendar thing. As in, if I would import this in my calendar app my app would be unusable. |
|
@joostlek are you saying this should just be a sensor entity with the current price? |
|
And maybe a service with json return for forecasting data. But I'm not sure if my opinion is shared by others so I'm open to other arguments |
|
I'm struggling with a similar use case (sensor vs calendar) The calendar has the benefit of conveying future information as the sensor will only show current values. If I want to do an automation that needs to know the future tariff start time and duration the sensor is not enough. |
|
@joostlek the best case I have, is that a sensor entity will only ever tell you whats happening now, but the calendar entity allows more flexible automations with offsets from the start and end of the segment. https://www.home-assistant.io/integrations/calendar/#automation The primary usecase I can think of here is that Tesla Powerwalls take a few minutes for certain actions to take effect, so a user may wish to have automations run a few minutes before a certain tarrif period starts or ends. I fully accept people may never want to see this on a calandar in the UI, but its calendar like data that the Home Assistant calendar platform serves well. There is a future possibility that I will allow editing through the create and delete event actions. |
|
@joostlek can I please bump this review. |
|
I will throw this for in the architecture meeting on thursday |
|
@joostlek did you get an architecture answer on this? |
|
looking into this issue before submitting yet another integration tracking tariff using a calendar entity. |
|
This was discussed in the architecture meeting, but it was forgotten to update here. The conclusion was:
I think it's fine to split that in two PRs: One which adds the calendar (this PR) and one which adds a service returning data. |
|
Thanks for letting me know @emontnemery, @joostlek does that mean we can push this one through? |
56c4bdb to
45aa5fc
Compare
|
|
||
| def flatten(data: dict[str, Any], parent: str | None = None) -> dict[str, Any]: | ||
| def flatten( | ||
| data: dict[str, Any], parent: str | None = None, exceptions: list[str] | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| data: dict[str, Any], parent: str | None = None, exceptions: list[str] | None = None | |
| data: dict[str, Any], parent: str | None = None, *, exceptions: list[str] | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this achieve? Is it simply forcing exceptions (now skip_keys) to be named?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. It improves readability because it's clearer what the optional passed in list is for
|
@Bre77 please address the review comments and mark the PR ready for review when done |
Add calendar entities for energy site tariff schedules, providing: - Buy tariff calendar showing energy purchase pricing periods - Sell tariff calendar showing energy sell-back pricing periods Features: - Displays current active pricing period - Shows seasonal pricing variations - Handles time-of-use (TOU) periods - Supports midnight crossing periods
- Update TeslemetryEnergyInfoCoordinator to preserve tariff structure during flattening - Fix test file to only test energy tariff calendars - Update snapshots for energy price calendar entities
45aa5fc to
09c50ce
Compare
Co-authored-by: Erik Montnemery <[email protected]>
| from_day = period_def.get("fromDayOfWeek", 0) | ||
| to_day = period_def.get("toDayOfWeek", 6) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct that the API doesn't always set fromDayOfWeek + toDayOfWeek, and are the defaults correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the data outside of the API is a protobuf, which treats missing values as their defaults, so we have to fill this back in, as the default is the full week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Can you add a comment in the code then with a reference to where the defaults are defined?
| from_hour = period_def.get("fromHour", 0) % 24 | ||
| from_minute = period_def.get("fromMinute", 0) % 60 | ||
| to_hour = period_def.get("toHour", 0) % 24 | ||
| to_minute = period_def.get("toMinute", 0) % 60 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The defaults here differ from the defaults for weekdays, is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont know what your referring to, this code has no concept of weekdays and weekends, its just start days of week and end days of week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for example, the default here is all values as zero, which equals a full day due to the Handle periods that cross midnight logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that the default for fromDayOfWeek is 0, which means Monday, and the default for toDayOfWeek is 6 which means Sunday, whereas the defaults for from_hour, from_minute, to_hour, to_minute are all 0 which means midnight.
Hence if nothing is set, the period starts at 00:00 Monday, and ends at 00:00 Sunday, that is, the default period spans 6 full days, not a full week. Is that correct?
| # Handle periods that cross midnight | ||
| if end_time <= start_time: | ||
| potential_end_time = end_time + timedelta(days=1) | ||
| if start_time <= now < potential_end_time: | ||
| end_time = potential_end_time | ||
| elif (start_time - timedelta(days=1)) <= now < end_time: | ||
| start_time -= timedelta(days=1) | ||
| else: | ||
| continue | ||
| elif not (start_time <= now < end_time): | ||
| continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add test cases which assert the behavior here. It's very easy to mess up, and even if your code is correct now it might break tomorrow if the code is changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure which behavior specifically you are referring to, as this would already be tested with the fixture and snapshot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a single, non-parametrized, test case each to test the state of the calendar entity as well as testing the get_events service.
What I mean is that you should add parametrization or separate test cases testing the different branches to account for crossing midnight, crossing weeks and so on, since that logic is non trivial and very easy to mess up.
|
|
||
| return None # No active period found for the current time and season | ||
|
|
||
| async def async_get_events( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments as for the event property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried to address feedback in multiple places where I noticed.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |

Proposed change
Add calendar entities for energy site tariff schedules, providing:
Features:
This is a carve out from #142894
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: