Update part cmd
#1568
Merged
Update part cmd
#1568
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
|
I've now added a "variants" table as well. Again, feedback is welcome! |
|
Somehow I can not build this under MSYS2 mingw (without GNU readline installed). Latest git main is okay. I think this has nothing to do with your changes in this PR though. Maybe you want to merge git main changes. Thanks. |
|
It is the same for MSVC build. No idea why. Git main is okay but not this PR. On the other hand, github action is fine. So it is kind of strange. |
|
Using the github action binary for testing. @MCUdude In fact, I think the memory info is alreay too verbose for |
|
The output format looks nice though. |
|
I've updated the branch, so please try to build again.
Everything is possible. I put it in there for demonstration purposes. But we can print just the information we want when Avrdude is executed (with -v or -vv), and print everything with the |
I think the build issue is not related to your PR. I have installed back GNU Readline and ncurses and now everything is back to normal for my local MinGW build.
That will be my preference. But let's hear from @stefanrueger as well. |
|
Overall I think this PR is quite nice to have. |
|
Another table formatting option is removing the pipes separating the columns: |
I am okay either way, with slight prerference towards using the pipes to seperare the columns. |
|
I decided I like the table better without the pipes separating the colums, so I removed them. It's easy to add them back in if this choice turns out to be controversial. Now the @stefanrueger any thoughts? |
This looks okay to me. Let's see if @stefanrueger likes it or not. |
|
BTW, it is interesting that I need to blow the git repo and then I can build under MSVC. Quite strange. Maybe I occasionally use MSYS2 git which disturbs the normal git for Windows. Just a note to myself. |
|
I like how the tables look and I like that we do something with the variants. My own preference would be to output the variants on I am at sixes and sevens over using Here how it would look like: |
I can do that but most likely in a separate PR after this one got merged. Too busy right now. |
|
Since you asked... I prefer both: trimming the spaces left of the colon from the strings in main.c and adding extra whitespace to the programmer-specific information so everything lines up. The task comes to finding a sweetspot column location that works for both. There is something else: For scripts it is neater if they can request only the memory info or only the variants. I think you suggested these options earlier, @MCUdude. And it would be cool if the diff --git a/src/avrpart.c b/src/avrpart.c
index 3be90249..0d29e99f 100644
--- a/src/avrpart.c
+++ b/src/avrpart.c
@@ -866,7 +866,7 @@ void avr_display(FILE *f, const AVRPART *p, const char *prefix, int verbose) {
fprintf(f, "%sAVR Part : %s\n", prefix, p->desc);
fprintf(f, "%sProgramming modes : %s\n", prefix, prog_modes_str(p->prog_modes));
- if(verbose > 1) {
+ if(strlen(prefix) == 0 || verbose > 1) {
avr_mem_display(f, p, prefix);
avr_variants_display(f, p, prefix);
}
diff --git a/src/term.c b/src/term.c
index 28d1058b..89460ee9 100644
--- a/src/term.c
+++ b/src/term.c
@@ -1560,19 +1560,54 @@ finished:
static int cmd_part(const PROGRAMMER *pgm, const AVRPART *p, int argc, char *argv[]) {
- if(argc > 1) {
+ int help = 0, onlymem = 0, onlyvariants = 0, invalid = 0, itemac = 1;
+
+ for(int ai = 0; --argc > 0; ) { // Simple option parsing
+ const char *q;
+ if(*(q=argv[++ai]) != '-' || !q[1])
+ argv[itemac++] = argv[ai];
+ else {
+ while(*++q) {
+ switch(*q) {
+ case '?':
+ case 'h':
+ help++;
+ break;
+ case 'v':
+ onlyvariants++;
+ break;
+ case 'm':
+ onlymem++;
+ break;
+ default:
+ if(!invalid++)
+ pmsg_error("(part) invalid option %c, see usage:\n", *q);
+ q = "x";
+ }
+ }
+ }
+ }
+ argc = itemac; // (arg,c argv) still valid but options have been removed
+
+ if(argc > 1 || help || invalid || (onlymem && onlyvariants)) {
+ if(onlymem && onlyvariants)
+ pmsg_error("(part) cannot mix -v and -m\n");
msg_error(
"Syntax: part\n"
- "Function: display the current part information\n"
+ "Function: display the current part information including memory and variants\n"
+ "Options:\n"
+ " -m only display memory information\n"
+ " -v only display variants information\n"
);
return -1;
}
- term_out("\v");
- avr_display(stdout, p, "", 0);
- avr_mem_display(stdout, p, "");
- if(verbose)
+ if(onlymem)
+ avr_mem_display(stdout, p, "");
+ else if(onlyvariants)
avr_variants_display(stdout, p, "");
+ else if(!onlymem && !onlyvariants)
+ avr_display(stdout, p, "", 0);
term_out("\v");
return 0;See how you like that |
|
Maybe it's another idea to not use This would require the This is the output in the terminal If we go down this route then the |
|
Thanks for the suggestions @stefanrueger! I've done some additional tweaks and added I've tested this PR using the Power Debugger, STK500 (v1 FW), STK500 (v2 FW), STK600, and AVR Dragon. No colon inconsistencies anymore! |
|
I like the output of this PR. |
|
Excellent. Let's hear what @stefanrueger has to say. But I think this PR is ready to be merged in the next mergefest! |
|
@stefanrueger I've now implemented your suggested improvements. Please review! BTW I'm so glad we teamed up and added strutil.c to the code base! It makes my (avrdude)life so much easier!
I'll see if I can have a look at this again tonight. |
leftovers from when I introduced the variants command
|
Looks good now, so will merge probably tomorrow (with three other older PRs). |
|
Small thing: the lines condition the computation of the length of the memory string on I took the liberty to push a commit onto the PR to do this. |
MCUdude
added a commit
to MCUdude/avrdude
that referenced
this pull request
Jan 21, 2024
stefanrueger
added a commit
that referenced
this pull request
Jan 22, 2024
Make linuxgpio.c compile #1568
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.
I'm creating this unfinished PR to start a discussion on how we want the
partcommand in terminal mode, and (and output in verbose mode) to look like. I've removed a bunch of what I'd consider "non-useful information" to the average user.The memory array width is dependent on the character width of the differetnt entries. However, the code is a bit ugly, so feel free to suggest how it can be further improved.
It would be neat if the memories could be sorted based on their offset, but I couldn't figure out a clean way of doing this. @stefanrueger?
Are there other things we should show/print in the
partcommand?This is what the output looks like on classic AVRs, modern AVRs, and Xmegas.