Skip to content
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

Add readline library replacement for MSVC #1264

Merged
merged 2 commits into from
Jan 5, 2023

Conversation

mariusgreuel
Copy link
Contributor

Here is an alternative to readline using standard Windows mechanisms. It does not do any of the readline magic, however, it does keep the bootloader alive.

@mcuee Can you give this one a shot, please?

BTW, I am away for today.

@mcuee mcuee added the enhancement New feature or request label Jan 4, 2023
@mcuee
Copy link
Collaborator

mcuee commented Jan 4, 2023

Nice one. This seems to work pretty well.

  1. Keep alive works, I can see two fast LED flashing.

  2. Pipeline works well.

C:\work\avr\avrdude_test\avrdude_bin> echo "dump flash 0 0x40"  | 
.\avrdude_pr1264_msvc -c urclock -P COM6 -p m328p -qqt
avrdude> 0000  0c 94 5c 00 0c 94 6e 00  0c 94 6e 00 0c 94 6e 00  | .\. .n. .n. .n.|
0010  0c 94 6e 00 0c 94 6e 00  0c 94 6e 00 0c 94 6e 00  | .n. .n. .n. .n.|
0020  0c 94 6e 00 0c 94 6e 00  0c 94 6e 00 0c 94 6e 00  | .n. .n. .n. .n.|
0030  0c 94 6e 00 0c 94 6e 00  0c 94 6e 00 0c 94 6e 00  | .n. .n. .n. .n.|

avrdude>

MINGW64 /c/work/avr/avrdude_test/avrdude_bin
$ echo "d fl 0 32; quit" | tr \; \\n | ./avrdude_pr1264_msvc -c urclock -P COM6 -p m328p -qqt
avrdude> 0000  0c 94 5c 00 0c 94 6e 00  0c 94 6e 00 0c 94 6e 00  | .\. .n. .n. .n.|
0010  0c 94 6e 00 0c 94 6e 00  0c 94 6e 00 0c 94 6e 00  | .n. .n. .n. .n.|

avrdude>
  1. CTRL-Z works as quit. CTRL-D is interpreted as an invalid command. I think this is not bad at all.
    CTRL/SHIFT/ALT and CAPSLOCK do not seem to cause time out. This is great.
C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_pr1264_msvc -c urclock -P COM6 -p m328p -qqt
avrdude> ^D
avrdude_pr1264_msvc error: (cmd) command ♦ is invalid
avrdude> ^Z

C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_pr1264_msvc -c urclock -P COM6 -p m328p -qqt
avrdude> ^Z

C:\work\avr\avrdude_test\avrdude_bin>
  1. Long sleep will cause timeout, that is acceptable for now.
MINGW64 /c/work/avr/avrdude_test/avrdude_bin
$ (echo r fl 0x7e00 0x30; sleep 3; echo r fl 0x7f00 0x30; echo quit) 
| ./avrdude_pr1264_msvc -c urclock -P COM6 -p m328p -qqt
avrdude> 7e00  11 24 84 b7 14 be 81 ff  f0 d0 85 e0 80 93 81 00  |.$..............|
7e10  82 e0 80 93 c0 00 88 e1  80 93 c1 00 86 e0 80 93  |................|
7e20  c2 00 80 e1 80 93 c4 00  8e e0 c9 d0 25 9a 86 e0  |............%...|

avrdude> avrdude_pr1264_msvc warning: programmer is not responding
avrdude_pr1264_msvc warning: programmer is not responding
avrdude_pr1264_msvc error: unable to read flash page at addr 0x7f00
avrdude_pr1264_msvc error: (dump) error reading flash address 0x07f00 of part ATmega328P
                           read operation not supported on memory type flash
avrdude> avrdude_pr1264_msvc warning: programmer is not responding

@mcuee
Copy link
Collaborator

mcuee commented Jan 4, 2023

PR #1264 seems to sort out #1097 as well.

  1. F7 will bring in the history. It is interesting that it may have the history of previous runs.
    Capture1

  2. If I click the last entry, it will present in the command line. Once I click RETURN, it will run.
    Capture2

@mcuee
Copy link
Collaborator

mcuee commented Jan 4, 2023

Long sleep will cause timeout, that is acceptable for now.

@mariusgreuel

Actually it can be made to work, if we update the version to 5.02.

