diff --git a/openevsehttp/commands.py b/openevsehttp/commands.py index 2b3ff34..46bec9f 100644 --- a/openevsehttp/commands.py +++ b/openevsehttp/commands.py @@ -53,7 +53,12 @@ def _flag_ota_if_started(self, response: Any) -> None: normalized.get("msg") == "started" or normalized.get("msg") in SUCCESS_ANSWERS ): + _LOGGER.debug("Firmware update started, setting ota_update flag.") self._status["ota_update"] = 1 + else: + _LOGGER.debug( + "Firmware update response did not indicate start: %s", normalized + ) async def get_schedule(self) -> Mapping[str, Any] | list[Any]: """Return the current schedule.""" @@ -131,7 +136,12 @@ async def set_override( time_limit: int | None = None, auto_release: bool | None = None, ) -> Any: - """Set the manual override status.""" + """Set the manual override status. + + Fetches the current override payload first and merges existing values + into the request payload. This prevents the firmware from clearing/resetting + previously configured properties that are not passed in the function call. + """ if not self._version_check("4.0.1"): _LOGGER.debug("Feature not supported for older firmware.") raise UnsupportedFeature @@ -149,6 +159,18 @@ async def set_override( raise ValueError data: dict[str, Any] = {} + if isinstance(response, Mapping): + for key in ( + "state", + "charge_current", + "max_current", + "energy_limit", + "time_limit", + "auto_release", + ): + if key in response: + data[key] = response[key] + if auto_release is not None: data["auto_release"] = auto_release @@ -381,6 +403,7 @@ async def firmware_check(self) -> dict | None: url = f"{base_url}ESP32_WiFi_V4.x/releases/latest" else: url = f"{base_url}ESP8266_WiFi_v2.x/releases/latest" + _LOGGER.debug("Firmware check URL: %s", url) except AwesomeVersionCompareException: _LOGGER.debug("Non-semver firmware version detected.") return None @@ -412,6 +435,7 @@ async def _firmware_check_with_session( method, ) async with http_method(url) as resp: + _LOGGER.debug("Firmware check response status: %d", resp.status) if resp.status != 200: return None message = await resp.text() @@ -422,19 +446,51 @@ async def _firmware_check_with_session( return None if not isinstance(message, dict): + _LOGGER.debug( + "Invalid JSON response type from GitHub: %s", type(message) + ) return None + _LOGGER.debug( + "GitHub release metadata successfully fetched for version: %s", + message.get("tag_name"), + ) + # Match browser_download_url based on buildenv download_url = None buildenv = self._config.get("buildenv") assets = message.get("assets", []) - if buildenv and assets: + if not buildenv: + _LOGGER.debug( + "Cannot resolve firmware asset: missing buildenv in config." + ) + assets = [] + elif not isinstance(assets, list): + _LOGGER.debug("Invalid GitHub assets payload: %r", assets) + assets = [] + else: + _LOGGER.debug("Matching buildenv '%s' against assets", buildenv) target_filename = f"{buildenv}.bin" for asset in assets: + if not isinstance(asset, Mapping): + continue if asset.get("name") == target_filename: download_url = asset.get("browser_download_url") + _LOGGER.debug("Found matching firmware asset: %s", download_url) break + if buildenv and not download_url: + _LOGGER.debug( + "Could not find asset matching target filename '%s.bin' in assets: %s", + buildenv, + [ + asset.get("name") + for asset in assets + if isinstance(asset, Mapping) + ] + if assets + else "None", + ) return { "latest_version": message.get("tag_name"), @@ -461,10 +517,16 @@ async def update_firmware( raise UnsupportedFeature if firmware_bytes is not None and firmware_url is not None: + _LOGGER.error("Cannot specify both firmware_bytes and firmware_url") raise ValueError("Cannot specify both firmware_bytes and firmware_url") + if firmware_bytes is not None and len(firmware_bytes) == 0: + _LOGGER.error("Empty firmware bytes provided") + raise ValueError("Empty firmware bytes provided") + if firmware_url is not None: if not isinstance(firmware_url, str) or not firmware_url.strip(): + _LOGGER.error("Invalid firmware_url: %s", firmware_url) raise ValueError("Invalid firmware_url") url = f"{self.url}update" @@ -485,13 +547,20 @@ async def update_firmware( response = await self.process_request( url=url, method="post", rapi=form_data ) + _LOGGER.debug("Firmware upload request completed. Response: %s", response) self._flag_ota_if_started(response) return response # 2. Resolve URL from GitHub if not specified if firmware_url is None: + _LOGGER.debug( + "No firmware URL provided. Resolving latest matching firmware from GitHub." + ) check_result = await self.firmware_check() if not check_result or not check_result.get("browser_download_url"): + _LOGGER.error( + "Could not resolve latest firmware download URL from GitHub." + ) raise RuntimeError( "Could not resolve latest firmware download URL from GitHub." ) @@ -503,6 +572,7 @@ async def update_firmware( "Requesting OpenEVSE to download and update from: %s", firmware_url ) response = await self.process_request(url=url, method="post", data=data) + _LOGGER.debug("Firmware update request completed. Response: %s", response) self._flag_ota_if_started(response) return response diff --git a/tests/conftest.py b/tests/conftest.py index 93fe2e2..39f43fa 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -291,6 +291,7 @@ class RegisteredMock(NamedTuple): class AiohttpClientMocker: def __init__(self): self.mocks = [] + self.requests = [] self._patcher = None def __enter__(self): @@ -358,6 +359,7 @@ def options(self, *args, **kwargs): async def _request_mock(self, method: str, str_or_url: Any, **kwargs: Any): url_str = str(str_or_url) method_upper = method.upper() + self.requests.append((method_upper, url_str, kwargs)) matching_mock = None matching_index = -1 diff --git a/tests/test_commands.py b/tests/test_commands.py index 33b1e74..d6b8e2c 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -1060,6 +1060,8 @@ async def test_update_firmware_bytes(test_charger, mock_aioclient, caplog): "Uploading firmware binary to http://openevse.test.tld/update (14 bytes)" in caplog.text ) + assert "Firmware upload request completed. Response: OK" in caplog.text + assert "Firmware update started, setting ota_update flag." in caplog.text assert test_charger.ota_update is True @@ -1080,6 +1082,11 @@ async def test_update_firmware_url(test_charger, mock_aioclient, caplog): "Requesting OpenEVSE to download and update from: http://github.com/release.bin" in caplog.text ) + assert ( + "Firmware update request completed. Response: {'msg': 'started'}" + in caplog.text + ) + assert "Firmware update started, setting ota_update flag." in caplog.text assert test_charger.ota_update is True @@ -1120,14 +1127,43 @@ async def test_update_firmware_auto(test_charger, mock_aioclient, caplog): with caplog.at_level(logging.DEBUG): response = await test_charger.update_firmware() assert response == {"msg": "started"} + assert ( + "No firmware URL provided. Resolving latest matching firmware from GitHub." + in caplog.text + ) + assert "Detected firmware: 4.1.7" in caplog.text + assert "Using version: 4.1.7" in caplog.text + assert ( + "Firmware check URL: https://api.github.com/repos/OpenEVSE/ESP32_WiFi_V4.x/releases/latest" + in caplog.text + ) + assert "Firmware check response status: 200" in caplog.text + assert ( + "GitHub release metadata successfully fetched for version: v4.1.2" + in caplog.text + ) + assert ( + "Matching buildenv 'openevse_esp32-gateway' against assets" in caplog.text + ) + assert ( + "Found matching firmware asset: https://github.com/OpenEVSE/releases/download/v4.1.2/openevse_esp32-gateway.bin" + in caplog.text + ) assert ( "Requesting OpenEVSE to download and update from: https://github.com/OpenEVSE/releases/download/v4.1.2/openevse_esp32-gateway.bin" in caplog.text ) + assert ( + "Firmware update request completed. Response: {'msg': 'started'}" + in caplog.text + ) + assert "Firmware update started, setting ota_update flag." in caplog.text assert test_charger.ota_update is True -async def test_update_firmware_auto_missing_buildenv(test_charger, mock_aioclient): +async def test_update_firmware_auto_missing_buildenv( + test_charger, mock_aioclient, caplog +): """Test update_firmware raises RuntimeError when buildenv asset is missing.""" test_charger._config = {"version": "4.1.7", "buildenv": "openevse_esp32-gateway"} @@ -1150,35 +1186,47 @@ async def test_update_firmware_auto_missing_buildenv(test_charger, mock_aioclien body=json.dumps(github_response), ) - with pytest.raises( - RuntimeError, - match="Could not resolve latest firmware download URL from GitHub.", - ): - await test_charger.update_firmware() + with caplog.at_level(logging.DEBUG): + with pytest.raises( + RuntimeError, + match=r"Could not resolve latest firmware download URL from GitHub\.", + ): + await test_charger.update_firmware() + assert ( + "Could not find asset matching target filename 'openevse_esp32-gateway.bin' in assets: ['other_env.bin']" + in caplog.text + ) + assert ( + "Could not resolve latest firmware download URL from GitHub." in caplog.text + ) -async def test_update_firmware_both_provided(test_charger): +async def test_update_firmware_both_provided(test_charger, caplog): """Test update_firmware raises ValueError when both bytes and URL are provided.""" test_charger._config["version"] = "4.1.7" - with pytest.raises( - ValueError, match="Cannot specify both firmware_bytes and firmware_url" - ): - await test_charger.update_firmware( - firmware_url="http://url", firmware_bytes=b"bytes" - ) + with caplog.at_level(logging.DEBUG): + with pytest.raises(ValueError): + await test_charger.update_firmware( + firmware_url="http://url", firmware_bytes=b"bytes" + ) + assert "Cannot specify both firmware_bytes and firmware_url" in caplog.text -async def test_update_firmware_url_invalid(test_charger): +async def test_update_firmware_url_invalid(test_charger, caplog): """Test update_firmware raises ValueError when firmware_url is empty or invalid type.""" test_charger._config["version"] = "4.1.7" - with pytest.raises(ValueError, match="Invalid firmware_url"): - await test_charger.update_firmware(firmware_url="") + with caplog.at_level(logging.DEBUG): + with pytest.raises(ValueError): + await test_charger.update_firmware(firmware_url="") + assert "Invalid firmware_url: " in caplog.text - with pytest.raises(ValueError, match="Invalid firmware_url"): - await test_charger.update_firmware(firmware_url=" ") + with pytest.raises(ValueError): + await test_charger.update_firmware(firmware_url=" ") + assert "Invalid firmware_url: " in caplog.text - with pytest.raises(ValueError, match="Invalid firmware_url"): - await test_charger.update_firmware(firmware_url=123) # type: ignore + with pytest.raises(ValueError): + await test_charger.update_firmware(firmware_url=123) # type: ignore + assert "Invalid firmware_url: 123" in caplog.text async def test_update_firmware_unsupported(test_charger): @@ -1188,7 +1236,7 @@ async def test_update_firmware_unsupported(test_charger): await test_charger.update_firmware(firmware_url="http://url") -async def test_update_firmware_error_response(test_charger, mock_aioclient): +async def test_update_firmware_error_response(test_charger, mock_aioclient, caplog): """Test update_firmware doesn't set ota_update on error responses.""" test_charger._config["version"] = "4.1.7" test_charger._status = {} @@ -1197,8 +1245,89 @@ async def test_update_firmware_error_response(test_charger, mock_aioclient): status=200, body='{"msg":"error"}', ) - response = await test_charger.update_firmware( - firmware_url="http://github.com/release.bin" + with caplog.at_level(logging.DEBUG): + response = await test_charger.update_firmware( + firmware_url="http://github.com/release.bin" + ) + assert response == {"msg": "error"} + assert ( + "Firmware update response did not indicate start: {'msg': 'error'}" + in caplog.text + ) + assert test_charger.ota_update is False + + +async def test_update_firmware_assets_invalid_type( + test_charger, mock_aioclient, caplog +): + """Test update_firmware handles non-list assets gracefully.""" + test_charger._config = {"version": "4.1.7", "buildenv": "openevse_esp32-gateway"} + + # Mock GitHub releases but with assets as a dict (invalid type) + github_response = { + "tag_name": "v4.1.2", + "body": "release notes", + "html_url": "https://github.com/OpenEVSE/releases/v4.1.2", + "assets": {"name": "not_a_list"}, + } + + mock_aioclient.get( + "https://api.github.com/repos/OpenEVSE/ESP32_WiFi_V4.x/releases/latest", + status=200, + body=json.dumps(github_response), ) - assert response == {"msg": "error"} - assert test_charger.ota_update is False + + with caplog.at_level(logging.DEBUG): + with pytest.raises( + RuntimeError, + match=r"Could not resolve latest firmware download URL from GitHub\.", + ): + await test_charger.update_firmware() + assert "Invalid GitHub assets payload: {'name': 'not_a_list'}" in caplog.text + + +async def test_update_firmware_assets_invalid_item( + test_charger, mock_aioclient, caplog +): + """Test update_firmware handles non-mapping assets gracefully.""" + test_charger._config = {"version": "4.1.7", "buildenv": "openevse_esp32-gateway"} + + # Mock GitHub releases with assets list containing non-mapping elements (e.g., a string) + github_response = { + "tag_name": "v4.1.2", + "body": "release notes", + "html_url": "https://github.com/OpenEVSE/releases/v4.1.2", + "assets": [ + "invalid_asset_string", + { + "name": "openevse_esp32-gateway.bin", + "browser_download_url": "http://url", + }, + ], + } + + mock_aioclient.get( + "https://api.github.com/repos/OpenEVSE/ESP32_WiFi_V4.x/releases/latest", + status=200, + body=json.dumps(github_response), + ) + + mock_aioclient.post( + "http://openevse.test.tld/update", + status=200, + body='{"msg":"started"}', + ) + + with caplog.at_level(logging.DEBUG): + response = await test_charger.update_firmware() + assert response == {"msg": "started"} + assert "Found matching firmware asset: http://url" in caplog.text + + +async def test_update_firmware_bytes_empty(test_charger, caplog): + """Test update_firmware raises ValueError when empty firmware_bytes are provided.""" + test_charger._config["version"] = "4.1.7" + with caplog.at_level(logging.DEBUG): + with pytest.raises(ValueError): + await test_charger.update_firmware(firmware_bytes=b"") + assert "Empty firmware bytes provided" in caplog.text diff --git a/tests/test_managers.py b/tests/test_managers.py index b08b8df..0088982 100644 --- a/tests/test_managers.py +++ b/tests/test_managers.py @@ -48,7 +48,15 @@ async def test_set_override( with caplog.at_level(logging.DEBUG): status = await test_charger.set_override("active") assert status == {"msg": "OK"} - assert "Override data: {'state': 'active'}" in caplog.text + assert mock_aioclient.requests[-1][2]["json"] == { + "state": "active", + "charge_current": 0, + "max_current": 0, + "energy_limit": 0, + "time_limit": 0, + "auto_release": True, + } + caplog.clear() mock_aioclient.post( TEST_URL_OVERRIDE, @@ -56,44 +64,79 @@ async def test_set_override( body='{"msg": "OK"}', ) status = await test_charger.set_override("active", 30) - assert "Override data: {'state': 'active', 'charge_current': 30}" in caplog.text + assert mock_aioclient.requests[-1][2]["json"] == { + "state": "active", + "charge_current": 30, + "max_current": 0, + "energy_limit": 0, + "time_limit": 0, + "auto_release": True, + } + caplog.clear() + mock_aioclient.post( TEST_URL_OVERRIDE, status=200, body='{"msg": "OK"}', ) status = await test_charger.set_override(charge_current=30) - assert "Override data: {'charge_current': 30}" in caplog.text + assert mock_aioclient.requests[-1][2]["json"] == { + "state": "active", + "charge_current": 30, + "max_current": 0, + "energy_limit": 0, + "time_limit": 0, + "auto_release": True, + } + caplog.clear() + mock_aioclient.post( TEST_URL_OVERRIDE, status=200, body='{"msg": "OK"}', ) status = await test_charger.set_override("active", 30, 32) - assert ( - "Override data: {'state': 'active', 'charge_current': 30, 'max_current': 32}" - in caplog.text - ) + assert mock_aioclient.requests[-1][2]["json"] == { + "state": "active", + "charge_current": 30, + "max_current": 32, + "energy_limit": 0, + "time_limit": 0, + "auto_release": True, + } + caplog.clear() + mock_aioclient.post( TEST_URL_OVERRIDE, status=200, body='{"msg": "OK"}', ) status = await test_charger.set_override("active", 30, 32, 2000) - assert ( - "Override data: {'state': 'active', 'charge_current': 30, 'max_current': 32, 'energy_limit': 2000}" - in caplog.text - ) + assert mock_aioclient.requests[-1][2]["json"] == { + "state": "active", + "charge_current": 30, + "max_current": 32, + "energy_limit": 2000, + "time_limit": 0, + "auto_release": True, + } + caplog.clear() + mock_aioclient.post( TEST_URL_OVERRIDE, status=200, body='{"msg": "OK"}', ) status = await test_charger.set_override("active", 30, 32, 2000, 5000) - assert ( - "Override data: {'state': 'active', 'charge_current': 30, 'max_current': 32, 'energy_limit': 2000, 'time_limit': 5000}" - in caplog.text - ) + assert mock_aioclient.requests[-1][2]["json"] == { + "state": "active", + "charge_current": 30, + "max_current": 32, + "energy_limit": 2000, + "time_limit": 5000, + "auto_release": True, + } + caplog.clear() with pytest.raises(ValueError): with caplog.at_level(logging.DEBUG):