[ESP Linenoise] new esp linenoise with multiconsole support (IDFGH-9126) #10526

chipweinberger · 2023-01-12T01:15:11Z

Related PR: #9998

Adds new esp_linenoise_x functions.

esp_linenoise_handle_t esp_linenoise_create();
char* esp_linenoise(...)
esp_err_t  esp_linenoise_probe(...)
esp_err_t  esp_linenoise_set_stdin(...)
esp_err_t  esp_linenoise_set_stdout(...)
esp_err_t  esp_linenoise_set_multiline(...)
esp_err_t  esp_linenoise_set_dumb_mode(...)
esp_err_t  esp_linenoise_is_dumb_mode(...)
esp_err_t  esp_linenoise_set_allow_empty(...)
esp_err_t  esp_linenoise_set_max_line_length(...)
esp_err_t  esp_linenoise_clear_screen(...)
esp_err_t  esp_linenoise_set_completions_callback(...)
esp_err_t  esp_linenoise_set_hints_callback(...)
esp_err_t  esp_linenoise_set_free_hints_callback(...)
esp_err_t  esp_linenoise_add_completion(...)
esp_err_t  esp_linenoise_history_add(...)
esp_err_t  esp_linenoise_history_set_max_length(...)
esp_err_t  esp_linenoise_history_save(...)
esp_err_t  esp_linenoise_history_load(...)
esp_err_t  esp_linenoise_history_free(...)
esp_err_t  esp_linenoise_free(...)
esp_err_t  esp_linenoise_destroy(...)

Side Comment: Trying to push this PR along. It is tested working. It would be great for an Espressif employee to do any necessary "finishing touches", any extra docs, desired code cleanup, small api changes, etc. It's difficult to merge large features as a 3rd party developer. That said, I've gotten the PR as close to mergable as I can.

Motivation:

Now that ESP32 supports multiple serial interfaces (UART, Usb CDC, Usb Serial / JTAG), we need the ability to open a console on each interface.
A single serial interface is not enough. Physical ports may sometimes be physically blocked, or sometimes required for alternate uses (i.e. usb serial -> usb host).

Features:

backwards compatible, no code changes required for original linenoise users
support multiple instances of Linenoise, in order to support multiple consoles
explicit stdin & stdout redirection support
support errno == EWOULDBLOCK, to (eventually) support console deinitialization. see: [Console] fix console deinit: add ability to unblock linenoise, to fix deinit deadlock (IDFGH-8529) #9983

Code Changes:

esp_linenoise.c / esp_linenoise.h - now contains the majority of the linenoise code
linenoise/linenoise.c / linenoise/linenoise.h - for compatibility, just wraps esp_linenoise.c
commands.c - support calling esp_console_run() on multiple independent threads.

Tested:

on ESP32-S3
basic console example
'edit' linenoise mode
uart

Future:

full error handling: the original linenoise implementation does not check every error, example: fputc

KaeLL · 2023-01-12T17:17:45Z

Completely unrelated question: why do you often create new PRs instead of updating the ones you already opened? Isn't tracking progress one of the features of PRs?

chipweinberger · 2023-01-12T19:03:10Z

Thanks for the question.

Its only if the old PR needs a huge rewrite.

There's a few reasons:

access. I want to easily access and refer to the old implementation, both in git and on github, so it's best for the new work to be on new branch and new PR.
clarity. Multiple PRs is easier to follow than 3 rewrites in the same PR with dozens of outdated comments.
staleness. Big PRs can become stale because this immense history becomes a burden. No one wants to look at dozens of comments, nor should they have to.

Of course you can still look at all the old comments - I always link to it. So, this gives me the best of both approaches IMO.

igrr

Thanks for the massive PR @chipweinberger!

I've taken the first look, the structure looks fine to me. To be honest, I have trouble reviewing esp_linenoise.c since I can't tell which code is the original one and which you have modified. I'll try to open both files side by side and do a more detailed review later.

A few other things:

Should we replace the usage of the old linenoise APIs in components/console/esp_console_repl.c with the new ones? Same for the examples?
Should the old APIs be marked as __attribute__((deprecated, "please use esp_linenoise_xxxx instead"))? If yes, this should also be mentioned under docs/en/migration-guides/release-5.x/5.1.
Do the new APIs even need to be called "linenoise"? The name is cool, but frankly is not self-explanatory. Perhaps esp_readline or esp_lineedit? Not sure about this, just an idea. Once we add a new public API, we can't change its name easily.
Now that the functions don't use a global context, seems like we can unit test them quite easily? You definitely aren't expected to do this, but we would still need this before the PR can be merged internally.
Probably need to update the docs:
- add the new esp_linenoise.h header file to docs/doxygen/Doxyfile
- remove the mention of the old linenoise functions from docs/en/api-reference/system/console.rst
- add .. include-build-file:: inc/esp_linenoise.inc in the same file to add the generated API reference
Same as with the unit tests, you can leave the documentation changes to us, if you prefer.

