Skip to content

Fix review feedback: clean up imports, restore tests, fix charge rate bounds logic, remove MQTT retain#292

Merged
MaStr merged 3 commits intomax_charge_ratefrom
copilot/sub-pr-260
Mar 10, 2026
Merged

Fix review feedback: clean up imports, restore tests, fix charge rate bounds logic, remove MQTT retain#292
MaStr merged 3 commits intomax_charge_ratefrom
copilot/sub-pr-260

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 10, 2026

  • Remove redundant sys.path manipulation and unused imports in tests/batcontrol/test_core.py
  • Add missing tests for api_set_production_offset (valid value 0.7 and negative invalid value) in tests/batcontrol/test_production_offset.py
  • Fix bounding logic in core.py limit_battery_charge_rate to properly handle min_pv_charge_rate > max_pv_charge_rate
  • Remove retain=True from publish_limit_battery_charge_rate in mqtt_api.py
  • Move min_pv_charge_rate > max_pv_charge_rate guard into __init__() (fires warning once at startup, clamps self.min_pv_charge_rate so limit_battery_charge_rate() can rely on the invariant)
  • Read max_pv_charge_rate from config['inverter'] first (with inverter attribute as fallback) to avoid MagicMock comparison issues in tests

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

…x bounds logic, remove retain

Co-authored-by: MaStr <1036501+MaStr@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 10, 2026

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • test.example.com
    • Triggering command: /usr/bin/python python -m pytest tests/ --cov=src/batcontrol --log-cli-level=DEBUG --log-cli-format=%(asctime)s [%(levelname)8s] %(name)s: %(message)s --log-cli-date-format=%Y-%m-%d %H:%M:%S (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title [WIP] Add operation mode to limit battery charge rate Fix review feedback: clean up imports, restore tests, fix charge rate bounds logic, remove MQTT retain Mar 10, 2026
@MaStr
Copy link
Copy Markdown
Owner

MaStr commented Mar 10, 2026

Please move the guard against the min_max problems in the initialization area, assuming that these values are non changable parameters from the config file @copilot

Co-authored-by: MaStr <1036501+MaStr@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 10, 2026

Please move the guard against the min_max problems in the initialization area, assuming that these values are non changable parameters from the config file @copilot

Done in commit 274fa8e. The min_pv_charge_rate > max_pv_charge_rate check now runs once at __init__() time (lines 152–167 in core.py): a warning is logged and self.min_pv_charge_rate is permanently clamped to self.max_pv_charge_rate. limit_battery_charge_rate() can now rely on the invariant without any per-call guard logic. Also switched max_pv_charge_rate to be read from config['inverter'] first (falling back to the inverter attribute) for correctness when the value is in the config.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • test.example.com
    • Triggering command: /usr/bin/python python -m pytest tests/ --cov=src/batcontrol --log-cli-level=DEBUG --log-cli-format=%(asctime)s [%(levelname)8s] %(name)s: %(message)s --log-cli-date-format=%Y-%m-%d %H:%M:%S (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@MaStr MaStr marked this pull request as ready for review March 10, 2026 15:59
@MaStr MaStr merged commit 554657e into max_charge_rate Mar 10, 2026
@MaStr MaStr deleted the copilot/sub-pr-260 branch March 10, 2026 15:59
MaStr added a commit that referenced this pull request Mar 10, 2026
* feat: Implement limit battery charge rate functionality

- 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.

* Enhance reslient wrapper

* fix: Address PR #260 review feedback

- Add assertions to test_production_offset_applied_to_forecast to verify
  that last_production[1] and [2] reflect the 50% offset, and remove
  misplaced test logic that was left over from deleted test methods
- Add warning log in api_set_mode when MODE_LIMIT_BATTERY_CHARGE_RATE is
  used but _limit_battery_charge_rate is still -1 (no limit set), so
  callers are informed about the silent fallback to allow-discharging
- Remove extra blank line at end of tests/batcontrol/inverter/test_fronius_ids.py
- Remove unused Mock import from tests/batcontrol/test_core.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix: Correct swapped docstrings in InverterInterface

set_mode_avoid_discharge was documented as 'allow discharge mode' and
set_mode_allow_discharge was documented as 'avoid discharge mode'.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* docs: Clarify min_pv_charge_rate comment in config

The previous comment implied that setting min_pv_charge_rate to 0 was
needed to allow complete charge blocking. In reality, the implementation
skips the minimum floor when limit_charge_rate == 0, so a requested
limit of 0 always results in complete charge blocking regardless of
min_pv_charge_rate. The comment now reflects this actual behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix review feedback: clean up imports, restore tests, fix charge rate bounds logic, remove MQTT retain (#292)

* Initial plan

* fix: Address PR review feedback - clean up imports, restore tests, fix bounds logic, remove retain

Co-authored-by: MaStr <1036501+MaStr@users.noreply.github.com>

* refactor: Move min/max PV charge rate guard to initialization

Co-authored-by: MaStr <1036501+MaStr@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: MaStr <1036501+MaStr@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: MaStr <1036501+MaStr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants