add BlockingIoDriver to make stdin/out/err calls blocking

[Vollbrecht]

No description provided.

[Vollbrecht]

This PR is not implementing the usb_serial driver for raw use. That can be done separately. It just adds some skeleton peripheral and only is concerned about the stdio stuff.

[ivmarkov]

@Vollbrecht

I've avoided the VFS stdout/in/err redirection topic for a long time, because it is complex.

However, if we are to introduce some control w.r.t. blocking vs non-blocking IO, I think we need to summarize - in English:
(a) How the VFS stdout/in/err redirection to a peripheral actually works
(b) How exactly we'll model it in Rust

I've tried to do (a) below and after that, validate your approach (b) against it. Please read on and if you spot anything incorrect, comment so that we can arrive at the status quo we both agree on.

(a) How VFS stdout/in/err works

Invariant 1 - configuration of stdin/out/err initialization (very important)

Whether UART0/1, or JTAG or USB-CDC will be used for the primary console output (and ditto for the secondary / "dev" console output) is controlled at compile-time ONLY using CONFIG_* kconfig settings.

• I.e. while you can change - at runtime - whether the IO would be blocking or non-blocking, you cannot change at runtime the type of the peripheral to which the console is directed (i.e. change from UART to JTAG or the other way around)
• (You can sort of do a similar change by closing the stdout/in/err streams and re-opening (overriding) those with different file descriptors (actually FILE* C structures) but this works above the VFS layer, and if you do this, the VFS layer would still be having the UART0 (or JTAG or whatever) peripheral you've configured with CONFIG_* settings) "grabbed" and used and registered in the VFS layer, and thus should be unavailable to the user in the Peripherals struct (more on that later))
• While you can "mess" with some ESP-IDF public functions at runtime by calling these unsafely, they won't have the desired effect and will just make the situation worse, not better. For example, you can call again esp_vfs_console_register and it will override the primary_vfs redirection, but would override it still according to the CONFIG_* settings, and will not free the previous primary_vfs.

Invariant 2 - VFS stdin/out/err initialization

Because the stdin/out/err should operate from very early in the program startup, the VFS stdin/out/err initialization is done implicitly, before the execution hits app_main (and thus Rust) code

Invariant 3 - VFS stdin/out/err deinitialization

De-initialization of stdin/out/err VFS redirection never happens. I.e. the peripheral to which the standard streams are directed is grabbed inside VFS from before program startup, and remains grabbed forever throughout the lifecycle of the program.

Invariant 4 - Blocking vs nonblocking IO

• VFS always starts in nonblocking mode (the naming of this thing is a misnomer, as output is actually blocking; input is nonblocking)
• Mode can be changed at runtime from nonblocking to blocking and the other way around

Conclusions

What follows from the first three invariants, is that a certain peripheral - or even two peripherals, if the secondary "dev" console is enabled - (be it UART0, or UART1 or JTAG or USB-CDC) is simply not available during the lifetime of the program, because it is "grabbed" in the VFS layer and stays there forever - possibly with some pins as well - at least for the UART case? Not sure how JTAG and CDC work w.r.t. grabbing of specific pins...

This is to our advantage though, as what we can do is conditionalize the availability of the UART0, UART1, JTAG and (in future) USBCDC peripherals in the Peripherals struct based on CONFIG_* settings i.e. not allow the user to use UART0 if it is grabbed - at compile time - by the VFS layer (and the VFS layer itself is enabled in that the VFS component is used).

(b) How exactly we'll model it in Rust

Peripherals availability

• (Now) You are correct that we DO need to model all peripherals to which the stdout/err/in can be directed. So yes, we do need usb serial/jtag peripheral as part of this PR - not for anything else, but to mark it as "unavailable" in the Peripherals struct if it is configured as a primary or secondary (dev) console target with CONFIG_* settings
  • (Future) By the way, having a JtagDriver on top of the JTAG peripheral would be very cool in future, as the user might this way utilize the Jtag peripheral for sending receiving stuff via USB (that is, in case JTAG is not "grabbed" by the VFS layer for the duration of the program) - just like it is possible today for the UartDriver
    • (Future) We can also implement - in esp_idf_svc::io::vfs a way to mount a JtagDriver instance (or the raw Jtag peripheral - it is the same) in the VFS file-tree under a "file" (a character device really) - so this way the user can use regular std::io APIs to send/receive data via Jtag
    • (Future) We probably need to expose a mounting capability for the existing UartDriver (or the raw UART* peripherals) in the VFS layer, as this is also currently supported
• (Now) We need to conditionalize the same way the availability of UART0 and UART1 in the Peripherals struct - i.e. make them unavailable there if they are used for the VFS console
• (Future) In future, we need to conditionalize the used UART pins (very annoying I agree) as well as the potential usage of a future USB-CDC peripheral, for the s2 (and s3, I think)

Switching between blocking and nonblocking IO for the configured stdin/out/err peripheral

From Invariant 4 it follows, that modeling the blocking mode can indeed be done with a "driver" or "state" or something - a token the user initializes and by the very initialization of the token, the mode of operation switches from nonblocking to blocking. Once the token is dropped, the operation switched back from blocking to nonblocking.

The advantage of this approach is that it is symmetric with how "mounting" is currently modeled in esp_idf_svc::io::vfs for the EventFs, FatFs and Spiffs drivers - you hold on to these tokens ("mounts") for the duration of while you want these to be available. And by the way, it would be good if this logic is moved to esp_idf_svc::io::vfs rather than sitting in esp_idf_hal::io, as it is part of the VFS layer, which currently lives in esp_idf_svc, not in the hal.

The disadvantage is that there is nothing stopping users from calling this token twice. Your idea of passing the jtag peripheral (or UART0/1 peripheral) to the constructor of the token (= BlockingIoDriver?) is not ideal, because - due to Invariants 1, 2 and 3, this peripheral should not be available in the Peripherals struct in the first place - as it is already "taken" by the VFS layer for the duration of the program.

While the above is not the end of the world (trying to instantiate another BlockingIoDriver might just fail with a runtime error) it asks the question whether we should model it this way in the first place? Perhaps we can just implement a static function - esp_idf_svc::io::vfs::set_stdio_mode(mode: StdIoMode) that switches from blocking to non-blocking mode and vice versa? This way users don't need to hold-on to anything. Again, the key difference with - say - SDMMC is that while SDMMC starts always with the peripheral available in the Peripherals struct, and then the user instantiates a driver on it and then a FatFS filesystem on top of it and finally mounts the FatFS filesystem in the VFS layer, with VFS stdin/out/err redirection, the program starts with all of that already done, and done forever - we can only control blocking vs nonblocking, so making this in a way that it resembles the "mount token" code might actually be misleading.

===========

I'll now follow up with some inline comments in the PR, along the spirit of the above ^^^.

[ivmarkov]

Let's move all of this code to esp_idf_svc::io::vfs.

[Vollbrecht]

@ivmarkov Thanks for the feedback!

Making it all compile time might be a good idea. A couple of things that are still open questions on that specific point. If we introduce this we should probably also gate the default gpio pins to not be available for whatever combination currently is selected.

I will start here with an overview of how different configs will effect the availability of peripherals / gpios, and what it means for a "BlockingIoDriver" availability.

Configuration Space

Demonstrated on esp32c3 for simplicity.

• The default:

  • console_slot_1 => "esp_idf_esp_console_uart" cfg is set => gpio20,21 not available => uart0 not available
  • console_slote_2 => "esp_idf_esp_console_secondary_usb_serial_jtag" => gpio 18,19 not available => usb_serial not available

  In this configuration BlockingIoDriver would be using uart0.

• User set CONFIG_ESP_CONSOLE_USB_SERIAL_JTAG=y
  This will automatically change the following behind the scene by idf build system.

  • console_slot_1 => "esp_idf_esp_console_usb_serial_jtag" => gpio 18,19 not available => usb_serial not available
  • console_slot_2 => "esp_idf_esp_console_secondary_none"

  In this configuration BlockingIoDriver would be using usb_serial.

  uart0 and its gpio is now free.

• User set's CONFIG_ESP_CONSOLE_NONE=y
  The build system do the following. Notice that in this configuration usb_serial is still used as an output on slot 2 !

  • console_slot_1 => "esp_idf_esp_console_none" => all uarts are now available => all uart pins are free
  • console_slot_2 => "esp_idf_esp_console_secondary_usb_serial_jtag" => gpio 18,19 not available => usb_serial not available

  In this configuration BlockingIoDriver is not available since it must be in slot_1 to work!

• User sets both CONFIG_ESP_CONSOLE_NONE=y & CONFIG_ESP_CONSOLE_SECONDARY_NONE=y
  both usb_serial and all uarts are free to use and all pins are free

  In this configuration BlockingIoDriver is not available since nothing is in slot_1.

• User sets CONFIG_ESP_CONSOLE_UART_CUSTOM=y or CONFIG_ESP_CONSOLE_UART_CUSTOM_NUM_1=y

  • console_slot_1 => "esp_idf_esp_console_uart_custom" => if num_0 its not available and same for num_1 => making gpio's unavailable in this mode is much harder because of two reasons:
    • we have to potential gate every possible gpio - so we would need to either introduce a macro that annotates every gpio or a lot of manual work
    • i think we currently filter out CONFIG_ESP_CONSOLE_UART_TX_GPIO at the build system level so we don't get a rust cfg so we can't now what the user sets the custom gpio pins to.

  With this problems here i would say we should go the pragmatic route and if a user sets CONFIG_ESP_CONSOLE_UART_CUSTOM and uses non-default gpio pins that he needs to be aware of it. Since its a lot of work to handle this case.

  • console_slot_2 => "esp_idf_esp_console_secondary_usb_serial_jtag" => gpio 18,19 not available

  In this configuration BlockingIoDriver is using either uart0 or uart 1 depending on num_0 / num_1

Now additional cases for usb-otg devices like a esp32s2:

• console_slot_2 is never available, e.g its only a feature with usb_serial_jtag devices

• User sets CONFIG_ESP_CONSOLE_USB_CDC=y

  • console_slote_1 => "esp_idf_esp_console_usb_cdc" => gpio 19/20 are used - all uarts are free.

  In this configuration BlockingIoDriver is not available since there doesn't exist a driver for it afaik.

• User uses USB-STACK in any other form have to do the work at runtime. For example a user is setting CONFIG_TINYUSB_CDC_ENABLED=y.
  A user can redirect the std io streams onto usb by calling "esp_tusb_init_console()" but they don't mention a) if its than blocking or not b) they seam to ignore the complete console_slot thing.

  In this configuration BlockingIoDriver is also not available, since its not clear if the api is than blocking or not using tinyusb.

Difference between usb_cdc in otg devices vs usb_cdc in usb_serial_jtag devices.

First of all it would be factual wrong to rename a usb_serial into jtag. The usb_serial_jtag controller is a USB Composite device witch means it consists of independent hardware that consist of a cdc_acm hardware interface and a jtag_command_processor. When we are reading and writing over usb_cdc we are not using the jtag_command_processor in any form physically. There are independent registers internally and writing and reading from them are independent.

This opens the possibility of two different drivers

• usb_cdc driver: esp-idf refers to this as the the esp_driver_usb_serial_jtag driver. A little bit of an unfortunate name as it still refers to both in its name.
• communication through the jtag interface to the host. This is typically done via semihosting calls that expose a similar println style interface and is also used via the tracing module. This is done internally via an rtt buffer. This will actively send JTAG frames over the internal jtag bus to the internal jtag_command_processor (or an external jtag probe), and then forward them over the registers(independent of usb_cdc) to the USB HOST PC.

Because of this i want to make it clear that the BlockingIoDriver is using the usb_cdc hardware interface here. If you don't like usb_serial as a name i am ok with renaming it into into usb_cdc or something but i am not ok naming it JTAG as its misleading.

Now when we are talking about usb_cdc on esp's usb-otg devices. Its basically a software solution ontop of the usb_transceiver. In other words its not a zero setup hardware solution but needs a complex usb setup chain. It also is mutual exusive with other usb scenarios on the device. E.g as soon as tinyusb stack is used this option is unavailable.

What does this mean for a potential usb_serial / usb_cdc peripheral in our device tree?

• a) two different peripheral variants to represent them. On usb_serial_jtag devices we have a usb_cdc peripheral and on usb_otg we have a usb_otg device or something.
• b) have one usb_cdc device that represents both.

