SAMD/NXT fixes: CGMI flag never set, relay-open hang, GMI ADC pin, RAPI buffer overflows#54
Conversation
The transfer of g_hasCGMI into the runtime flag word was wrapped in #ifdef OEV6, but OEV6 is only defined for Arduino IDE builds (the !PLATFORMIO block in open_evse.h). On the SAMD (OpenEVSE NXT) PlatformIO build, initTarget() hardwires g_hasCGMI = true yet ECF_CGMI was never set, so hasCGMI() returned false on CGMI hardware. Confirmed live on an NXT: $GE reports flags 0121 with bit 1000 clear. Running with hasCGMI()==false on the NXT means: - chargingOn() skips zcWaitRelayClose(), so the relay close is never zero-cross timed despite RELAY_ZC_SWITCH (the open is timed, the close is not) - random-phase closes with full inrush and contact bounce - the continuous ground check and RELAY_CLOSURE_FAULT check never run - ground and stuck-relay use the legacy per-half-wave AC pin semantics, which are inverted for a continuous GMI line: a correctly grounded, correctly sensing NXT false-trips STUCK_RELAY (including at POST) Also clear a stale ECF_CGMI bit loaded from EEPROM flags when the hardware is not CGMI, since the flag reflects detected hardware rather than a user setting. Verified: samd and m328p_core envs build clean.
waitCurrentZero() spun until the measured current dropped below CURRENT_ZERO_THRESHOLD_MA (100 mA), but with the SAMD OpenEVSE NXT default calibration that exit is unreachable. The computed current is ma = reading * scale - offset, and the SAMD defaults are scale 37, offset -135, so ma = 37 * reading + 135. Even at a dead-zero ammeter reading the floor is 135 mA, which is always > the 100 mA threshold. Because ZC switching is enabled by default and the relay is closed when this runs, every non-emergency chargingOff() at the normal end of a charge session entered an infinite loop with the relay still closed. The loop calls WDT_RESET() each pass, so the watchdog never fired to recover it -- the controller simply hung with power still flowing. Bound the wait with an overall deadline (CURRENT_ZERO_TIMEOUT_MS) so it degrades to opening the relay anyway instead of hanging. The threshold, the ma formula, and the calibration defaults are unchanged; the early return below threshold and WDT_RESET() are preserved, and the deadline check uses wrap-safe unsigned subtraction. I sized the timeout at 1000 ms. readAmmeter() samples for up to CURRENT_SAMPLE_INTERVAL (35 ms) per call, so 1 s allows roughly 28 sampling windows, and at 50 Hz it spans about 50 AC cycles -- ample time to catch a genuine current zero on hardware where the exit is reachable, while capping the worst case at one second when it is not.
… pin The SAMD zero-cross detector sampled the GMI line through an AdcPin built on GMI_ADC_PIN (Arduino pin 3 = PA09) and calling analogRead(). That never touched PA09. The SAMD Arduino core's analogRead() does 'if (pin < A0) pin += A0;', so analogRead(3) samples pin 3 + A0(14) = 17 = A3 = PA04 -- an unconnected, floating pin. Independently, the arduino_zero variant lists PA09 with No_ADC_Channel, so analogRead() has no channel for it anyway. PA09's real ADC input is AIN[17], which the variant table cannot express. The result was zero-cross timing derived from floating-pin noise: fake crossings that still passed the 33-100 Hz sanity check produced garbage $GZ line-frequency output and mistimed relay switching, and when nothing crossed each relay operation burned the full 105 ms detection busy-wait. Read PA09 correctly with a direct SAMD21 ADC access (INPUTCTRL.MUXPOS = 0x11 = AIN[17]) in new gmiAdcBegin/gmiAdcRead/gmiAdcEnd helpers in the SAMD target layer, following the core analogRead sequence (SYNCBUSY waits around INPUTCTRL/ENABLE/SWTRIG, discard the first conversion after the mux change, enable/read/disable so it coexists with analogRead on the other pins). PA09 doubles as the digital ACLINE2 ground-monitor input (pinAC2, INP_PU). measureAcFreq now brackets its sampling burst with begin/end: begin switches PA09 to the analog mux with the pull-up disabled (an enabled pull-up would bias the sample), and end restores it to a digital input with pull-up via pinMode(INPUT_PULLUP), faithfully reproducing DigitalPin INP_PU. The dead adcGmi member is removed so nobody calls its broken read(), and a comment documents the AIN[17]/No_ADC_Channel limitation and the analogRead remap trap so this is not simplified back to analogRead().
ECF_OVERCURRENT_DISABLED was defined as 0x0400, the exact bit already used by ECF_TEMP_CHK_DISABLED. Both features are compiled into every PlatformIO build (TEMPERATURE_MONITORING and -D OVERCURRENT_THRESHOLD=5), so the two flags aliased the same bit in m_wFlags. Because these flags are persisted to EEPROM, sending RAPI $FF O 0 to disable the overcurrent check also silently and permanently disabled temperature monitoring, and $FF T 0 disabled the overcurrent hard-fault check. The effect survived reboots. I moved ECF_OVERCURRENT_DISABLED to 0x4000, the only free bit in the ECF_ table (ECVF_ is a separate namespace, so ECVF_BOOT_LOCK 0x4000 does not collide). All references to the flag are symbolic, so nothing depends on the old numeric value. RAPI transmits flag words as hex, so a unit that previously had overcurrent-disabled persisted in EEPROM will read that bit under the old 0x0400 meaning and revert to overcurrent-enabled after this change. That is the safe direction.
…28P typo The $GI (get MCU id) handler had two problems. First, its AVR-specific branch was guarded by #ifdef TARGET_M238P, a typo: the real macro is TARGET_M328P. That branch (leading space, 6 raw id bytes, remaining bytes as hex) was therefore dead on every build, and m328p fell through to the generic hex #else. I corrected the guard. m328p has MCU_ID_LEN 10, so its branch emits 1 + 6 + 8 = 15 chars, which fits comfortably. Second, the generic #else branch writes 2*MCU_ID_LEN hex chars plus a NUL into buffer[ESRAPI_BUFLEN]. On SAMD MCU_ID_LEN is 16, so the sixteenth sprintf placed its NUL at buffer[32], one past the 32-byte buffer, and clobbered the adjacent bufCnt member. response() then assembled the reply into g_sTmp[TMP_BUF_SIZE] as '$' + "OK" + ' ' + buffer + " :XX" sequence id + "^XX" checksum + CR + NUL. For SAMD $GI that is 1 + 2 + 1 + 32 + 4 + 4 + 1 = 45 bytes, well past the 34-byte g_sTmp, so the write ran off the end of that buffer too. I sized both buffers per target so AVR pays nothing. ESRAPI_BUFLEN is now 40 on SAMD (worst case 2*16+1 = 33) and stays 32 elsewhere. TMP_BUF_SIZE is 48 on SAMD (worst case 45) and stays at the LCD-derived 34 on AVR, whose largest reply ($GS with a sequence id) is exactly 34 and whose $GI needs only 33. Two #error guards, keyed off MCU_ID_LEN, now fail the build if either buffer is ever sized below its worst case.
The root platformio.ini passed -D NO_GROUND_RECORD_DELAY=2000, but the firmware reads NO_GND_RECORD_DELAY (defaulted to 0 in open_evse.h and used in J1772EvseController.cpp to gate recording of a NO-GROUND trip). Because the two spellings never matched, the define landed on nothing and the macro stayed at its 0 default, so the intended 2 second suppression of spurious NO-GROUND trip recording has never been active in any PlatformIO build. I renamed the flag to NO_GND_RECORD_DELAY so it reaches the code.
|
Thanks for the PR. Fixes 4,5,6 can be merged as is. The flag will never be set a priori Fix 2 looks good. On a higher level, unrelated to this PR, but concerning waitCurrentZero(): a. Perhaps the CURRENT_ZERO_THRESHOLD_MA should be bumped up or the default scale/offset should be adjusted so the loop doesn't run to the timeout as easily? b. chargingOff() is called in J1772EvseController::Init() prior to the m_AmmeterCurrentOffset/m_CurrentScaleFactor getting loaded, the the init code either has to be move prior to that call, or it should be changed to chargingOff(1) Fix 3 looks solid, but I don't know the low level commands well enough to comment on whether or not it works as designed. Unrelated to the PR, but concerning the concept of waiting for a zero crossing to open the relay, has anyone verified with a spec sheet or scope that this is even feasible? I am wondering if it's even physically possible for the relay to open the relay fast enough for this code to have the desired effect |
|
Good catch on m_GfiRetryCnt getting reset on every GFI fault; yes, that line needs to be deleted. AUTOSVCLEVEL/TIME_LIMIT/CHARGE_LIMIT are enabled only for legacy non-WiFi builds. @chris1howell - need to tell the WiFi guys to get rid of Auto as a service level option thanks for the heads up on the samd_dev_ice build. I'm the only one who uses that. It must have gotten messed up when someone else changed the platformio.ini. I'll fix it next time I do some debugging with the ICE |
Review feedback: the flag is never set a priori, so the else branch clearing it is dead. Also switch the boot-time chargingOff() to an emergency open - Init() calls it before the ammeter scale/offset are loaded from EEPROM, so the graceful path could wait on garbage readings, and there is no current to break at boot anyway.
…n floor With the SAMD default ammeter calibration (scale 37, offset -135) an idle ammeter computes 135 mA, so the 100 mA threshold could never be reached and waitCurrentZero() always ran to its timeout. 200 mA clears the floor with margin and is still far below any current level the relay contacts care about.
|
Thanks for the review — all three items are in:
Some hardware feedback in the meantime: my NXT is now running this branch. The stuck-at-UNKNOWN pilot I reported earlier turned out to be a missing ground screw on my unit — with that landed and this firmware flashed, it POSTs clean, rests at state A, and On your zero-crossing question — I think healthy skepticism is warranted, for two reasons. First, the code times coil de-energization And noted on the Auto service level — I can take care of removing it from the WiFi firmware's UI side. |
AUTOSVCLEVEL (auto service-level detection) is compiled only into legacy non-WiFi firmware builds, so Auto is a non-functional option on the WiFi firmware this GUI serves. Per the firmware maintainer, the WiFi UI should drop it, leaving L1 and L2: OpenEVSE/open_evse#54 (comment) A device still reporting service: 0 now coerces to L2 (the common default) via `|| 2` so the select stays valid instead of binding to the removed option and rendering blank. Regenerated settings-evse shot. Claude-Session: https://claude.ai/code/session_01XMvqVxVAkjyjvFMmHwtFdQ
|
Thanks for the clarification on the zero crossing algorithm. Yes, besides unit to unit variance, there's also temperature variation. As you stated, the only cost added by this code is a time delay. However, a commercial OE-based vendor had to do a lot of hardware gymnastics to get his big contactors to open fast enough to pass UL last year, so it might be an issue? Any idea how long the code takes to execute? Anyway, since it's a feature that can be disabled, it doesn't hurt to just keep it. Going forwards, let's keep the master branch for fully vetted release code only, and submit all PRs against a development branch. I just archived the old obsolete development branch and created a new dev branch. Let's do all changes in this branch |
|
The zero crossing only adds a cycle for non priority stops, GFCI still reacts immediately. I did some basic tuning on a scope but Zero Cross will very likely need additional tuning for temperature. |
3 participants
While bringing up an OpenEVSE NXT (SAMD,
D9.0.0.SAMD) behind an ESP32 WiFi module I hit some odd relay behavior and ended up auditing the controller firmware. This branch fixes the six concrete problems I could verify, one commit each. Draft for discussion — none of it has been run on NXT hardware yet beyond compiling (I can't flash the SAMD from this bench), so I'd like eyes from someone who can, especially on the ADC change.Fixes
1.
ECF_CGMIwas never set on SAMD builds — theg_hasCGMI→ flag transfer inInit()was inside#ifdef OEV6, which only Arduino-IDE builds define. Confirmed live on my NXT:$GEreports flags0121with bit1000clear. Consequences of running a CGMI board withhasCGMI()==false: the relay close is never zero-cross timed (the open is — asymmetric), the continuous ground check and relay-closure-fault check never run, and ground/stuck-relay fall back to the legacy per-half-wave semantics, which are inverted for a continuous GMI line. Fix un-gates the transfer and also clears a stale EEPROM-carried bit on non-CGMI hardware.2.
waitCurrentZero()could hang forever with the relay closed. Exit needs computed current < 100 mA, but the SAMD default calibration (scale 37, offset −135) floors the computed value at 135 mA even for a dead-zero reading — the exit is unreachable, andWDT_RESET()inside the loop hides it from the watchdog. Every normal (non-emergency) end of charge would hang. Bounded the wait at 1 s (~28 ammeter sampling windows / ~50 AC cycles).3. The SAMD zero-cross detector read the wrong pin.
GMI_ADC_PINis Arduino pin 3 (PA09), butanalogRead()remapspin < A0by+= A0, so it sampled pin 17 = A3 = PA04, which is unconnected — and the arduino_zero variant marks PA09No_ADC_Channelanyway, so PA09 is unreachable throughanalogRead()entirely. Floating-pin noise can fake plausible 33–100 Hz "crossings", so$GZand the ZC switch timing were garbage. Replaced with a direct ADC read of AIN[17] (INPUTCTRL.MUXPOS = 0x11), bracketed around the sampling burst, with the PA09 digital pull-up (it doubles as the ACLINE2/ground-monitor input) disabled during sampling and restored after. This follows the core's enable/discard-first-conversion/read/disable sequence so it coexists withanalogRead()on the other channels. This is the commit that most needs hardware verification.4.
ECF_OVERCURRENT_DISABLEDcollided withECF_TEMP_CHK_DISABLED— both were0x0400, so$FF O 0persistently disabled temperature monitoring and$FF T 0disabled the overcurrent check. Moved overcurrent to the free bit0x4000. A unit that had persisted overcurrent-disabled will revert to enabled, which is the safe direction.5.
$GIoverflowed two buffers on SAMD. With a 16-byte MCU id, the hex formatting writes 33 bytes into the 32-byte RAPIbuffer(the NUL lands onbufCnt), andresponse()then assembles ~45 bytes into the 34-byteg_sTmp. Sized both per target (SAMD only — zero AVR RAM cost) and added#errorguards tying the sizes toMCU_ID_LEN. Also fixed the#ifdef TARGET_M238Ptypo that left the intended AVR id format dead code.6.
-D NO_GROUND_RECORD_DELAY=2000never matched the code macro (NO_GND_RECORD_DELAY), so the 2 s suppression of spurious NO-GROUND trip recording has never been active in a PlatformIO build. Renamed the flag.Verification
pio run -e samd -e m328p_core -e m328p_LCD_WIFI -e m328p_noWiFi— all green on every commit.$GEflag word); fixes 2, 4, 5, 6 are arithmetic/config, traced in the source; fix 3 is compile-verified only and needs a scope or a$GZsanity check on a powered NXT.While auditing I also noted (not fixed here, happy to file separately): the GFI retry counter resets on every fresh fault entry so
GFI_RETRY_COUNTnever limits recurring live faults;AUTOSVCLEVELandTIME_LIMIT/CHARGE_LIMITare only defined for Arduino-IDE builds andm328p_noWiFi, so the shipping PlatformIO envs answer$SL A(which the WiFi module's "Auto" service level sends) withNK;$SKis missing itstokenCntguard; and[env:samd_ice_dev]'sbuild_src_flagsoverride drops the inherited flags and doesn't build.