Feature/351 firmware factorytest wmenu add an exit command

[Yukti007]

Links

• Closes Firmware, Factorytest_wMenu, add an exit command. #351

What & Why

• Add a global "exit" command (any case) to stop the current test or prompt and go back to the main menu.
• Create g_globalExitRequested to handle exiting safely inside loop().
• Add isExitCommand() to check if the user typed "exit".
• Update readLineOrMenuAbort() and promptYesNo() to detect "exit" and trigger the exit.
• Update loop() to check for g_globalExitRequested first.
• Prevent partial typing from running the wrong menu command.

Validation / How to Verify

1. From main menu, type:
   • exit, EXIT, Exit, eXiT
   • Confirm system prints global exit message and reprints menu.
2. During each of the following, type exit + Enter:
   • Y/N prompt (e.g., LED test confirmation)
   • WiFi SSID input
   • WiFi password input
   • Run-All sequence
3. Confirm:
   • Current operation aborts immediately after Enter.
   • Menu is restored.
   • No reboot occurs.
   • No unintended tests are marked PASS.
   • System remains responsive.

Artifacts (attach if relevant)

Checklist

• Only related changes : FactoryTest_wMenu.ino
• Folder structure respected, work directory : krake\Firmware\factoryTest\FactoryTest_wMenu\FactoryTest_wMenu.ino

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 39329f1e0d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Handle global exit inside long-running command flows

The new global-exit flag is only consumed at the top of loop(), so if exit is entered during runAllTests() (or any command path that does not return to loop() quickly), the operation keeps running additional tests before the abort is honored. In this commit readLineOrMenuAbort()/promptYesNo() set g_globalExitRequested, but runAllTests() never checks it, so the "exit current operation immediately" behavior is broken specifically during Run-All and can execute unintended hardware tests after the operator asked to stop.

Useful? React with 👍 / 👎.

[ForrestErickson]

Vocabulary of a Professional Programmer

I hope this will help.
I think that it is best not to use the word "exit" for this concept. Rather "break"
these words have specific connotation in programing profession.

Connotation Versus Denotation

In computer programing, compare and contrast exit and break

[TheBakedPotato]

This whole function can be simplified with the String::equalsIgnoreCase() method.

// Case-insensitive check for exact string "exit"
static bool isExitCommand(const String &s)
{
  return s.equalsIgnoreCase("exit");
}

[TheBakedPotato]

I realize now the char* exit should probably be defined in a const. Well, you get the idea. lol

[TheBakedPotato]

I should also be clear on why this works. The char*, the exit string, is implicitly converted into a String object because of the String::String(const char*) constructor. As a note, for those unaware like I was until just now, C++ will implicitly convert types if there is a matching constructor matching that type. In this case, there is a String constructor for type const char* so it gets used. The only time this does not happen implicitly is if the constructor is proceeded with the keyword explicit (which in my opinion should be the default).

Now, being a String this CAN incur memory/heap allocation. One concern is that system does not have available memory for heap allocation, but that won't be a concern in this test. The OTHER and very serious concerns in embedded systems is memory fragmentation due to the lack of MMU. Lucky for us, the ESP32 SoC has an MMU so memory fragmentation I believe shouldn't happen.

Anyways, this is all to say that your solution is not necessarily incorrect. Or even sub-optimal. In fact, some may argue it is the "technically correct" solution. However, especially since this is a test and not running the firmware for a long period of time where the aforementioned problems COULD be encountered, I don't see anything wrong with using a String to make your life easier.

[TheBakedPotato]

Vocabulary of a Professional Programmer

I hope this will help. I think that it is best not to use the word "exit" for this concept. Rather "break" these words have specific connotation in programing profession.

Connotation Versus Denotation

In computer programing, compare and contrast exit and break

In this case I do not believe this distinction is relevant. The exit functionality that was added was to exit a running test. That is fairly standard language used when describing UI navigation. And the test framework very much is a UI system since it requires user interaction to make selections, etc.

When using an application on your phone or computer and you are in the settings menu it is very common language to say "you are exiting" or "you should exit" the settings menu. This use case is no different.

[Yukti007]

Global Exit Command

I added a global exit command (case-insensitive) to stop the current test and return to the main menu.

Current Behavior

• Typing exit at the main menu simply refreshes the menu.
• Typing exit during any Y/N prompt or text input immediately stops that step and returns to the menu.
• During Run All, typing exit stops the sequence before the next test runs.

Clarification

At the moment, exit stops at logical test boundaries.

For example, if a test is in the middle of a short wait (such as a few seconds of speaker playback or a Wi-Fi connection attempt), it will complete that short wait before returning to the menu. It does not interrupt delays mid-execution.

Open Question

Before adding additional checks inside those loops, I’d like feedback:

Do we want:

• A simpler behavior where exit stops between tests and prompts (current implementation), or
• A stricter behavior where exit interrupts even short waits immediately?

Naming

I used exit because it is user-facing (e.g., “exit menu” or “exit test”).

If the team prefers the word break instead, I have no problem changing it for consistency — just let me know and I will update it.

[nk25719]

[Yukti007]

Implemented @TheBakedPotato 's suggestion by simplifying the break detection using equalsIgnoreCase().

Per @ForrestErickson recommendation, renamed the user command from exit to break for terminology consistency.

Additionally, updated long-running test loops (Inputs, Speaker, WiFi STA, RS-232) to check g_globalBreakRequested cooperatively. The system now aborts immediately at loop level with proper peripheral cleanup, rather than waiting for delays or blocking operations to complete.

[nk25719]

Sent the "break" command at: 18:17:25.824 and that caused a reboot.
Repeated the test couple times, every time I send the "break" command, the board reboots.

[TheBakedPotato]

Purely asking for clarification, not looking for a change. But why is the FIRMWARE_VERSION in the form of w.x.y.z? Not saying this is incorrect in anyway at all, just seems "unconventional" and not sure what the different numbers mean.

[Yukti007]

here according to me we are following 0.major.minor.patch type versioning, since we haven't launched any version thus it's 0 there while after that we are following the normal semantic versioning, Also i changed it from 2.x to 3.x as i am adding a new feature that we think is a minor change.
Hope this is correct and clears your doubt.

[ForrestErickson]

Regarding, ' just seems "unconventional" ' It is because we did not know the phrase ' semantic versioning ' till recently when you got us up to speed. Good work on that!

[TheBakedPotato]

This is more about style and convention, but it might be best to still put the return false; in braces. I know C/C++ allow NOT putting in braces but by using braces it makes it much more clear what the scope is of the if statement. I believe not using braces can sometimes lead to odd and unexpected bugs because a change is made and it's not clear that thr body to the if statement was modified.

So my suggestion is to do:

if (g_globalBreakRequested)
{
  return false;
}

[Yukti007]

It looks like the reset is likely caused by repeated String heap allocations during serial parsing when "break" is entered.

For example, when the user types b, r, e, a, k, each character is appended to a String, which may reallocate memory internally as it grows. Then trim() and equalsIgnoreCase() can also involve additional heap activity. On embedded systems, repeated small allocations like this can fragment the heap over time. If a new allocation cannot be satisfied cleanly, it can result in a crash and software reset (SW_CPU_RESET), which matches what we’re seeing.

To reduce this risk, I’m removing the String-based comparison and replacing it with fixed character checks (e.g., checking each letter manually). This avoids any additional heap allocation during command detection.

Longer term, I plan to migrate serial input parsing from dynamic String objects to fixed-size char[] buffers. That way memory usage becomes deterministic, which is generally safer and more predictable for factory firmware.

[Yukti007]

Update on GLOBAL BREAK changes

What I changed

• Replaced equalsIgnoreCase() with strcasecmp() to avoid implicit String creation.
• Removed String usage from:
  • Main loop() menu parsing (now fixed char buffer)
  • promptYesNo() (now fixed char buffer)
• Added g_globalBreakRequested checks inside long-running loops (Inputs, Speaker, WiFi STA, RS232, Run-All).
• Ensured peripherals are safely stopped on break (dfPlayer.stop(), WiFi.disconnect(), rs232.end()).

This removes heap allocation from the main interactive/break path and makes break handling more deterministic.

Left for future cleanup

• readLineOrMenuAbort() still uses String.
• runTest_RS232() still uses a String RX buffer.
• Some non-critical WiFi string handling still uses String.

These are not in tight loops but can be migrated to char[] in a future refactor for full determinism.

[ForrestErickson]

The String Object Problem and Fix

Good work Yukti,
It is also very good for Nagham to have seen this problem even just as a user.

Regarding Problems with Strings on uControllers.
Some time ago I discovered a well written essay on the problem with String objects on limited memory systems like UNO and ESP32. I no longer know where to look for that essay but this AI search result is similar:

Lee Can Not C Style String Well

FYI,
I use String objects in some simple sketches but never any intending for long term use.
I can write with out fear of contradiction that I am NOT able to write code with C style strings well and would benefit from a good tutorial series.

[TheBakedPotato]

Update on GLOBAL BREAK changes

What I changed

* Replaced `equalsIgnoreCase()` with `strcasecmp()` to avoid implicit `String` creation.

* Removed `String` usage from:
  
  * Main `loop()` menu parsing (now fixed `char` buffer)
  * `promptYesNo()` (now fixed `char` buffer)

* Added `g_globalBreakRequested` checks inside long-running loops (Inputs, Speaker, WiFi STA, RS232, Run-All).

* Ensured peripherals are safely stopped on break (`dfPlayer.stop()`, `WiFi.disconnect()`, `rs232.end()`).

This removes heap allocation from the main interactive/break path and makes break handling more deterministic.

Left for future cleanup

* `readLineOrMenuAbort()` still uses `String`.

* `runTest_RS232()` still uses a `String` RX buffer.

* Some non-critical WiFi string handling still uses `String`.

These are not in tight loops but can be migrated to char[] in a future refactor for full determinism.

Oh boy. My bad! I think I misinterpreted the MMU the ESP32 has. It seems to only used for mapping external memory from the SPI flash or SRAM. I don't think it is in use for the embedded RAM hence the memory fragmentation issues. I am very sorry for suggesting to use the String type.