dani007200964 / Shellminator

Shellminator is a shimple shell interface for embedded devices
https://www.shellminator.org/
MIT License
84 stars 7 forks source link

ESP32C3 command function callback selection fails #19

Closed hpsaturn closed 4 months ago

hpsaturn commented 4 months ago

Summary

I'm testing this library that is amazing and beautiful, good job!. I already migrated and implemented it on my ESP32 WiFi CLI and works fine with the ESP32 core, but with the esp32c3 when you have many commands >=20, the commander maybe has a memory issue but it is choosing a different callback for some commands.. very weird..

To Reproduce

Steps to reproduce the behavior:

  1. The used board is: LOLIN C3 mini (esp32-c3-devkitm-1)
  2. The used microcontroller is: esp32c3
  3. The used environment and it's version is: PlatformIO Espressif 6.7.0
  4. The used OS on your computer is: Debian 11

Sample code:

#include <Arduino.h>

#include "Commander-API.hpp"
#include "Commander-IO.hpp"
#include "Shellminator-IO.hpp"
#include "Shellminator.hpp"

Shellminator shell(&Serial);

const char logo[] =

    "   _____ __         ____          _             __            \r\n"
    "  / ___// /_  ___  / / /___ ___  (_)___  ____ _/ /_____  _____\r\n"
    "  \\__ \\/ __ \\/ _ \\/ / / __ `__ \\/ / __ \\/ __ `/ __/ __ \\/ ___/\r\n"
    " ___/ / / / /  __/ / / / / / / / / / / / /_/ / /_/ /_/ / /    \r\n"
    "/____/_/ /_/\\___/_/_/_/ /_/ /_/_/_/ /_/\\__,_/\\__/\\____/_/     \r\n"
    "\r\n\033[0;37m"
    "Visit on GitHub:\033[1;32m https://github.com/dani007200964/Shellminator\r\n\r\n"

    ;

Commander commander;

void cat_func(char *args, Stream *response);
void sum_func(char *args, Stream *response);
void led_func(char *args, Stream *response);
void passw_func(char *args, Stream *response);
void dog1_func(char *args, Stream *response) { response->print("Hello from dog1 function!\r\n"); }
void dog2_func(char *args, Stream *response) { response->print("Hello from dog2 function!\r\n"); }
void dog3_func(char *args, Stream *response) { response->print("Hello from dog3 function!\r\n"); }
void dog4_func(char *args, Stream *response) { response->print("Hello from dog4 function!\r\n"); }
void dog5_func(char *args, Stream *response) { response->print("Hello from dog5 function!\r\n"); }
void dog6_func(char *args, Stream *response) { response->print("Hello from dog6 function!\r\n"); }
void dog7_func(char *args, Stream *response) { response->print("Hello from dog7 function!\r\n"); }

Commander::API_t API_tree[] = {
    apiElement("cat", "Description for cat command.", cat_func),
    apiElement("cat", "Description for cat command.", cat_func),
    apiElement("cat", "Description for cat command.", cat_func),
    apiElement("cat", "Description for cat command.", cat_func),
    apiElement("cat", "Description for cat command.", cat_func),
    apiElement("dog1", "Description for dog1 command.", dog1_func),
    apiElement("dog2", "Description for dog2 command.", dog2_func),
    apiElement("dog3", "Description for dog3command.", dog3_func),
    apiElement("dog4", "Description for dog4 command.", dog4_func),
    apiElement("dog5", "Description for dog5 command.", dog5_func),
    apiElement("dog6", "Description for dog6 command.", dog6_func),
    apiElement("dog7", "Description for dog7 command.", dog7_func),
    apiElement("led", "Toggle the buit-in LED.", led_func),
    apiElement("mem", "Toggle the buit-in LED.", led_func),
    apiElement("led3", "Toggle the buit-in LED.", led_func),
    apiElement("led4", "Toggle the buit-in LED.", led_func),
    apiElement("led5", "Toggle the buit-in LED.", led_func),
    apiElement("led6", "Toggle the buit-in LED.", led_func),
    apiElement("led7", "Toggle the buit-in LED.", led_func),
    apiElement("led8", "Toggle the buit-in LED.", led_func),
    apiElement("led9", "Toggle the buit-in LED.", led_func),
    apiElement("led10", "Toggle the buit-in LED.", led_func),
    apiElement("led11", "Toggle the buit-in LED.", led_func),
    apiElement("led12", "Toggle the buit-in LED.", led_func),
    apiElement("passw", "Toggle the buit-in LED.", passw_func),
    apiElement("sum", "This function sums two number from the argument list.", sum_func)};

void passw_func(char *args, Stream *response) {
  char *passw;
  response->printf("%s\r\n", String(passw).c_str());
}

String getDeviceInfo() {
  String info = "==================\r\n";
  info = info + "MEM: " + String(ESP.getFreeHeap() / 1024) + "Kb\r\n";
  return info;
}

void setup() {
  Serial.begin(115200);
  delay(4000);
  shell.clear();
  shell.attachLogo(logo);
  Serial.println("Program begin...");
  commander.attachDebugChannel(&Serial);
  commander.attachTree(API_tree);
  commander.init();
  shell.attachCommander(&commander);
  shell.begin("test");
}

void loop() { shell.update(); }

void cat_func(char *args, Stream *response) { response->print("Hello from cat function!\r\n"); }

void led_func(char *args, Stream *response) { response->print(getDeviceInfo()); }

