Skip to content

Bugfix stmApp.cpp: double idx++ mis-maps valve status list (+ gvlst trailing comma)#7

Open
hubertciebiada wants to merge 2 commits into
SurfGargano:masterfrom
hubertciebiada:fix/valve-status-parser-double-idx
Open

Bugfix stmApp.cpp: double idx++ mis-maps valve status list (+ gvlst trailing comma)#7
hubertciebiada wants to merge 2 commits into
SurfGargano:masterfrom
hubertciebiada:fix/valve-status-parser-double-idx

Conversation

@hubertciebiada

Copy link
Copy Markdown

Problem

Two bugs on the same valve-status path (APP_PRE_GETVLSTATUS / gvlst),
the ESP32 parser and the STM32 builder.

1. ESP32 parser — double idx++ (software_esp32/src/stmApp.cpp)

The loop that reads the valve status list increments the index twice per
iteration, once in the for clause and once in the body:

for (uint8_t idx=0; idx<nActuators; idx++) {
    if ((cmdptr=strchr(ps,','))!=NULL) *cmdptr='\0';
    val8 = atoi(ps);
    actuators[idx].state = (val8 & 0x7f);
    actuators[idx].calibration = (val8>=0x80);
    if (cmdptr!=NULL) ps=cmdptr+1;
    idx++;   // ← extra increment
}

The status of valve k lands in actuators[2*k], and every odd slot is
left stale. With 12 valves the loop runs 6 times and fills slots
0,2,4,6,8,10 only. Observed in practice as phantom CONNECTED entries
at idx 9 and 11 on the /valves list — the mirrored statuses of valves
5 and 6 (5→10, 6→12 overflows, while the parser reads far enough to
smear values into 9/11 as ps walks the string).

This is the same copy-pasted pattern already fixed for the 1-Wire
temps/volts sensor lists in the merged PR #5 — this is the third
instance of it, on the valve list.

2. STM32 builder — wrong index for comma separator (software_stm32/src/communication.cpp)

The response is assembled with the wrong variable in the separator test:

for (uint8_t xx=0; xx<ACTUATOR_COUNT; xx++) {
    memset(sendbuffer,0x0,sizeof(sendbuffer));
    itoa(myvalvemots[xx].status, sendbuffer, 10);
    COMM_SER.print(sendbuffer);
    if (x<ACTUATOR_COUNT-1) COMM_SER.print(",");   // ← x, not xx
}

x is the function-level scratch variable (communication.cpp:175),
assigned from atoi(argptr[0]) by other command handlers; the gvlst
block never sets it and its loop index is xx. With x==0 every
element including the last gets a comma → a trailing comma in the reply.

Verification

  • Response format is gvlst <count> <s0>,<s1>,... with one status
    token per valve
    — see software_stm32/src/communication.cpp:405-410.
    <count> equals the number of tokens, so exactly one idx++ per
    element is correct on the ESP32 side.
  • x is never assigned inside the gvlst builder block; xx is the
    loop index, so the separator must key off xx.
  • Identical mechanism and fix as PR Bugfix stmApp.cpp : double idx++ skips half of 1-Wire sensors #5, which corrected the same
    double-increment in the temps parser (stmApp.cpp) and the volts
    parser copied from it.

Fix

  • stmApp.cpp: remove the redundant idx++ from the parser loop body.
  • communication.cpp: use the loop index xx in the separator test.

Two separate commits, one per file/MCU.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Mr14XmEnfZ2kFVhfQzupAg

hubertciebiada and others added 2 commits July 2, 2026 22:28
Problem

In software_esp32/src/stmApp.cpp the parser for the valve status list
(APP_PRE_GETVLSTATUS / "gvlst") increments the loop index twice per
iteration -- once in the for clause and once in the body:

    for (uint8_t idx=0; idx<nActuators; idx++) {
        if ((cmdptr=strchr(ps,','))!=NULL) *cmdptr='\0';
        val8 = atoi(ps);
        actuators[idx].state = (val8 & 0x7f);
        actuators[idx].calibration = (val8>=0x80);
        if (cmdptr!=NULL) ps=cmdptr+1;
        idx++;   // <- extra increment
    }

The STM32 response format is "gvlst <count> <s0>,<s1>,..." with one
status token per valve (software_stm32/src/communication.cpp:405-410),
so one idx++ per element is correct.

Effect

Status of valve k is written to actuators[2*k], and every odd slot is
left stale. With 12 valves the loop terminates after 6 iterations and
fills slots 0,2,4,6,8,10 only. Observed in practice as phantom
CONNECTED entries at idx 9 and 11 on the /valves list -- mirror images
of the real statuses of valves 5 and 6.

Same class of bug as the temps/volts sensor-list parsers fixed in
PR SurfGargano#5; this is the third instance of the copy-pasted pattern.

Fix

Remove the redundant idx++ from the loop body. The for clause's own
increment now advances by one per element.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mr14XmEnfZ2kFVhfQzupAg
Problem

In software_stm32/src/communication.cpp the builder of the valve status
list (APP_PRE_GETVLSTATUS / "gvlst") tests the wrong variable when
deciding whether to append a comma separator:

    for (uint8_t xx=0; xx<ACTUATOR_COUNT; xx++) {
        memset(sendbuffer,0x0,sizeof(sendbuffer));
        itoa(myvalvemots[xx].status, sendbuffer, 10);
        COMM_SER.print(sendbuffer);
        if (x<ACTUATOR_COUNT-1) COMM_SER.print(",");   // <- x, not xx
    }

x is the function-level scratch variable (communication.cpp:175), set
from atoi(argptr[0]) by other command handlers; the gvlst block never
assigns it and its loop index is xx. The comma decision therefore keys
off a stale value instead of the current position.

Effect

The separator is placed by chance: with x==0 (initial) every element
including the last is followed by a comma -> a trailing comma
"s0,s1,...,s11," in the response. The ESP32 tolerates it, but the
output is malformed and the intent (no trailing comma) is not met.

Fix

Use the loop index xx in the separator test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mr14XmEnfZ2kFVhfQzupAg
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant