Conversation
- Added `min_pv_charge_rate` and `max_pv_charge_rate` to inverter configuration. - Introduced `MODE_LIMIT_BATTERY_CHARGE_RATE` to allow limiting PV charging while permitting battery discharge. - Updated `Batcontrol` class to handle new charge rate limits and modes. - Implemented `limit_battery_charge_rate` method in `Batcontrol` to apply dynamic limits based on configuration. - Enhanced `Dummy` and `FroniusWR` inverter classes to support the new limit battery charge mode. - Updated MQTT publishing to include commands for setting battery charge limits. - Added unit tests for the new functionality, including edge cases for charge limits.
There was a problem hiding this comment.
Pull request overview
This pull request introduces a new operation mode "MODE_LIMIT_BATTERY_CHARGE_RATE" (mode 8) to enable peak shaving by limiting the battery charge rate from PV sources. The feature allows batcontrol to send a MAX_CHARGE command to the inverter to avoid charging the battery at maximum available PV power.
Changes:
- Added MODE_LIMIT_BATTERY_CHARGE_RATE constant and implementation across core, inverter interface, and all inverter implementations
- Extended MQTT API with limit_battery_charge_rate topic for external control
- Added comprehensive test coverage for the new mode including core functionality and inverter-specific implementations
- Updated configuration with new parameters: min_pv_charge_rate and max_pv_charge_rate documentation
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/batcontrol/core.py | Added MODE_LIMIT_BATTERY_CHARGE_RATE mode, limit_battery_charge_rate method, API methods for setting/getting limit, and integration with inverter control flow |
| src/batcontrol/inverter/inverter_interface.py | Added abstract method set_mode_limit_battery_charge to interface |
| src/batcontrol/inverter/fronius.py | Implemented set_mode_limit_battery_charge using TimeOfUse CHARGE_MAX rules with validation |
| src/batcontrol/inverter/mqtt_inverter.py | Implemented set_mode_limit_battery_charge publishing mode and limit to MQTT command topics |
| src/batcontrol/inverter/dummy.py | Added dummy implementation and modified refresh_api_values/shutdown methods |
| src/batcontrol/inverter/resilient_wrapper.py | Added resilience wrapper for set_mode_limit_battery_charge method |
| src/batcontrol/mqtt_api.py | Added publish_limit_battery_charge_rate method and updated documentation with mode 8 |
| config/batcontrol_config_dummy.yaml | Documented min_pv_charge_rate and max_pv_charge_rate parameters with mode 8 usage |
| tests/batcontrol/test_core.py | New comprehensive test suite for MODE_LIMIT_BATTERY_CHARGE_RATE functionality |
| tests/batcontrol/test_production_offset.py | Fixed whitespace issues and updated mocks for InverterControlSettings compatibility |
| tests/batcontrol/inverter/test_*.py | Added tests for set_mode_limit_battery_charge in all inverter implementations |
| # Call run to apply the offset | ||
| batcontrol.run() | ||
|
|
||
| # Check that production was offset correctly | ||
| # Note: production[0] is adjusted for elapsed time in current interval | ||
| # so we only check indices [1] and [2] for exact values | ||
| assert batcontrol.last_production is not None | ||
| # Check that offset was applied (values should be roughly half) | ||
| assert batcontrol.last_production[1] == pytest.approx(1000, rel=0.01) | ||
| assert batcontrol.last_production[2] == pytest.approx(1500, rel=0.01) | ||
| # For [0], just check it's less than original | ||
| assert batcontrol.last_production[0] < 500 # Should be ~500 or less due to elapsed time | ||
|
|
||
| def test_production_offset_api_set_valid(self, mock_config): | ||
| """Test setting production offset via API with valid value""" | ||
| with patch('batcontrol.core.tariff_factory'), \ | ||
| patch('batcontrol.core.inverter_factory'), \ | ||
| patch('batcontrol.core.solar_factory'), \ | ||
| patch('batcontrol.core.consumption_factory'): | ||
|
|
||
| batcontrol = Batcontrol(mock_config) | ||
|
|
||
| # Set via API | ||
| batcontrol.api_set_production_offset(0.7) | ||
|
|
||
| # Should be updated | ||
| assert batcontrol.production_offset_percent == 0.7 | ||
|
|
||
| def test_production_offset_api_set_invalid_negative(self, mock_config): | ||
| """Test setting production offset via API with invalid negative value""" | ||
| with patch('batcontrol.core.tariff_factory'), \ | ||
| patch('batcontrol.core.inverter_factory'), \ | ||
| patch('batcontrol.core.solar_factory'), \ | ||
| patch('batcontrol.core.consumption_factory'): | ||
|
|
||
|
|
||
| batcontrol = Batcontrol(mock_config) | ||
| original_value = batcontrol.production_offset_percent | ||
|
|
||
| # Try to set invalid value | ||
| batcontrol.api_set_production_offset(-0.5) | ||
|
|
||
| # Should not be updated | ||
| assert batcontrol.production_offset_percent == original_value |
There was a problem hiding this comment.
The test method 'test_production_offset_applied_to_forecast' is incomplete. After line 164, the test should include assertions to verify that the production offset was correctly applied (e.g., checking batcontrol.last_production values). Instead, lines 166-173 contain unrelated test logic for testing invalid negative values in api_set_production_offset, which appears to be remnants from the deleted 'test_production_offset_api_set_valid' and 'test_production_offset_api_set_invalid_negative' test methods. This creates a broken test that doesn't properly validate the production offset functionality and includes misplaced test logic.
| elif mode == MODE_LIMIT_BATTERY_CHARGE_RATE: | ||
| self.limit_battery_charge_rate(self._limit_battery_charge_rate) |
There was a problem hiding this comment.
When api_set_mode is called with MODE_LIMIT_BATTERY_CHARGE_RATE (8), it uses the stored value in self._limit_battery_charge_rate. However, if this value is still at its default of -1 (no limit), the limit_battery_charge_rate method will fall back to allow_discharging mode (line 576-578). This means setting mode 8 via API without first setting a limit value will silently switch to mode 10 instead, which is confusing behavior. Consider adding a validation check or warning in api_set_mode to inform users they need to set a valid limit first, or consider using a different default value (e.g., max_pv_charge_rate).
|
|
||
| if __name__ == '__main__': | ||
| unittest.main() | ||
|
|
There was a problem hiding this comment.
There's an extra blank line at the end of the file (line 472). According to PEP8 standards mentioned in the repository guidelines, source files should end with a single newline character, not multiple blank lines. This extra blank line should be removed.
| import pytest | ||
| import sys | ||
| import os | ||
| from unittest.mock import Mock, MagicMock, patch |
There was a problem hiding this comment.
Import of 'Mock' is not used.
| from unittest.mock import Mock, MagicMock, patch | |
| from unittest.mock import MagicMock, patch |
1 participant
Create a new operation mode "limit battery charge rate".
This enables batcontrol to send a MAX_CHARGE Command to the inverter to avoid charging the battery at maximum available pv power.
Reason: Implementing peak shaving