Make remote id tag configurable and reset on Home Assistant transaction start#1832
Make remote id tag configurable and reset on Home Assistant transaction start#1832mightytry wants to merge 7 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a configurable ChangesChargePoint remote id handling
Sequence Diagram(s)sequenceDiagram
participant Config as Configuration (ocpp.remote_id_tag)
participant CP as ChargePoint
participant CS as CentralSystem
participant Metrics as _metrics
CP->>Config: get CONF_REMOTE_ID_TAG
Config-->>CP: configured tag or absent
CP->>CP: validate/strip or generate 20-char fallback
CP->>CS: RemoteStartTransaction request
CS-->>CP: RemoteStartTransactionResponse (Accepted)
CP->>Metrics: set connector[id_tag] = self._remote_id_tag
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
mightytry
had a problem deploying
to
continuous-integration
GitHub Actions
Failure
![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/chargepoint.py (1)
297-303: Replace unused loop variable with underscore.The loop variable
ion line 302 is unused and should be replaced with_per Python convention.🔎 Apply this diff to follow Python conventions:
def get_remote_id_tag(self) -> str: """Get remote id tag from configuration.yaml or generate a random 20 char one.""" config = self.hass.data[DOMAIN].get(CONFIG, {}) alphabet = string.ascii_uppercase + string.digits return config.get( - CONF_REMOTE_ID_TAG, "".join(secrets.choice(alphabet) for i in range(20)) + CONF_REMOTE_ID_TAG, "".join(secrets.choice(alphabet) for _ in range(20)) )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/configuration.yaml(1 hunks)custom_components/ocpp/chargepoint.py(3 hunks)custom_components/ocpp/const.py(1 hunks)custom_components/ocpp/ocppv16.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/ocpp/ocppv16.py (2)
custom_components/ocpp/chargepoint.py (2)
value(85-87)value(90-92)tests/test_charge_point_v16.py (2)
value(4252-4258)value(4261-4263)
🔇 Additional comments (5)
config/configuration.yaml (1)
50-50: LGTM!The new configuration option is properly formatted and demonstrates the feature with a clear test value.
custom_components/ocpp/const.py (1)
31-31: LGTM!The new constant follows the existing naming convention and is properly positioned in the file.
custom_components/ocpp/chargepoint.py (2)
54-54: LGTM!The import is correctly added and follows the existing pattern.
284-284: LGTM!The refactoring to use
get_remote_id_tag()improves maintainability and enables configuration-driven behavior.custom_components/ocpp/ocppv16.py (1)
629-630: Verify connector-specific metric update in remote transaction start.The
start_transactionmethod (line 621) accepts aconnector_idparameter but updates the global metricself._metrics[0][cstat.id_tag.value]rather than connector-specific metrics. However, theon_start_transactionhandler (line 1084) uses the connector_id in its metric key:self._metrics[(connector_id, cstat.id_tag.value)].Clarify the design intent: Is the global metric update at line 630 intentional (tracking the remote_id_tag used globally across all connectors) or should it be changed to
self._metrics[(connector_id, cstat.id_tag.value)]to track per-connector remote start operations? This matters especially if the same charge point has multiple connectors and you callstart_transactionwith different connector IDs.
mightytry
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
custom_components/ocpp/chargepoint.py (1)
296-302:get_remote_id_tag()is non-idempotent when no config value is set.Each call without
CONF_REMOTE_ID_TAGconfigured generates a new random tag. Today this is safe because_remote_id_tagis assigned once in__init__, but the public method name implies it can be called freely. If it's ever invoked again (e.g., fromocppv16.pyor a future refactor), the returned value won't match_remote_id_tag, breaking authorization checks at line 652.Consider either caching the generated value or generating the fallback lazily only when needed:
Proposed fix
def get_remote_id_tag(self) -> str: """Get remote id tag from configuration.yaml or generate a random 20 char one.""" config = self.hass.data[DOMAIN].get(CONFIG, {}) - alphabet = string.ascii_uppercase + string.digits - return config.get( - CONF_REMOTE_ID_TAG, "".join(secrets.choice(alphabet) for i in range(20)) - ) + tag = config.get(CONF_REMOTE_ID_TAG) + if tag is None: + alphabet = string.ascii_uppercase + string.digits + tag = "".join(secrets.choice(alphabet) for _ in range(20)) + return tagThis also avoids eagerly generating the random string when the config key is present.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@custom_components/ocpp/chargepoint.py`:
- Around line 296-302: The get_remote_id_tag method currently eagerly generates
a random tag in the config.get default and returns blank or non-string values
from configuration; update get_remote_id_tag to (1) retrieve the raw value from
self.hass.data[DOMAIN].get(CONFIG, {}) using CONF_REMOTE_ID_TAG, (2) validate
that the retrieved value is a non-empty string (strip and check length), and (3)
only generate and return a 20-character fallback (using secrets.choice over
alphabet) lazily when the configured value is missing, empty, or not a string;
ensure the method always returns a valid str id_tag. Use the existing symbols
get_remote_id_tag, CONF_REMOTE_ID_TAG, DOMAIN, and CONFIG to locate and change
the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6816a52-4cfa-4e3f-a47d-cea2ca06aef3
📒 Files selected for processing (2)
custom_components/ocpp/chargepoint.pycustom_components/ocpp/ocppv16.py
🚧 Files skipped from review as they are similar to previous changes (1)
- custom_components/ocpp/ocppv16.py
There was a problem hiding this comment.
Pull request overview
This PR introduces a configurable remote_id_tag for OCPP remote-start transactions and addresses an issue where an RFID-driven Id Tag could persist into subsequent Home Assistant-initiated starts.
Changes:
- Add
remote_id_tagas a YAML configuration option (constant + example config). - Initialize the chargepoint’s
_remote_id_tagfrom YAML (or generate a random one if not configured). - Reset the charger-level
Id Tagmetric to_remote_id_tagwhen an OCPP 1.6 RemoteStartTransaction is accepted.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| custom_components/ocpp/ocppv16.py | Resets charger-level Id Tag to the remote tag after accepted remote start (OCPP 1.6). |
| custom_components/ocpp/const.py | Adds CONF_REMOTE_ID_TAG constant for YAML config lookup. |
| custom_components/ocpp/chargepoint.py | Loads _remote_id_tag from YAML (or generates a random default). |
| config/configuration.yaml | Adds an example remote_id_tag entry under the ocpp: YAML config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
custom_components/ocpp/chargepoint.py (1)
300-300: ⚡ Quick winReplace unused loop variable with underscore.
The loop variable
iis not used in the comprehension. By convention, use_for unused variables.♻️ Proposed fix
- fallback = "".join(secrets.choice(alphabet) for i in range(20)) + fallback = "".join(secrets.choice(alphabet) for _ in range(20))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/ocpp/chargepoint.py` at line 300, The list comprehension that builds the fallback string uses an unused loop variable `i`; change it to `_` so the expression becomes a comprehension using `_` instead of `i` (locate the assignment to `fallback` that calls `"".join(secrets.choice(alphabet) for i in range(20))` and replace `i` with `_`), keeping the rest (alphabet, secrets.choice, and length 20) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@custom_components/ocpp/chargepoint.py`:
- Line 300: The list comprehension that builds the fallback string uses an
unused loop variable `i`; change it to `_` so the expression becomes a
comprehension using `_` instead of `i` (locate the assignment to `fallback` that
calls `"".join(secrets.choice(alphabet) for i in range(20))` and replace `i`
with `_`), keeping the rest (alphabet, secrets.choice, and length 20) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1cee8a45-91a1-471d-8aa3-a712ddca65e6
📒 Files selected for processing (2)
custom_components/ocpp/chargepoint.pycustom_components/ocpp/ocppv16.py
|
Real-world use-case - this PR is required for myenergi zappi in OCPP Commercial Mode This PR is currently the only way to do PIN-less authorized charging from Home Assistant on a myenergi zappi running in OCPP Commercial Mode. In Commercial Mode the zappi rejects any RemoteStartTransaction whose idTag is not a registered Virtual OCPP Tag (generated from a myaccount PIN). The current behaviour - a random 20-char _remote_id_tag, regenerated on every restart — is therefore always rejected, so HA can never start a charge. With this PR I set remote_id_tag to a registered tag and HA-initiated charge_control start works (e.g. door-access automation → release, no PIN at the unit). Applied on v0.10.15, resolves it cleanly. The PR is small (+26/-2), MERGEABLE, CI green (CodeRabbit + Snyk), and backward-compatible (random fallback if unset). @lbbrhzn any chance of a review/merge? Happy to help if anything is needed — thanks! 🙏 |
remote_id_tagas configuration option in configuration.yamlId Tagwas set by using the wall box RFID, it was not set back to_remote_id_tagwhen using home assistant to start afterwardSummary by CodeRabbit