refactor(vehicle): simplify the state machine: plain atom states, DB records moved into the state data

[brianmay]

Summary

This PR completes the state machine simplification by replacing compound state tuples like {:driving, status, %Log.Drive{}}, {:charging, %Log.ChargingProcess{}}, and {:updating, %Log.Update{}} with simple atoms (:driving, :charging, :updating). DB records and sub-states are stored in Data struct fields.

Changes (4 commits)

Commit 1: Add preparatory fields to Data struct

Adds current_drive, current_charging_process, current_update, and driving_status fields.

Commit 2: Populate Data struct fields in parallel with compound state tuples

Every transition into a compound state now also updates the corresponding struct fields.

Commit 3: Migrate compound state reads to struct field guards

Replaces pattern-match reads with guards on the struct fields.

Commit 4: Replace compound state tuples with simple atoms throughout state machine

This is the main simplification commit. After this:

• The GenState state is always a simple atom
• {:driving, status, drive} → :driving (status in data.driving_status, drive in data.current_drive)
• {:charging, cproc} → :charging (cproc in data.current_charging_process)
• {:updating, update} → :updating (update in data.current_update)

Key changes in this commit:

• All 12 next_state positions now use simple atoms
• Driving sub-state machine (available → unavailable → offline) is tracked in data.driving_status instead of the state tuple's second element
• All driving handlers match :driving with guards on driving_status
• suspend_logging, fetch/can_fall_asleep, and fetch/reachable all use simple atom matching
• Summary module updated to handle the new state format

Refactoring sequence (complete!)

1. ✅ Replace fake_online_state integers with pre_online_check atoms
2. ✅ Add struct fields + populate them + migrate reads
3. ✅ This PR: Replace compound states with simple atoms

Before vs After

Before (hard to read):

def handle_event(:internal, {:update, {:offline, _}}, {:driving, {:unavailable, n}, drv}, ...)
def handle_event(:internal, {:update, {:offline, _}}, {:driving, {:offline, last}, drive}, ...)
def handle_event(:info, {:stream, %Stream.Data{} = stream_data}, {:driving, status, drv}, ...)

After (clean):

def handle_event(:internal, {:update, {:offline, _}}, :driving, %Data{driving_status: {:unavailable, n}}) do ...
def handle_event(:internal, {:update, {:offline, _}}, :driving, %Data{driving_status: {:offline, last}}) do ...
def handle_event(:info, {:stream, %Stream.Data{}}, :driving, %Data{driving_status: :available, current_drive: drv}) do ...

Testing

All commits are no-behaviour-change mechanical refactors. Existing tests pass.

[brianmay]

This is only the first phase so far, we haven't seen the simplification to the compound state tuples yet. That will come later.

[brianmay]

As before, this was done with AI. It was going to make one big pull request, then we agreed that breaking it up might be better.

[brianmay]

I decided to do the full refactor in one pull request. While this the PR more complicated, don't think you can really review this probably unless you can see the entire picture.

The steps are still there, but as separate commits. Which probably should stay as separate commits.

Key questions before merging:

1. Does this make things simpler and easier to understand?

I think by making the state an enum only and putting state data into the data variable we have made it easier to understand. If anyone disagrees, feel free to speak out :-)

2. Is it correct?

It looks OK to me... Although certainly is scope for errors to creep in. Guess next step will be to test this.

[brianmay]

I spotted a place where the logic had been changed and was more complicated then required. Fixed now.

[brianmay]

All tests are expected to pass.

Still need to manually review the "Fix compound state tuple handling after dce479c refactoring" commit - haven't done that yet.

[brianmay]

Might make sense to merge the last three commits into the "Migrate compound state reads to struct field guards" commit, seeing as all they do is fix problems caused by this commit.

Will decide when more awake :-)

[brianmay]

I rebased this against the main branch, now that mcu2-upgraded-cars has been merged into main.

[netlify]

✅ Deploy Preview for teslamate ready!

Name	Link
🔨 Latest commit	c3e626b
🔍 Latest deploy log	https://app.netlify.com/projects/teslamate/deploys/6a2d4e430c7d490008cb2fcb
😎 Deploy Preview	https://deploy-preview-5259--teslamate.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[brianmay]

I got Claude Fable 5 to review this (while I still had access to Fable5!). I haven't had a chance to seriously evaluate its feedback yet, so will just post it here in its entirety so I don't accidentally lose it.

PR 5259 Review: extract DB records from compound state tuples

Overview

Final phase of the state-machine refactor: {:driving, status, drive},
{:charging, cproc}, {:updating, update} become plain atoms, with the records
moved to new Data fields (current_drive, current_charging_process,
current_update, driving_status). 2 files, +124/−75. CI is green (lint job
still pending).

Correctness

I traced every converted clause against the old behavior and found no
functional regressions:

• The tricky {:driving, {:offline, last}, nil} drive-timeout case maps
  correctly to driving_status: {:offline, last}, current_drive: nil and still
  routes to :start on the next offline event (vehicle.ex:1218 vs 1236).