C:\work\avr\avrdude_test\avrdude-fork [pr-msvc-readline-plan-b ≡ +0 ~1 -0 !]> git diff
diff --git a/src/msvc/readline.cpp b/src/msvc/readline.cpp
index 786bf247..667ff7ad 100644
--- a/src/msvc/readline.cpp
+++ b/src/msvc/readline.cpp
@@ -13,7 +13,7 @@
 #include "readline/readline.h"
 #include "readline/history.h"

-int rl_readline_version = 0x0500;
+int rl_readline_version = 0x0502;

 static char* rl_prompt;
 static rl_vcpfunc_t* rl_handler;

MINGW64 /c/work/avr/avrdude_test/avrdude_bin
$ (echo r fl 0x7e00 0x30; sleep 10; echo r fl 0x7f00 0x30; echo quit) | 
./avrdude_pr1264_msvc_mod -c urclock -P COM6 -p m328p -qqt
avrdude> 7e00  11 24 84 b7 14 be 81 ff  f0 d0 85 e0 80 93 81 00  |.$..............|
7e10  82 e0 80 93 c0 00 88 e1  80 93 c1 00 86 e0 80 93  |................|
7e20  c2 00 80 e1 80 93 c4 00  8e e0 c9 d0 25 9a 86 e0  |............%...|

avrdude> 7f00  a6 01 a0 e0 b1 e0 2c 91  30 e0 11 96 8c 91 11 97  |......,.0.......|
7f10  90 e0 98 2f 88 27 82 2b  93 2b 12 96 fa 01 0c 01  |.../.'.+.+.... .|
7f20  87 be e8 95 11 24 4e 5f  5f 4f f1 e0 a0 38 bf 07  |.....$N__O...8..|

avrdude>

@mcuee
Copy link
Collaborator

mcuee commented Jan 4, 2023

@mariusgreuel

I think this PR can be merged with the minor version update.

@stefanrueger
Please review the codes as well.

@mcuee
Copy link
Collaborator

mcuee commented Jan 4, 2023

As for timeout without pressing any keys, yes this still happens but it takes more than 30minutes for me to get it happen. So it is not bad at all.

C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_pr1264_msvc_mod -c urclock -P COM6 -p m328p -qqt
avrdude>
avrdude>
avrdude>
avrdude>
avrdude> avrdude_pr1264_msvc_mod error: unable to write: sorry no info avail
avrdude_pr1264_msvc_mod error: unable to write: sorry no info avail
avrdude_pr1264_msvc_mod error: unable to write: sorry no info avail
...

Debug log.

.\avrdude_pr1264_msvc_mod -c urclock -P COM6 -p m328p -t -vvvv
...
...
...
avrdude_pr1264_msvc_mod: send: 0 [30]   [20]
avrdude_pr1264_msvc_mod: recv: . [14]
avrdude_pr1264_msvc_mod: recv: . [10]
avrdude_pr1264_msvc_mod: send: 0 [30]   [20]
avrdude_pr1264_msvc_mod: recv: . [14]
avrdude_pr1264_msvc_mod: recv: . [10]
avrdude_pr1264_msvc_mod: send: 0 [30]   [20]
avrdude_pr1264_msvc_mod: recv: . [14]
avrdude_pr1264_msvc_mod: recv: . [10]
avrdude_pr1264_msvc_mod: send: 0 [30]   [20]
avrdude_pr1264_msvc_mod ser_send() [ser_win32.c:450] error: unable to write: sorry no info avail
avrdude_pr1264_msvc_mod: send: 0 [30]   [20]
avrdude_pr1264_msvc_mod ser_send() [ser_win32.c:450] error: unable to write: sorry no info avail
avrdude_pr1264_msvc_mod: send: 0 [30]   [20]
avrdude_pr1264_msvc_mod ser_send() [ser_win32.c:450] error: unable to write: sorry no info avail
avrdude_pr1264_msvc_mod: send: 0 [30]   [20]
avrdude_pr1264_msvc_mod ser_send() [ser_win32.c:450] error: unable to write: sorry no info avail

@mariusgreuel
Copy link
Contributor Author

I think this PR can be merged with the minor version update

Thanks, I missed the recent change in term.c - just update the code.

@mariusgreuel
Copy link
Contributor Author

It seems to sort out #1097 as well.
F7 will bring in the history

Actually, I think it is still broken. I think it is a bug in the ANSI API of Windows, which is probably only triggered by some Unicode to MBCS conversion. When you select dump something it just returns d, which may trick you to believe that it worked.

