Skudak/stm32-openinverter-teslacharger
2
0
Fork 0
Code Issues Pull Requests 2 Actions Packages Projects Releases Wiki Activity

fix: CheckDelay returns false forever when timedly is negative (-S2) #5

Merged
bastian merged 1 commits from fix/checkdelay-negative-timedly into main 2026-05-10 09:34:45 -04:00
Conversation 0 Commits 1 Files Changed 2
+10 -4
bastian commented 2026-05-10 09:34:39 -04:00
Owner
Copy Link
Copy Source

Summary

Upstream OpenInverter bug surfaced after recent SKUDAK PRs invalidated saved params. Bench symptom (reported by Kyle): teslacharger detects EVSE, shows AC limit, but stays stuck in WaitStart and never proceeds to Enable / Activate / Run.

This is not a regression from any SKUDAK code change — git log -- src/charger.cpp shows no SKUDAK commit has touched the state machine. But every SKUDAK PR that bumped the param table (uaux broadcast, expanded 0x210, 0x210→0x211, version suffix) invalidates the saved-params CRC and resets timedly to its -1 default — which is when the latent upstream bug surfaces.

Root cause

src/charger.cpp:85-91:

bool CheckDelay()
{
   uint32_t now = rtc_get_counter_val();
   uint32_t start = Param::GetInt(Param::timedly) * 60;
   return start <= 0 || (now - startTime) > start;
}

With timedly = -1 (the param's documented "no delay" sentinel and its default):

Step Value
Param::GetInt(timedly) -1 (signed int)
-1 * 60 -60 (signed int math)
Assigned to uint32_t start 0xFFFFFFC4 ≈ 4.29 billion
start <= 0 false (uint32_t can never be negative)
(now - startTime) > 4294967236 false for ~136 years

CheckDelay() returns false forever; charger wedges in WaitStart.

Fix

Use signed math for the short-circuit so timedly <= 0 still means "start immediately":

int start = Param::GetInt(Param::timedly) * 60;
return start <= 0 || (now - startTime) > (uint32_t)start;

The (uint32_t)start cast is only reached when start > 0 (short-circuit OR), so it can't reinterpret a negative as huge. (now - startTime) is uint32_t - uint32_t — well-defined modular arithmetic, handles RTC wraparound correctly.

timedly start First clause Result
-1 (no-delay sentinel) -60 start <= 0 true returns true → progress
0 0 start <= 0 true returns true → progress
5 (5 min) 300 false falls through to elapsed check
10000 (max) 600000 false elapsed check, well within int range

Note on CheckTimeout()

The companion CheckTimeout() at line 75 has the same uint32_t pattern but works "correctly by accident" because timelim = -1 makes timeout huge and (now - startTime) > timeout is false, which matches the intended "no timeout" semantics. Leaving that alone — fixing it would change behaviour, not fix bugs.

Version suffix

VER suffix bumped -S1 → -S2 so the OI web UI clearly shows the post-fix firmware. Verified in built binary as 4=1.19.R-S2.

Build

text data bss dec hex filename
25192 2008 320 27520 6b80 stm32_charger

Links clean; pre-existing pedantic warning on extern "C" int main unchanged.

Upstream consideration

This bug is also present in johanneshuebner/stm32-charger. Worth a PR upstream once we've confirmed the fix on the bench.

Test plan

  • Flash and verify OI web UI shows version 4=1.19.R-S2
  • With timedly = -1 (default), plug EVSE → state should progress past WaitStart within seconds
  • Set timedly = 5 (5 min) → state should sit in WaitStart for ~5 min before progressing (verifies positive-delay still works)
  • No regression on the per-charger DigIo enable / proximity / cablelim gates

🤖 Generated with Claude Code

## Summary Upstream OpenInverter bug surfaced after recent SKUDAK PRs invalidated saved params. **Bench symptom** (reported by Kyle): teslacharger detects EVSE, shows AC limit, but stays stuck in WaitStart and never proceeds to Enable / Activate / Run. This is **not** a regression from any SKUDAK code change — `git log -- src/charger.cpp` shows no SKUDAK commit has touched the state machine. But every SKUDAK PR that bumped the param table (uaux broadcast, expanded 0x210, 0x210→0x211, version suffix) invalidates the saved-params CRC and resets `timedly` to its `-1` default — which is when the latent upstream bug surfaces. ## Root cause `src/charger.cpp:85-91`: ```c bool CheckDelay() { uint32_t now = rtc_get_counter_val(); uint32_t start = Param::GetInt(Param::timedly) * 60; return start <= 0 || (now - startTime) > start; } ``` With `timedly = -1` (the param's documented "no delay" sentinel and its default): | Step | Value | |---|---| | `Param::GetInt(timedly)` | `-1` (signed int) | | `-1 * 60` | `-60` (signed int math) | | Assigned to `uint32_t start` | `0xFFFFFFC4` ≈ 4.29 billion | | `start <= 0` | **false** (uint32_t can never be negative) | | `(now - startTime) > 4294967236` | false for ~136 years | `CheckDelay()` returns `false` forever; charger wedges in WaitStart. ## Fix Use signed math for the short-circuit so `timedly <= 0` still means "start immediately": ```c int start = Param::GetInt(Param::timedly) * 60; return start <= 0 || (now - startTime) > (uint32_t)start; ``` The `(uint32_t)start` cast is only reached when `start > 0` (short-circuit OR), so it can't reinterpret a negative as huge. `(now - startTime)` is `uint32_t - uint32_t` — well-defined modular arithmetic, handles RTC wraparound correctly. | `timedly` | `start` | First clause | Result | |---|---|---|---| | `-1` (no-delay sentinel) | `-60` | `start <= 0` true | returns true → progress | | `0` | `0` | `start <= 0` true | returns true → progress | | `5` (5 min) | `300` | false | falls through to elapsed check | | `10000` (max) | `600000` | false | elapsed check, well within `int` range | ## Note on `CheckTimeout()` The companion `CheckTimeout()` at line 75 has the same `uint32_t` pattern but works "correctly by accident" because `timelim = -1` makes `timeout` huge and `(now - startTime) > timeout` is false, which matches the intended "no timeout" semantics. Leaving that alone — fixing it would *change* behaviour, not fix bugs. ## Version suffix VER suffix bumped `-S1 → -S2` so the OI web UI clearly shows the post-fix firmware. Verified in built binary as `4=1.19.R-S2`. ## Build ``` text data bss dec hex filename 25192 2008 320 27520 6b80 stm32_charger ``` Links clean; pre-existing pedantic warning on `extern "C" int main` unchanged. ## Upstream consideration This bug is also present in `johanneshuebner/stm32-charger`. Worth a PR upstream once we've confirmed the fix on the bench. ## Test plan - [ ] Flash and verify OI web UI shows version `4=1.19.R-S2` - [ ] With `timedly = -1` (default), plug EVSE → state should progress past WaitStart within seconds - [ ] Set `timedly = 5` (5 min) → state should sit in WaitStart for ~5 min before progressing (verifies positive-delay still works) - [ ] No regression on the per-charger DigIo enable / proximity / cablelim gates 🤖 Generated with [Claude Code](https://claude.com/claude-code)
bastian added 1 commit 2026-05-10 09:34:39 -04:00
fix: CheckDelay returns false forever when timedly is negative
Build Firmware / Build stm32-teslacharger (pull_request) Successful in 1m1s
Details
5dc715558f 
Upstream openinverter bug surfaced after recent SKUDAK PRs.

Symptom (reported by Kyle on the bench): teslacharger detects EVSE,
shows the AC limit, but stays stuck in WaitStart and never proceeds
to Enable / Activate / Run. State machine looks like it's the OFF
state but it's actually one step further on.

Root cause in src/charger.cpp:85-91:

    bool CheckDelay()
    {
       uint32_t now = rtc_get_counter_val();
       uint32_t start = Param::GetInt(Param::timedly) * 60;
       return start <= 0 || (now - startTime) > start;
    }

With timedly = -1 (the param's documented "no delay" sentinel and its
default value), the math is:

    Param::GetInt(timedly) -> -1 (signed int)
    -1 * 60 -> -60 (signed)
    assigned to uint32_t -> 0xFFFFFFC4 (~4.29 billion)
    start <= 0 -> false (uint32_t can't be negative)
    (now - startTime) > 4294967236 -> false for ~136 years

So CheckDelay() returns false forever; charger wedges in WaitStart.

Why "it was working before": every SKUDAK PR that bumped the param
table (uaux broadcast, expanded 0x210, 0x210->0x211, version suffix)
invalidates the saved-params CRC and resets timedly to its -1 default.
Kyle had a non-default value (probably 0) saved before.

Fix: signed math for the short-circuit so timedly <= 0 still means
"start immediately." Cast to uint32_t only on the right operand, which
is only evaluated when start > 0 — can't reinterpret negative as huge.

The companion CheckTimeout() at line 75 has the same uint32_t pattern
but works "correctly by accident" because timelim = -1 makes timeout
huge and (now - startTime) > timeout is false, which matches the
intended "no timeout" semantics. Leaving that alone — fixing it would
change behaviour, not bugs.

VER suffix bumped -S1 -> -S2 so the OI web UI clearly shows the
deployed firmware (post-fix). Confirmed in built binary as
"4=1.19.R-S2".

Build: text=25192, links clean, pre-existing pedantic warning unchanged.

This bug is also present upstream (johanneshuebner/stm32-charger);
worth a PR upstream once we've confirmed the fix on the bench.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bastian merged commit fb721f7392 into main 2026-05-10 09:34:45 -04:00
bastian deleted branch fix/checkdelay-negative-timedly 2026-05-10 09:34:45 -04:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
Kyle bastian
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Skudak/stm32-openinverter-teslacharger#5
Delete Branch "fix/checkdelay-negative-timedly"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?