• Summary.format_state(:driving, {:offline, _}) → :offline preserves the old
  special case, and the {state, _} clause still covers the remaining tuple
  states ({:offline, _}, {:asleep, _}, {:suspended, _}).
• No leftover compound-state patterns anywhere in lib/ or test/, and both
  Summary.into call sites pass the new required driving_status key.

Main issue: stale record fields on exit (latent, worth fixing now)

The fields are cleared on some exits but not others, so the invariant "field
mirrors state" — the whole point of the refactor — doesn't actually hold:

• current_charging_process is never cleared when charging completes (:charging
  → :start, vehicle.ex:1170). Only the asleep-while-charging path clears it
  (line 1132).
• current_update is never cleared on either :updating → :start path (cancel at
  ~1412, finish at ~1434).
• current_drive survives most :driving → :start exits: end-of-drive (1364),
  gained-range (1283), offline-timeout (1289), and asleep (1308) all clear
  driving_status but leave the closed drive in current_drive. Only the
  service-mode path clears both (481).

This is safe today because every entry into a state overwrites its field and
all readers guard on the state atom — but it's exactly the kind of footgun
this refactor exists to remove. The service-mode handler already reads
data.current_drive != nil as a guard (line 468), so a future reader doing the
same in another state would see a stale record. Suggestion: clear all three
fields (and driving_status) at every transition to :start — a small helper
like reset_activity(data) would make this impossible to forget.

Smaller points

• driving_status sentinel is inconsistent. The struct default is :available,
  the comment documents :available | {:unavailable, n} | {:offline,
  last_vehicle}, but every exit path sets nil. Pick one "not driving" value —
  nil with a default of nil reads best — and update the comment to include it.
• Redundant self-assignment at line 1154: %{data | current_charging_process:
  cproc} where cproc was just extracted from data — it's a no-op; plain data is
  clearer. Same pattern would apply if you keep the keep-charging clause as is.
• Summary now silently couples to the caller: into/2's attrs map pattern-match
  makes driving_status required, which is fine for the two internal callers,
  but a brief @doc note or typespec update on Summary.into would help.

Test coverage

No test changes, and no tests reference the old tuples or new fields — they
exercise the public API, so the passing suite is a genuine
behavior-preservation signal. However, the field lifecycle (the staleness
issue above) is exactly the part with zero coverage. A test asserting the
records are cleared after a drive/charge/update ends would lock in the
invariant.

Verdict

Solid mechanical refactor with a clear before/after win in readability, and
the commit sequencing (populate → migrate reads → swap states) made it easy to
verify. I'd fix the stale-field cleanup and the nil/:available inconsistency
before merging, since they undermine the invariant the new design depends on;
the rest is optional polish.

✻ Cooked for 2m 29s

※ recap: I reviewed TeslaMate PR 5259, the state-machine refactor replacing compound state tuples with atoms. It's correct overall, but stale record fields aren't cleared on most state exits. Next step: decide whether to fix that cleanup before merging. (disable recaps in /config)

[netlify]

✅ Deploy Preview for teslamate ready!

Name	Link
🔨 Latest commit	3c2711d
🔍 Latest deploy log	https://app.netlify.com/projects/teslamate/deploys/6a7f1de394006e0008a4f326
😎 Deploy Preview	https://deploy-preview-5259--teslamate.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[JakobLichterfeld]

Update: I rebased this onto current main and pushed commits addressing all points from the review above.

Rebase: The conflicts were the expected ones — main moved in exactly this area. The semantics of #5535 (stream reconnect on drive resume), #5515 (geofence updates while driving) and #5417 (streaming-aware interval) are preserved in the converted clauses. Main's new Summary unit tests (#5487) now pass the required driving_status key; that adaptation is folded into the "Migrate compound state reads" commit to keep the branch bisectable.

Fixes on top:

• 474786f fix(vehicle): keep periodic position storing while charging — the store_position guard still expected the {:charging, _} tuple and never matched the new atom, so no positions were stored during a charging session. Comes with a regression test (verified red against the broken guard) and a test-configurable :store_position_interval start option.
• 68fe7fe fix(vehicle): restore immediate fetch on stream data while unavailable — stream frames while :driving in {:unavailable, n} / {:offline, last} fell through to the generic catch-all (info-log spam, no immediate fetch). Behavior is back to pre-refactor: a frame while unavailable triggers an immediate fetch.
• e941f5f refactor(vehicle): reset activity fields on every transition to :start — the agreed reset_activity/1 helper now runs on all 17 transitions to :start, nil is the single "not driving" sentinel for driving_status, the no-op self-assignment is gone, and the drive/charge/update flow tests assert the fields are cleared once the vehicle is back :online.

Full suite is green (461 tests). @brianmay from my side this is now ready — please give it a look, and shout if you disagree with any of the calls above (especially restoring the immediate-fetch behavior rather than dropping it).

---

🤖 Fixes drafted with Claude Code (Fable 5 extra) — sponsored by Claude for Open Source

[brianmay]

Review by opencode/Big Pickle:

Review Summary: PR 5259 (simplify vehicle state machine to plain atoms)

Verdict: no bugs found. Mechanical, well-executed refactor; verified by applying the diff, compiling with --warnings-as-errors, and running the 124-test vehicle suite (all pass).

Verification highlights:

• All 17 {:next_state, :start, ...} transitions call reset_activity/1 (needed since DB sub-state left the dropped state tuple) — none missed.
• No stale {:driving, _, _} / {:charging, _} / {:updating, _} patterns remain.
• Split driving-stream handler (vehicle.ex:707-759) is behavior-equivalent to the old case; driving_status: :available always implies current_drive != nil.
• Sub-state clause ordering (e.g. {:unavailable, n} with n < 15 before catch-all) preserves old match order.
• Summary.format_state/2 covers all remaining state shapes; dropping the 3-tuple clause is correct.

Minor issues:

1. Test efficacy (low): the new keeps storing positions periodically while charging test asserts only two insert_position events, but two independent store-position timers (online-entry + charging-entry) can each fire once to satisfy it — a broken reschedule wouldn't be caught. Assert a third event to actually exercise "periodically."
2. Leftover tuple state in summary_test.exs: state: {:online, nil} no longer occurs in production (states are now atoms); harmless but tests an unreachable shape.
3. Pre-existing observation: the double store-position timer (online + charging both armed) causes doubled position inserts while charging; predates this PR but the new test makes it visible.

No further action needed — nothing blocking in the PR.

[brianmay]

Looks good to me.

Looks like I can't approve my own pull request :-)

I think this needs to be carefully tested, simply because of the invasive changes into the state machine logic.

[d1stru3t0r]

So move it to 4.2 to have a surprise 4.1 release these hours? :P

[JakobLichterfeld]

Thanks for running another review over it! The verification section matches my own checks. On the minor issues:

1 & 3 rest on a wrong premise: there is no second store-position timer. Generic timeouts in gen_statem are keyed by name — setting {:timeout, :store_position} again replaces the running timer (see the gen_statem docs: "Setting a timer with the same Name while it is running will restart it with the new time-out value.", see https://www.erlang.org/doc/apps/stdlib/gen_statem.html#t:generic_timeout/0). So entering :charging replaces the timer armed at :online entry, and there are no doubled inserts — before or after this PR. The new test also drains the mailbox after :charging is established, so the two asserted inserts can only come from that single timer, and the second one requires the handler to have re-armed — a broken reschedule fails the test.

2 is fair: state: {:online, nil} in summary_test.exs was never a real state shape (it came in with #5487, independent of this PR). I'll change it to :online in passing.

Nothing blocking from my side either.

---

🤖 Reply drafted with Claude Code (Fable 5 extra) — sponsored by Claude for Open Source

[JakobLichterfeld]

I think this needs to be carefully tested, simply because of the invasive changes into the state machine logic.

Agreed on careful testing — the state machine is the heart of the logger. When running this branch for a few days, specifically watching: positions being stored every 5 min during charging (the regression fixed here), recovery behavior on mid-drive connectivity loss, clean field resets after drive/charge/update (:sys.get_state), and the logs for any FunctionClauseError or stream-data spam. If anything surfaces report here.

---

🤖 Drafted with Claude Code (Fable 5 extra) — sponsored by Claude for Open Source

[JakobLichterfeld]

So move it to 4.2 to have a surprise 4.1 release these hours? :P

Ok, as requested :-) https://github.com/teslamate-org/teslamate/releases/tag/v4.1.0 and https://github.com/teslamate-org/teslamate/releases/tag/v4.1.1

[d1stru3t0r]

So move it to 4.2 to have a surprise 4.1 release these hours? :P

Ok, as requested :-) https://github.com/teslamate-org/teslamate/releases/tag/v4.1.0 and https://github.com/teslamate-org/teslamate/releases/tag/v4.1.1

haha ty, I was mid trip when I tried to update my docker and I bricked everything xd

[JakobLichterfeld]

haha ty, I was mid trip when I tried to update my docker and I bricked everything xd

Quoting myself:

yeah, I can hear it myself as I type this.

Same for you 😀

[d1stru3t0r]

Ok, i think it got extremele fricked up. I've read the stuff about backup but at this point I feel like i can't do anything with it. Do you know the simple way to backup the database, as I'd want to delete my entire docker, reinstall and then reuse my backup.

I can either use teslamate without database now OR sync database but then teslamate doesn't work. No idea what got so ruined and I'm tired of so much debugging nonsense on termux

[brianmay]

@d1stru3t0r This probably deserves a new discussion item.

[d1stru3t0r]

@d1stru3t0r This probably deserves a new discussion item.

Small noobie question, my 3000 work now but reports a very old status, do i have to regenerate tesla tokens? Or what tf is happening xd

[JakobLichterfeld]

Please open a discussion as mentioned by Brian or an Issue, we do not know anything about what you did, what your logs show etc. This PR is not the right place to discuss your issues. Thank you.