[Console] more secondary output choices (IDFGH-8525) #9979

chipweinberger · 2022-10-14T13:41:27Z

❌ EDIT: NOT MERGED, created new pr with different approach: #10132 ❌

Related Issue: #9877

2 commits:

panic.c - Separate PR: #10027

log to the secondary console as well

Kconfig

Add UART and USB CDC "Seconday Console" options to kConfig & implement.

choice ESP_CONSOLE_SECONDARY

    config ESP_CONSOLE_SECONDARY_UART_DEFAULT
        bool "UART"
        depends on ! ESP_CONSOLE_UART_DEFAULT && ! ESP_CONSOLE_UART_CUSTOM

    config ESP_CONSOLE_SECONDARY_USB_CDC
        bool "USB CDC"
        depends on ! ESP_CONSOLE_USB_CDC

Just like before, it is only shown if it is not the primary console.

Tested on an ESP32-S3.

Alvin1Zhang · 2022-10-17T01:09:12Z

Thanks for your contribution.

chipweinberger · 2022-10-18T21:10:44Z

The most important change is the panic.c change, so that the stacktrace it printed on the secondary console as well.

If you want me to revise this PR and make fewer changes, let me know. The panic.c change is the only one I really need =)

chipweinberger · 2022-10-23T00:25:30Z

I split this up into 2 commits, and made the first commit its own PR: #10027

igrr · 2022-11-02T08:48:02Z

@chipweinberger The panic part has been merged, could you please rebase the remaining part of this PR?

chipweinberger · 2022-11-02T17:40:21Z

components/esp_system/Kconfig

+
+        config ESP_CONSOLE_SECONDARY_UART_DEFAULT
+            bool "UART0 Default"
+            depends on ! ESP_CONSOLE_UART_DEFAULT && ! ESP_CONSOLE_UART_CUSTOM


to keep code complexity down, I just implemented the "Default" UART. But it may be worthwhile to implement custom UART as well. Perhaps someone else can do that if they need it.

chipweinberger · 2022-11-02T17:41:34Z

@igrr rebased!

igrr

Looks good, just one question about panic_print_char_uart_secondary_default.

We also need to add some tests for these new configurations to prevent them from getting broken. But that might require us to set up a new test environment (with both UART and USB ports connected) so I'm not going to ask you to do this.

igrr · 2022-11-02T17:47:23Z

components/esp_system/panic.c

@@ -81,14 +81,25 @@ static void panic_print_char_uart(const char c)
 }
 #endif // CONFIG_ESP_CONSOLE_UART

+#if CONFIG_ESP_CONSOLE_SECONDARY_UART_DEFAULT


Since CONFIG_ESP_CONSOLE_SECONDARY_UART_DEFAULT depends on !ESP_CONSOLE_UART_DEFAULT && !ESP_CONSOLE_UART_CUSTOM, would it be possible to merge this with the code block above? (I might be missing the reason to keep it separate.)

Right. I forgot I added && !ESP_CONSOLE_UART_CUSTOM.

I added that because so you can't accidentally configure a CUSTOM_UART and a SECONDARY_UART_DEFAULT to use the same port.

Perhaps I should use fancier KConfig settings to solve this? Something like && !(ESP_CONSOLE_UART_CUSTOM && UART_NUM == 0)

Perhaps I should use fancier KConfig settings to solve this? Something like && !(ESP_CONSOLE_UART_CUSTOM && UART_NUM == 0)

You can, but in that case please add a comment explaining that this is defensive programming / "belts-and-braces", to help the readers of the code understand the reason for this redundant condition.

I'm sorry, looks like the code here is still the same? Is it possible to merge the code of panic_print_char_uart_secondary_default into panic_print_char_usb_cdc, since they are guaranteed to not exist at the same time?

