Cardata: status detection for "New Class" vehicles#30938
Conversation
📝 WalkthroughWalkthroughThe BMW cardata package gains support for a new vehicle catalogue field, 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@vehicle/bmw/cardata/api.go`:
- Line 20: The string "vehicle.body.chargingPort.combinedStatus" in the
requiredKeys slice literal is missing a trailing comma after it. Add a comma
immediately after this string entry to fix the compilation error in the slice
literal.
In `@vehicle/bmw/cardata/provider.go`:
- Around line 210-220: The code contains a typo in the function call on line 210
where sices.Contains is used, but sices is undefined. Replace sices with slices
to use the Go standard library slices package which provides the Contains
function. This will resolve the compile error in the Status() method.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 03b6523a-d450-4109-8a6d-f55a451feafc
📒 Files selected for processing (2)
vehicle/bmw/cardata/api.govehicle/bmw/cardata/provider.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@vehicle/bmw/cardata/api.go`:
- Line 20: Remove the `"vehicle.body.chargingPort.combinedStatus"` entry from
the `requiredKeys` collection in the BMW cardata API configuration. Since the
template documentation treats `combinedStatus` as optional and varies by vehicle
model, and the `Status()` method already contains fallback logic to handle its
absence, this key should not be enforced as mandatory for container creation.
This aligns the implementation with the documented design that allows optional
data points per vehicle model.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4acc5d3f-0a0f-4ee8-836b-50e08c20de13
📒 Files selected for processing (2)
vehicle/bmw/cardata/api.govehicle/bmw/cardata/provider.go
🚧 Files skipped from review as they are similar to previous changes (1)
- vehicle/bmw/cardata/provider.go
mfuchs1984
changed the title
|
Fehlt hier noch was? |
|
Ja, es hier eine Herausforderung: es gibt keinen connected status, der über mqtt kommt. Die Fahrzeugdetektion könnte dennoch über Dann bleibt aber immer noch die korrekte Detektion nach evcc Neustart mit angestecktem, aber nicht ladendem Auto übrig, zudem ist unklar, ob die Statusdetektion beim Anstecken 100% funktioniert. Aus meiner Sicht:
Kanns leider wegen Urlaub gerade nicht machen... |
|
… oder mal BMW support anpingen |
|
Hast du da einen Kontakt? Sonst müsste das wohl einer oder am besten mehrere der Neue Klasse iX3 Besitzer @HMSC2019 @werner2000x-svg @LukasM42 @olivarel @Oliver26316 @Bernd2211 @granini22 machen. |
|
Contact info@evcc.io |
|
Ich habe hier einen IX3 NA5 stehen. Wenn Ihr mir sagt, was Ihr braucht oder wo ich schauen soll, dann gebt bescheid. Aktuell habe ich Version installiert. |
|
BMW cardata support hat prompt geantwortet. Ich gehe aber davon aus, dass es eine Weile dauern wird, bis sich das technisch etwas tut. Werde den PR also fertig machen. Notfalls wäre dann halt welcomecharge für die Detektion des Fahrzeugs notwendig. |
c733d64 to
0d76948
Compare
|
I changed the order of evaluation of charging status and port status.
Tested this extensively with my i5 to make sure not to introduce regressions for CLAR platform vehicles. |
|
Added a general hint concerning welcomecharge to the template for cases where vehicle detection is unreliable. This already helped some i3 owners in the past, so it should be useful independent from the not yet known necessity to activate it for New Class vehicles. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
Status(), the error from readingvehicle.drivetrain.electricEngine.charging.hvStatusis ignored (cs, _); if this is intentional, consider adding a brief comment to clarify that a missing hvStatus is expected and falls back to port-based detection. - The repeated literal slices passed to
slices.Containsfor charge status evaluation could be refactored into named package-level variables or maps to avoid duplication and make it easier to maintain or extend the set of recognized states.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Status()`, the error from reading `vehicle.drivetrain.electricEngine.charging.hvStatus` is ignored (`cs, _`); if this is intentional, consider adding a brief comment to clarify that a missing hvStatus is expected and falls back to port-based detection.
- The repeated literal slices passed to `slices.Contains` for charge status evaluation could be refactored into named package-level variables or maps to avoid duplication and make it easier to maintain or extend the set of recognized states.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
3 participants
See #29446
vehicle.body.chargingPort.statusis alwaysnullon new class vehicles, thus addedvehicle.body.chargingPort.combinedStatuswhich gives values. Since this datapoint is telematics only (thus, not added to the template), I added more values forvehicle.drivetrain.electricEngine.charging.statusandvehicle.drivetrain.electricEngine.charging.hvStatusthat should allow to detectStatusB. EspeciallyINITIALIZATIONhas been seen in all logs after connecting, so it should allow to reliably detect the status.Since for "New Class" vehicles, no mqtt charging port status is available, this PR changes the order of evaluation of charging status and port status.
Previously, the port status was checked first, when not available, the status function returned api.StatusNone and api.ErrNotAvailable. Since port status is currently not available through mqtt on "New Class" vehicles, this made vehicle detection impossible
Now, it evaluates the charging status first. This is available through mqtt for all vehicles. Since "Neue Klasse" vehicles go through INITIALIZATION after connecting, this should allow to detect api.StatusB. In case this does not work reliably, "New Class" vehicle owners might enable welcomecharge, so "CHARGINGACTIVE" -> api.StatusC can be used for vehicle detection.