I would for now go with option a) since we don't have any wrappers for tinyusb / usb-otg and at this point we have enough of cfg that we currently have to take care off ;D

addressing other open points from feedback

• move stuff to esp_idf_svc::io::vfs.
  That's fine with me
• generic version of BlockingIoDriver:
  I did it mainly to get something started and it was needed because we had two different drop impels. This will go away all with a compile time cfg based solution.
• mutual exlusion of BlockingIoDriver:
  Not a problem with a compile time based solution. BlockingIoDriver is dependent on what is set in esp_console_slot1 and will otherwise not be available.
• hold a token vs free standing function:
  I think overall its nicer to objectify it a bit here, as it automatically handles the lifetime and droping the stuff. So nothing one can simply forget. Regardless we need to track internally if a user constructed it once, or make it possible to only create one instance of it. I will do it like with the other cases with a internal static or something,

TLDR:

• if we handle compile time peripherals we should also try to handle compile time gpios as much as its feasible currently.
• will impl a compile time version as outlined above.
• will ignore complexity of custom uart gpio's if user uses non_default uart pins in sdkconfig.
• will ignore usb-otg based software usb_cdc solutions for now.
• go with a single token a user creates to create the BlockingIoDriver and will revert to Unblocking if object is droped.

[ivmarkov]