void sum_func(char *args, Stream *response) {
  int a = 0;
  int b = 0;
  int argResult;
  int sum = 0;
  argResult = sscanf(args, "%d %d", &a, &b);
  if (argResult != 2) {
    response->print("Argument error! Two numbers required, separated with a blank space.\r\n");
    return;
  }
  sum = a + b;
  response->print(a);
  response->print(" + ");
  response->print(b);
  response->print(" = ");
  response->println(sum);
}

PlarformIO ini file:

[common]
platform = espressif32 @ 6.7.0
framework = arduino
upload_speed = 1500000
monitor_speed = 115200
monitor_filters = 
  esp32_exception_decoder
  ; time
build_flags = 
  -D CORE_DEBUG_LEVEL=0         # For debugging set to 3 and enable debug mode in the app
  -D COMMANDER_USE_WIFI_CLIENT_RESPONSE=1
lib_deps = 
  https://github.com/dani007200964/Shellminator.git
  https://github.com/dani007200964/Commander-API

board_build.partitions = min_spiffs.csv

[env:esp32]
platform = espressif32 @ 4.4.0
extends = common
board = lolin32
build_flags = 
  ${common.build_flags}

[env:esp32c3]
platform = espressif32 @ 6.7.0
extends = common
board = esp32-c3-devkitm-1
build_flags = 
  ${common.build_flags}
  -D ARDUINO_USB_CDC_ON_BOOT=1
  -D ARDUINO_USB_MODE=1

Output:

After the run the command help and select the dog5, the commander has a confusion and it choose the dog1:

test:$ dog5
Hello from dog1 function!

screenshot20240607_190004

Expected behavior

That the commander choose the right callback and don't mix these ones.

Screenshots

With the ESP32 WRover works so good: screenshot20240607_185013

If applicable, add screenshots to help explain your problem.

Additional context

I also have a full implementation of my WiFi manager with this library in the branch improv_serialterminal here

Thanks in Advance!!

dani007200964 commented 4 months ago

Hi! Thanks for the detailed report.

The first problem that I can see is that you use multiple commands with the same name.

This could corrupt the internal binary tree, that holds the commands. The second problem could be an strcmp problem, but I have to test it.

@hpsaturn Can you please tell me the exact version of Commander-API? It can be found in Commander-API.hpp

hpsaturn commented 4 months ago

Thanks for your quickly response. Well, I think that repeated names is no the problem, because in my other implementation I don't have these.. but let me check.. the other thing is that in esp32 works fine this example..

dani007200964 commented 4 months ago

Sorry, I need a bit more time to ramp up to the problem. Currently I'm checking your project. Looks cool BTW.

dani007200964 commented 4 months ago

I believe I know the solution because I've spent many hours investigating this issue. My problems were specific to the Pi Pico and the Arduino IDE. I also encountered issues with Risc-V based ESP devices. The issue isn't with Shellminator, but rather with Commander-API.

In the past, I aimed to make the command parser as lightweight as possible and made a significant mistake with the strcmp functions. It turns out that strcmp doesn't always return -1, 0, or 1. Since the return type is int, it can vary with different architectures. I wasn't aware of this and used int8_t or int32_t to store the result to save space and make the memory usage more deterministic. But what happens when you try to store a signed 16-bit number in an 8-bit variable? It disrupts the entire binary tree structure.

This issue is fixed in the DEV version, but since the DEV version is frequently updated, I highly recommend not using it until a stable release is available.

As a quick fix, you can modify the following lines in Commander-API.cpp:

https://github.com/dani007200964/Commander-API/blob/042c9c3286f06133482753843b3c345c4852e75a/src/Commander-API.cpp#L607-L622

https://github.com/dani007200964/Commander-API/blob/042c9c3286f06133482753843b3c345c4852e75a/src/Commander-API.cpp#L211-L227

You need to change the type of the comp_res variables to int.

I know this is not the prettiest solution, but over the past year, I haven't had enough time to create a fix for this problem because my friends and I didn't encounter any issues with STM32 and AVR devices. You are the first to actually discover this :bug:

Please check if this fixes your problem.

Just a hint: In your version, it is also a known issue that two commands with the same name can corrupt the binary tree. Please try to avoid those scenarios, even when you are just stress-testing the system. Trust me, I've had some headaches because of this :wink:

hpsaturn commented 4 months ago

On the other hand, you able to test this issue using this branch, and this:

git clone https://github.com/hpsaturn/esp32-wifi-cli.git
git checkout improv_serialterminal
cd esp32-wifi-cli
pio run -e esp32c3 --target upload

Then try for example the command in the shell, sleep and the output will be the output from setled command.

dani007200964 commented 4 months ago

The greatest challenge right now is to find a C3 in the lab. Give me a few minutes, I will try my best, but I had only one suitable microcontroller lying around, and it was one year ago :neutral_face:

hpsaturn commented 4 months ago

Ok.. I'm going to eat something, and then I'm going to test this fix that you mentioned. Thanks!

hpsaturn commented 4 months ago

I tested your new branch strcmp-fix and it is working like charm! Thanks again!

dani007200964 commented 4 months ago

I will merge it into the main branch and create a release for you. Please note that firstly Arduino CI will update, it usually takes one or two days, and another one or two days is needed for PlaformIO servers as well.

hpsaturn commented 4 months ago

well, for PlatformIO is more easy and faster if you use the CLI of them to do the publication:

https://docs.platformio.org/en/latest/librarymanager/creating.html#publishing

the only thing, is that you need add a manifest, library.json for each library, like is described there. The other advantage is that you also could have the dependencies of Shellminator there and Pio will could do the resolution of these.