Add support for the USB CDC Serial JTAG console

[pjsg]

Fixes #3526.

• [x] This PR is for the dev branch rather than for the release branch.
• [x] This PR is compliant with the other contributing guidelines as well (if not, please describe why).
• [ ] I have thoroughly tested my contribution.
• [ ] The code changes are reflected in the documentation at docs/*.

If the USB CDC console is configured, then it appears as the default console for the Lua REPL. It also appears as UART 0 and bumps the other uart(s) down one. The baud rate cannot be configured as there is no actual serial line.

When configured this way, stdin and stdout are connected to this device. Unfortunately, async reading is not possible at the moment. Thus the input side is handled by doing blocking reads in a seperate thread, and then posting the data into the Lua environment. Happily this is very similar to how the uart code works.

[tomsci]

In case anyone is interested I've rebased this onto latest dev-esp32 on https://github.com/tomsci/nodemcu-firmware/commits/tomsci-dev-esp32-idf4 along with support for the (non-jtag) S2 USB CDC port. Although it doesn't currently work if you have SPI RAM enabled (on the esp32s2) so I need to look into that some more.

[jmattsson]

I'm wondering if we need to completely change our approach to the Lua input. With the variation of consoles that the ESP32 series provide now, it makes me think we should switch to using plain old posix stdin for the Lua purposes. We'd have to somehow work out how to make that play nice with the uart module though... There are definitely some pesky legacy assumptions built into our API from the 8266 days.

[jonsmirl]

Check this out, https://github.com/jonsmirl/nodemcu-firmware/commit/80c76ec721086644a09973e51abb699c3a93fefd

That commit shows how to use ESP IDF linenoise() to implement a console. ESP IDF linenoise() works for every console type (serial, CDC, JTAG) and you control it via menuconfig. Using linenoise() removes the console support from the generic UART code.

I don't know exactly how to set the Lua prompt. Needs something like this

const char *prompt = get_prompt(L, firstline);
//use prompt
lua_pop(L, 1);  /* remove prompt */

[jonsmirl]

The posix stdin approach should work on ESP IDF except O_NONBLOCK support is broken (I am working with them to fix it). Until they fix it using linenoise() is a better solution. Plus linenoise has history and editing support.

[jonsmirl]

This is how I modified standard lua before moving to nodemcu.

/*
** Prompt the user, read a line, and push it into the Lua stack.
*/
static int pushline (lua_State *L, int firstline) {
  size_t l;
  const char *prmt = get_prompt(L, firstline);
  char *b = linenoise(prmt);
  if (b == NULL)
    return 0;  /* no input (prompt will be popped by caller) */
  /* Add the command to the history */
  linenoiseHistoryAdd(b);
  lua_pop(L, 1);  /* remove prompt */
  l = strlen(b);
  if (l > 0 && b[l-1] == '\n')  /* line ends with newline? */
    b[--l] = '\0';  /* remove it */
  if (firstline && b[0] == '=')  /* for compatibility with 5.2, ... */
    lua_pushfstring(L, "return %s", b + 1);  /* change '=' to 'return' */
  else
    lua_pushlstring(L, b, l);
  linenoiseFree(b);
  return 1;
}

[jmattsson]

@jonsmirl I like this approach!

I think our main challenge then is going to be how we change the deal with uart.on()'s last argument, which is how we to date have controlled the linking between the UART console and the Lua REPL. Do we make a breaking change and simply remove it? Or do we pause/resume the linenoise feeding task, even if such a thing really does not belong in the uart module? Thoughts, anyone?

[jonsmirl]

What about going back to the original lua console code and use pushline() like I modified above? https://github.com/lua/lua/blob/master/lua.c#L504 Modifying pushline provides a very easy and unobtrusive ESP console solution.

I've only been using NodeMCU for two days so I have no experience with legacy issues. I do know that all of the new Espressif CPUs have the integrated JTAG/console so that is definitely something you will want to support.

Edit - I see now, that will block lua and you don't want to do that.

[jonsmirl]

If you use linenoise() in a separate task, I don't know how to communicate the correct prompt to it.

[jmattsson]

Looking at the linenoise stuff now, I'm not sure that would work for us as-is, since we currently have the requirement to be able to both receive the console data in a uart.on() callback handler, as well as getting into the LVM. If we simply used linenoise, I think we'll simply be pulling the rug from under linenoise when we configure said UART. I believe it's already the case that /dev/uart/X do not work because the VFS would be competing with our reading from the UART buffers.