Is that something that we need to support? (Surely this is possible in theory, I just don't recall anyone ever asking for such feature. So if this makes the code ever so slightly more complex, I'd say that let's avoid the complexity.)

Understood, and I agree. 2 UART consoles is not something I really care to support either.

But, having them treated separately (as my code currently does), is IMO the simpler approach. Simplicity was my only motivation for having 2 UART panic functions, 1 for each.

I'd need to think more about how they could be combined, given that the primary implementation has configurable GPIO pins, UART Num, etc. I need to make sure those values are set correctly.

I'll take another look at combining them, and see how the code turns out.

Actually, there is a super great use-case for 2 UART log outputs - being able to send logs over UART to a 2nd microcontroller.

This is actually a feature I've fantasized about having, hah.

Logs are super valuable, and in remote locations you want a super reliable way to store them, and access them. A 2nd microcontroller dedicated to processing logs is the most reliable way to make sure they are stored, especially during low ram, heap corruption, hardware issues, etc.

Couldn't you use a single UART and connect the TX pin to the debug port and the other microcontroller?

Ya that seems smarter. I can't think of a downside for doing it that way.

I'll look more into combining them.

components/esp_system/Kconfig

igrr

Looks like the pre-commit check is failing. Please install the pre-commit hook following the instructions here and check your commit. (I think it has to do with some trailing whitespace.)

Another thing, have you tried building with the usb_cdc secondary console option for ESP32-S2? It seems to me that more changes are necessary. For example, searching for CONFIG_ESP_CONSOLE_USB_CDC, there is this part in components/esp_system/port/soc/esp32s2/CMakeLists.txt:

if(CONFIG_ESP_CONSOLE_USB_CDC)
    target_sources(${COMPONENT_LIB} PRIVATE "${CMAKE_CURRENT_LIST_DIR}/usb_console.c")
endif()

Probably we will get a linker error unless we change the condition to consider ESP_CONSOLE_SECONDARY_USB_CDC.

There is also some code dependent on ESP_CONSOLE_USB_CDC in bootloader_support.

Similar point about CONFIG_ESP_CONSOLE_UART — searching for this option, there are multiple references to it in the code. I'm not sure that secondary console will be fully functional if we don't consider CONFIG_ESP_CONSOLE_SECONDARY_UART_DEFAULT in the same manner. Similar note for CONFIG_ESP_CONSOLE_UART_NUM.

igrr · 2022-11-03T14:39:37Z

components/esp_system/Kconfig

+
+        config ESP_CONSOLE_SECONDARY_USB_CDC
+            bool "USB CDC"
+            depends on SOC_USB_OTG_SUPPORTED && !ESP_CONSOLE_USB_CDC


Suggested change

depends on SOC_USB_OTG_SUPPORTED && !ESP_CONSOLE_USB_CDC

depends on SOC_USB_OTG_SUPPORTED && !ESP_CONSOLE_USB_CDC && !TINY_USB

to match the USB CDC option above

chipweinberger · 2022-11-03T18:16:45Z

I'll do some more testing and make sure they all work.

Will report back.

chipweinberger · 2022-11-04T02:51:00Z

🥳🥳🥳🥳

Okay. @igrr Made some big changes to the diff.

I combined the primary & secondary UART, like you suggested. It's a good change.

I added more internal KConfigs for when either primary or secondary is enabled.

I've tested:

building S2, all 6-ish permutations
building S3, all 14-ish permutations (CDC does not support dual core yet)
Running the Console Example for S2, UART, UART + CDC, (I don't have a USB port to test CDC alone)
Running the Console Example for S3, UART, Serial JTAG, UART + Serial JTAG, Serial JTAG + UART

It should be good to merge.

chipweinberger · 2022-11-04T02:54:02Z

components/bootloader_support/src/bootloader_console.c

+#endif
+#ifdef CONFIG_ESP_CONSOLE_IS_USB_SERIAL_JTAG_ENABLED
+    bootloader_console_init_usb_serial_jtag();
+#endif


I tested this change. logging to both ports during bootloader works.

chipweinberger · 2022-11-04T02:58:42Z

components/esp_system/include/esp_private/usb_console.h

@@ -58,6 +58,8 @@ ssize_t esp_usb_console_available_for_read(void);

 bool esp_usb_console_write_available(void);

+bool esp_usb_console_read_available(void);


This fixes some build errors for CDC. The only build error left is the Static Assert about dual core.

chipweinberger · 2022-11-08T01:05:27Z

Closing this PR in favor a a new one, with less noise.

#10132

chipweinberger mentioned this pull request Oct 14, 2022

[Usb Serial JTAG] [Deadlock] [Console Deinit] esp_vfs_usb_serial_jtag_use_nonblocking() causes deadlock (IDFGH-8520) #9974

Open

espressif-bot added the Status: Opened Issue is new label Oct 14, 2022

github-actions bot changed the title ~~[Console] more secondary output choices~~ [Console] more secondary output choices (IDFGH-8525) Oct 14, 2022

chipweinberger mentioned this pull request Oct 15, 2022

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

Open

chipweinberger force-pushed the user/chip/more-secondary-console-choices branch 3 times, most recently from 289c70e to 91de9b0 Compare October 18, 2022 08:26

chipweinberger force-pushed the user/chip/more-secondary-console-choices branch 2 times, most recently from 5585835 to 929cb73 Compare October 18, 2022 23:39

chipweinberger mentioned this pull request Oct 23, 2022

[Panic] also print to secondary USB Serial/JTAG Console (IDFGH-8578) #10027

Merged

chipweinberger force-pushed the user/chip/more-secondary-console-choices branch from 929cb73 to 39e1d5a Compare October 23, 2022 00:22

igrr self-requested a review November 2, 2022 08:48

chipweinberger force-pushed the user/chip/more-secondary-console-choices branch from 39e1d5a to 3d90e99 Compare November 2, 2022 17:34

chipweinberger commented Nov 2, 2022

View reviewed changes

igrr reviewed Nov 2, 2022

View reviewed changes

chipweinberger force-pushed the user/chip/more-secondary-console-choices branch from 3d90e99 to 9f6b5a3 Compare November 2, 2022 18:33

igrr reviewed Nov 2, 2022

View reviewed changes

components/esp_system/Kconfig Show resolved Hide resolved

chipweinberger force-pushed the user/chip/more-secondary-console-choices branch 3 times, most recently from be836b0 to aef0fc0 Compare November 2, 2022 19:05

igrr reviewed Nov 3, 2022

View reviewed changes

chipweinberger force-pushed the user/chip/more-secondary-console-choices branch 3 times, most recently from b58104d to f8ba0d2 Compare November 4, 2022 02:32

chipweinberger force-pushed the user/chip/more-secondary-console-choices branch from f8ba0d2 to ce26284 Compare November 4, 2022 02:46

chipweinberger commented Nov 4, 2022

View reviewed changes

chipweinberger force-pushed the user/chip/more-secondary-console-choices branch 2 times, most recently from b66cabb to 4565d02 Compare November 4, 2022 03:24

[Console] more secondary output choices

3b90fdb

chipweinberger force-pushed the user/chip/more-secondary-console-choices branch from 4565d02 to 3b90fdb Compare November 4, 2022 03:31

chipweinberger mentioned this pull request Nov 8, 2022

[Console] more secondary output choices (try 2) (IDFGH-8688) #10132

Open

chipweinberger closed this Nov 8, 2022

chipweinberger deleted the user/chip/more-secondary-console-choices branch November 18, 2022 23:37

espressif-bot assigned zikalino May 1, 2023

espressif-bot added Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: Opened Issue is new labels May 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] more secondary output choices (IDFGH-8525) #9979

[Console] more secondary output choices (IDFGH-8525) #9979

chipweinberger commented Oct 14, 2022 •

edited

Loading

Alvin1Zhang commented Oct 17, 2022

chipweinberger commented Oct 18, 2022

chipweinberger commented Oct 23, 2022 •

edited

Loading

igrr commented Nov 2, 2022

chipweinberger Nov 2, 2022 •

edited

Loading

chipweinberger commented Nov 2, 2022

igrr left a comment

igrr Nov 2, 2022

chipweinberger Nov 2, 2022 •

edited

Loading

igrr Nov 2, 2022 •

edited

Loading

chipweinberger Nov 2, 2022

igrr Nov 3, 2022

igrr Nov 3, 2022

chipweinberger Nov 3, 2022 •

edited

Loading

chipweinberger Nov 3, 2022 •

edited

Loading

igrr Nov 3, 2022

chipweinberger Nov 3, 2022 •

edited

Loading

igrr left a comment •

edited

Loading

igrr Nov 3, 2022

chipweinberger commented Nov 3, 2022

chipweinberger commented Nov 4, 2022 •

edited

Loading

chipweinberger Nov 4, 2022

chipweinberger Nov 4, 2022 •

edited

Loading

chipweinberger commented Nov 8, 2022

	depends on SOC_USB_OTG_SUPPORTED && !ESP_CONSOLE_USB_CDC
	depends on SOC_USB_OTG_SUPPORTED && !ESP_CONSOLE_USB_CDC && !TINY_USB

		@@ -58,6 +58,8 @@ ssize_t esp_usb_console_available_for_read(void);

		bool esp_usb_console_write_available(void);

		bool esp_usb_console_read_available(void);

[Console] more secondary output choices (IDFGH-8525) #9979

[Console] more secondary output choices (IDFGH-8525) #9979

Conversation

chipweinberger commented Oct 14, 2022 • edited Loading

panic.c - Separate PR: #10027

Kconfig

Alvin1Zhang commented Oct 17, 2022

chipweinberger commented Oct 18, 2022

chipweinberger commented Oct 23, 2022 • edited Loading

igrr commented Nov 2, 2022

chipweinberger Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

chipweinberger commented Nov 2, 2022

igrr left a comment

Choose a reason for hiding this comment

igrr Nov 2, 2022

Choose a reason for hiding this comment

chipweinberger Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

igrr Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

chipweinberger Nov 2, 2022

Choose a reason for hiding this comment

igrr Nov 3, 2022

Choose a reason for hiding this comment

igrr Nov 3, 2022

Choose a reason for hiding this comment

chipweinberger Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

chipweinberger Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

igrr Nov 3, 2022

Choose a reason for hiding this comment

chipweinberger Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

igrr left a comment • edited Loading

Choose a reason for hiding this comment

igrr Nov 3, 2022

Choose a reason for hiding this comment

chipweinberger commented Nov 3, 2022

chipweinberger commented Nov 4, 2022 • edited Loading

chipweinberger Nov 4, 2022

Choose a reason for hiding this comment

chipweinberger Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

chipweinberger commented Nov 8, 2022

chipweinberger commented Oct 14, 2022 •

edited

Loading

chipweinberger commented Oct 23, 2022 •

edited

Loading

chipweinberger Nov 2, 2022 •

edited

Loading

chipweinberger Nov 2, 2022 •

edited

Loading

igrr Nov 2, 2022 •

edited

Loading

chipweinberger Nov 3, 2022 •

edited

Loading

chipweinberger Nov 3, 2022 •

edited

Loading

chipweinberger Nov 3, 2022 •

edited

Loading

igrr left a comment •

edited

Loading

chipweinberger commented Nov 4, 2022 •

edited

Loading

chipweinberger Nov 4, 2022 •

edited

Loading