@stefanrueger
Copy link
Collaborator

Brilliant. Very happy about the progress. Code looks clean and solid (pity I cannot test). Thanks a lot @mariusgreuel for all the implementation branches and @mcuee for unfailing enthusiasm, testing and suggestions.

So, on the next mergefest (once PR #1265 is under the hood) I merge this PR #1264 one (and ignore PR #1259) - then it looks like v7.1 is ready as can be...

@mcuee
Copy link
Collaborator

mcuee commented Jan 5, 2023

It seems to sort out #1097 as well.
F7 will bring in the history

Actually, I think it is still broken. I think it is a bug in the ANSI API of Windows, which is probably only triggered by some Unicode to MBCS conversion. When you select dump something it just returns d, which may trick you to believe that it worked.

In this particular case, what you described is the behavior of git main. For this PR, it does return the whole "dump flash 0x3e00 0x40" in the command line. I just need to hit RETURN to execute it.

Please refer to the screenshots in the previous comment.
#1264 (comment)

  1. F7 will bring in the history. It is interesting that it may have the history of previous runs.
    Capture1

  2. If I click the last entry, it will present in the command line. Once I click RETURN, it will run.
    Capture2

@mcuee
Copy link
Collaborator

mcuee commented Jan 5, 2023

Using Windows batch file to test under Windows Terminal Powershell or CMD prompt.
Edit: this is not correct.
I still need help on the equivelent fo the following shell command to batch file PowerShell script.

echo "d fl 0 32; quit" | tr \; \\n | ./avrdude_pr1264_msvc -c urclock -P COM6 -p m328p -qqt

The following batch file is not equivalent to the above shell script.

C:\work\avr\avrdude_test\avrdude_bin> cat .\test1.bat
@echo off
echo r fl 0x7e00 0x30 |.\avrdude_pr1264_msvc_v1 -c urclock -P COM6 -p m328p -qqt
timeout 10 > nul
echo r fl 0x7f00 0x30 |.\avrdude_pr1264_msvc_v1 -c urclock -P COM6 -p m328p -qqt
@echo on

C:\work\avr\avrdude_test\avrdude_bin> .\test1.bat
avrdude> 7e00  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
7e10  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
7e20  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|

avrdude>
avrdude> 7f00  fa 9a f9 9a 31 96 da 13  f4 cf 23 c0 a0 e0 b1 e0  |....1.....#.....|
7f10  4d d0 1f c0 c3 30 41 f4  33 d0 c8 2f 3d d0 85 91  |M....0A.3../=...|
7f20  1b d0 c1 50 e1 f7 15 c0  c1 30 71 f4 29 d0 c8 2f  |...P.....0q.)../|

avrdude>
C:\work\avr\avrdude_test\avrdude_bin>

@stefanrueger
Copy link
Collaborator

@mcuee Looks like you have either the long-sleep-in-pipe problem (if version is 5.0.0) or needing quit at the end of pipe/file input (if version is 5.0.2). I believe everyone is better off if they don't need quit at the end of pipe/file. Having a slow pipe input is somehow rare and a bit contrived (I think!). What's your preference? My suggestion is to change This line to compare >= 0x0600 as it's not clear when exactly readline no longer had problems with pipe input. This should remove the need to have an explicit quit at the end of pipe/file input. I can do this without a PR when I merge this evening.

@mcuee
Copy link
Collaborator

mcuee commented Jan 5, 2023

@mcuee Looks like you have either the long-sleep-in-pipe problem (if version is 5.0.0) or needing quit at the end of pipe/file input (if version is 5.0.2). I believe everyone is better off if they don't need quit at the end of pipe/file. Having a slow pipe input is somehow rare and a bit contrived (I think!). What's your preference? My suggestion is to change This line to compare >= 0x0600 as it's not clear when exactly readline no longer had problems with pipe input. This should remove the need to have an explicit quit at the end of pipe/file input. I can do this without a PR when I merge this evening.

Hmm, 5.02 works fine now. It even works with long sleep. No need to have the explicit exit command either. So NO NEED to change this PR. It can be merged as it is.

Sorry but just wondering which test result makes you think the other way? I can edit the test results to avoid the confusion.

The only issue is the occasional time out, usually after long time of idle, which we can live with.

The last comment is to seek help to see if I can test long sleep under PowerShell or CMD prompt. I can only test under MSYS2 Bash prompt now -- the test result for long sleep is PASS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants