Add services for managing Time-of-Use (TOU) schedule for Growatt integration#154703
Conversation
|
@joostlek , any chance you can have a look at this one? |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
- Update fixture names from mock_growatt_api to mock_growatt_v1_api/mock_growatt_classic_api - Fix min_write_time_segment mock to accept variable arguments - Use correct Classic API fixture for non-MIN device test
Rename services from update_min_time_segment/read_min_time_segments to update_time_segment/read_time_segments to support future expansion to other inverter types beyond MIN devices.
- Fix bare Exception catches in service handlers, replace with specific API exceptions - Remove debug logging leftover from development - Add comprehensive error handling tests for services - Improve test coverage for service error paths - All 42 tests passing with 85% overall coverage
1198730 to
a85fe48
Compare
joostlek
left a comment
There was a problem hiding this comment.
Okay I need to look at this when I am not about to head to bed but I think this is a simple thing you can add to make the tests better :)
|
@joostlek - I need guidance on a topic. This would be translated into something like growatt_server.update_charge_time_period in the growatt integration Question: should we prefix the services, so and or WDYT? Want to avoid a breaking change and get it right first time... |
|
I'm on mobile now (I'm on holiday and I was reviewing this yesterday (or at least trying to) because I was bored). But I would need to check what the current parameters were. In a way I would say that making the service generic enough to be used by both devices makes sense (as then blueprints and automations can be shared with a broader audience). But if the input parameters are drastically different then that probably doesn't make much sense. So that would be my first preference, other than that, there are quite a number of growatt models, do we have any vision on if other inverters get the same? Or if they don't have it, do they carry the same type of parameters? If the set of parameters in this PR is only specific for time segments and the one you are talking now is specific for charging ours, then it would make sense to have specific names for them, if we expect every model to have a wildly different set of parameters, having the model name in there would probably make sense (but now that I type this it probably isn't ideal either as you'd have "update segments (min)" "update segments (mix)" and whatnot, so when automating it can be harder to recognise what service to use when. In any case, thanks for sharing your concern! I'll try to get back to this after I recover from my massive jet lag that I will have next weekend. Also, please don't forget to send me a message on discord so we can add you to the appropriate channels as codeowner :) |
|
@joostlek - After checking all the different Growatt inverter APIs, I've found that there are 4 different ways they handle time-of-use/charge/discharge controls. Each type works completely differently. The 4 TypesMIN Inverters - Time Segments
NOAH Inverters - Basic Time Segments
MIX(SPH)/SPA Inverters - Charge/Discharge Periods
STORAGE Inverters - Basic Periods
Propsed Service NamesMain models get clean names: MIN
MIX/SPA
Niche models get "basic_" prefix: NOAH
STORAGE
Why This Makes Sense
So to summarize, the service in this PR is unique for the min inverters, but I still think the naming makes sense. |
|
Any thoughts on this @joostlek ? |
I think its easier for the library to handle the differences and the integration just use the same methods but I added these interfaces https://github.com/indykoning/PyPi_GrowattServer/pull/125/files#diff-12865f9986e5f28194397aae0efed2b7abc9137d18c0a09197682e6c7e65cd01R156 and which i had used like so https://github.com/johanzander/growatt_server_upstream/pull/7/files#diff-33680c02b8a262be8cbd5f7298d8434e770d9fac3944865a872259b2ba23f96eR217 or https://github.com/johanzander/growatt_server_upstream/pull/7/files#diff-33680c02b8a262be8cbd5f7298d8434e770d9fac3944865a872259b2ba23f96eR289 as example |
|
@GraemeDBlue - for sensors, numbers and switches, I agree the integration would benefit from using the "unified" V1 methods from the library. But for actions/services - based on what my quick analysis, the control mechanisms are quite different. Or do you mean that if would be possible to have a unified service for all inverters (MIN,MIX, etc.)? What parameters would it have in that case? |
|
@johanzander so I had the idea to create different versions in service.yaml for each type, with all the correct parameters to match and then depending on which type you have it would only load the relevant one. I have it working on my own branch like that, Im on my phone atm, otherwise I would show you my example code. |
|
yes, that it basically what I propose too. but not to prefix the inverter API type in the methods name, but name them for what they do (see above) - which is also what they are named in the PR. Another strong argument for this is because there is no logic on which inverter types use which API type. I am about to update the documentation with which inverters use the MIN API, and it is a mix of MIC, MIN, MOD and MID: home-assistant/home-assistant.io#41713. If we would name the method min_update_time_segment, that would be very confusing for users of MIC, MOD and MID inverters... |
|
@johanzander so we could do something similar to this https://github.com/johanzander/growatt_server_upstream/pull/7/files#diff-2e5c84f931c30e94910ccdaf32e17d82e03b6dfc328e6e2613a9d867984c6fc0R916-R944 but instead of https://github.com/johanzander/growatt_server_upstream/pull/7/files#diff-2ff0453b12810299de3e3acd602d6b5dbb9a566d0fe9e44cb6bfd6d118283d07R105 and https://github.com/johanzander/growatt_server_upstream/pull/7/files#diff-2ff0453b12810299de3e3acd602d6b5dbb9a566d0fe9e44cb6bfd6d118283d07R47 but instead of mix/tlx etc we create a group1, group2 etc for the systems with similar capabilities and IO |
|
@joostlek - would appreciate merge or feedback on this PR (hopefully the former :-))! |
|
Hi @johanzander, @joostlek |
|
Ready from my side, awaiting feedback from @joostlek ! |
|
Don't worry, the first step is just doing our due diligence. I haven't found time to get back to reviewing this PR yet |
| BATT_MODE_MAP = { | ||
| "load-first": BATT_MODE_LOAD_FIRST, | ||
| "0": BATT_MODE_LOAD_FIRST, | ||
| "battery-first": BATT_MODE_BATTERY_FIRST, | ||
| "1": BATT_MODE_BATTERY_FIRST, | ||
| "grid-first": BATT_MODE_GRID_FIRST, | ||
| "2": BATT_MODE_GRID_FIRST, | ||
| } |
…gration (home-assistant#154703) Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
Proposed change
Adds two new services to the Growatt Server integration for managing Time-of-Use
(TOU) battery scheduling on MIN inverters:
growatt_server.update_time_segment: Configure individual time segments (1-9)with battery operation mode, time range, and enable/disable state
growatt_server.read_time_segments: Read current configuration of all 9 timesegments
These services enable users to automate their battery charging/discharging schedules
based on electricity rates, solar generation, or other factors.
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: