fix: car never reverting back from 'charging' when using Teslamate #1109
Conversation
WalkthroughThe constant Changes
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
cguedel
changed the title
cguedel
changed the title
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/tesla_custom/teslamate.py(3 hunks)
🔇 Additional comments (2)
custom_components/tesla_custom/teslamate.py (2)
34-34: LGTM! Good practice using constants.Adding the
CHARGING_STATE_STOPPEDconstant follows good coding practices by replacing magic strings with named constants, improving maintainability and consistency with the existingCHARGING_STATE_CHARGINGconstant.
99-101: LGTM! Proper use of the new constant.The update to use
CHARGING_STATE_STOPPEDinstead of the hardcoded string"Stopped"is consistent with the constant addition and improves code maintainability.
| else: | ||
| self.update_charging_state(car, CHARGING_STATE_STOPPED) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Core fix looks good, but verify interaction with plugged_in handler.
The addition of the else clause correctly addresses the issue where charging state wasn't being updated when the car stopped charging. However, there's a potential logical concern:
When a car is unplugged (not charging), this handler sets charging state to "Stopped", but the plugged_in handler in cast_plugged_in would set it to "Disconnected". Depending on the order of MQTT messages, the final charging state could be inconsistent.
Consider whether the logic should check if the car is plugged in before setting the state to "Stopped":
🏁 Script executed:
#!/bin/bash
# Description: Search for other references to charging states to understand the expected behavior
# Expected: Find usage patterns of Disconnected vs Stopped states
echo "=== Searching for charging state references ==="
rg -A 3 -B 3 "Disconnected|Stopped|charging_state" --type py
echo "=== Searching for plugged_in related logic ==="
rg -A 5 -B 5 "plugged_in" --type pyLength of output: 12264
🏁 Script executed:
#!/bin/bash
echo "=== Showing cast_plugged_in definition ==="
rg -n "def cast_plugged_in" -n custom_components/tesla_custom/teslamate.py -A10 -B5
echo
echo "=== Showing is_car_state_charging ==="
rg -n "def is_car_state_charging" -n custom_components/tesla_custom/teslamate.py -A5 -B5
echo
echo "=== Showing MAP_DRIVE_STATE mapping ==="
rg -n "MAP_DRIVE_STATE" -n custom_components/tesla_custom/teslamate.py -A5 -B5Length of output: 2073
Gate “Stopped” updates on plugged-in status to avoid race
In async_handle_new_data (custom_components/tesla_custom/teslamate.py), the state branch currently does:
if is_car_state_charging(state):
self.update_charging_state(car, CHARGING_STATE_CHARGING)
else:
self.update_charging_state(car, CHARGING_STATE_STOPPED)Because the plugged_in handler can later set “Disconnected,” an unplug event may be overwritten by the next state message.
• File: custom_components/tesla_custom/teslamate.py
– Around the elif mqtt_attr == "state" block, change to:
else:
- self.update_charging_state(car, CHARGING_STATE_STOPPED)
+ # Only mark “Stopped” if still plugged in; unplugged should remain “Disconnected”
+ if car.conn_charge_cable:
+ self.update_charging_state(car, CHARGING_STATE_STOPPED)Please update this logic so that a “Disconnected” state from the plugged_in handler cannot be overridden by subsequent state messages.
🤖 Prompt for AI Agents
In custom_components/tesla_custom/teslamate.py around lines 372 to 373, the else
clause sets the charging state to "Stopped" without checking if the car is
plugged in, which can cause the "Disconnected" state set by the plugged_in
handler to be overwritten. Modify the else branch to first check if the car is
plugged in before setting the charging state to "Stopped," ensuring that if the
car is unplugged, the "Disconnected" state remains and is not overridden by
subsequent state messages.
There was a problem hiding this comment.
@cguedel Thanks again! Can you consider whether the ai is correct with it's suggestion please? I can merge it once you review and decide one way or the other.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@alandtse Looks like the AI might be correct, the car could change from "charging" to "online" while being disconnected I think. I didn't realize the charging state was charging/stopped/disconnected. I'll need to think about a solution for this.
cguedel
changed the title
|
@alandtse After digging through this some more, I'm not actually sure why the code observes Happy to change this further if you think that's the wrong way, but from my (very limited) testing, it looks like it works when plugging in, starting to charge, stopping the charge, disconnecting the charger. |
|
No worries. The AI can be wrong. If someone reports it as a reality, then we can fix it then. |
2 participants
Observe
charging_statetopic directly to update charging state of car when using Teslamate.Fixes #1108
Summary by CodeRabbit