@Vollbrecht
I'm browsing the VFS code all morning (particularly the UART and USB-SERAL/JTAG "VFS" "drivers") and it is even more complex than I thought.

Let's not rush the idea of marking e.g. UART0 and its pins as "grabbed" in VFS if the console is configured for UART. This might have benefits but also disadvantages.

I need more time to comprehend the C code I'm seeing.

[ivmarkov]

It seems the way how UART, JTAG and USB-CDC operate as "files" in the VFS layer is fundamentally incompatible with our notion of Peripherals and in partuclar, that a certain peripheral - at any point in time - is "owned" by just one piece of code.

This is not even related to the stdin/out/err streams, but is a machinery you can use on your own (and which stdin/err/out streams use as well).

Here's an example to make it clearer:
If I just call uart_vfs_dev_register - a defined, public UART API (which is also called by the ESP IDF bootstrapping machinery when you have selected UART as your primary and/or secondary console output), I get the following three "files" in the VFS:

• /dev/uart/0 -> This is UART0
• /dev/uart/1 -> This is UART1
• /dev/uart/2 -> This is UART2

Does that mean that ALL uarts are now "grabbed" by the VFS layer?
Yes and no, simultaneously (which is the problem!). Specifically:

• Just having these three "files" in the VFS is harmless. You can continue using UartDriver with any of the UART0/1/2 peripherals
• Even fopen-ing any of these three files (and then fclosing them) is completely harmless too. These ops are no-op for the UART driver (and I suspect but need to check - for jtag and usb-cdc too)

