feature(console): add command user context support (IDFGH-11280)

[alonbl]

Outline

Current implementation implicitly forces the developer to use global variables
to enter its context during the command invocation, this change enables each
module to register a context for command to find without the need to manage
global variables.

No API breakage.

Fields added

   esp_console_cmd_t::func_context    - (*)(int argc, char **argv, void *context)

Functions added

   esp_err_t esp_console_cmd_set_context(const char *cmd, void *context)

Usage

   esp_console_cmd_register(&cmd));
   esp_console_cmd_set_context(cmd.command, (void *)"context"));

[CLAassistant]

All committers have signed the CLA.

[github-actions]

	Messages
📖	Good Job! All checks are passing!

---

👋 Welcome alonbl, thank you for your first contribution to espressif/esp-idf project!

📘 Please check Contributions Guide for the contribution checklist, information regarding code and documentation style, testing and other topics.

🖊️ Please also make sure you have read and signed the Contributor License Agreement for espressif/esp-idf project.

Pull request review and merge process you can expect

Espressif develops the ESP-IDF project in an internal repository (Gitlab). We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.

1. An internal issue has been created for the PR, we assign it to the relevant engineer
2. They review the PR and either approve it or ask you for changes or clarifications
3. Once the Github PR is approved, we synchronize it into our internal git repository
4. In the internal git repository we do the final review, collect approvals from core owners and make sure all the automated tests are passing
   • At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
5. If the change is approved and passes the tests it is merged into the master branch
6. On next sync from the internal git repository merged change will appear in this public Github repository

---

Generated by 🚫 dangerJS against 867f9d0

[0xjakob]

@alonbl We're taking a look at this now and will come back here soon.

[0xjakob]

@alonbl We discussed the PR and in general, it looks good! We have one concern, however: The current version would add a global context value which is passed to any command callback registered with esp_console_cmd_t::func_context . What is the difference to having a global context variable anywhere in the user code? What exact problem are you trying to solve?

[alonbl]

@alonbl We discussed the PR and in general, it looks good! We have one concern, however: The current version would add a global context value which is passed to any command callback registered with esp_console_cmd_t::func_context . What is the difference to having a global context variable anywhere in the user code? What exact problem are you trying to solve?

Hi,

Global variables are evil, the libraries should be designed to minimize their use, this helps to keep track of resources. Look at the http_server implementation as a reference which is done nicely. Having application context managed in several places, aka passed as parameters to functions (at is should) and have it also copied in global variable is bad practice as the two (or more) require synchronization.

The next stage of this patch would be the ability to register a context per command using esp_console_cmd_register(), and the next one is to have a context per console instance.

The end use case is the following: In application each module registers its own console commands as decoupled from one from another, a command of each module should be invoked in the correct context.

As you well know, the console module is very important to embedded system and is missing a lot of love, for example:

1. having a console instance (vs global) as best practice to manage multiple ports ([ESP Linenoise] new esp linenoise with multiconsole support (IDFGH-9126) #10526)
2. to be able to provide different console behavior to different devices (aka console context, just like http_server)
3. unregister command to allow resource cleanups of modules.
4. ability to stop the console task nicely from outside of the console ([Usb Serial JTAG] [Deadlock] [Console Deinit] esp_vfs_usb_serial_jtag_use_nonblocking() causes deadlock (IDFGH-8520) #9974)
5. ability to invoke a command within specific context (decouple).

What do you think? I can workout a modified patch that will allow a context per command (step#2), will it be better?

Thanks,

[0xjakob]

Yes, one context per command sounds more flexible to us.

If you have any question, feel free to come back to us, we'll provide help where possible. Thanks a lot already!

[igrr]

Don't the two contexts ("per command" and "per esp_console_run invocation") solve different kinds of problems?

• Context per command: potentially allows having a single command implementation handle multiple commands. For example, you can have a generic implementation (cmd_write) of a set of commands (write_str, write_int, write_blob), and register multiple commands all using the same handler function. The handler will distinguish which command to run based on the context provided during registration.
• Context per esp_console_run call: allows the command to access variables specific to the console instance. For example, when running two instances of console in different tasks, the command would have access to the state related to the current instance.

So one solution doesn't seem to be "better" or "worse" than the other, they seem to solve different problems.

The end use case is the following: In application each module registers its own console commands as decoupled from one from another, a command of each module should be invoked in the correct context.

Could you please clarify what specific issue (other than global variables being evil) did you run into trying to implement this? For example, consider multiple "modules" providing console commands:

• nvs
• wifi
• system

Each module is decoupled from the other module, and each has its own set of commands. There is shared state (static variables) within each module, but the modules don't have shared state between themselves. I guess your use case is somewhat different, could you please explain in more detail?

(I am not debating whether having user-supplied context in the API is good or not; I would just like to understand the issue you are running to better.)

[alonbl]

Hello @igrr and @0xjakob,

Thank you for your feedback, I pushed an alternate version in which the context is per command.
Can you please review this one? it should answer most of your concerns and be usable for OOP like command model.

Implementation note: I did not want to add the context to esp_console_cmd_t because it usually lives in static context.

Regards,
Alon

[igrr]

I think for unit test purposes, you don't need to start the whole REPL. You can call esp_console_run and verify that the expected callback got called with the expected context. (Either by setting some variable from the callback, or by temporarily substituting stdout using open_memstream and then asserting that the expected string was printed by the callback.)

In this case, the test doesn't need the [ignore] tag and will be run automatically in CI.

[alonbl]

Done, I think.

[0xjakob]

@alonbl The current approach looks good. Thanks for adding the unit test! We'll handle this and after going through internal CI and reviewing, it will appear on master if there are no concerns. We'll keep you updated.

[0xjakob]

sha=867f9d0dbbe130f4ea46bdf87a0b28d1cbcd3c91

[0xjakob]

@alonbl We've merged the code to our internal code base, from where it will be synced to the master branch soon. The code looks slightly different now as we adjusted the naming a bit, but your authorship has been retained, of course.

[0xjakob]

@alonbl The change has been merged to master. However, we'll change the API once more so that the context can be set with esp_console_cmd_set_context() esp_console_cmd_register() instead of having a separate setting function. The new version should open less room for errors.

[alonbl]

@alonbl The change has been merged to master. However, we'll change the API once more so that the context can be set with esp_console_cmd_set_context() instead of having a separate setting function. The new version should open less room for errors.

Hi @0xjakob,
Thank you for your great work!
I do not understand the esp_console_cmd_set_context() is now used to set the context what is the change?
Regards,

[0xjakob]

@alonbl Sorry, it's a typo. I edited my post. It means that esp_console_cmd_register() now sets the context, based on the arugments in const esp_console_cmd_t *cmd, given to that function.

[0xjakob]

All the relevant code is merged. Hence, we're closing this PR (to mark it as finished).