-
Notifications
You must be signed in to change notification settings - Fork 8.2k
west: add monitor feature with options #97955
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
west: add monitor feature with options #97955
Conversation
460b77b to
b0357b2
Compare
I will also provide test_monitor.py.
b0357b2 to
96aa2b4
Compare
scripts/west-commands.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use same style as other help texts, i.e. start with a lowercase verb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as mentioned, this needs to start with a lowercase verb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"monitor serial output from a board" probably works :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did update this when you commented. Outdated file I guess. I'll re-word on next push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't dived into the code yet, but why do you approach this with a "profiles" mechanism? Isn't it possible to have a similar working like runners with sub-classes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will also evaluate this, sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pdgendt So you mean having modules files like espressif.py as vendor profile sub-class instead of a common profiles.py? The custom vendor profile here is just a default set of configurations to be used when west monitor is called - to avoid lots of arguments - which differs a bit from current runners with sub-classes do. I mean, runner extend various options for the particular vendor where the goal here isn't to extend arguments but set the default ones.
96aa2b4 to
25b5964
Compare
Add serial monitor for Zephyr with: - Auto-reconnect on USB disconnect - Automatic addr2line decoding from build/CMakeCache.txt - Custom vendor profiles - Reset control via DTR/RTS lines - Ctrl-R to reset, Ctrl-] to exit Usage: west monitor # Basic monitor west monitor --decode # With address decoding west monitor --vendor espressif # Vendor defaults Signed-off-by: Sylvio Alves <sylvio.alves@espressif.com>
25b5964 to
7cdcafb
Compare
Quality Gate Passed Quality Gate passed
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code
PR now only needs adjustments in the "profile/vendor" configuration. Will hold continuing with additional updates until we get a clear review on whether this can indeed be merged/maintained or not. In negative response, we can close it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is nice, I think we need a discussion on whether this is something we want/need to maintain in-tree when external, maintained alternatives already exist. See the comments on #97954 (comment) an onwards.
Uh oh!
There was an error while loading. Please reload this page.
Add serial monitor for Zephyr with:
More details in #97954