Edit: I see I've missed this comment:

Trying to push this PR along. It is tested working. It would be great for an Espressif employee to do any necessary "finishing touches", any extra docs, desired code cleanup, small api changes, etc. It's difficult to merge large features as a 3rd party developer.

In that case, please ignore the "few other things" and the review nitpicks I've posted, we'll take over the PR. Thank you for the contribution!

igrr · 2023-01-19T03:21:16Z

components/console/commands.c

@@ -184,27 +169,32 @@ static const cmd_item_t *find_command_by_name(const char *name)

 esp_err_t esp_console_run(const char *cmdline, int *cmd_ret)
 {
-    if (s_tmp_line_buf == NULL) {
-        return ESP_ERR_INVALID_STATE;


Probably still need to return ESP_ERR_INVALID_STATE if esp_console_init wasn't called?

igrr · 2023-01-19T03:22:19Z

components/console/esp_console.h

@@ -12,9 +12,7 @@ extern "C" {
 #include <stddef.h>
 #include "sdkconfig.h"
 #include "esp_err.h"
-
-// Forward declaration. Definition in linenoise/linenoise.h.
-typedef struct linenoiseCompletions linenoiseCompletions;


Could we keep this as a forward declaration instead of pulling in the dependency on esp_linenoise.h header?

igrr · 2023-01-19T03:24:04Z

components/console/esp_console.h

 */
-void esp_console_get_completion(const char *buf, linenoiseCompletions *lc);
+void esp_console_get_completion(const char *buf, esp_linenoise_completions_t *lc);


Just to check: with this change, does the examples/system/console/advanced example from IDF v5.0 compile okay with this brach?

(I assume the answer is "yes" since you didn't have to modify the example. Is that due to that `#include "esp_linenoise.h" above which you have added?)

I've done my testing with the "basic" example. But I think you are correct.

igrr · 2023-01-19T03:30:13Z

components/console/esp_linenoise.c

+ * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD
+ *
+ * SPDX-License-Identifier: Apache-2.0


To reflect the copyrights and the actual license below, suggest:

Suggested change

* SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD

*

* SPDX-License-Identifier: Apache-2.0

* SPDX-FileCopyrightText: 2010-2016 Salvatore Sanfilippo <antirez at gmail dot com>

* SPDX-FileCopyrightText: 2010-2013 Pieter Noordhuis <pcnoordhuis at gmail dot com>

* SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD

*

* SPDX-License-Identifier: BSD-2-Clause

igrr · 2023-01-19T03:31:52Z

components/console/esp_linenoise.c

+    .state = {0} \
+}
+
+struct esp_linenoise_t stdio = LINENOISE_DEFAULT;


is it possible to avoid overriding the standard stdio name?

(also, can this variable be static, and prefixed with s_?)

Yes good point. I should have noticed this.

igrr · 2023-01-19T03:35:34Z

components/console/esp_linenoise.c

+    struct esp_linenoise_t def = LINENOISE_DEFAULT;
+    esp_linenoise_handle_t handle = (esp_linenoise_handle_t) malloc(sizeof(struct esp_linenoise_t));
+    memcpy(handle, &def, sizeof(struct esp_linenoise_t));


Suggested change

struct esp_linenoise_t def = LINENOISE_DEFAULT;

esp_linenoise_handle_t handle = (esp_linenoise_handle_t) malloc(sizeof(struct esp_linenoise_t));

memcpy(handle, &def, sizeof(struct esp_linenoise_t));

esp_linenoise_handle_t handle = (esp_linenoise_handle_t) malloc(sizeof(struct esp_linenoise_t));

if (handle == NULL) {

return NULL;

}

*handle = (esp_linenoise_t) LINENOISE_DEFAULT;

igrr · 2023-01-19T03:36:32Z

components/console/esp_linenoise.c

+    return handle;
+}
+
+char *esp_linenoise(esp_linenoise_handle_t handle, const char *prompt)


Suggested change

char *esp_linenoise(esp_linenoise_handle_t handle, const char *prompt)

char *esp_linenoise_get_line(esp_linenoise_handle_t handle, const char *prompt)

or something like this — we don't have to keep the names 1:1 with the original version, and can make them more self-explanatory.

igrr · 2023-01-19T03:39:23Z

components/console/esp_linenoise.h

+ * 
+ * @returns the console command the user entered, or NULL if failure.
+ */
+char* esp_linenoise(esp_linenoise_handle_t handle, const char *prompt);


All other functions return esp_err_t, perhaps we could apply the same treatment to this one?

Suggested change

char* esp_linenoise(esp_linenoise_handle_t handle, const char *prompt);

esp_err_t esp_linenoise_get_line(esp_linenoise_handle_t handle, const char *prompt, char** out_line);

Yes I considered that strongly as well. I would be in favor of that.

Preferably have the user pass the buffer. We would need to handle the linenoise wrapper differently - but that shouldn't be too hard tho. Just have the wrapper do the malloc.

esp_err_t esp_linenoise_get_line(esp_linenoise_handle_t handle, const char *prompt, char* out_line, size_t len);

igrr · 2023-01-19T03:39:37Z

components/console/esp_linenoise.h

+ * 
+ * @returns A new linenoise handle.
+ */
+esp_linenoise_handle_t esp_linenoise_create();


Suggested change

esp_linenoise_handle_t esp_linenoise_create();

esp_linenoise_handle_t esp_linenoise_create(void);

igrr · 2023-01-19T03:40:20Z

components/console/esp_linenoise.h

+
+
+/**
+ * @brief Linenoise does not necessarily allocate with malloc. Any memory returned by


This maybe was true for the original library, is this still necessary?

I had the same thought. Ideally we should eliminate returning memory that we allocate ourself, IMO.

esp_err_t esp_linenoise_get_line(esp_linenoise_handle_t handle, const char *prompt, char* out_line, size_t len);

chipweinberger · 2023-01-19T04:07:39Z

To be honest, I have trouble reviewing esp_linenoise.c since I can't tell which code is the original one and which you have modified.

Yes, let me push a new change that splits this into 2 commits, so that there is a diff. edit: nvm, seems you are okay taking this over!

Should we replace the usage of the old linenoise APIs in components/console/esp_console_repl.c with the new ones? Same for the examples?

Eventually! That's a lot for single PR!

Should the old APIs be marked as attribute((deprecated, "please use esp_linenoise_xxxx instead"))? If yes, this should also be mentioned under docs/en/migration-guides/release-5.x/5.1.

My vote would be to deprecate it in 5.2. More specifically, I don't have time to do the above ^^ in the 5.1 release window, but perhaps Espressif has the time!

Do the new APIs even need to be called "linenoise"? The name is cool, but frankly is not self-explanatory. Perhaps esp_readline or esp_lineedit? Not sure about this, just an idea. Once we add a new public API, we can't change its name easily.

Hmm! My vote is keep the linenoise name. I agree it's not clear, but there is documentation value in knowing where the code is based from. That said, I like the esp_lineeditproposal as well. Edit: I like lineedit better.

Now that the functions don't use a global context, seems like we can unit test them quite easily? You definitely aren't expected to do this, but we would still need this before the PR can be merged internally.

Could they use the '0' handle? I have this written so that handle '0' uses a global context. That might be controversial, but it makes the linenoise wrappers very easy to write.

CLAassistant · 2023-04-20T06:35:56Z

All committers have signed the CLA.

chipweinberger · 2024-01-22T17:31:49Z

bump

mickeyl · 2024-03-25T17:52:55Z

This looks great, I'd really like this to land in preparation of better console support. Good work, @chipweinberger!

cmorganBE · 2024-06-07T16:45:32Z

Also asking for a bump on this PR. I'd like to implement a second console on my system, with separate commands (as its used for a separate kind of user), and this is a first step towards making that easier.

chipweinberger force-pushed the user/chip/esp-linenoise branch from bb74e1e to 57742bc Compare January 12, 2023 01:30

espressif-bot added the Status: Opened Issue is new label Jan 12, 2023

github-actions bot changed the title ~~[ESP Linenoise] new esp version of linenoise with multiconsole support~~ [ESP Linenoise] new esp version of linenoise with multiconsole support (IDFGH-9126) Jan 12, 2023

chipweinberger force-pushed the user/chip/esp-linenoise branch from 57742bc to 785d059 Compare January 12, 2023 01:42

chipweinberger mentioned this pull request Jan 12, 2023

[Linenoise 2] multi-console support & support for replacing stdin/stdout (IDFGH-8546) #9998

Closed

chipweinberger changed the title ~~[ESP Linenoise] new esp version of linenoise with multiconsole support (IDFGH-9126)~~ [ESP Linenoise] new esp linenoise with multiconsole support (IDFGH-9126) Jan 12, 2023

[ESP Linenoise] new esp version of linenoise with multiconsole support

1776d09

chipweinberger force-pushed the user/chip/esp-linenoise branch 7 times, most recently from 3128014 to 2e24c0c Compare January 12, 2023 04:42

[ESP Linenoise] use more espressif style formatting

cb3a3ef

chipweinberger force-pushed the user/chip/esp-linenoise branch from 2e24c0c to cb3a3ef Compare January 12, 2023 05:04

igrr reviewed Jan 19, 2023

View reviewed changes

espressif-bot added Status: Selected for Development Issue is selected for development and removed Status: Opened Issue is new labels Jan 19, 2023

suda-morris requested review from suda-morris and o-marshmallow January 19, 2023 05:28

espressif-bot assigned SoucheSouche Mar 29, 2023

espressif-bot assigned zikalino and unassigned SoucheSouche May 5, 2023

chipweinberger mentioned this pull request Jun 9, 2023

[Console] 🚀🚀 Proposal: Console Needs More Flexibility 🚀🚀 (IDFGH-8532) #9986

Open

chipweinberger mentioned this pull request Aug 20, 2023

esp_console_set_vprintf feature request (IDFGH-10885) #12087

Closed

alonbl mentioned this pull request Nov 9, 2023

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

Closed

espressif-bot unassigned zikalino Jan 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ESP Linenoise] new esp linenoise with multiconsole support (IDFGH-9126) #10526

[ESP Linenoise] new esp linenoise with multiconsole support (IDFGH-9126) #10526

chipweinberger commented Jan 12, 2023 •

edited

Loading

KaeLL commented Jan 12, 2023

chipweinberger commented Jan 12, 2023 •

edited

Loading

igrr left a comment •

edited

Loading

igrr Jan 19, 2023

igrr Jan 19, 2023

igrr Jan 19, 2023

chipweinberger Jan 19, 2023

igrr Jan 19, 2023

igrr Jan 19, 2023

chipweinberger Jan 19, 2023

igrr Jan 19, 2023

igrr Jan 19, 2023

igrr Jan 19, 2023

chipweinberger Jan 19, 2023

chipweinberger Jan 19, 2023 •

edited

Loading

igrr Jan 19, 2023

igrr Jan 19, 2023

chipweinberger Jan 19, 2023

chipweinberger commented Jan 19, 2023 •

edited

Loading

CLAassistant commented Apr 20, 2023 •

edited

Loading

chipweinberger commented Jan 22, 2024

mickeyl commented Mar 25, 2024

cmorganBE commented Jun 7, 2024

	char esp_linenoise(esp_linenoise_handle_t handle, const char prompt)
	char esp_linenoise_get_line(esp_linenoise_handle_t handle, const char prompt)

	char* esp_linenoise(esp_linenoise_handle_t handle, const char *prompt);
	esp_err_t esp_linenoise_get_line(esp_linenoise_handle_t handle, const char prompt, char* out_line);

	esp_linenoise_handle_t esp_linenoise_create();
	esp_linenoise_handle_t esp_linenoise_create(void);



		/**
		* @brief Linenoise does not necessarily allocate with malloc. Any memory returned by

[ESP Linenoise] new esp linenoise with multiconsole support (IDFGH-9126) #10526

Are you sure you want to change the base?

[ESP Linenoise] new esp linenoise with multiconsole support (IDFGH-9126) #10526

Conversation

chipweinberger commented Jan 12, 2023 • edited Loading

KaeLL commented Jan 12, 2023

chipweinberger commented Jan 12, 2023 • edited Loading

igrr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipweinberger Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipweinberger commented Jan 19, 2023 • edited Loading

CLAassistant commented Apr 20, 2023 • edited Loading

chipweinberger commented Jan 22, 2024

mickeyl commented Mar 25, 2024

cmorganBE commented Jun 7, 2024

chipweinberger commented Jan 12, 2023 •

edited

Loading

chipweinberger commented Jan 12, 2023 •

edited

Loading

igrr left a comment •

edited

Loading

chipweinberger Jan 19, 2023 •

edited

Loading

chipweinberger commented Jan 19, 2023 •

edited

Loading

CLAassistant commented Apr 20, 2023 •

edited

Loading