Looking at what Espressif have done themselves in their console component, they have custom cased it for UART/JTAG/CDC, so maybe @tomsci 's patch is the way to go despite the #ifdefs that I'm not fond of?

It's been ages since I had to work in this area of the code. I think our headaches can be summarised as:

• We currently don't support jtag/cdc consoles
• For legacy reasons, the uart module makes assumptions that one of the uarts is the console. We probably need to break this assumption, since it is not true any more.
• For legacy reasons we support both receiving uart data in a user callback, as well as feeding that same data into the LVM. Arguably we could say that going forward it's up to the user to feed said data to node.input() if they want it processed by the LVM as well, but that would be a breaking change.
• We need better receive performance (think 1MBIT uart links) than doing single-byte reads, so simply layering the uart.on() callback on top of e.g. linenoise or VFS is not an option.

• Ideally we don't want to have to keep updating things whenever Espressif adds a new type of console.

  The "easiest" solution given this would be, in my view, to:

  • Declare that consoles are not necessarily UARTs
  • Move the LVM input handling over to use either linenoise or stdin directly
  • When the console is a UART, prevent the uart module from stealing it (i.e. raise an error from uart.start(), and not unconditionally start it in user_main()). Considering the IDF's support for a secondary output console these days, we should probably prevent the uart module from touching that one too, if it's configured.
  • If necessary, introduce a new console module that would provide a console.on() to allow a user to capture console input.

That solution would however lose us some existing functionality, namely the ability to stop/start the console itself (unless we make the hypothetical console module support that). Has anyone ever used that feature? It sounds of questionable use. The other loss would be the ability to install a uart.on() callback handler on the console. This functionality might be of use if there is multiplexed data arriving on the uart, and it has to switch between modes. That said, not even I have abused the UART in such a manner (that I recall). And again, it could be addressed with a hypothetical console module, but it would be a breaking change.

On the other hand, this approach would gain us chip-side line editing support if we're using linenoise. Plus we wouldn't have to maintain a heap of #ifdefd input code that really should just be happy with getting the bytes from whatever stdin is set up by the IDF as. Aligning ourselves better to the IDF way of doing things means less work for us, and less fighting against the IDF in the first place. Long gone are the days when I had to implement my own UART ISR in order to implement the Lua console.

We'd be neutral on the /dev/uart/X issues, in that a user opening the file matching the console for reading, it would still compete for bytes with the linenoise task. I think it's reasonable to leave that as a user-beware thing.

@pjsg Thoughts? You've spent more time in this area recently than I have. I can put in some work towards resolving this, but I don't want to do it without some discussion & agreement first.

[jmattsson]

Ah, linenoise only supports a "pull" API, which means it's not compatible with the "push" / async architecture we have and can't be used as a replacement for our feed_lua_input(). I'll see how I go with hitting stdin/stdout directly instead.

[jmattsson]

I've put together a quick proof of concept here. I don't have an S2 board with direct-to-USB, and the S3 CDC-ACM refused to build, so I haven't been able to test the CDC ACM one, but in theory it should work. Both the UART and USB-SER-JTAG ones work, though there seems to be a flush issue with the prompt. Haven't investigated yet.

If anyone has recommendations for a cheap S2 dev board that brings out the CDC-ACM, that'd be great :)

[jonsmirl]

fcntl(fileno(stdin), F_SETFL, 0); is broken in the ESP IDF. fcntl(fileno(stdin), F_SETFL, O_NONBLOCK); should set non-blocking and 0 turn it off. If you dig around you will see that it powers on in blocking mode, then the ESP console code sets it to non-block via O_NONBLOCK. Once you set O_NONBLOCK you can't turn it off again because the API is broken. So that call to 0 should be putting it back into blocking mode, but it doesn't work. I've sent them the details of this a couple of weeks ago and they are looking at it. The way the current stdin ESP code works stdin is always non-blocking and you can't do anything about it.

When I started on this I was using the standard lua and modified pushline() which needs to block. The stdin approach did not work because I could not set it to block. That pushed me into linenoise. nodemcu is the opposite, it does not want to block so the stdin approach should be used.

[jmattsson]

I'm not seeing that here. My new console task is happily doing a blocking read(fileno(stdin),...) in its loop. There are no EWOULDBLOCK returns, nor is the watchdog complaining about the task never going idle. Of course, I'm not using the console component, so nothing may have set O_NONBLOCK in the first place.

