Fix terminal write edge cases; add one read mode and add quell command #1025
Conversation
Using strtoll() can only return numbers in the range [-2^63, 2^63-1]. This means that 0xffffFFFFffffFFFF (2^64-1) will be out of range and is written as max LL. Actually, every 64-bit number with high-bit set will wrongly be written as max LL. This commit uses strtoull() instead to fix this, and checks for unsiged out- of-range error. strtoull() also has the neat benefit that input with a minus sign is treated like C unsigned numbers, ie, -u is also a valid unsigned number if only u is one. In case the input is meant to be treated as signed, it is therefore still OK to use strtoull() in the first instance only that in this case a second check against the range of the signed domain is necessary.
Integers can be hexadecimal, decimal or octal. An optional case-insensitive suffix specifies their size: HH: 8 bit, H/S: 16 bit, L: 32 bit, LL: 64 bit An optional U suffix makes a number unsigned. Ordinary 0x hex numbers are always treated as unsigned. +0x or -0x hex numbers are treated as signed unless they have a U suffix. Unsigned integers cannot be larger than 2^64-1. If n is an unsigned integer then -n is also a valid unsigned integer as in C. Signed integers must fall into the [-2^63, 2^63-1] range or a correspondingly smaller range when a suffix specifies a smaller type. Out of range signed numbers trigger a warning. Ordinary 0x hex numbers with n hex digits (counting leading zeros) use the smallest size of 1, 2, 4 and 8 bytes that can accommodate any n-digit hex number. If a suffix specifies a size explicitly the corresponding number of least significant bytes are written. Otherwise, signed and unsigned integers alike occupy the smallest of 1, 2, 4, or 8 bytes needed to accommodate them in their respective representation.
The code no longer accepts valid mantissa-only doubles that are integer rejects, eg, 078 or ULL overflows. These are most likely input errors by the user: 8 is not an octal digit, they might have typed 17 hex digits, not 16. It's just too hard to explain that 0xffffFFFFffffFFFFf writes 0x4430000000000000, which is the correct double representation of the valid 17-digit hex mantissa that strtod() is perfectly happy to accept.
Sets the quell_progress global variable that can be, and is, consulted by programmers. Setting quell_progress to a positive number also switches off progress bars. It is currently not possible to switch on progress bars again: that is enabled in main.c once at the start of AVRDUDE. That code in main should move to avr.c to enable report_update() to consult quell_progress directly. Will do at another time when touching main.c and avr.c. smr
|
For details, see #1020 (comment) Tips for reviewing: Either read term.c on its own or follow the commits one by one to see what's happened. Here is a command to check terminal write cases with the following input file test-cases.txt: |
I agree. I believe both avr-gcc and Microchip XC8 support only 32bit float/double/long double. |
Interesting. It is okay under MSYS2 mingw64 under Windows. I will test out macOS as well. |
AVR-GCC can produce 64-bit double for some time now, and avr-libc caught up implementing the multilib scheme Georg-Johann Lay suggested. There are several things (like printf implementation) though that can only handle 32-bit double by now. |
|
I'd also vote for option 3, default to 32-bit "double" unless "d" is given. |
|
You could also go FORTRAN :-), 128E5 is REAL * 4, 128D5 is REAL * 8. :-)) (Just kidding, of course.) |
@MCUdude Without this PR it is okay. |
Edit: sorry no, this is not working. |
|
It seems to me the following does not work for macOS. A quick hack (not the proper fix) can sorted out the echo issue, but I have no idea why |
…lpha() etc Some C libraries assign true to isalpha(0xff), isdigit(0xff) or ispunct(0xff), which means that the Operating System terminal sees a character 0xff which it may not have a useful display character for. This commit only outputs printable ASCII characters for an AVRDUDE terminal dump reducing the risk of the OS terminal not being able to print the character properly.
@mcuee No it isn't! The test used avrdude without the PR. I can tell, because the PR also changes the error message
@MCUdude Explanation. Amongst other things, the PR defuses the isctype() bomb that makes terminal crash on some systems: Now, the code for which letter to print in chardump() is still the same: No ? assignment in sight, right? I suspect what happens is that the C library of that compiler has assigned true to isalpha(0xff), isdigit(0xff) or ispunct(0xff), and your OS-terminal decided to show a ? for a letter it didn't have a font character for. Test my hpothesis by piping the AVRDUDE-terminal output to Solution. Upgrade to Linux (just kidding) or replace above with That should work. isascii() protects users from Libraries that return unexpected vaues for is...() calls with 0x80...0xff. Will do in a push to this PR shortly. |
Option 3 it is. Suggest also keeping F for continuity. |
Well, the distinguished FORTRAN would be most happy if they could cut and paste lists of constants from their source onto into an AVRDUDE terminal write for the ATtiny85 EEPROM that keeps the moon rocket in orbit ;-). The ATtiny85, not the EEPROM that is. |
|
Can we require the readline library? The prompt ">>>" problem has to do with readline(), its poor man's fgets() replacement and OS-terminal echo. One of these could happen when I/O is a pipe/file ( A. Good. No redundant B. Annoying. Looks unprofessional. Echos the input twice. Also, when written to file/pipe. C. Does not echo the input, but echos the prompt?!? For a long successful input file you get a ton of prompts (w/o newline). D. Reasonable. Only shows output when redirected to pipe/file. Makes sense, but not great for logging etc. Somehow we seem to be getting a mixture of A, B or C. Two questions:
The best user experience is with readline. It would be best if we were able to require that library for building avrdude. Going without is painful as it means we would have to write a poor man's replacement that is less of a mess than the current implementation. For now, I'll adopt the quick fix that @mcuee suggested. |
|
Just pushed a few more changes to this PR.
|
|
I now consider this PR finished, and will start work on the actual issue of making writes to flash work. Unless I hear otherwise, I plan to merge the PR into main in the next few days. |
|
Seems to work fine on MacOS now. Excellent work @stefanrueger! |
|
Windows: MSYS2 mingw64. I think everything is okay now. Windows Powershell. |
@stefanrueger The output under MSYS2 mingw64 is now in line the But the output under Windows Terminal Powershell is a bit strange. Readline is also available for MSVC from vcpkg. But I have no idea how to build with MSVC. |
|
@stefanrueger Default build without libreadline. I think this is okay. Building with libreadline. |
|
@stefanrueger Question: is this a problem or not? |
|
Under Linux (Ubuntu 20.04). bt with debug build. |
|
Under FreeBSD it seems to crash more often than Linux. |
Wow! Great catch, @mcuee. I haven't touched the parsing of the input lines and their splitting into tokens, so this is probably a "pre-existing" condition that escaped me when I read the code. What about the change that I just pushed? |
@stefanrueger BTW, I just found out that previously I did not build the Linux version with readline support as I did not install the development package under my Ubuntu 20.04 machine (dual boot with Windows 11). Now I built it with proper readline support. And yes, latest git main or 7.0 release will crash. So this is not introduced by your PR but rather pre-exisitng bug. |
|
I think this is good enough now to be merged. |
For details, see #1020 (comment)