Remove clear profiles from set_charge_rate. #1718
Conversation
WalkthroughThe change simplifies v1.6 charging-profile handling by removing the internal _profile_ids_for helper and refactoring set_charge_rate. The new flow directly uses a provided profile or, otherwise, performs SMART checks, resolves units via GetConfiguration, builds a schedule, tries ChargePointMaxProfile on connector 0, and falls back to TxDefaultProfile. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor HA as HA
participant CP as ChargePoint v1.6
participant CS as Charge Station
HA->>CP: set_charge_rate(target, rate, profile?)
alt Custom profile provided
CP->>CS: SetChargingProfile(profile)
alt Accepted
CS-->>CP: Accepted
CP-->>HA: True
else Rejected/Exception
CS-->>CP: Rejected/Error
CP->>HA: notify_ha(failure)
CP-->>HA: False
end
else No profile provided
CP->>CP: check SMART support
alt SMART unsupported
CP-->>HA: False
else SMART supported
CP->>CS: GetConfiguration(ChargingScheduleAllowedChargingRateUnit)
CS-->>CP: unit or empty
CP->>CP: _mk_schedule(rate, unit, relative)
CP->>CS: SetChargingProfile(conn=0, purpose=ChargePointMax, id=8, stack_level=N)
alt CPMax Accepted
CS-->>CP: Accepted
CP-->>HA: True
else CPMax Rejected/Error
CS-->>CP: Rejected/Error
CP->>CS: SetChargingProfile(conn=target|1, purpose=TxDefault, id=8, stack_level=N-1)
alt TxDefault Accepted
CS-->>CP: Accepted
CP-->>HA: True
else TxDefault Rejected/Error
CS-->>CP: Rejected/Error
CP->>HA: notify_ha(failure)
CP-->>HA: False
end
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jthun
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1718 +/- ##
==========================================
- Coverage 96.03% 95.90% -0.13%
==========================================
Files 12 12
Lines 2897 2881 -16
==========================================
- Hits 2782 2763 -19
- Misses 115 118 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
custom_components/ocpp/ocppv16.py (1)
404-406: Consider notifying user when Smart Charging is not supported.When Smart Charging is not supported, the method logs an info message but doesn't notify the user. Consider adding a notification to inform the user why the charge rate change failed.
if prof.SMART not in self._attr_supported_features: _LOGGER.info("Smart charging is not supported by this charger") + await self.notify_ha("Smart charging is not supported by this charger") return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
custom_components/ocpp/ocppv16.py(1 hunks)tests/test_additional_charge_point_v16.py(0 hunks)tests/test_charge_point_v16.py(0 hunks)tests/test_set_charge_rate_v16.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/test_additional_charge_point_v16.py
- tests/test_charge_point_v16.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_set_charge_rate_v16.py (2)
custom_components/ocpp/ocppv16.py (2)
ChargePoint(83-1226)set_charge_rate(380-491)custom_components/ocpp/enums.py (2)
Profiles(65-80)ConfigurationKey(125-179)
custom_components/ocpp/ocppv16.py (3)
custom_components/ocpp/chargepoint.py (5)
notify_ha(973-984)get_configuration(451-453)value(84-86)value(89-91)set_charge_rate(401-409)custom_components/ocpp/ocppv201.py (2)
get_configuration(511-532)set_charge_rate(345-420)custom_components/ocpp/api.py (1)
handle_set_charge_rate(650-664)
🔇 Additional comments (13)
custom_components/ocpp/ocppv16.py (6)
388-402: LGTM! Clean error handling for custom profile path.The implementation correctly handles both exceptions and rejections when using a custom profile, with appropriate logging and user notification.
408-414: Good defensive programming with unit fallback.The code appropriately defaults to "Amps" when the charging rate unit is not reported by the charger, ensuring the method doesn't fail due to missing configuration.
424-431: Good error handling for stack level parsing.The code safely handles parsing errors by defaulting to stack level 1, preventing failures when the configuration value is missing or malformed.
433-440: LGTM! Clean helper function for schedule creation.The nested helper function
_mk_scheduleis well-structured and keeps the schedule creation logic DRY.
442-462: LGTM! Well-structured CPMaxProfile attempt with appropriate error handling.The code properly attempts to set a ChargePointMaxProfile first, with debug logging for both exceptions and rejections. The silent failure handling is appropriate since it falls back to TxDefaultProfile.
464-491: LGTM! Robust fallback implementation with user notifications.The TxDefaultProfile fallback is well implemented with:
- Smart connector selection (defaulting to 1 when unspecified)
- Stack level adjustment for picky chargers
- Proper user notifications for both rejections and exceptions
tests/test_set_charge_rate_v16.py (7)
1-10: LGTM! Clear test file documentation.The header comment clearly explains the testing approach of using the production ChargePoint class with monkeypatched dependencies.
29-45: Well-designed test fixture using minimal initialization.The fixture cleverly bypasses full initialization to create a lightweight ChargePoint instance with only the necessary attributes for testing
set_charge_rate.
48-86: LGTM! Comprehensive test for custom profile exception handling.The test properly verifies that exceptions during custom profile setting result in:
- Returning False
- Triggering a user notification
- Not calling
get_configuration(as it's not needed for custom profiles)
88-115: LGTM! Proper test for unsupported Smart Charging.The test correctly verifies that when Smart Charging is not supported:
- The method returns False
- No notifications are sent (though as suggested in the main file review, a notification might be helpful)
- Neither
get_configurationnorcallmethods are invoked
117-154: LGTM! Good test for CPMax exception with TxDefault fallback.The test properly simulates a CPMax failure (exception) that successfully falls back to TxDefault, verifying the return value is True and no notifications are sent.
156-188: LGTM! Clean test for CPMax rejection with TxDefault fallback.The test correctly verifies the fallback from a rejected CPMax to an accepted TxDefault, confirming the method returns True without notifications.
190-222: LGTM! Comprehensive test for complete failure scenario.The test properly verifies that when both CPMax is rejected and TxDefault raises an exception:
- The method returns False
- A user notification is triggered to inform about the failure
|
@drc38, I think this one would be good to include in a release soon. Seems like some chargers are sensitive to ClearChargingProfile. |
Remove clear profiles from set_charge_rate (revert to 0.9 style). Adjust tests.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests