-
Notifications
You must be signed in to change notification settings - Fork 144
Add help menu to CommonCLI.cpp #635
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
I keep wanting a help menu in the CLI but it isn't there. Had a look and realised a small refactor would help make this maintainable. I have: - made a single source of truth for the output strings - added a number of formatter functions to enable small help responses (essentially grouped things) - added the calling routines for the above into the main logic CLI message responses for use over the mesh need to be <= 160 chars which I *think* I have achieved here. Needed to group things so that it didn't just try to dump everything in one hit and go way over that 160 limit. Was also conscious of future expansion so I hope these messages are still small enough for additional options in the future. I believe this is a direct drop-in with no other files / code affected. This is first commit after development though and has not yet been tested on a device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to have help! I was constantly looking for this as well :)
if (memcmp(command, CMD_HELP, 4) == 0) { | ||
if (memcmp(command, "help device cmds", 16) == 0) { format_help_device_cmds(reply); return; } | ||
if (memcmp(command, "help device keys", 16) == 0) { format_help_device_keys(reply); return; } | ||
if (memcmp(command, "help radio cmds", 15) == 0) { format_help_radio_cmds(reply); return; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably make sense to use the constants here as well, something like:
if (memcmp(command, "help radio cmds", 15) == 0) { format_help_radio_cmds(reply); return; } | |
if (memcmp(command, CMD_HELP + SPACE + KEY_RADIO + "cmds", 15) == 0) { format_help_radio_cmds(reply); return; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO it would be better to tokenize commands (e.g. start ota
> cmd: 'start
' & arg: 'ota
']) and use those rather than string comparisons on command + space (e.g. "password"
vs "password "
).
Something along these lines:
#include <string.h>
#include <stdio.h>
...
void handleCommand(char *input) {
// split by spaces
char *cmd = strtok(input, " ");
char *arg = strtok(NULL, " "); // first argument
char *arg2 = strtok(NULL, " "); // optional second, etc.
if (cmd == NULL) return;
if (strcmp(cmd, "start") == 0) {
if (arg && strcmp(arg, "ota") == 0) {
Serial.println("Starting OTA...");
// startOTA();
} else {
Serial.println("Unknown start option");
}
} else if (strcmp(cmd, "stop") == 0) {
Serial.println("Stopping...");
} else if (strcmp(cmd, "status") == 0) {
Serial.println("Status is OK");
} else {
Serial.println("Unknown command");
}
}
While, as mentioned earlier, I would welcome a command line help, I wonder if the output may be too chatty for LoRa? Perhaps the help should be handled in the clients rather than the firmware?
@@ -180,88 +366,88 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, const char* command, ch | |||
} else { | |||
strcpy(reply, "Error, invalid params"); | |||
} | |||
} else if (memcmp(command, "password ", 9) == 0) { | |||
} else if (memcmp(command, CMD_PASSWORD, 9) == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original string is nine characters "password "
(note the space at the end), whereas the constants is "password"
(no space, 8 characters). So it looks like this if statement will be false.
I keep wanting a help menu in the CLI but it isn't there. Had a look and realised a small refactor would help make this maintainable. I have:
CLI message responses for use over the mesh need to be <= 160 chars which I think I have achieved here. Needed to group things so that it didn't just try to dump everything in one hit and go way over that 160 limit. Was also conscious of future expansion so I hope these messages are still small enough for additional options in the future.
I believe this is a direct drop-in with no other files / code affected. This is first commit after development though and has not yet been tested on a device. Please can I ask for help on this as it might take me a bit to actually test it (I am away from devices right now).