Where it becomes harmful, and where the peripheral becomes "grabbed" (but just for the duration of the operation) is if you call VFS write or read on these files -either in C (or in Rust - same story of course).
What happens then is that the VFS IO code reads/writes by (ab)using the UART registers. Or reads/writes by (ab)using the UART driver (if you have instructed it to do so). Writes/reads via the UART driver are possible, because - as it turns out - the UART driver is specially crafted in that it is static. So when you "install it" (or call UartDriver::new from Rust), the static structure corresponding to the UART driver gets filled-in (and is emptied on driver de-installation). And the VFS uart code just uses this static structure. And because it is static, it does have access to it of course (simplifying a bit but that's roughly how it works).

So in a way, doing VFS read/write on /dev/uartX borrows - but ONLY for the duration of the read/write operation - either the driver, or the registers of the UART peripheral. But only for the duration of the operation. And reads/writes with whatever configuration the UART peripheral is currently having.

I no longer believe we can express this pattern in a safe way in Rust.
Moreover, I don't think it is fair to declare ALL UART peripherals (or some pins) as "grabbed" by the VFS layer, if the user had called uart_vfs_dev_register. Not that this is even possible, as this is a runtime call. And then again - all of this is "below the surface" of to what stdin/stdout/stderr is redirected.

So what to do?

1. I think we need to back-off from declaring the UART peripherals (or the other two - jtag and usb-cdc, or pins) as grabbed based on CONF settings.
   This would anyway only work for this UARTX peripheral, which happens to be the one where stdin/out/err is redirected (currently) but not for the other UART peripherals. I.e. a simple write in /dev/uart/2 will mess it up if the UART3 peripheral is currently used by the user elsewhere, with the UartDriver and this has nothing to do with console redirection.

Users just have to be careful about the fact that "owning" a UARTX peripheral somewhere still leaves the /dev/uart/X backdoor in the VFS through which one can push/pull stuff via that same peripheral. whether that could lead to a crash I don't know, but is definitely breaking our peripheral ownership system, but alas, we have to live with that.

2. When we expose uart_vfs_dev_use_driver shall we do it by requiring the user to provide the UARTX peripheral to the API call (and possibly the pins)?

It becomes too complex and next to impossible to track I think. And after all, even if the user does not call uart_vfs_dev_use_driver, the relevant UART peripheral and its pins might already be (ab)used by the VFS code for outputting stuff via UART registers, so what gives?

And again please note that all of the above has NOTHING to do with stdin/out/err in fact! This is just how VFS works w.r.t. uart/jtag/usb-cdc!

3. Shall we even do something, or just tell users to unsafely call uart_vfs_dev_use_driver (and make sure they keep the corresponding UartDriver around)?

Let me think about this....

[Vollbrecht]

The primary two goal's of the API should be the following:

• Provide a more discoverable place compared to the shedful of different places a user needs to look into ESP-IDF docu to pice the things together. it should bring enough documentation such that users can simply start using it a blocking standard IO api.
• All this options around using uart or usb_cdc / jtag etc is confusing for new people i think. E.g picking the right options is harder than it needs to be. So unify it into a thing that does the job according to what the user is using under the hood probabbly helps people.

I think handling the availability of gpio's or peripherals is more of a secondary goal overall, and as you also pointed out its quite a complex undertaking.

So i would suggest to go with just a light wrapper around the different unsafe calls, and provide the necessary docu ontop of that wrapper, mention the correct sdkconfigs a user needs to set, while still making it a compile time solution like discussed ( to guarantee exclusiveness), but doesn't bother to take arguments with any peripherals or gpio's, and only internally make sure that it doesn't cant get called multiple times by an internal static?

[ivmarkov]

The primary two goal's of the API should be the following:

• Provide a more discoverable place compared to the shedful of different places a user needs to look into ESP-IDF docu to pice the things together. it should bring enough documentation such that users can simply start using it a blocking standard IO api.
• All this options around using uart or usb_cdc / jtag etc is confusing for new people i think. E.g picking the right options is harder than it needs to be. So unify it into a thing that does the job according to what the user is using under the hood probabbly helps people.

I think handling the availability of gpio's or peripherals is more of a secondary goal overall, and as you also pointed out its quite a complex undertaking.

I agree.

So i would suggest to go with just a light wrapper around the different unsafe calls, and provide the necessary docu ontop of that wrapper, mention the correct sdkconfigs a user needs to set, while still making it a compile time solution like discussed ( to guarantee exclusiveness), but doesn't bother to take arguments with any peripherals or gpio's, and only internally make sure that it doesn't cant get called multiple times by an internal static?

How about....:

In fact, your original approach might then be just good enough!

• While it does require the user to provide the peripheral (i.e. UARTX and/or USB-SERIAL) ... which is at the same time kind of "grabbed" already, it is at least a safe(r) way to indicate which of the dev/uart/X "files" we are referring to, which we want the system to switch to blocking mode.
• We won't be providing the driver, but just the peripheral, so that the system can continue using whatever baud settings and pins are currently provided for that peripheral
• We might - at a later step - implement - in the UART driver - an unsafe "configuration" utility (v.r.t. pins and baud rates) that configures a UART peripheral without installing the driver too

Let me re-review the comments to your changes in light of this.

[ivmarkov]

I've removed all my previous feedback and re-added a new one in light of everything discussed.

Other than moving the VFS portion to esp_idf_svc::io::vfs, simplifying BlockingUartIo and BlockingSerialIo, and - most importantly - fixing one lifetime-related bug I don't have anything else to add ATM.

[ivmarkov]

I'm not sure we need BlockingIoDriver. See below.

[ivmarkov]

Why not just

struct BlockingUartIo<'d> {
    uart: u8,
    _d: PhantomData<&'d mut ()>, // You do need this, or else user would be able to call `BlockingUartIo::new` multiple times on the _same_ `&mut UART` without dropping first the previous `BlockingUartIo`s, which is not ideal
}

impl<'d> BlockingUartIo<';d> {
    pub fn new<P: Uart>(uart: impl Peripheral<P = P> + 'd) -> Result<Self, EspError> {
        // TODO: Call driver initialize etc. etc.
        Ok(Self {
            uart: P::uart(),
            _d: PhantomData,
         })
    }
}

impl<'d> Drop for BlockingUartIo<'d> {
    fn drop(&mut self) {
        // TODO: Deinstall the driver and switch to nonblocking mode using `self.uart`
    }
}

... and ditto for BlockingUsbSerialIo? No extra BlockingIoDriver, no generics. Just one lifetime 'd which we do need if we want to prevent the user from calling BlockingUartIo twice for the same UART peripheral.

[ivmarkov]

See above how we can simplify by removing the T generics and removing BlockingIoDriver.
Also - even if it is a bit mouthful, how about BlockingUsbSerialIo?