Multiple chargers with central services #1498
Conversation
fix config migration when keys missing
add minor version
Add invalid data test for config flow
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_config_flow.py (2)
72-124: Add test coverage for measurand selection error cases.While the discovery flow test is thorough, consider adding test cases for:
- No measurands selected
- Invalid measurand combinations
- Error recovery paths
Example test case for no measurands selected:
# Test no measurands selected empty_measurands = {value: False for value in DEFAULT_MONITORED_VARIABLES.split(",")} result_empty = await hass.config_entries.flow.async_configure( result_cp["flow_id"], user_input=empty_measurands ) assert result_empty["type"] == data_entry_flow.FlowResultType.FORM assert result_empty["errors"] == {"base": "no_measurands_selected"}
126-161: Add test case for non-duplicate CP IDs.The test effectively handles duplicate CP IDs, but consider adding a test case for different CP IDs to ensure the system correctly allows multiple unique chargers.
Example addition:
# Test different CP IDs are allowed info2 = {"cp_id": "different_cp_id", "entry": entry} result3 = await hass.config_entries.flow.async_init( DOMAIN, context={"source": config_entries.SOURCE_INTEGRATION_DISCOVERY}, data=info2, ) assert result3["type"] == data_entry_flow.FlowResultType.FORM
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_config_flow.py(2 hunks)
🧰 Additional context used
📓 Learnings (1)
tests/test_config_flow.py (1)
Learnt from: drc38
PR: lbbrhzn/ocpp#1498
File: tests/test_config_flow.py:124-124
Timestamp: 2025-01-28T09:21:24.150Z
Learning: The OCPP integration does not use options flow for configuration updates. Error handling in the config flow is primarily focused on measurand selection validation with the "no_measurands_selected" error.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests
🔇 Additional comments (3)
tests/test_config_flow.py (3)
5-21: LGTM! Well-organized imports and constants.The imports and constants are properly organized and all are necessary for the test cases.
57-68: LGTM! Proper handling of configuration data.The test correctly removes the CONF_CPIDS before flow configuration as it's added during the flow, and the assertions are properly updated.
Line range hint
1-184: LGTM! Well-structured test suite with good coverage.The test suite is well-organized and covers the main configuration flows. The removal of options flow tests is correct as confirmed by the retrieved learnings. While the current coverage is good, the suggested improvements would make it more robust.
|
Measureands selection is working correctly now, but as soon as the second charger is connected, the first charger becomes unavailable. try EV ocpp simulator, you will see what it does. |
Thanks, @rbb-dev can you post this again for the latest failure please. |
use cp_id for migration
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
tests/conftest.py (1)
44-46: Consider parameterizing the charge point ID mock.Given that this PR adds support for multiple chargers, the hard-coded
"test_cp_id"return value might not be sufficient for testing all scenarios. Consider:
- Parameterizing the fixture to accept different charge point IDs
- Adding test cases for multiple charger scenarios
Here's a suggested implementation:
@pytest.fixture(name="bypass_get_data") -def bypass_get_data_fixture(): +def bypass_get_data_fixture(request): """Skip calls to get data from API.""" future = asyncio.Future() future.set_result(websockets.asyncio.server.Server) + cp_id = getattr(request, "param", "test_cp_id") with ( patch("websockets.asyncio.server.serve", return_value=future), patch("websockets.asyncio.server.Server.close"), patch("websockets.asyncio.server.Server.wait_closed"), - patch("homeassistant.core.StateMachine.get", return_value="test_cp_id"), + patch("homeassistant.core.StateMachine.get", return_value=cp_id), ): yieldUsage example:
@pytest.mark.parametrize("bypass_get_data", ["cp_1", "cp_2"], indirect=True) def test_multiple_chargers(bypass_get_data): # Test implementation passcustom_components/ocpp/__init__.py (1)
18-58: Consider grouping related configuration constants.The configuration constants could be better organized by grouping them into related categories:
- Network configuration (HOST, PORT, SSL)
- WebSocket settings (WEBSOCKET_*)
- Charger settings (IDLE_INTERVAL, MAX_CURRENT)
- Monitoring settings (MONITORED_VARIABLES)
# Network settings CONF_HOST = ... CONF_PORT = ... CONF_SSL = ... CONF_SSL_CERTFILE_PATH = ... CONF_SSL_KEYFILE_PATH = ... # WebSocket settings CONF_WEBSOCKET_CLOSE_TIMEOUT = ... CONF_WEBSOCKET_PING_TRIES = ... CONF_WEBSOCKET_PING_INTERVAL = ... CONF_WEBSOCKET_PING_TIMEOUT = ... # Charger settings CONF_IDLE_INTERVAL = ... CONF_MAX_CURRENT = ... CONF_METER_INTERVAL = ... # Monitoring settings CONF_MONITORED_VARIABLES = ... CONF_MONITORED_VARIABLES_AUTOCONFIG = ...tests/test_config_flow.py (4)
55-67: Enhance basic config flow test coverage.Add assertions to verify:
- The structure of the created entry
- The presence of required fields
- The correct handling of default values
assert result["type"] == data_entry_flow.FlowResultType.CREATE_ENTRY assert result["title"] == "test_csid_flow" assert result["data"] == MOCK_CONFIG_CS assert result["result"] + # Verify entry structure + assert CONF_HOST in result["data"] + assert CONF_PORT in result["data"] + # Verify default values + assert result["data"].get(CONF_SSL) == DEFAULT_SSL + assert result["data"].get(CONF_WEBSOCKET_PING_TRIES) == DEFAULT_WEBSOCKET_PING_TRIES
70-143: Add error case handling and improve test documentation.The discovery flow test should include:
- Documentation of test scenarios
- Error case handling for measurand configuration
- Verification of state changes
async def test_successful_discovery_flow(hass, bypass_get_data): - """Test a discovery config flow.""" + """Test a discovery config flow. + + Test cases: + 1. Initial charger discovery and configuration + 2. Manual measurand selection + 3. Addition of second charger + """ # Mock the config entry for the central system config_entry = MockConfigEntry( domain=DOMAIN, data=MOCK_CONFIG_CS, entry_id="test_cms_disc", title="test_cms_disc", version=2, minor_version=0, ) + + # Test error cases for measurand configuration + with pytest.raises(InvalidData): + measurand_input = {} # Empty measurands should fail + await hass.config_entries.flow.async_configure( + result_cp["flow_id"], user_input=measurand_input + )
145-181: Add edge cases to duplicate CPID test.Consider adding test cases for:
- Case-sensitive CP ID handling
- Special character handling in CP IDs
- Retry behavior after abort
async def test_duplicate_cpid_discovery_flow(hass, bypass_get_data): """Test discovery flow with duplicate CP ID.""" + # Test case sensitivity + info_case = {"cp_id": "TEST_CP_ID", "entry": entry} + result_case = await hass.config_entries.flow.async_init( + DOMAIN, + context={"source": config_entries.SOURCE_INTEGRATION_DISCOVERY}, + data=info_case, + ) + assert result_case["type"] == data_entry_flow.FlowResultType.FORM + + # Test special characters + info_special = {"cp_id": "test-cp@id", "entry": entry} + result_special = await hass.config_entries.flow.async_init( + DOMAIN, + context={"source": config_entries.SOURCE_INTEGRATION_DISCOVERY}, + data=info_special, + ) + assert result_special["type"] == data_entry_flow.FlowResultType.FORM
183-202: Expand failed config flow test scenarios.Add test cases for:
- Invalid port numbers
- Invalid SSL configurations
- Network timeout scenarios
- Invalid websocket parameters
async def test_failed_config_flow(hass, error_on_get_data): """Test failed config flow scenarios.""" + # Test invalid port + invalid_port_config = MOCK_CONFIG_CS.copy() + invalid_port_config[CONF_PORT] = -1 + with pytest.raises(InvalidData): + await hass.config_entries.flow.async_configure( + result["flow_id"], user_input=invalid_port_config + ) + + # Test invalid SSL config + invalid_ssl_config = MOCK_CONFIG_CS.copy() + invalid_ssl_config[CONF_SSL] = True + invalid_ssl_config[CONF_SSL_CERTFILE_PATH] = "nonexistent.pem" + with pytest.raises(InvalidData): + await hass.config_entries.flow.async_configure( + result["flow_id"], user_input=invalid_ssl_config + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
custom_components/ocpp/__init__.py(2 hunks)tests/conftest.py(2 hunks)tests/test_config_flow.py(2 hunks)
🧰 Additional context used
📓 Learnings (1)
tests/test_config_flow.py (1)
Learnt from: drc38
PR: lbbrhzn/ocpp#1498
File: tests/test_config_flow.py:124-124
Timestamp: 2025-01-28T09:21:24.150Z
Learning: The OCPP integration does not use options flow for configuration updates. Error handling in the config flow is primarily focused on measurand selection validation with the "no_measurands_selected" error.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests
🔇 Additional comments (2)
tests/conftest.py (2)
34-34: Well-documented patch purpose!The comment clearly explains that the state machine patch is used for migration purposes to return the charge point ID.
Line range hint
35-43: Well-structured fixture for API mocking!The fixture effectively mocks the websockets server behavior. Consider adding test cases that verify the behavior when the websocket connection fails or times out.
Let's check if there are tests covering websocket failure scenarios:
| # Close server | ||
| central_sys = hass.data[DOMAIN][entry.entry_id] | ||
| central_sys._server.close() | ||
| await central_sys._server.wait_closed() | ||
| unloaded = await hass.config_entries.async_unload_platforms( | ||
| entry, PLATFORMS | ||
| ) | ||
| # Unload services | ||
| # print(hass.services.async_services_for_domain(DOMAIN)) | ||
| for service in hass.services.async_services_for_domain(DOMAIN): | ||
| hass.services.async_remove(DOMAIN, service) | ||
| # Unload platforms if a charger connected | ||
| if central_sys.connections == 0: | ||
| unloaded = True | ||
| else: | ||
| unloaded = await hass.config_entries.async_unload_platforms( | ||
| entry, PLATFORMS | ||
| ) | ||
| # Remove entry |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout and error handling to unload process.
The unload entry function could be improved with:
- Timeout for server closure to prevent hanging
- Error handling for service removal
- Clear documentation of connection state handling
async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
"""Handle removal of an entry."""
unloaded = False
if DOMAIN in hass.data:
if entry.entry_id in hass.data[DOMAIN]:
central_sys = hass.data[DOMAIN][entry.entry_id]
+ try:
+ # Add timeout to prevent hanging
+ with async_timeout.timeout(10):
central_sys._server.close()
await central_sys._server.wait_closed()
+ except asyncio.TimeoutError:
+ _LOGGER.error("Timeout waiting for server to close")
+ except Exception as err:
+ _LOGGER.error("Error closing server: %s", err)
+ # Document connection state handling
+ _LOGGER.debug("Current connections: %d", central_sys.connections)
for service in hass.services.async_services_for_domain(DOMAIN):
- hass.services.async_remove(DOMAIN, service)
+ try:
+ hass.services.async_remove(DOMAIN, service)
+ except Exception as err:
+ _LOGGER.error("Error removing service %s: %s", service, err)Committable suggestion skipped: line range outside the PR's diff.
* fix on connect issues * fix entity state test
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
custom_components/ocpp/api.py (1)
Line range hint
150-161: Enhance SSL context security configuration.The SSL context creation lacks important security configurations.
Add these security enhancements:
if self.settings.ssl: self.ssl_context = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) + self.ssl_context.minimum_version = ssl.TLSVersion.TLSv1_2 + self.ssl_context.verify_mode = ssl.CERT_REQUIRED + self.ssl_context.options |= ssl.OP_NO_TLSv1 | ssl.OP_NO_TLSv1_1 localhost_certfile = self.settings.certfile localhost_keyfile = self.settings.keyfile
🧹 Nitpick comments (4)
custom_components/ocpp/number.py (1)
Line range hint
194-198: Improve native value handling.The current implementation updates the native value even when the retrieved value is None, which could lead to inconsistent state.
Consider this improvement:
-value = self.central_system.get_metric(self.cpid, self.metric) -if value is not None: - self._attr_native_value = value -return self._attr_native_value +new_value = self.central_system.get_metric(self.cpid, self.metric) +if new_value is not None: + self._attr_native_value = new_value + self.async_write_ha_state() +return new_valuecustom_components/ocpp/switch.py (1)
Line range hint
115-125: Simplify state handling in is_on property.The current implementation has nested conditions and multiple state updates. This could be simplified for better readability.
Consider this improvement:
-if self.entity_description.metric_state is not None: - resp = self.central_system.get_metric( - self.cpid, self.entity_description.metric_state - ) - if resp in self.entity_description.metric_condition: - self._state = True - else: - self._state = False -return self._state +if self.entity_description.metric_state is None: + return self._state + +resp = self.central_system.get_metric( + self.cpid, self.entity_description.metric_state +) +self._state = resp in self.entity_description.metric_condition +return self._statecustom_components/ocpp/sensor.py (2)
44-82: Optimize sensor entity creation.The current implementation creates a new list for each charger and processes metrics individually. This could be optimized.
Consider using a set for unique metrics and list comprehension:
-SENSORS = [] -for metric in list( - set( - cp_id_settings[CONF_MONITORED_VARIABLES].split(",") - + list(HAChargerSession) - ) -): - SENSORS.append( - OcppSensorDescription( - key=metric.lower(), - name=metric.replace(".", " "), - metric=metric, - ) - ) +monitored_metrics = set(cp_id_settings[CONF_MONITORED_VARIABLES].split(",")) +session_metrics = set(HAChargerSession) +SENSORS = [ + OcppSensorDescription( + key=metric.lower(), + name=metric.replace(".", " "), + metric=metric, + ) + for metric in monitored_metrics | session_metrics +]
Line range hint
213-220: Improve state restoration robustness.The state restoration doesn't validate the restored values against current configuration.
Add validation for restored values:
if restored := await self.async_get_last_sensor_data(): + if restored.native_unit_of_measurement == self._attr_native_unit_of_measurement: self._attr_native_value = restored.native_value - self._attr_native_unit_of_measurement = restored.native_unit_of_measurement + else: + _LOGGER.warning( + "Restored unit %s doesn't match current unit %s for %s", + restored.native_unit_of_measurement, + self._attr_native_unit_of_measurement, + self.entity_id, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
custom_components/ocpp/api.py(5 hunks)custom_components/ocpp/button.py(3 hunks)custom_components/ocpp/number.py(4 hunks)custom_components/ocpp/sensor.py(5 hunks)custom_components/ocpp/switch.py(5 hunks)custom_components/ocpp/translations/en.json(3 hunks)custom_components/ocpp/translations/i-default.json(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- custom_components/ocpp/translations/en.json
- custom_components/ocpp/translations/i-default.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests
🔇 Additional comments (1)
custom_components/ocpp/button.py (1)
92-93: Add error handling for invalid charger configurations.The code assumes that the charger settings will always be valid. Consider adding validation and error handling.
Also applies to: 96-97
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
custom_components/ocpp/api.py (1)
361-474: 🛠️ Refactor suggestionReduce code duplication in service handlers.
The error handling code is duplicated across all service handlers. As suggested in a previous review, consider using a decorator.
This issue was previously identified. Please refer to the existing suggestion for implementing the
@check_charger_availabledecorator to handle the common error checking pattern.
🧹 Nitpick comments (4)
custom_components/ocpp/api.py (4)
101-142: Consider refactoring service registration to reduce code duplication.The service registration blocks are repetitive. Consider creating a helper method to register services.
+ def _register_service( + self, + service_name: str, + handler, + schema: vol.Schema | None = None, + supports_response: SupportsResponse | None = None, + ): + """Helper method to register services.""" + kwargs = {"schema": schema} if schema else {} + if supports_response: + kwargs["supports_response"] = supports_response + self.hass.services.async_register(DOMAIN, service_name, handler, **kwargs) def __init__(self, hass: HomeAssistant, entry: ConfigEntry): # ... existing init code ... - self.hass.services.async_register( - DOMAIN, - csvcs.service_configure.value, - self.handle_configure, - CONF_SERVICE_DATA_SCHEMA, - supports_response=SupportsResponse.OPTIONAL, - ) + self._register_service( + csvcs.service_configure.value, + self.handle_configure, + CONF_SERVICE_DATA_SCHEMA, + SupportsResponse.OPTIONAL, + ) # ... repeat for other services
212-237: Enhance error handling specificity.The generic exception handling could mask specific setup issues. Consider catching and handling specific exceptions.
try: config_flow = False for cfg in self.settings.cpids: if cfg.get(cp_id): config_flow = True cp_settings = ChargerSystemSettings(**list(cfg.values())[0]) _LOGGER.info(f"Charger match found for {cp_settings.cpid}:{cp_id}") _LOGGER.debug(f"Central settings: {self.settings}") if not config_flow: info = {"cp_id": cp_id, "entry": self.entry} await self.hass.config_entries.flow.async_init( DOMAIN, context={"source": SOURCE_INTEGRATION_DISCOVERY}, data=info ) return self.cpids.update({cp_settings.cpid: cp_id}) await async_setup_charger( self.hass, self.entry, cs_id=self.id, cpid=cp_settings.cpid, cp_id=cp_id ) - except Exception as e: - _LOGGER.error(f"Failed to setup charger {cp_id}: {str(e)}") + except KeyError as e: + _LOGGER.error(f"Failed to setup charger {cp_id}: Missing configuration key {str(e)}") + except ValueError as e: + _LOGGER.error(f"Failed to setup charger {cp_id}: Invalid configuration value {str(e)}") + except Exception as e: + _LOGGER.error(f"Failed to setup charger {cp_id}: Unexpected error {str(e)}") return
264-335: Extract common ID lookup logic.The ID lookup and charge point validation is repeated across multiple methods. Consider extracting this into a helper method.
+ def _get_charge_point(self, id: str): + """Get charge point by either cpid or cp_id.""" + cp_id = self.cpids.get(id, id) + return self.charge_points.get(cp_id) def get_metric(self, id: str, measurand: str): """Return last known value for given measurand.""" - cp_id = self.cpids.get(id, id) - if cp_id in self.charge_points: - return self.charge_points[cp_id]._metrics[measurand].value + cp = self._get_charge_point(id) + if cp: + return cp._metrics[measurand].value return None
420-444: Improve input validation in charge rate handler.The method could benefit from explicit validation of mutually exclusive parameters and better error handling for JSON parsing.
async def handle_set_charge_rate(self, call): """Handle the data transfer service call.""" amps = call.data.get("limit_amps", None) watts = call.data.get("limit_watts", None) id = call.data.get("conn_id", 0) custom_profile = call.data.get("custom_profile", None) + # Validate that only one parameter is provided + params = sum(1 for p in (custom_profile, watts, amps) if p is not None) + if params != 1: + raise HomeAssistantError( + translation_domain=DOMAIN, + translation_key="invalid_params", + translation_placeholders={"message": "Exactly one of custom_profile, limit_watts, or limit_amps must be provided"}, + ) if custom_profile is not None: if type(custom_profile) is str: - custom_profile = custom_profile.replace("'", '"') - custom_profile = json.loads(custom_profile) + try: + custom_profile = custom_profile.replace("'", '"') + custom_profile = json.loads(custom_profile) + except json.JSONDecodeError as e: + raise HomeAssistantError( + translation_domain=DOMAIN, + translation_key="invalid_json", + translation_placeholders={"message": str(e)}, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/ocpp/api.py(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests
🔇 Additional comments (2)
custom_components/ocpp/api.py (2)
40-82: LGTM! Well-structured service data schemas.The validation schemas are well-defined with appropriate type checking and required/optional field specifications.
445-474: LGTM! Well-structured configuration handlers.The configuration handlers are well-implemented with proper error handling and return types.
* add decorator for handle calls * fix linting * fix linting * fix linting
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
custom_components/ocpp/api.py (4)
100-142: Add service documentation.Consider adding docstrings to describe the purpose and expected behavior of each registered service. This would help users understand how to use these services correctly.
self.hass.services.async_register( DOMAIN, csvcs.service_configure.value, self.handle_configure, CONF_SERVICE_DATA_SCHEMA, supports_response=SupportsResponse.OPTIONAL, + # Add a docstring like: + """Configure a charger setting. + + This service allows you to configure OCPP settings on the charger. + Requires a device ID and OCPP key-value pair. + """ )
Line range hint
150-175: Enhance SSL context error handling.Consider adding explicit error handling for SSL certificate loading to provide clearer error messages to users.
- await self.hass.async_add_executor_job( - partial( - self.ssl_context.load_cert_chain, - localhost_certfile, - keyfile=localhost_keyfile, - ) - ) + try: + await self.hass.async_add_executor_job( + partial( + self.ssl_context.load_cert_chain, + localhost_certfile, + keyfile=localhost_keyfile, + ) + ) + except (FileNotFoundError, ssl.SSLError) as e: + _LOGGER.error(f"Failed to load SSL certificates: {str(e)}") + raise HomeAssistantError( + translation_domain=DOMAIN, + translation_key="ssl_error", + translation_placeholders={"error": str(e)}, + )
272-343: Consider extracting the ID resolution logic.The ID resolution logic is repeated across multiple methods. Consider extracting it into a helper method to improve maintainability.
+ def _resolve_id(self, id: str) -> str: + """Resolve either cpid or cp_id to cp_id.""" + return self.cpids.get(id, id) - def get_metric(self, id: str, measurand: str): - cp_id = self.cpids.get(id, id) + def get_metric(self, id: str, measurand: str): + cp_id = self._resolve_id(id)
386-437: Add return type hints to service handlers.Consider adding return type hints to all service handlers for better code maintainability and IDE support.
- async def handle_clear_profile(self, call, cp): + async def handle_clear_profile(self, call, cp) -> None: - async def handle_update_firmware(self, call, cp): + async def handle_update_firmware(self, call, cp) -> None: - async def handle_get_diagnostics(self, call, cp): + async def handle_get_diagnostics(self, call, cp) -> None: - async def handle_data_transfer(self, call, cp): + async def handle_data_transfer(self, call, cp) -> None: - async def handle_set_charge_rate(self, call, cp): + async def handle_set_charge_rate(self, call, cp) -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/ocpp/api.py(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests
🔇 Additional comments (4)
custom_components/ocpp/api.py (4)
5-82: LGTM! Well-structured service schemas with appropriate validation.The service data schemas are well-defined with proper type validation and consistent naming conventions.
Line range hint
83-98: LGTM! Clean initialization with proper settings handling.The class initialization is well-structured with clear attribute definitions and proper use of type hints.
212-271: LGTM! Robust connection handling with proper error handling.The implementation follows the suggested improvements from the past review, including proper error handling and logging for charger setup.
369-384: LGTM! Well-implemented decorator for availability checking.The decorator effectively centralizes the availability check logic and provides consistent error handling across all service handlers.
* revert device registry setup logic * fix services test * fix ref * get new cs in test * fix linting * remove id check
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
tests/test_charge_point_v16.py (1)
Line range hint
202-239: Consider adding more descriptive assertion messages.While the test is well-structured, consider adding descriptive messages to assertions to improve test failure debugging:
- assert ws2.subprotocol is None + assert ws2.subprotocol is None, "Expected no subprotocol for the websocket connection"custom_components/ocpp/api.py (3)
40-82: Add docstrings to service data schemas.Consider adding docstrings to explain the purpose and usage of each service data schema. This would improve code maintainability and help other developers understand the expected data format.
101-142: Add error handling for service registration.Consider wrapping the service registration calls in a try-except block to handle potential registration failures gracefully.
+ try: self.hass.services.async_register( DOMAIN, csvcs.service_configure.value, self.handle_configure, CONF_SERVICE_DATA_SCHEMA, supports_response=SupportsResponse.OPTIONAL, ) # ... other service registrations ... + except Exception as e: + _LOGGER.error(f"Failed to register services: {str(e)}") + raise HomeAssistantError( + translation_domain=DOMAIN, + translation_key="service_registration_failed", + translation_placeholders={"error": str(e)}, + )
212-255: Enhance logging for connection handling.The connection handling could benefit from more detailed logging to aid in troubleshooting. Consider adding debug logs for key steps in the connection process.
if cp_id not in self.charge_points: try: + _LOGGER.debug(f"Starting connection process for charger {cp_id}") config_flow = False for cfg in self.settings.cpids: if cfg.get(cp_id): config_flow = True cp_settings = ChargerSystemSettings(**list(cfg.values())[0]) _LOGGER.info(f"Charger match found for {cp_settings.cpid}:{cp_id}") _LOGGER.debug(f"Central settings: {self.settings}") if not config_flow: + _LOGGER.debug(f"No configuration found for {cp_id}, initiating discovery flow") info = {"cp_id": cp_id, "entry": self.entry} await self.hass.config_entries.flow.async_init( DOMAIN, context={"source": SOURCE_INTEGRATION_DISCOVERY}, data=info, ) return self.cpids.update({cp_settings.cpid: cp_id}) + _LOGGER.debug(f"Setting up charger {cp_id} with settings: {cp_settings}")custom_components/ocpp/chargepoint.py (3)
139-147: Add type hints to constructor parameters.Consider adding type hints to improve code readability and enable better IDE support.
def __init__( self, - id, # is charger cp_id not HA cpid + id: str, # is charger cp_id not HA cpid connection, version: OcppVersion, hass: HomeAssistant, entry: ConfigEntry, central: CentralSystemSettings, charger: ChargerSystemSettings, ):
341-373: Extract magic numbers into constants.Consider extracting the timeout counter threshold into a constant for better maintainability.
+ # Constants for connection monitoring + MAX_TIMEOUT_RETRIES = 3 + async def monitor_connection(self): """Monitor the connection, by measuring the connection latency.""" self._metrics[cstat.latency_ping.value].unit = "ms" self._metrics[cstat.latency_pong.value].unit = "ms" connection = self._connection timeout_counter = 0 while connection.state is State.OPEN: try: await asyncio.sleep(self.cs_settings.websocket_ping_interval) # ... rest of the code ... if timeout_counter > self.cs_settings.websocket_ping_tries: _LOGGER.debug( f"Connection to '{self.id}' timed out after '{self.cs_settings.websocket_ping_tries}' ping tries", ) raise timeout_exception
Line range hint
505-597: Add detailed documentation for phase data processing.The phase data processing logic is complex and would benefit from detailed documentation explaining the calculations and assumptions.
def process_phases(self, data: list[MeasurandValue]): - """Process phase data from meter values .""" + """Process phase data from meter values. + + This method processes electrical measurements for three-phase power systems: + - Handles line-to-neutral and line-to-line voltage measurements + - Converts line-to-line voltages to line-to-neutral using √3 + - Aggregates current and power measurements across phases + - Handles special cases for non-standard reporting formats + + Args: + data: List of MeasurandValue objects containing phase measurements + + Note: + - Line-to-neutral voltages are averaged + - Line-to-line voltages are converted to line-to-neutral + - Current and power values are summed across phases + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
custom_components/ocpp/__init__.py(2 hunks)custom_components/ocpp/api.py(5 hunks)custom_components/ocpp/button.py(3 hunks)custom_components/ocpp/chargepoint.py(7 hunks)custom_components/ocpp/number.py(4 hunks)custom_components/ocpp/sensor.py(5 hunks)custom_components/ocpp/switch.py(5 hunks)tests/test_charge_point_v16.py(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- custom_components/ocpp/sensor.py
- custom_components/ocpp/button.py
🧰 Additional context used
📓 Learnings (1)
custom_components/ocpp/api.py (1)
Learnt from: drc38
PR: lbbrhzn/ocpp#1498
File: custom_components/ocpp/api.py:0-0
Timestamp: 2025-01-30T01:49:36.840Z
Learning: The user (drc38) is receptive to code quality improvements and refactoring suggestions that reduce code duplication and improve maintainability.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests
🔇 Additional comments (14)
custom_components/ocpp/number.py (3)
78-78: LGTM! Class and parameter renaming.The class has been appropriately renamed from
OcppNumbertoChargePointNumber, and the constructor parameter has been updated fromcp_idtocpidfor consistency.Also applies to: 88-92
124-127: Implement or remove the commented-out available property.The commented-out code suggests incomplete implementation.
60-73: 🛠️ Refactor suggestionVerify the iteration logic for multiple chargers.
The code now supports multiple chargers by iterating over
entry.data[CONF_CPIDS]. However, the maximum current value is being set from the entry level rather than from each charger's settings.Consider updating the code to use charger-specific settings:
for charger in entry.data[CONF_CPIDS]: cp_id_settings = list(charger.values())[0] cpid = cp_id_settings[CONF_CPID] for ent in NUMBERS: if ent.key == "maximum_current": - ent.initial_value = entry.data.get( - CONF_MAX_CURRENT, DEFAULT_MAX_CURRENT - ) - ent.native_max_value = entry.data.get( - CONF_MAX_CURRENT, DEFAULT_MAX_CURRENT - ) + ent.initial_value = cp_id_settings.get( + CONF_MAX_CURRENT, DEFAULT_MAX_CURRENT + ) + ent.native_max_value = cp_id_settings.get( + CONF_MAX_CURRENT, DEFAULT_MAX_CURRENT + ) cpx = ChargePointNumber(hass, central_system, cpid, ent) entities.append(cpx)custom_components/ocpp/switch.py (2)
69-75: LGTM! Proper handling of multiple chargers.The switch setup correctly iterates over multiple chargers and creates switch entities for each one.
89-93: LGTM! Constructor parameter update.The constructor parameter has been appropriately updated from
cp_idtocpidfor consistency.custom_components/ocpp/__init__.py (3)
108-126: LGTM! Improved device registry handling.The code now properly creates both central system and charger devices with appropriate identifiers and relationships.
206-221: 🛠️ Refactor suggestionAdd timeout and error handling to unload process.
The unload entry function could be improved with timeout and error handling.
Apply this diff to improve the unload process:
async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: """Handle removal of an entry.""" unloaded = False if DOMAIN in hass.data: if entry.entry_id in hass.data[DOMAIN]: central_sys = hass.data[DOMAIN][entry.entry_id] + try: + with async_timeout.timeout(10): central_sys._server.close() await central_sys._server.wait_closed() + except asyncio.TimeoutError: + _LOGGER.error("Timeout waiting for server to close") + except Exception as err: + _LOGGER.error("Error closing server: %s", err) + # Document connection state handling + _LOGGER.debug("Current connections: %d", central_sys.connections) for service in hass.services.async_services_for_domain(DOMAIN): - hass.services.async_remove(DOMAIN, service) + try: + hass.services.async_remove(DOMAIN, service) + except Exception as err: + _LOGGER.error("Error removing service %s: %s", service, err)Likely invalid or redundant comment.
138-198: 🛠️ Refactor suggestionAdd data validation and error handling to migration logic.
The migration function could be improved with better error handling and data validation.
Apply this diff to improve the migration function:
async def async_migrate_entry(hass, config_entry: ConfigEntry): """Migrate old entry.""" + try: _LOGGER.debug( "Migrating configuration from version %s.%s", config_entry.version, config_entry.minor_version, ) if config_entry.version > 2: return False if config_entry.version == 1: old_data = {**config_entry.data} + if not isinstance(old_data, dict): + _LOGGER.error("Invalid configuration data type") + return False + csid_data = {} cpid_data = {} # ... existing code ... new_data = csid_data cp_id = hass.states.get(f"sensor.{cpid_data[CONF_CPID]}_id") if cp_id is None: + _LOGGER.error("Charger state not found") return False new_data.update({CONF_CPIDS: [{cp_id: cpid_data}]}) hass.config_entries.async_update_entry( config_entry, data=new_data, minor_version=0, version=2 ) + except Exception as e: + _LOGGER.error("Migration failed: %s", str(e)) + return False _LOGGER.debug( "Migration to configuration version %s.%s successful", config_entry.version, config_entry.minor_version, ) return TrueLikely invalid or redundant comment.
tests/test_charge_point_v16.py (6)
4-4: LGTM! Well-organized imports and constants.The new imports and constants are appropriately used throughout the code. The
SERVICES_ERRORlist is a good addition for testing error scenarios with limited profiles.Also applies to: 9-9, 67-73
76-167: LGTM! Well-structured test helper functions.The helper functions are well-organized with:
- Clear parameter naming (cpid instead of cs)
- Comprehensive test coverage
- Proper error handling
- Good reusability across test cases
Line range hint
275-336: LGTM! Well-implemented value restoration test.The test effectively verifies the central system's ability to restore values after simulated data loss. Good use of assertions to validate the restored values.
Line range hint
348-542: LGTM! Comprehensive normal operation tests.The test suite effectively covers:
- Normal charger operations
- Multi-charger scenarios
- Different energy measurement units
- Various meter value formats
553-621: LGTM! Thorough error scenario testing.Good coverage of error scenarios with:
- Rejection messages
- Service unavailability
- Error handling with HomeAssistantError
Line range hint
624-1183: LGTM! Well-implemented ChargePoint test class.The class provides:
- Comprehensive coverage of OCPP operations
- Good error handling with accept flag
- Clear method documentation
- Proper use of decorators for action handlers
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
custom_components/ocpp/config_flow.py (1)
Line range hint
78-104: Add validation for numeric fields and CPID format.Consider adding validation rules to ensure data integrity:
STEP_USER_CP_DATA_SCHEMA = vol.Schema( { - vol.Required(CONF_CPID, default=DEFAULT_CPID): str, + vol.Required(CONF_CPID, default=DEFAULT_CPID): vol.Match(r'^[a-zA-Z0-9-_]+$'), - vol.Required(CONF_MAX_CURRENT, default=DEFAULT_MAX_CURRENT): int, + vol.Required(CONF_MAX_CURRENT, default=DEFAULT_MAX_CURRENT): vol.All( + int, vol.Range(min=0, max=100) + ), - vol.Required(CONF_METER_INTERVAL, default=DEFAULT_METER_INTERVAL): int, + vol.Required(CONF_METER_INTERVAL, default=DEFAULT_METER_INTERVAL): vol.All( + int, vol.Range(min=0) + ), - vol.Required(CONF_IDLE_INTERVAL, default=DEFAULT_IDLE_INTERVAL): int, + vol.Required(CONF_IDLE_INTERVAL, default=DEFAULT_IDLE_INTERVAL): vol.All( + int, vol.Range(min=0) + ), # ... rest of the schema } )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/ocpp/config_flow.py(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests
🔇 Additional comments (4)
custom_components/ocpp/config_flow.py (4)
Line range hint
3-77: LGTM! Clean separation of central system configuration schema.The separation of configuration schemas and the addition of necessary imports look good. This provides a clear distinction between central system and charger-specific configurations.
105-118: 🛠️ Refactor suggestionInitialize all class attributes.
Some attributes are still uninitialized:
def __init__(self): """Initialize.""" self._data: dict[str, Any] = {} - self._cp_id: str - self._entry: ConfigEntry + self._cp_id: str = "" + self._entry: ConfigEntry | None = None self._measurands: str = ""Likely invalid or redundant comment.
179-196: 🛠️ Refactor suggestionImprove error handling in measurands step.
The error handling for measurands selection needs improvement:
- if not set(selected_measurands).issubset(set(MEASURANDS)): + if not selected_measurands: errors["base"] = "no_measurands_selected" return self.async_show_form( step_id="measurands", data_schema=STEP_USER_MEASURANDS_SCHEMA, errors=errors, ) + if not set(selected_measurands).issubset(set(MEASURANDS)): + errors["base"] = "invalid_measurands" + return self.async_show_form( + step_id="measurands", + data_schema=STEP_USER_MEASURANDS_SCHEMA, + errors=errors, + ) else: self._measurands = ",".join(selected_measurands)Likely invalid or redundant comment.
155-168:⚠️ Potential issueFix potential race condition in charger configuration.
The current implementation might have race conditions when multiple chargers are being configured simultaneously:
if user_input is not None: # Don't allow duplicate cpids to be used self._async_abort_entries_match({CONF_CPID: user_input[CONF_CPID]}) - self._data[CONF_CPIDS].append({self._cp_id: user_input}) + # Create a new copy of the data to avoid race conditions + updated_data = {**self._data} + if CONF_CPIDS not in updated_data: + updated_data[CONF_CPIDS] = [] + updated_data[CONF_CPIDS].append({self._cp_id: user_input}) if user_input[CONF_MONITORED_VARIABLES_AUTOCONFIG]: - self._data[CONF_CPIDS][-1][self._cp_id][CONF_MONITORED_VARIABLES] = ( + updated_data[CONF_CPIDS][-1][self._cp_id][CONF_MONITORED_VARIABLES] = ( DEFAULT_MONITORED_VARIABLES ) return self.async_update_reload_and_abort( self._entry, - data_updates=self._data, + data_updates=updated_data, )Likely invalid or redundant comment.
|
@rbb-dev this version works for me https://github.com/drc38/ocpp/releases/tag/0.8.13 if there are issues for you check the config entry output to make sure the chargers have been entered and saved by HA. |
This can be tested here: https://github.com/drc38/ocpp/tree/0.8.6
Please take a backup first as the config entry will be migrated and to go back will require restoring the backup or starting from a clean install of an older version of the integration.
Summary by CodeRabbit
Release Notes for OCPP Integration v0.8.0
New Features
Configuration Changes
Bug Fixes
Performance Improvements
Breaking Changes