Prework for multiple chargers per cs #1480
Conversation
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1480 +/- ##
==========================================
- Coverage 94.69% 94.31% -0.38%
==========================================
Files 12 12
Lines 1943 2042 +99
==========================================
+ Hits 1840 1926 +86
- Misses 103 116 +13 ☔ View full report in Codecov by Sentry. |
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
drc38
changed the title
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
|
A release with these changes is available here: https://github.com/drc38/ocpp/releases/tag/0.8.1 |
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
drc38
had a problem deploying
to
continuous-integration
GitHub Actions
Failure
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
This reverts commit 6c32421.
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe pull request introduces a comprehensive refactoring of the OCPP (Open Charge Point Protocol) Home Assistant integration, focusing on enhancing configuration management and supporting multiple charge points. The changes span across multiple files, introducing new data structures, modifying configuration flows, and updating how charge point identifiers are handled. The primary goals include improving flexibility in managing charge point configurations, adding support for multiple charge points, and streamlining the configuration process. Changes
Sequence DiagramsequenceDiagram
participant User
participant ConfigFlow
participant CentralSystem
participant ChargePoint
User->>ConfigFlow: Start Configuration
ConfigFlow->>ConfigFlow: Central System Config
ConfigFlow->>ConfigFlow: Charge Point Config
ConfigFlow->>CentralSystem: Create Central System
CentralSystem->>ChargePoint: Create Charge Points
ChargePoint-->>CentralSystem: Register Charge Points
CentralSystem-->>User: Configuration Complete
Poem
Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (11)
custom_components/ocpp/api.py (2)
44-50: Consider initializingself.cpidsandself.charge_pointstogetherThe mappings for
self.cpidsandself.charge_pointsare related. Consolidating their initialization might improve code readability and maintainability.You could create a helper function or data structure to manage charge point mappings more effectively.
201-205: Avoid repeated code for ID mappingThe logic for mapping
idtocp_idis repeated across several methods. This can be refactored into a helper method to reduce code duplication and potential errors.Create a helper method
_get_cp_id:def _get_cp_id(self, id: str) -> str: cp_id = self.cpids.get(id) if cp_id is None: cp_id = id return cp_idThen use it in your methods:
def get_metric(self, id: str, measurand: str): cp_id = self._get_cp_id(id) # Rest of the method...custom_components/ocpp/button.py (1)
50-50: Retrieve firstcpidfromCONF_CPIDSwith consideration for future supportExtracting the first
cpidfromCONF_CPIDSmaintains current functionality. However, consider iterating over allcpidsinCONF_CPIDSto fully support multiple charge points as intended.tests/test_config_flow.py (1)
53-58: Add test coverage for error cases in the configuration flow.The test only covers the successful path. Consider adding test cases for:
- Empty charge point configuration
- Invalid charge point configuration
- Duplicate charge point IDs
tests/charge_point_test.py (1)
30-37: Consider consolidating entity ID construction.The entity ID construction pattern is repeated across multiple functions. Consider extracting it to a helper function to improve maintainability.
def _build_entity_id(domain: str, cpid: str, key: str) -> str: """Build entity ID from components.""" return f"{domain}.{cpid}_{key}"Also applies to: 40-48, 54-61
custom_components/ocpp/const.py (2)
175-179: Remove or implement the commented post_init method.The commented post_init method appears to be a work in progress. Either implement it correctly or remove it to maintain clean code.
Also, there's a typo in the increment operation (
i =+ 1should bei += 1).
171-173: Improve documentation for the mapping field.The current comment "maps [{cp_id: cpid}] order to match cpids settings" could be more descriptive. Consider adding more detailed documentation explaining the purpose and structure of this mapping.
custom_components/ocpp/sensor.py (1)
47-50: Consider simplifying the monitored variables handling.The current implementation combines monitored variables from configuration and HAChargerSession using string splitting and set operations. Consider moving this logic to a helper method for better maintainability.
+def get_monitored_metrics(config_vars: str, session_vars: list) -> list: + """Combine and deduplicate monitored variables.""" + return list(set(config_vars.split(",") + list(session_vars))) - for metric in list( - set( - entry.data[CONF_CPIDS][0][cpid][CONF_MONITORED_VARIABLES].split(",") - + list(HAChargerSession) - ) - ): + for metric in get_monitored_metrics( + entry.data[CONF_CPIDS][0][cpid][CONF_MONITORED_VARIABLES], + list(HAChargerSession) + ):tests/const.py (1)
132-149: Consider adding negative test cases.While the current test configurations cover valid scenarios, consider adding test cases for:
- Empty CPIDS list
- Invalid CPID format
- Missing required fields in CPID configuration
Also applies to: 152-170
tests/test_charge_point_v16.py (1)
413-482: Add test coverage for error handling in service tests.The service tests should include additional test cases for error scenarios.
Consider adding test cases for:
- Network timeouts
- Invalid response formats
- Connection drops during service calls
custom_components/ocpp/translations/es.json (1)
19-19: Fix typo in Spanish help text.The word "hechar" is misspelled. It should be "echar".
- "description": "Si necesitas ayuda con la configuración puedes hechar un vistazo en: https://github.com/lbbrhzn/ocpp", + "description": "Si necesitas ayuda con la configuración puedes echar un vistazo en: https://github.com/lbbrhzn/ocpp",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
custom_components/ocpp/__init__.py(3 hunks)custom_components/ocpp/api.py(4 hunks)custom_components/ocpp/button.py(3 hunks)custom_components/ocpp/chargepoint.py(7 hunks)custom_components/ocpp/config_flow.py(4 hunks)custom_components/ocpp/const.py(4 hunks)custom_components/ocpp/number.py(4 hunks)custom_components/ocpp/ocppv16.py(14 hunks)custom_components/ocpp/ocppv201.py(6 hunks)custom_components/ocpp/sensor.py(6 hunks)custom_components/ocpp/switch.py(5 hunks)custom_components/ocpp/translations/de.json(1 hunks)custom_components/ocpp/translations/en.json(2 hunks)custom_components/ocpp/translations/es.json(1 hunks)custom_components/ocpp/translations/i-default.json(2 hunks)custom_components/ocpp/translations/nl.json(1 hunks)tests/charge_point_test.py(2 hunks)tests/const.py(2 hunks)tests/test_charge_point_v16.py(11 hunks)tests/test_charge_point_v201.py(13 hunks)tests/test_config_flow.py(2 hunks)tests/test_init.py(3 hunks)tests/test_sensor.py(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
custom_components/ocpp/ocppv16.py
[warning] 249-250: custom_components/ocpp/ocppv16.py#L249-L250
Added lines #L249 - L250 were not covered by tests
custom_components/ocpp/chargepoint.py
[warning] 499-499: custom_components/ocpp/chargepoint.py#L499
Added line #L499 was not covered by tests
custom_components/ocpp/config_flow.py
[warning] 139-139: custom_components/ocpp/config_flow.py#L139
Added line #L139 was not covered by tests
[warning] 157-157: custom_components/ocpp/config_flow.py#L157
Added line #L157 was not covered by tests
custom_components/ocpp/api.py
[warning] 60-61: custom_components/ocpp/api.py#L60-L61
Added lines #L60 - L61 were not covered by tests
[warning] 214-214: custom_components/ocpp/api.py#L214
Added line #L214 was not covered by tests
custom_components/ocpp/__init__.py
[warning] 144-144: custom_components/ocpp/init.py#L144
Added line #L144 was not covered by tests
🔇 Additional comments (46)
custom_components/ocpp/config_flow.py (2)
139-139: Line not covered by testsThe line
measurands = await self.async_step_measurands()is not covered by tests. It's important to create unit tests that cover this branch to ensure the configuration flow works as expected.Would you like assistance in writing unit tests to cover this scenario?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 139-139: custom_components/ocpp/config_flow.py#L139
Added line #L139 was not covered by tests
157-157: Line not covered by testsThe line
return ",".join(selected_measurands)is not covered by tests. Adding tests for selecting measurands will help catch potential issues.I can help you generate unit tests for this method if needed.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 157-157: custom_components/ocpp/config_flow.py#L157
Added line #L157 was not covered by testscustom_components/ocpp/__init__.py (1)
144-144: Line not covered by tests: Migration downgrade pathThe line
return Falseindicates that migration cannot proceed ifconfig_entry.version > 1. This scenario is not covered by tests.Adding tests for downgrade scenarios can help ensure that the migration function behaves correctly when users downgrade from a future version.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 144-144: custom_components/ocpp/init.py#L144
Added line #L144 was not covered by testscustom_components/ocpp/api.py (2)
214-214: Line not covered by testsThe line
cp_id = idinsidedel_metricis not covered by tests. This may lead to untested scenarios where metrics are not deleted properly.Would you like help in writing tests that cover this code path?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 214-214: custom_components/ocpp/api.py#L214
Added line #L214 was not covered by tests
60-61: Ensure SSL certificate paths are valid and securedAssigning SSL certificate and key file paths directly from settings may pose security risks if not properly validated. Ensure that the paths are secure and handle any potential exceptions.
[security]
Consider adding error handling for cases where the certificate files are missing or unreadable.
try: localhost_certfile = self.settings.certfile localhost_keyfile = self.settings.keyfile except Exception as e: _LOGGER.error(f"Error loading SSL certificates: {e}") return False🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 60-61: custom_components/ocpp/api.py#L60-L61
Added lines #L60 - L61 were not covered by testscustom_components/ocpp/ocppv16.py (9)
39-42: ImportOcppVersionandMeasurandValuefromchargepointmoduleThe addition of these imports aligns with their usage in the code and ensures that the required classes are available.
57-58: IncludeChargerSystemSettingsin importsAdding
ChargerSystemSettingsto the imports reflects its usage in the constructor and is necessary for type hinting and accessing charger settings.
77-77: UpdateChargePointconstructor to includechargerparameterAdding the
chargerparameter of typeChargerSystemSettingsand passing it to the superclass ensures that charger-specific settings are correctly initialized and managed throughout the class hierarchy.Also applies to: 88-88
101-102: Access monitored variables fromself.settingsUpdating
get_supported_measurandsto useself.settings.monitored_variablesandself.settings.monitored_variables_autoconfigaligns with the new settings structure and improves code readability.
153-153: Configure intervals usingself.settingsUsing
self.settings.meter_intervalandself.settings.idle_intervalinset_standard_configurationensures that the correct intervals are applied based on the charger settings.Also applies to: 157-157
207-207: Checkforce_smart_chargingin settingsThe condition
if self.settings.force_smart_chargingenables forceful activation of smart charging based on the charger settings, enhancing configurability.
247-250: Refactor exception handling fornof_connectorsand add testsInstead of catching a broad
TypeError, consider explicitly checking ifself._metrics[cdet.connectors.value].valueisNonebefore converting it to an integer. This improves code clarity and prevents unintended exceptions.Also, since these lines are not covered by tests, please add unit tests to ensure this code path is tested, addressing the static analysis warning.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 249-250: custom_components/ocpp/ocppv16.py#L249-L250
Added lines #L249 - L250 were not covered by tests
572-572: Update device info usingself.settings.cpidReplacing
self.central.cpidwithself.settings.cpidinasync_update_device_info_v16ensures consistency with the new settings structure and accurately reflects the charge point identifier.
657-657: Useself.settings.cpidin asynchronous update tasksUpdating the calls to
self.hass.async_create_task(self.update(self.settings.cpid))ensures that the correct charge point identifier is used across various async methods, maintaining consistency.Also applies to: 708-708, 715-715, 768-768, 801-801, 819-819
tests/test_sensor.py (4)
14-14: Importcreate_configurationandremove_configurationfunctionsImporting
create_configurationandremove_configurationfromcharge_point_testmodularizes the configuration setup and teardown processes in tests, enhancing code reuse.
22-25: UpdateMockConfigEntrywith detailed identifiersSetting
entry_id,title,version, andminor_versionin theMockConfigEntryprovides clearer identification and version control for the config entry used in the test.
28-29: Initialize configuration usingcreate_configurationUsing
create_configurationto set up the configuration entry ensures consistency with other tests and centralizes configuration logic.
40-40: Clean up configuration withremove_configurationCalling
remove_configurationat the end of the test ensures proper teardown, preventing side effects in subsequent tests.custom_components/ocpp/button.py (3)
17-17: ImportCONF_CPIDSto support multiple charge pointsUpdating the import to
CONF_CPIDSreflects the transition towards supporting multiple charge point identifiers within the integration.
55-55: InstantiateChargePointButtonwithcpidPassing
cpidtoChargePointButtonensures the button entity is correctly associated with the specific charge point.
69-69: UpdateChargePointButtonto usecpidconsistentlyRenaming the parameter and all instances from
cp_idtocpidimproves code consistency and aligns with the updated configuration keys.Also applies to: 73-73, 77-77, 81-81, 87-87, 92-92
tests/test_init.py (1)
57-89: LGTM! Comprehensive migration test coverage.The test thoroughly verifies:
- Configuration entry migration
- Data structure validation
- Version updates
- Cleanup
custom_components/ocpp/switch.py (2)
87-101: LGTM! Clean transition to the new CPID structure.The changes consistently update the ChargePointSwitch class to use the new CPID structure while maintaining proper device identification and functionality.
68-68: Add error handling for empty CPIDS list.The current implementation assumes that CONF_CPIDS always contains at least one entry. Consider adding validation to handle cases where the list might be empty.
custom_components/ocpp/const.py (1)
140-170: LGTM! Well-structured dataclasses for configuration.The ChargerSystemSettings and CentralSystemSettings dataclasses provide a clean and type-safe way to manage configuration data.
custom_components/ocpp/sensor.py (1)
92-108: LGTM! Consistent implementation of the new CPID structure.The ChargePointMetric class has been properly updated to use the new CPID structure while maintaining all functionality.
tests/const.py (1)
26-73: LGTM! Well-structured test configurations.The mock configurations are well organized and cover different scenarios including basic setup, flow configuration, and the new CPIDS structure.
custom_components/ocpp/ocppv201.py (3)
57-58: LGTM! Import changes align with the new settings structure.The imports are updated to include both
CentralSystemSettingsandChargerSystemSettings, maintaining backward compatibility while introducing the new settings class.
93-93: LGTM! Constructor signature updated to support both settings types.The constructor now accepts both
centralandchargerparameters, allowing for a smooth transition to the new settings structure while maintaining backward compatibility.Also applies to: 104-104
110-110: LGTM! References updated to use the new settings structure.All references to
self.central.cpidhave been consistently updated to useself.settings.cpid, andDEFAULT_METER_INTERVALis now accessed throughself.settings.meter_interval.Also applies to: 146-146, 691-691
custom_components/ocpp/chargepoint.py (5)
45-46: LGTM! Import changes align with the new configuration structure.The imports are updated to include both settings types and the new
CONF_CPIDSconstant.Also applies to: 52-52
182-188: LGTM! Constructor updated to support both settings types.The constructor now accepts both settings types and correctly initializes them, with schema validation using the new settings structure.
Also applies to: 203-205, 209-210
571-571: LGTM! Device registration and update logic improved.The device registration now correctly handles multiple identifiers, and the update method properly uses the new
cpidparameter.Also applies to: 590-594
807-807: LGTM! Metric access updated to use new settings.The metric access is updated to use the new settings structure for constructing entity IDs.
471-481: Verify the websocket ping configuration.The connection monitoring now uses settings from
cs_settingsfor websocket configuration. Ensure that the default values for these settings are appropriate for production use:
websocket_ping_intervalwebsocket_ping_timeoutwebsocket_ping_triesAlso applies to: 487-487, 494-494, 499-499, 501-501
✅ Verification successful
Websocket ping configuration is appropriate for production use
The default values are well-balanced:
websocket_ping_interval: 20 secondswebsocket_ping_timeout: 20 secondswebsocket_ping_tries: 2 attemptsThese settings provide reliable connection monitoring while maintaining reasonable network overhead.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for websocket configuration settings in the codebase # Expected: Find default values and any validation logic for these settings # Search for websocket ping configuration echo "Searching for websocket ping configuration..." rg -A 5 "websocket_ping_(interval|timeout|tries)" # Search for CentralSystemSettings class definition echo "Searching for CentralSystemSettings class definition..." ast-grep --pattern 'class CentralSystemSettings { $$$ }'Length of output: 9919
Script:
#!/bin/bash # Search for config validation and default values echo "Searching for config validation and defaults..." rg -A 10 "CONF_WEBSOCKET_PING" custom_components/ocpp/config_flow.py custom_components/ocpp/config_validation.py custom_components/ocpp/const.py # Search for schema or validation echo "Searching for schema or validation..." rg "vol\.Schema|WEBSOCKET.*DEFAULT" custom_components/ocpp/Length of output: 5467
Script:
#!/bin/bash # Search for default websocket constants echo "Searching for default websocket constants..." rg "^DEFAULT_WEBSOCKET" custom_components/ocpp/const.pyLength of output: 299
tests/test_charge_point_v16.py (3)
4-4: LGTM! Test imports and helper functions properly organized.The imports are well-organized, and the helper functions are extracted for better reusability.
Also applies to: 37-45
46-63: LGTM! Service lists well-defined for testing.The service lists are properly categorized into
SERVICESandSERVICES_ERRORfor comprehensive testing.
70-82: LGTM! Test functions updated to use cpid parameter.The test functions are correctly updated to use the
cpidparameter instead ofcs.tests/test_charge_point_v201.py (2)
1227-1232: LGTM! Config entries properly structured.The config entries are correctly configured with version information and test data.
Also applies to: 1256-1261
1154-1157: LGTM! Charge point ID retrieval updated.The charge point ID retrieval is correctly updated to use the new list-based approach.
Also applies to: 1206-1209
custom_components/ocpp/translations/en.json (2)
5-5: Well-structured separation of configurations!The split into "OCPP Central System Configuration" and "OCPP Charger Configuration" provides a clear separation of concerns and lays good groundwork for supporting multiple chargers per charging station.
Also applies to: 21-21
14-17: Comprehensive WebSocket connection management!The addition of detailed WebSocket configuration parameters enables fine-tuned control over connection behavior and reliability.
custom_components/ocpp/translations/i-default.json (1)
Line range hint
1-24: Excellent translation consistency across all language files!The restructuring of configuration options is consistently implemented across all translation files (nl, es, de, en, i-default), maintaining proper translations while ensuring a uniform configuration experience regardless of the chosen language.
custom_components/ocpp/translations/nl.json (1)
14-16: LGTM! Clean separation of central system and charger configurations.The restructuring of configuration steps improves code organization by:
- Keeping central system settings in the "user" section
- Moving charger-specific settings to the new "cp_user" section
This change aligns well with the PR objective of preparing for multiple chargers per charging station.
Also applies to: 17-23
custom_components/ocpp/translations/es.json (1)
14-16: LGTM! Configuration structure matches other translation files.The separation of central system and charger configurations is consistent with other language files.
Also applies to: 17-23
custom_components/ocpp/translations/de.json (1)
17-18: LGTM! Configuration structure is consistent and complete.The changes maintain proper organization while including:
- SSL configuration fields in the central system section
- Charger-specific settings in the "cp_user" section
- Additional auto-configuration option for monitored variables
Also applies to: 20-29
| self._data[CONF_CPIDS] = [] | ||
| 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]}) |
There was a problem hiding this comment.
Prevent duplicate CPIDs in charger configuration
The method self._async_abort_entries_match is not appropriate here. To avoid adding duplicate CPIDs, you should check if the CPID already exists in self._data[CONF_CPIDS].
Consider adding a validation check before appending the new CPID:
for cpid_dict in self._data[CONF_CPIDS]:
if user_input[CONF_CPID] in cpid_dict:
errors["base"] = "duplicate_cpid"
return self.async_show_form(
step_id="cp_user",
data_schema=STEP_USER_CP_DATA_SCHEMA,
errors=errors,
)This will prevent duplicate CPIDs from being added and provide feedback to the user.
| # Don't allow servers to use same websocket port | ||
| self._async_abort_entries_match({CONF_PORT: user_input[CONF_PORT]}) |
There was a problem hiding this comment.
Use correct method to prevent duplicate entries
The method self._async_abort_entries_match is not valid. To prevent multiple entries with the same port, you should set a unique ID and abort if it already exists.
Apply this diff to fix the issue:
- self._async_abort_entries_match({CONF_PORT: user_input[CONF_PORT]})
+ await self.async_set_unique_id(f"ocpp_{user_input[CONF_PORT]}")
+ self._abort_if_unique_id_configured()This change sets a unique ID based on the port and ensures that only one entry per port can be configured.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Don't allow servers to use same websocket port | |
| self._async_abort_entries_match({CONF_PORT: user_input[CONF_PORT]}) | |
| # Don't allow servers to use same websocket port | |
| await self.async_set_unique_id(f"ocpp_{user_input[CONF_PORT]}") | |
| self._abort_if_unique_id_configured() |
| async def async_step_measurands(self, user_input=None) -> ConfigFlowResult | str: | ||
| """Select the measurands to be shown.""" | ||
|
|
||
| errors: dict[str, str] = {} | ||
| if user_input is not None: | ||
| selected_measurands = [m for m, value in user_input.items() if value] | ||
| if set(selected_measurands).issubset(set(MEASURANDS)): | ||
| self._data[CONF_MONITORED_VARIABLES] = ",".join(selected_measurands) | ||
| return self.async_create_entry( | ||
| title=self._data[CONF_CSID], data=self._data | ||
| ) | ||
| return ",".join(selected_measurands) |
There was a problem hiding this comment.
Ensure async_step_measurands returns the correct type
The method async_step_measurands should return a ConfigFlowResult, not a string. Returning a string can lead to unexpected behavior.
Modify the method to store the selected measurands and proceed accordingly:
if user_input is not None:
selected_measurands = [m for m, value in user_input.items() if value]
if set(selected_measurands).issubset(set(MEASURANDS)):
- return ",".join(selected_measurands)
+ self._data["measurands"] = ",".join(selected_measurands)
+ self._data[CONF_CPIDS].append({user_input[CONF_CPID]: user_input})
+ return self.async_create_entry(title=self._data[CONF_CSID], data=self._data)
else:
errors["base"] = "measurand"This change ensures the method returns a ConfigFlowResult and continues the flow properly.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 157-157: custom_components/ocpp/config_flow.py#L157
Added line #L157 was not covered by tests
| measurands = await self.async_step_measurands() | ||
| else: | ||
| measurands = DEFAULT_MONITORED_VARIABLES |
There was a problem hiding this comment.
Correct handling of measurands step
The call to async_step_measurands should return a ConfigFlowResult, but it's currently being assigned to measurands, which expects a string. This can cause issues in the configuration flow.
Refactor the code to properly handle the transition to the measurands step:
if not user_input[CONF_MONITORED_VARIABLES_AUTOCONFIG]:
- measurands = await self.async_step_measurands()
+ return await self.async_step_measurands()
else:
measurands = DEFAULT_MONITORED_VARIABLES
user_input[CONF_MONITORED_VARIABLES] = measurands
self._data[CONF_CPIDS].append({user_input[CONF_CPID]: user_input})
return self.async_create_entry(title=self._data[CONF_CSID], data=self._data)This ensures the flow moves to the measurands step correctly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| measurands = await self.async_step_measurands() | |
| else: | |
| measurands = DEFAULT_MONITORED_VARIABLES | |
| if not user_input[CONF_MONITORED_VARIABLES_AUTOCONFIG]: | |
| return await self.async_step_measurands() | |
| else: | |
| measurands = DEFAULT_MONITORED_VARIABLES | |
| user_input[CONF_MONITORED_VARIABLES] = measurands | |
| self._data[CONF_CPIDS].append({user_input[CONF_CPID]: user_input}) | |
| return self.async_create_entry(title=self._data[CONF_CSID], data=self._data) |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 139-139: custom_components/ocpp/config_flow.py#L139
Added line #L139 was not covered by tests
| async def async_migrate_entry(hass, config_entry: ConfigEntry): | ||
| """Migrate old entry.""" | ||
| _LOGGER.debug( | ||
| "Migrating configuration from version %s.%s", | ||
| config_entry.version, | ||
| config_entry.minor_version, | ||
| ) | ||
|
|
||
| if config_entry.version > 1: | ||
| # This means the user has downgraded from a future version | ||
| return False | ||
|
|
||
| if config_entry.version == 1: | ||
| old_data = {**config_entry.data} | ||
| csid_data = {} | ||
| cpid_data = {} | ||
| cpid_keys = { | ||
| CONF_CPID: DEFAULT_CPID, | ||
| CONF_IDLE_INTERVAL: DEFAULT_IDLE_INTERVAL, | ||
| CONF_MAX_CURRENT: DEFAULT_MAX_CURRENT, | ||
| CONF_METER_INTERVAL: DEFAULT_METER_INTERVAL, | ||
| CONF_MONITORED_VARIABLES: DEFAULT_MONITORED_VARIABLES, | ||
| CONF_MONITORED_VARIABLES_AUTOCONFIG: DEFAULT_MONITORED_VARIABLES_AUTOCONFIG, | ||
| CONF_SKIP_SCHEMA_VALIDATION: DEFAULT_SKIP_SCHEMA_VALIDATION, | ||
| CONF_FORCE_SMART_CHARGING: DEFAULT_FORCE_SMART_CHARGING, | ||
| } | ||
| csid_keys = { | ||
| CONF_HOST: DEFAULT_HOST, | ||
| CONF_PORT: DEFAULT_PORT, | ||
| CONF_CSID: DEFAULT_CSID, | ||
| CONF_SSL: DEFAULT_SSL, | ||
| CONF_SSL_CERTFILE_PATH: DEFAULT_SSL_CERTFILE_PATH, | ||
| CONF_SSL_KEYFILE_PATH: DEFAULT_SSL_KEYFILE_PATH, | ||
| CONF_WEBSOCKET_CLOSE_TIMEOUT: DEFAULT_WEBSOCKET_CLOSE_TIMEOUT, | ||
| CONF_WEBSOCKET_PING_TRIES: DEFAULT_WEBSOCKET_PING_TRIES, | ||
| CONF_WEBSOCKET_PING_INTERVAL: DEFAULT_WEBSOCKET_PING_INTERVAL, | ||
| CONF_WEBSOCKET_PING_TIMEOUT: DEFAULT_WEBSOCKET_PING_TIMEOUT, | ||
| } | ||
| for key, value in cpid_keys.items(): | ||
| cpid_data.update({key: old_data.get(key, value)}) | ||
|
|
||
| for key, value in csid_keys.items(): | ||
| csid_data.update({key: old_data.get(key, value)}) | ||
|
|
||
| new_data = csid_data | ||
| new_data.update({CONF_CPIDS: [{cpid_data[CONF_CPID]: cpid_data}]}) | ||
|
|
||
| hass.config_entries.async_update_entry( | ||
| config_entry, data=new_data, minor_version=0, version=2 | ||
| ) | ||
|
|
||
| _LOGGER.debug( | ||
| "Migration to configuration version %s.%s successful", | ||
| config_entry.version, | ||
| config_entry.minor_version, | ||
| ) | ||
|
|
||
| return True | ||
|
|
There was a problem hiding this comment.
Review and test the migration logic thoroughly
The async_migrate_entry function is critical for migrating configuration entries from version 1 to version 2. Ensure that all possible scenarios are handled, including edge cases where users might downgrade or have incomplete data.
Consider adding comprehensive unit tests to cover the migration process. This will help identify any potential issues and ensure a smooth transition for users.
Would you like assistance in creating tests for the migration logic?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 144-144: custom_components/ocpp/init.py#L144
Added line #L144 was not covered by tests
| @@ -56,15 +55,15 @@ async def async_setup_entry(hass, entry, async_add_devices): | |||
| """Configure the number platform.""" | |||
|
|
|||
| central_system = hass.data[DOMAIN][entry.entry_id] | |||
| cp_id = entry.data.get(CONF_CPID, DEFAULT_CPID) | |||
| cpid = list(entry.data[CONF_CPIDS][0].keys())[0] | |||
There was a problem hiding this comment.
Verify array access safety.
The code assumes the first element exists in entry.data[CONF_CPIDS] without bounds checking. This could raise an IndexError if the list is empty.
Add a safety check:
- cpid = list(entry.data[CONF_CPIDS][0].keys())[0]
+ if not entry.data[CONF_CPIDS]:
+ raise ValueError("No charge points configured")
+ cpid = list(entry.data[CONF_CPIDS][0].keys())[0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cpid = list(entry.data[CONF_CPIDS][0].keys())[0] | |
| if not entry.data[CONF_CPIDS]: | |
| raise ValueError("No charge points configured") | |
| cpid = list(entry.data[CONF_CPIDS][0].keys())[0] |
| self.cpid | ||
| ) and Profiles.SMART & self.central_system.get_supported_features(self.cpid): | ||
| resp = await self.central_system.set_max_charge_rate_amps( | ||
| self.cp_id, num_value | ||
| self.cpid, num_value |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for the central system calls.
The calls to central_system methods lack error handling, which could lead to unhandled exceptions if the central system is unavailable.
Add error handling:
async def async_set_native_value(self, value):
"""Set new value."""
num_value = float(value)
- if self.central_system.get_available(
- self.cpid
- ) and Profiles.SMART & self.central_system.get_supported_features(self.cpid):
- resp = await self.central_system.set_max_charge_rate_amps(
- self.cpid, num_value
- )
+ try:
+ if self.central_system.get_available(
+ self.cpid
+ ) and Profiles.SMART & self.central_system.get_supported_features(self.cpid):
+ resp = await self.central_system.set_max_charge_rate_amps(
+ self.cpid, num_value
+ )
+ if resp is True:
+ self._attr_native_value = num_value
+ self.async_write_ha_state()
+ except Exception as ex:
+ _LOGGER.error("Failed to set charge rate: %s", str(ex))
+ raise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.cpid | |
| ) and Profiles.SMART & self.central_system.get_supported_features(self.cpid): | |
| resp = await self.central_system.set_max_charge_rate_amps( | |
| self.cp_id, num_value | |
| self.cpid, num_value | |
| try: | |
| if self.central_system.get_available( | |
| self.cpid | |
| ) and Profiles.SMART & self.central_system.get_supported_features(self.cpid): | |
| resp = await self.central_system.set_max_charge_rate_amps( | |
| self.cpid, num_value | |
| ) | |
| if resp is True: | |
| self._attr_native_value = num_value | |
| self.async_write_ha_state() | |
| except Exception as ex: | |
| _LOGGER.error("Failed to set charge rate: %s", str(ex)) | |
| raise |
| config_data = copy.deepcopy(MOCK_CONFIG_DATA) | ||
| # config_data[CONF_MONITORED_VARIABLES] = ",".join(supported_measurands) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Uncomment and update the monitored variables configuration.
The commented configuration for monitored variables should be either removed or updated to use the new structure.
-# config_data[CONF_MONITORED_VARIABLES] = ",".join(supported_measurands)
+config_data[CONF_MONITORED_VARIABLES] = ",".join(supported_measurands)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| config_data = copy.deepcopy(MOCK_CONFIG_DATA) | |
| # config_data[CONF_MONITORED_VARIABLES] = ",".join(supported_measurands) | |
| config_data = copy.deepcopy(MOCK_CONFIG_DATA) | |
| config_data[CONF_MONITORED_VARIABLES] = ",".join(supported_measurands) |
@lbbrhzn still a bit of work but appreciate your views on the draft.
Maintains current functionality ie 1 charger per cs but tidies up the code and will make it easier to implement multiple chargers per cs in the future.
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the release notes:
Release Notes
New Features
Configuration Changes
Performance Improvements
Bug Fixes