Repository navigation
Charge spanning midnight error, more tests, extend token for MCP - #2852
Conversation
Increase token timeout for MCP
springfall2008
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR updates Predbat to version 8.27.2, addressing issues with inverter control logic for charge/discharge window management and testing improvements.
- Fixed button press logic to be more structured and conditional
- Enhanced charge window handling across midnight boundaries
- Extended access token lifetime from 1 hour to 24 hours for MCP OAuth
- Added comprehensive test coverage for midnight-spanning charge scenarios
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/predbat/predbat.py | Version bump to v8.27.2 |
| apps/predbat/web_mcp.py | Extended OAuth access token lifetime from 1 to 24 hours |
| apps/predbat/inverter.py | Refactored button press logic and charge schedule enable normalization for better code structure |
| apps/predbat/unit_test.py | Added test parameter for inverter_charge_time_minutes_end, additional logging, new test pages, and comprehensive midnight charge window test cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| print("Test: {}".format(test_name)) | ||
|
|
||
| inv.rest_data = None | ||
| inv.has_inverter_time_button_press = has_inverter_time_button_press |
There was a problem hiding this comment.
The variable name has_inverter_time_button_press should be inv_time_button_press to match the actual inverter attribute name used in inverter.py (line 209, 1989, 2059, 2415). This incorrect attribute name will cause the test to fail to set the intended inverter property.
| inv.rest_data["Timeslots"]["Charge_start_time_slot_1"] = prev_charge_start_time | ||
| inv.rest_data["Timeslots"]["Charge_end_time_slot_1"] = prev_charge_end_time | ||
| inv.rest_data["Control"] = {} | ||
| inv.rest_data["Control"]["Enable_Charge_Schedule"] = "enable" if prev_enable_charge else "disabled" |
There was a problem hiding this comment.
Inconsistent string values: uses 'enable' when true but 'disabled' when false (should be 'disable' to match line 570 and 638). This inconsistency could cause test failures or mask bugs in the actual code.
| inv.rest_data["Control"]["Enable_Charge_Schedule"] = "enable" if prev_enable_charge else "disabled" | |
| inv.rest_data["Control"]["Enable_Charge_Schedule"] = "enable" if prev_enable_charge else "disable" |
| if old_charge_schedule_enable in ["off", "disable"]: | ||
| old_charge_schedule_enable = "off" |
There was a problem hiding this comment.
The normalization only handles 'off' and 'disable' but doesn't handle other possible values like 'enable' or 'on'. Consider adding an else clause to normalize 'enable'/'on' to a consistent 'on' value, or documenting the expected values to ensure robust handling across different states.
| if old_charge_schedule_enable in ["off", "disable"]: | |
| old_charge_schedule_enable = "off" | |
| if str(old_charge_schedule_enable).lower() in ["off", "disable"]: | |
| old_charge_schedule_enable = "off" | |
| elif str(old_charge_schedule_enable).lower() in ["on", "enable"]: | |
| old_charge_schedule_enable = "on" |
#2847