Do you have a github issue number for it? I searched the open esp-idf issues but couldn't find anything.

[jonsmirl]

I reported it directly to an Espressif employee who entered into their internal system. What you are seeing is consistent, it is loading the console component which breaks things. My issued asked them to fix the console component. I do think if you turn on O_NONBLOCK you won't be able to turn it off again.

The console component is messed up because it has only been partially ported onto posix calls. It still has things like this in it which ignore O_NONBLOCK. void esp_vfs_usb_serial_jtag_use_nonblocking(void) void esp_vfs_usb_serial_jtag_use_driver(void)

I also pointed out that if they fixed the various issues in the console component they would be able to implement signal() and catch ctrl-C. Just proceed with caution when using ESP console and blocking behavior, the implementation behavior is not consistent.

[jonsmirl]

They are in the process of rewriting all of this, if you look in esp-idf/master console code is very different than v5.2.

[jmattsson]

Yeah feels like there is a lot of gremlins in the stdio related code at the moment (see recent commit in my proof-of-concept).

[jonsmirl]

@jmattsson I tried your console code from dius on my S3 system.

First problem I don't get any echo as I type, but after I hit return it echos. probably something with fflush or buffering. I also have to hit return before I can see the first prompt. Edit: read(fileno(stdin), linebuf, sizeof(linebuf)); is buffering the line.

Second problem, I had to set the stack up to 4K to keep from overflowing. I moved the stack into PSRAM. This is likely something else in my system, is there some what to tell what is causing it to use so much stack? Edit: 2KB seems to be ok, 1KB is not enough on the S3.

[jonsmirl]

I got echo working by adding fsync(fileno(stdout)); at the bottom of feed_lua_input().

That is what I meant by saying the ESP posix console code works inconsistently. setvbuf(stdout, NULL, _IONBF, 0); should have turned off buffering.

[jmattsson]

Yeah it feels like they haven't gotten their abstractions quite right yet. I added that fsync() to the console task, and upped the console task stack as requested.

[jonsmirl]

Where does it print the very first prompt? That needs a fsync(fileno(stdout)); right after it too.

[jmattsson]

Which Lua version? In 5.3 the first prompt should be the same as any other prompt. We start off by injecting a newline into the LVM to trigger the prompt print. See components/lua/lua-5.3/lua.c, at the bottom of the dojob() function for the print, early in pmain() for the newline injection.

[jonsmirl]

I had to set the read buffer down to one char. Obviously setvbuf(stdin, NULL, _IONBF, 0); setvbuf(stdout, NULL, _IONBF, 0); is not working on the USB JTAG serial. I read around in the IDF source for a while and didn't come up with an obvious way to turn it off.

static void console_task(void *)
{
  char linebuf[64];

  for (;;)
  {
    ssize_t n = read(fileno(stdin), linebuf, 1);
    if (n > 0)
    {
      // If we want to honor run_input, we'd need to check the return val
      feed_lua_input(linebuf, n);
      fsync(fileno(stdout));
    }
  }
}

[jonsmirl]

With read buffer at one char everything appears to work except the very first prompt doesn't appear until you type something.

Maybe you can spot a way to turn off buffering on the jtag serial? The _IONBF is CRT buffering, I think the problem is buffering in the driver itself.

[jonsmirl]

We have to use read of one char, IDF is just broken. if you read 100 bytes you end up here: https://github.com/espressif/esp-idf/blob/release/v5.2/components/vfs/vfs_usb_serial_jtag.c#L189 and usb_serial_jtag_read() is not going to return until it has read 100 bytes or a CR. usb_serial_jtag_read() just doesn't implement unbuffered reads.

[jmattsson]

🤦‍♀️ I guess it's not even worth #ifdefing the buffer size for console input. Let me push a change to just do char-by-char reading then.

[jmattsson]

Since this approach doesn't seem to be entirely dead in the water, I spent some time to make it actually process the console input in the correct thread context. That should also have addressed the unexpected need for larger stack size in the console task, but yell if it's being an issue again.

Also fixed the prompt-not-flushed issue on ACM console. Interestingly, to get the prompt to show up I had to use fsync(), as fflush() did not do it. So many gremlins in this area 😠

[jmattsson]

Have also ordered an S2 mini board, so should hopefully be able to test the third console mode at some point.