Skip to content
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

Improve CommandProcessing library #2748

Merged
merged 12 commits into from
Mar 27, 2024

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Mar 26, 2024

Here's a PR with a few suggestions to improve the CommandProcessing library.

Handling line endings is inconsistent

Running the CommandLine sample on Host we get this:

Sming>statusSming Framework Version : 5.1.0
ESP SDK version : Host-SDK-v1.0
Time = 26/03/2024 08:59:47
System Start Reason : 0
Sming>

On Esp8266 it looks different:

Sming>
statusSming Framework Version : 5.1.0
ESP SDK version : 3.0.3(8427744)
Time = 01/01/1970 06:42:14
System Start Reason : 6
Sming>

This is what it should be:

Sming>status
Sming Framework Version : 5.1.0
ESP SDK version : 3.0.3(8427744)
Time = 01/01/1970 06:42:14
System Start Reason : 6
Sming>

The currentEOL and associated methods seem redundant

The LineBuffer handles "\r", "\n", "\n\r" and "\r\n" the same way, so we can just use that.

command name is stored twice

Using a HashMap means the command name is stored both as a key, and in the Command object.
Let's just use a Vector instead.

localEcho

Private member variable set to true and never changed. Assume intention was isVerbose.

Consuming RAM for string storage

Since name, help and group strings never change, we should keep them in flash and save some RAM.
Best way I've found is storing them all in a single flash block, which requires only 4 bytes in the Command object.
This requires use of a CMDP_STRINGS macro when calling registerCommand.
To minimise disruption for existing code this can be enabled using CMDPROC_FLASHSTRINGS=1.

When using regular Strings, there's also a small saving to be had by storing these together in a CStringArray.

For comparison, here's what system_get_free_heap_size() reports for the CommandLine sample application.
The call is made at the end of init().

Commit free RAM
Current version using hashmap 51704
After initial tidy 51752
Use Vector 52104
Use CStringArray 52160
Use FlashString 52664

Welcome message not used

This was used by original TelnetServer sample application. Have restored display of welcome message to connecting clients.
Also getting junk on display because of TELNET escape sequences, so filtered those out too.

mikee47 added 5 commits March 26, 2024 13:17
Use simpler and consistent naming
Use HashMap more efficiently
Avoids duplicating storage of name.
Remove `currentEOL` and deprecate associated methods. Since either `\r` or `\n` are accepted not sure there's any need for this?
Assume intention was `isVerbose`
@mikee47 mikee47 force-pushed the feature/cmdproc-rewrite branch 2 times, most recently from bc01e9d to 2086535 Compare March 26, 2024 20:19
@mikee47 mikee47 force-pushed the feature/cmdproc-rewrite branch from 2086535 to 6fc1969 Compare March 26, 2024 20:29
@slaff slaff added this to the 5.2.0 milestone Mar 27, 2024
@mikee47 mikee47 changed the title [WIP] Improve CommandProcessing library Improve CommandProcessing library Mar 27, 2024
@mikee47 mikee47 requested a review from slaff March 27, 2024 15:03
@slaff slaff merged commit 99b2ccd into SmingHub:develop Mar 27, 2024
46 checks passed
@mikee47 mikee47 deleted the feature/cmdproc-rewrite branch March 27, 2024 15:15
@slaff slaff mentioned this pull request May 8, 2024
5 tasks
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.

2 participants