Terminal write and erase improvements #1320
Conversation
instead of having to figure out how many bytes the memory have left when filling with ellipsis mode
|
So you decided not to use This PR seems to work fine for a quick test. |
Yes, I'll stick with |
|
@stefanrueger I've now added support for negative addresses and length bytes, just like you suggested in #1319. Please give this PR a try and see if you are able to break anything. It appears to work and do what it should, I can start working on an even more improved erase command that also lets you specify an optional start address and length |
|
Looks good. |
|
Two missing things.
|
|
Review:
This would ensure the existing code could stay on unchanged. Note that |
|
Thanks for the review @stefanrueger! I had already figured out the issues pointed out in bullet point 1 and two, but I hadn't pushed the code yet. I'll spend some time to see if I can get things right |
It's now possible to erase a spesific memory using `erase <mem>`, and erase a section of a memory using `erase <mem> <addr> <len>`
in cmd_write. Negative values indicates the memory address counting from the highest address and down
Negative values indicates the memory address counting from the highest address and down
MCUdude
force-pushed
the
terminal-write-end-mem
branch
from
b9e2f44 to
d783fa1
Compare
|
@stefanrueger I've fixed the issues you pointed out, and I've also re-implemented the erase command to prevent modifying existing code. Feel free to review again! |
src/term.c
Outdated
| pmsg_error("(erase) %s memory type not defined for part %s\n", argv[1], p->desc); | ||
| return -1; | ||
| } | ||
| char *args[6] = {"write", memtype, "", "", "0xff", "..."}; |
There was a problem hiding this comment.
This is not wrong, but tradition has it that argv lists are terminated by NULL, so just for superstition, I'd write char *args[] = {"write", memtype, "", "", "0xff", "...", NULL}; Admittedly, it's unlikely that the write code ever looks for the terminating NULL.
There was a problem hiding this comment.
Thanks for your input, I wasn't aware of this so if I add a NULL to the args array, should I pass 6 or 7 as the argc number to cmd_write?
There was a problem hiding this comment.
argc must be 6 (no change)
There was a problem hiding this comment.
https://en.cppreference.com/w/cpp/language/main_function
argc - Non-negative value representing the number of arguments passed to the program from the environment in which the program is run.
argv - Pointer to the first element of an array of argc + 1 pointers, of which the last one is null and the previous ones, if any, point to null-terminated multibyte strings that represent the arguments passed to the program from the execution environment. If argv[0] is not a null pointer (or, equivalently, if argc > 0), it points to a string that represents the name used to invoke the program, or to an empty string.
There was a problem hiding this comment.
I had a look at the tokenize() function that provides the argc, argv pair for all the commands in the terminal. Indeed, that uses a terminating NULL (as convention has it). So better to change that line to char *args[] = {"write", memtype, "", "", "0xff", "...", NULL};
|
earmarked for the next mergefest |
|
Ahh, I forgot: Maybe the documentation of the read ellipses could be downgraded to simplify documentation, eg, removing the ellipses from the terminal help menus for read, from avrdude.1 and avrdude.texi; in the latter two I'd add a sentence in the read description to the effect of |
The ellipse is replaced by specifying the end memory address, -1
|
I'd be very grateful if you could write a few sentences that I could add to avrdude.1 and avrdude.texi that explains the negative numbers notation (and the fact that they don't represent the length anymore, but rather an end address). Technical things like this are difficult for me to get "just right", and the documentation you have already provided for the various terminal commands is really good and well-explained! |
|
[Edited to crisp up table] Would a table with formulae and examples be OK? Sth like The
Please double check examples! |
It's a reminder of naming the memory size sz. Feel free to remove it if you think it's clear enough from the last sentence before the table... Other than that we're getting in the territory of diminishing returns. Happy to merge during the next mergefest unless @mcuee can break it. |
|
Ah, I get it! I think it's fine as it is. Thanks for the help @stefanrueger! @mcuee can you see if you're able to break anything? |
|
I have done some simple tests and so far so good. Give me a few days more so that I can carry out more tests. |
|
While we still have a terminal mode PR open. |
|
Good catch, some commands have that extra newline, most notably read. Some of the extra newlines may be hard to find: the interaction of verbose progress reports, potential errors and normal output can be hard to understand/manage. But when called with |
It will be good to clean up and minimize the differences. I will suggest we remove most of the new lines. For example, I think we should remove the new line immediately after the |
I think we just need to reduce the number of new lines. Line breaks are good in some places (eg: verbose reports, errors report). But there are simply too many new lines right now. As for But I am reducing the usage of |
I have done more tests under Windows and Linux and this PR is good to go. We can deal with the new line improvement in another PR. |
Yes, most of the commands are trivial to fix, but the write progress bar would need some work. |
Whilest q is a proper alias for quit (explicitly mentioned in the command table) this is not strictly true for d, w and r: these only work because at this time there are no other commands starting with d, w or r than dump, write or read. Future extensions to the terminal may change this.
Done by implementing msg_xyz("\v") as conditional newline:
If the first character of the format of any of the msg_xyz functions
starts with the vertical tab \v then a newline character is printed
instead of the \v if this output stream is not at column 0, nothing
otherwise.
Indeed, it does. I have pushed a commit onto this PR that addresses double newlines. In a lot of cases a newline is printed to ensure whatever the output at the screen the next thing is written on a fresh line. That often causes extra newlines in case the output on screen was already terminated by a newline earlier. Naturally the function wanting to start output on a new line has no idea what's the state of the screen. My idea was to hijack a vertical tab
Effectively this allows |
|
It's great that lots of unnecessary double new lines are now gone. I slightly prefer double newlines before and after the progress bars, but It's not a deal breaker. Let's hear what the other dudes have to say! |
|
@stefanrueger and @MCUdude Latest testing results: |
I see. No problem. We can keep the behavior for |
|
I've just updated the terminal examples to reflect the "new" style where excess newlines were printed. However, We'll have to update a few examples in avrdude.texi as well, under the "Example Command Line Invocations" section. |
Great thinking! As it's meant to be examples, it might (ever so slightly) improve clarity if we pretended (just for the examples) that the prompt was |
Some newline characters removed for clairity
MCUdude
force-pushed
the
terminal-write-end-mem
branch
from
0b223f4 to
5facea9
Compare
You're right, and I just rebased the previous commit. I added a few new lines; the examples are easier to figure out just by looking at them. BTW I've now updated the "regular" CLI examples, so this PR should be good to go now. |
Excellent, that's a substantial improvement of the terminal now. Planning to merge in the next few days hopefully along with the ch341a programmer and three smaller PRs. |
#1319 related
The first new feature makes it easier to fill a section of a memory with data, as the user doesn't need to keep track of how many bytes there are left in the memory. I decided to use
...for this, because the read command also uses...for this:read eeprom 0x100 ...I've also extended the
erasecommand. Now the erase command can be used to only erase the user specified memory.I've yet to update the docs, but I'd like to hear your thoughts first!