[Console] more secondary output choices (try 2) (IDFGH-8688) #10132
+187
−92
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
changed the title
igrr
reviewed
Nov 8, 2022
| #if CONFIG_ESP_CONSOLE_IS_USB_SERIAL_JTAG_ENABLED | ||
| bootloader_console_init_usb_serial_jtag(); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Looks like here we may end up calling esp_rom_install_channel_putc (for redirecting ets_printf/esp_rom_printf) multiple times, and it will get redirected to the most recent destination. Is that the expected behavior?
There was a problem hiding this comment.
Which 2 functions both call esp_rom_install_channel_putc()?
To be honest, I'm not familiar with what "redirecting ets_printf" means.
Regardless, I only see thatbootloader_console_init_none() and bootloader_console_init_usb_cdc() call esp_rom_install_channel_putc(), but NONE is only true when the others are NOT enabled, so it is not a problem
chipweinberger
force-pushed
the
user/chip/more-secondary-console-choices-try-2
branch
from
3b90fdb to
84be4db
Compare
|
not having much luck with the precommit. lots of errors. Edit: I'm failing locally but it worked on github?.... Click me |
chipweinberger
force-pushed
the
user/chip/more-secondary-console-choices-try-2
branch
from
385b1c9 to
c55ff9f
Compare
|
I just cloned a fresh esp-idf and am still failing pre-commit locally, even on master branch. trying to get more details: |
igrr
approved these changes
Nov 8, 2022
|
sha=c55ff9f2c824555c21be9d338066b63d0dcd6366 |
|
Thank you for the contribution @chipweinberger! I will take it from here and add CI checks for this new feature. If there are any significant issue identified in the internal review, I will get back to you. Otherwise you should get a notification when the PR is merged. |
|
thanks for the review! ESP-IDF is very high quality code. |
|
Status update: this got stuck a bit because we needed to add new test environments where both UART and USB are connected to the host, to verify that console output is indeed coming to both destinations. |
|
Bump. Hoping the testing environment gets setup soon. |
|
We used to have a Maybe skip the new environment if it is difficult. |
|
The test environment has now been set up, what remains is to write the tests.
Exactly the reason I want to avoid repeating this mistake. You have probably seen how many issue reports are raised about USB_SERIAL_JTAG. |
|
any hope of getting into v5.1? It's been awhile |
espressif-bot
added
Status: Selected for Development
|
|
|
@zikalino, I've rebased the change. You might want to re-test it before merge, mainly to make sure it compiles. |
chipweinberger
force-pushed
the
user/chip/more-secondary-console-choices-try-2
branch
3 times, most recently
from
ab15902 to
78426e6
Compare
chipweinberger
force-pushed
the
user/chip/more-secondary-console-choices-try-2
branch
4 times, most recently
from
372fe03 to
de0c7d6
Compare
|
@igrr, hoping we can clear out some of these PRs. This PR is useful for 2 reasons:
|
|
@igrr will we ever be able to get this in hah =) It is more of a simplifying change than anything. |
👋 Welcome chipweinberger, thank you for your first contribution to 📘 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 expectEspressif 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.
|
chipweinberger
force-pushed
the
user/chip/more-secondary-console-choices-try-2
branch
2 times, most recently
from
02b428f to
1c66960
Compare
|
rebased again |
|
to explain the benefit of this PR to the best of my memory Console & logging are very limiting. Supported outputs must be chosen at compile time instead of runtime and esp-idf only supports uart primary w/ usb serial jtag secondary. that's very limiting this PR makes all logging outputs first class citizens, and also manages to simplify code |
espressif-bot
added
Status: In Progress
|
@chipweinberger Thanks for patiently keeping this PR alive! I have now added an app to test some of the secondary output options. It should be closer to being merged now. |
chipweinberger
force-pushed
the
user/chip/more-secondary-console-choices-try-2
branch
from
1c66960 to
d5a0fd2
Compare
|
@chipweinberger Thanks for fixing that typo. I ran into two more issues while testing this:
|
|
@igrr thanks for testing
It seems like CDC has changed a lot since this commit was made. I've pushed new changes as a new commit so you can see.
Even if you leave secondary disabled? I'm not sure why that would be. |
|
Can I add +99 here please. |
|
Sure! We haven't forgotten about this. In fact, we just merged a refactoring which should help us simplify how the console devices are managed, without having to add a whole bunch of ifdefs around the codebase. We will definitely revisit this topic once a few other vfs-related refactorings are done (e.g. extracting the CDC driver out of the |
|
This looks great, I'd really like this to land in preparation of better console support. Good work, @chipweinberger! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update Sept 29, 2023 (1 year later): to explain the motivation of this PR to the best of my memory
Console & logging are very limiting. Supported outputs must be chosen at compile time instead of runtime
and esp-idf only supports uart primary w/ usb serial jtag secondary. that's very limiting
this PR makes all logging outputs first class citizens, and also manages to simplify code
In addition to
USB Serial/JTAG, addUARTandUSB CDCas "Secondary Console" options to kConfig.🥳🥳🥳🥳
original PR: #9979
Okay. @igrr Made some big changes to the diff. I combined the primary & secondary UART, like you suggested. It's a good change.
For clarity, I added internal KConfigs for when either primary or secondary is enabled, i.e.
ESP_CONSOLE_IS_USB_CDC_ENABLEDI've tested:
It should be good to merge.