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

Portable readline with callback required #1028

Closed
stefanrueger opened this issue Jul 15, 2022 · 46 comments · Fixed by #1132
Closed

Portable readline with callback required #1028

stefanrueger opened this issue Jul 15, 2022 · 46 comments · Fixed by #1132
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@stefanrueger
Copy link
Collaborator

stefanrueger commented Jul 15, 2022

Terminal mode already gives a radically different user experience depending on which OS it is compiled for and whether or not the readline library has been compiled with AVRDUDE. I plan to enable the terminal to work with bootloaders that need being kept alive lest the WDT resets them. One working solution is using the readline library with character callbacks. This works like a charm in Linux.

Here is a standalone small C termreadline.c.txt
file that demonstrates how this works. Just compile $ cc termreadline.c -lreadline and give it a go:

echo quit | a.out 2>&1 | cat
avrdude> quit
quit: bye

If you use it interactively, every 1.8 s a dot should appear, which indicates where the code could keep the bootloader alive.

So, here the challenge for the coders amongst us: can we get this piece of code working for Windows, MacOS and all other platforms and compilers we hope that AVRDUDE will run on?

@stefanrueger stefanrueger added the help wanted Extra attention is needed label Jul 15, 2022
@dl8dtl
Copy link
Contributor

dl8dtl commented Jul 15, 2022

MacOS is a UNIX (even a certified one, IIRC).
As such, the usual Posix descriptor methods do work there without problems.
The challenge will be Windows here, because they do this in a much different way.

Other remarks (I know it's not production code, but nevertheless):

  • use poll() instead of select() (Posix interface)
  • int readytoread() declares a function taking an arbitrary number and kind of arguments; use int readytoread(void) (this is different in C++)
  • prefer bool over int for boolean operations (and true and false then)

@stefanrueger
Copy link
Collaborator Author

Cool as! So, can we expect a Posix-compliant version from you @dl8dtl that is likely to work on UNIX-type systems? That would reduce the problem space already enormously.

@dl8dtl
Copy link
Contributor

dl8dtl commented Jul 15, 2022

If you replace select() by poll(), I think your version would already be Posix.
(Most Posix systems will also accept the select() interface, but when standardizing, it has been superseded by poll().)

For WinAPI, we need someone knowledgable about it, to propose something that can then be #ifdefed for both. The poll() thingie is only a minor issue, just a bit of typing work.

@dl8dtl
Copy link
Contributor

dl8dtl commented Jul 15, 2022

Here's the diff for poll vs. select, it's really pretty trivial.

--- termreadline.c.orig 2022-07-15 21:10:55.655980000 +0200
+++ termreadline.c      2022-07-15 22:22:47.569289000 +0200
@@ -3,6 +3,7 @@
 #include <readline/readline.h>
 #include <readline/history.h>
 #include <unistd.h>
+#include <poll.h>
 
 // Standalone readline test
 
@@ -51,12 +52,15 @@
 
 // Any character in standard input available?
 static int readytoread() {
-  struct timeval tv = { 0L, 0L };
-  fd_set fds;
-  FD_ZERO(&fds);
-  FD_SET(0, &fds);
+  struct pollfd fds[1] = {
+    {
+      .fd = 0,
+      .events = POLLIN | POLLPRI | POLLERR | POLLHUP,
+      .revents = 0
+    }
+  };
 
-  return select(1, &fds, NULL, NULL, &tv) > 0;
+  return poll(fds, 1, 0) > 0;
 }
 
 // Callback processes commands whenever readline() has finished

(Edit: I forgot the > 0 comparison before.)

@dl8dtl
Copy link
Contributor

dl8dtl commented Jul 15, 2022

Maybe revents should be checked for one of the exceptional events as well.

@mcuee
Copy link
Collaborator

mcuee commented Jul 16, 2022

@stefanrueger
Windows can be a challege.

For example, I can build under MSYS2 mingw64 of your code with a quick patch but the result exe does not work properly.

$ diff termreadline.c termreadline_sr.c
6a7,10
> #if defined(WIN32)
> #include <winsock2.h>
> #endif
>

$ echo quit | ./termreadline_sr.exe 2>&1 | cat
avrdude> ................................... (just keep typing `.` and not accepting CTRL-C)

MINGW64 /c/work/avr/avrdude_test/readline
$ ./termreadline_sr.exe
avrdude> ... (hit CTRL-C)
MINGW64 /c/work/avr/avrdude_test/readline
$ quitquit

@mcuee
Copy link
Collaborator

mcuee commented Jul 16, 2022

If you replace select() by poll(), I think your version would already be Posix. (Most Posix systems will also accept the select() interface, but when standardizing, it has been superseded by poll().)

For WinAPI, we need someone knowledgable about it, to propose something that can then be #ifdefed for both. The poll() thingie is only a minor issue, just a bit of typing work.

@dl8dtl and @stefanrueger
FYI libusb older version may have the codes you can use.
Ref: https://github.com/libusb/libusb/tree/v1.0.23/libusb/os
https://github.com/libusb/libusb/blob/v1.0.23/libusb/os/poll_windows.c
https://github.com/libusb/libusb/blob/v1.0.23/libusb/os/poll_posix.c

Not so sure if simpler one works or not.
https://raw.githubusercontent.com/libressl-portable/portable/master/include/compat/poll.h

@mcuee
Copy link
Collaborator

mcuee commented Jul 16, 2022

Not so sure if simpler one works or not.
https://raw.githubusercontent.com/libressl-portable/portable/master/include/compat/poll.h

Maybe it can be used with minor modifications.

$ diff termreadline_sr.c termreadline.c
7,12d6
< #if defined(WIN32)
< #include "poll.h"
< #else
< #include <poll.h>
< #endif
<

$ gcc -o termreadline_sr termreadline_sr.c -lreadline -lws2_32

$ gcc -o termreadline_jw termreadline_jw.c -lreadline -lws2_32
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/11.3.0/../../../../x86_64-w64-mingw32/bin/ld.exe: 
C:\msys64\tmp\ccKZspNO.o:termreadline_j:(.text+0x1c0): undefined reference to `poll'
collect2.exe: error: ld returned 1 exit status

termreadline_jw.c.txt
termreadline_sr.c.txt

For Linux, both versions work fine.

mcuee@UbuntuSwift3:~/build/avr/readline$ echo quit | ./termreadline_sr 2>&1 | cat
avrdude> quit
quit: bye

mcuee@UbuntuSwift3:~/build/avr/readline$ echo quit | ./termreadline_jw 2>&1 | cat
avrdude> quit
quit: bye


@stefanrueger
Copy link
Collaborator Author

So, we have a solution that works in UNIXes. And we have an Any character ready? function for Windows. Progress! Here my summary of @mcuee and @dl8dtl's ideas:

#include "poll.h"

// Any character in standard input available?
static int readytoread(void) {
  struct pollfd fds = {   
    .fd = 0,
    .events = POLLIN | POLLPRI,
    .revents = 0,
  };

  return poll(&fds, 1, 0) > 0;
}

@mcuee, It'll be sufficient to #include "poll.h" because "abc.h" falls back on <abc.h> if it wasn't found in local places.

@dl8dtl re the evaluating revents, I don't know what kind of things we should look out for to harden the code against; I'd run with above and see whether there are cases that need checking in practice. I've removed POLLHUP and POLLERR from events, as they are ignored there, but checked for and reported in revents anyway (according to man poll).

@stefanrueger
Copy link
Collaborator Author

stefanrueger commented Jul 16, 2022

Is there a way in cmake or otherwise to require/install the readline library? The GNU readline library seems widely available in UNIX type systems and on MacOS. Hopefully, someone can point out a compatible readline library in Windows, too.

Reason for this that otherwise we would need to provide a poor man's readline() with callback. Can be done, eg, using a reasonable source of fgets() and transform it to a function that gets characters fed in a similar way to readline's callback. Only, this is bound to be a poor terminal input experience. The readline() source has 52k lines (8k of which in configure) and caters well for all sorts of systems and I/O (in comparison AVRDUDE source has 72k lines 20k of which in avrdude.conf). Any 20-liner replacement will suck out of necessity.

[edit: AVRDUDE 7.0 has 78k lines]

@mcuee
Copy link
Collaborator

mcuee commented Jul 16, 2022

@stefanrueger
As mentioned before that mingw32/64 have readline.
https://github.com/msys2/MINGW-packages/tree/master/mingw-w64-readline

But MinGW does not have native good "poll.h" so you need to see if the free alternative implementation works or not. Usually the simple ones are based on winsock2.
https://docs.microsoft.com/en-us/windows/win32/api/winsock2/

For MSVC, there is a vcpkg formula which can be integrated into CMake but it may be easier to use @mariusgreuel's method of having a avrdude specific readline repo for MSVC build.
https://github.com/microsoft/vcpkg/tree/master/ports/readline
https://github.com/microsoft/vcpkg/tree/master/ports/readline-win32
https://github.com/lltcggie/readline (the real codes are here).

@mcuee
Copy link
Collaborator

mcuee commented Jul 16, 2022

@stefanrueger
Here is the output of MSY2mingw64 build of PR 1025 using readline. It does work similar to Linux with readline support.

 MINGW64 /c/work/avr/avrdude_test/avrdude_sr
$ ./build_mingw64_nt-10.0-22000/src/avrdude.exe -c usbasp -qqp m328p -t
avrdude> erase
avrdude.exe: erasing chip
avrdude> write eeprom 0 0xdeadbeef 'a' 'v' 'r' " Hello World\n" 0x00000002
avrdude> read eeprom 0 0x20
0000  ef be ad de 61 76 72 20  48 65 6c 6c 6f 20 57 6f  |....avr Hello Wo|
0010  72 6c 64 0a 00 02 00 00  00 ff ff ff ff ff ff ff  |rld ............|

avrdude> quit

$ git diff
diff --git a/src/term.c b/src/term.c
index f02f95c..eb98c14 100644
--- a/src/term.c
+++ b/src/term.c
@@ -1476,7 +1476,7 @@ int terminal_mode(PROGRAMMER * pgm, struct avrpart * p)
       return argc;
     }

-#if !defined(HAVE_LIBREADLINE) || defined(WIN32) || defined(__APPLE__)
+#if !defined(HAVE_LIBREADLINE)
     fprintf(stdout, ">>> ");
     for (int i=0; i<argc; i++)
       fprintf(stdout, "%s ", argv[i]);

@mcuee
Copy link
Collaborator

mcuee commented Jul 16, 2022

Under macOS homebrew, they seem to prefer you to use pkg-config or export the compiler flags.

mcuee@mcuees-Mac-mini readline % brew info readline
readline: stable 8.1.2 (bottled) [keg-only]
Library for command-line editing
https://tiswww.case.edu/php/chet/readline/rltop.html
/opt/homebrew/Cellar/readline/8.1.2 (48 files, 1.7MB)
  Poured from bottle on 2022-03-08 at 16:13:22
From: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/readline.rb
License: GPL-3.0-or-later
==> Caveats
readline is keg-only, which means it was not symlinked into /opt/homebrew,
because macOS provides BSD libedit.

For compilers to find readline you may need to set:
  export LDFLAGS="-L/opt/homebrew/opt/readline/lib"
  export CPPFLAGS="-I/opt/homebrew/opt/readline/include"

For pkg-config to find readline you may need to set:
  export PKG_CONFIG_PATH="/opt/homebrew/opt/readline/lib/pkgconfig"

==> Analytics
install: 418,591 (30 days), 1,327,493 (90 days), 7,175,307 (365 days)
install-on-request: 14,060 (30 days), 44,992 (90 days), 231,983 (365 days)
build-error: 7 (30 days)

mcuee@mcuees-Mac-mini readline % export PKG_CONFIG_PATH="/opt/homebrew/opt/readline/lib/pkgconfig"
mcuee@mcuees-Mac-mini readline % gcc `pkg-config --cflags --libs readline` termreadline_sr.c -o termreadline_sr
termreadline_sr.c:40:1: warning: non-void function does not return a value [-Wreturn-type]
}
^
1 warning generated.
mcuee@mcuees-Mac-mini readline % gcc `pkg-config --cflags --libs readline` termreadline_jw.c -o termreadline_jw
termreadline_jw.c:40:1: warning: non-void function does not return a value [-Wreturn-type]
}
^
1 warning generated.

mcuee@mcuees-Mac-mini readline % echo quit | ./termreadline_sr 2>&1 | cat                                      
avrdude> quit
quit: bye

mcuee@mcuees-Mac-mini readline % echo quit | ./termreadline_jw 2>&1 | cat                                      
avrdude> quit
quit: bye

@mcuee
Copy link
Collaborator

mcuee commented Jul 17, 2022

@stefanrueger
BTW, your partdesc branch seems to be useful as well but it is not portable under Windows since fnmatch is not avaiable for MinGW or MSVC. It works well under Linux and macOS.

@dl8dtl
Copy link
Contributor

dl8dtl commented Jul 17, 2022

Since it is only used to match a simple pattern on the part name, it's probably easy enough to write a replacement.

@mcuee
Copy link
Collaborator

mcuee commented Jul 17, 2022

Since it is only used to match a simple pattern on the part name, it's probably easy enough to write a replacement.

Indeed.
https://github.com/stefanrueger/avrdude/blob/partdesc/src/developer_opts.c#L808

    // pattern match the name of the part with command line: FMP_CASEFOLD not available here :(
    if(fnmatch(partdesc, p->desc, 0) && fnmatch(partdesc, p->id, 0))
      continue;

Maybe the following can work.
https://stackoverflow.com/questions/35877738/windows-fnmatch-substitute
https://docs.microsoft.com/en-us/windows/win32/api/shlwapi/nf-shlwapi-pathmatchspecw

#ifdef _WIN32
            wchar_t wname[1024];
            wchar_t wmask[1024];

            size_t outsize;
            mbstowcs_s(&outsize, wname, de->d_name, strlen(de->d_name) + 1);
            mbstowcs_s(&outsize, wmask, wildcard.c_str(), strlen(wildcard.c_str()) + 1);

            if (PathMatchSpecW(wname, wmask))
                result.push_back (std::string(de->d_name));
            else
                continue;
#else
                if (fnmatch(wildcard.c_str(), de->d_name, 0))
                    continue;
                else
                    result.push_back (std::string(de->d_name));
#endif

@stefanrueger
Copy link
Collaborator Author

fnmatch

@mcuee Thanks for looking at the partdesc branch and catching portability issues before I could even propose these developer options (amongst others, describe parts in a way that can be used as input for avrdude.conf). The tip with PathMatchSpecW() looks great.

@mcuee
Copy link
Collaborator

mcuee commented Jul 31, 2022

PR #1025 has been merged. PR #1040 has been proposed for the partdesc branch and now it is good for portability.

Going back to the topic of readline, once the issues are sorted out (eg: proper detection of readline using CMake; or mandatory requirement of readline -- need to sort out MSVC), then the following quick fix can be changed to properly support macOS and MinGW when readline support is available.

avrdude/src/term.c

Lines 1479 to 1484 in affe4cb

#if !defined(HAVE_LIBREADLINE) || defined(WIN32) || defined(__APPLE__)
fprintf(stdout, ">>> ");
for (int i=0; i<argc; i++)
fprintf(stdout, "%s ", argv[i]);
fprintf(stdout, "\n");
#endif

@mcuee
Copy link
Collaborator

mcuee commented Sep 5, 2022

@mariusgreuel
Maybe you can help answer the question related to using readline with CMake. Thanks.

@mariusgreuel
Copy link
Contributor

@stefanrueger While looking at you demo, I am wondering what your requirements are. If you just want a function called periodically, this is not going to be a problem on Windows. However, you probably need another thread, and this is not how you implemented it using libreadline, which I take is single-threaded by design?

I am not aware of a readline package for Windows. Googling gives me a GNU libreadline package for Windows from 2008. Lots of code, probably unmaintained, not sure if it will work at all.

I did a little research, and it looks like if you do a fgets(), you will end of with a call to ReadFile(), which blocks until \n is received. In other words, you cannot sneak in between, time-out, or handle another event. If you want a "peek, process, sleep" cycle like you showed in a demo, you need to implement the whole command-line editing stuff yourself, just as libreadline does. One can certainly do this using the Windows console API (and perhaps this is what libreadline for Windows does), but that is going to be a mammoth task, given what the Windows console is capable of.

So, would it help to have a simple timer calling you on a separate thread?

@mcuee
Copy link
Collaborator

mcuee commented Sep 6, 2022

I am not aware of a readline package for Windows. Googling gives me a GNU libreadline package for Windows from 2008. Lots of code, probably unmaintained, not sure if it will work at all.

Not exactly very new (actually very old) but at least part of vcpkg.
https://github.com/microsoft/vcpkg/tree/master/ports/readline
https://github.com/microsoft/vcpkg/tree/master/ports/readline-win32
https://github.com/lltcggie/readline (the real codes are here).

@stefanrueger
Copy link
Collaborator Author

stefanrueger commented Sep 6, 2022

@mariusgreuel

Would it help to have simple timer calling you on a separate thread?

no

TLDR; requirements for Windows:

  1. readytoread(void) from termreadlinev2.c works, implemented with POSIX poll()
  2. GNU readline library with rl_set_prompt(), add_history(), rl_callback_handler_instal(), rl_callback_read_char() and rl_callback_handler_remove()

This to give Windows users the same experience as UN*x users with respect to (1) running AVRDUDE terminal with bootloaders and (2) input for terminal mode.

The long of it. I have working code for LINUX (not yet in mainline) that relies on the mechanisms in the demo termreadlinev2.c.

  1. The main objective is to enable the terminal with bootloaders, which typically time out after 250..8000 ms of inactivity. One way (actually, the only one I could come up with) is to send every 100 ms a GETSYNC command to the bootloader (a kind of NOP) while waiting for terminal input from the user. At other times some processing and communications with the bootloader takes place anyway, which resets the WDT of the bootloader. Here is the snag: one cannot just send a GETSYNC command every 100 ms in another thread: that is bound to be in the middle of some useful comms with the bootloader at some point, and the interspersed GETSYNC command would break that comms. So it is natural to process GETSYNC handshakes during the wait for characters (and also only start processing the fully read input line once any just sent GETSYNC command is finished).

  2. The secondary objective is to get every user of AVRDUDE the same terminal experience irrespective of the operating system, for which a readline library is defo cool (history, line editing, correct echo on command line, ...). Turns out, gnu readline is an excellent library, which would already have been excellent in 2008 (at which time it would have been some 20 years old). I have no qualms using a 2008 version, and I predict that all the functions listed above will be there. GNU readline is large and feature rich, but also old, venerable and mature (arguably one the most-used library routine that people didn't know they used).

Failing 2. I can write a poor man's substitute for fgets(), which serves the first objective assuming int readytoread(void) works under Windows. I would write this in terms of single character reads using the kernel read() that won't block because read() would only be called when readytoread() asserts one char can be read without blocking. But that's certainly not the same experience as readline() for which my demo implements non-blocking reads.

@stefanrueger
Copy link
Collaborator Author

Would it help to have simple timer calling you on a separate thread?

Yes, if there is another readline() library that is incompatible with GNU readline. In which case a separate timer in another thread could send GETSYNC commands to the bootloader whilst readline() is active; requires synchronisation points between start/end of readline() and the other thread.

@stefanrueger
Copy link
Collaborator Author

My preference is GNU readline (because even an old version will be great)

@mcuee
Copy link
Collaborator

mcuee commented Sep 6, 2022

My preference is GNU readline (because even an old version will be great)

The vcpkg readline-win32 package is a port of GNU readline 5.0 for Windows. readline 5.0 was rather old since it was released in 2004.

Just wondering if you can take a look at the alternative which does support Windows along with Linux and macOS.
https://github.com/AmokHuginnsson/replxx

@mariusgreuel
Copy link
Contributor

https://github.com/lltcggie/readline

@mcuee Thanks for the links! I just tried the readline-win32 package and it worked (somewhat) out of the box. I can compile with a few changes, type characters and the callback works! However, when I pressed Ctrl-R is crashed. Seems like it is C code from stone-age, not ready for Windows 64-bit pointers :-(.

one cannot just send a GETSYNC command every 100 ms in another thread

@stefanrueger Absolutely! If multi-threaded, we would have to do the synchronization ourselves, which might be a bit of a problem, since I just got rid of pthreads during the MSVC port and I am not keen on bringing it back.

The secondary objective is to get every user of AVRDUDE the same terminal experience irrespective of the operating system

Not sure if I would agree here. IMHO, people are expecting it to work like it usually does on their OS. For command-line applications, Windows offers a built-in readline equivalent. Besides basic editing, there is the wildly popular F7 shortcut, which brings up the history. You get that and other stuff for free when you do a fgets() (*). With libreadline I get nothing when I press F7, but of course Ctrl-R, etc.

That said, I think the people who use avrdude in terminal mode can manage using libreadline. I guess I just prefer what the system offers, just saying.

(*) Turns out F7 is broken in avrdude using fgets(). Need to look into this...

@mariusgreuel
Copy link
Contributor

Turns out F7 is broken in avrdude

See #1097

@mariusgreuel
Copy link
Contributor

termreadline demo using Windows and vcpkg readline-win32 for Visual Studio 2022.
https://github.com/mariusgreuel/termreadline

vcpkg install readline-win32
vcpkg install readline-win32:x64-windows

Adjust CMAKE_TOOLCHAIN_FILE in CMakeSettings.json to point to your vcpkg.

@stefanrueger
Copy link
Collaborator Author

stefanrueger commented Sep 6, 2022

@mariusgreuel
Thanks for your port of the termreadline demo for Windows. Really cool.

same terminal experience irrespective of the operating system

people are expecting it to work like it usually does on their OS

We are talking cross purposes. Same experience for me means having both, terminal for bootloaders and command line editing. As fgets() contains command line editing in Widows but no callback for asynchronous GETSYNC calls to the bootloader, it looks like either terminal for bootloaders or command line editing in Windows.

My preference is still to go along with the GNU readline library, as it seems readily available in Linux, FreeBSD, MacOS and --- with your port --- also Windows. Has the benefit of (almost) the same code everywhere. Happy to follow your lead for alternative solutions in Windows, though. It will be a few weeks before I present a PR implementing terminal for bootloaders, so perhaps best to decide then how this looks in Windows.

For the time being this has given me confidence that we can rely on availability of readline() in all OS that we cater for.

@mariusgreuel
Copy link
Contributor

My preference is still to go along with the GNU readline library

I have to admit, your solution is dirt-simple.

this has given me confidence that we can rely on availability of readline() in all OS that we cater for

I agree. No matter how the Windows solution is going to look like, I will not require a redesign of your code.

@stefanrueger
Copy link
Collaborator Author

Quick question @mariusgreuel In your code

https://github.com/mariusgreuel/termreadline/blob/792caef19efa003487b18e8b2684090299a88b25/termreadline.c#L62-L65

does _kbhit() only look at the physical keyboard or does it look at standard input (file descriptor 0)? This routine really needs to figure out whether the next read(0, ...) will block or not be it from a pipe or redirected from a file.

@stefanrueger
Copy link
Collaborator Author

Reading up on _kbhit() makes me think it does not work with stdin on Windows (and rather returns the whether reading from that is defined as console blocks). So, I suspect @mariusgreuel 's port needs to be changed so that

echo quit | termreadline
avrdude> quit
quit: bye

works in Windows.

@mariusgreuel
Copy link
Contributor

This routine really needs to figure out whether the next read(0, ...) will block or not be it from a pipe or redirected from a file.

I see what you mean. _kbhit() is not going to work, just tried it.

@mariusgreuel
Copy link
Contributor

Hm, this was a little more complicated than expected. Checking for an event via WaitForSingleObject(hStdin, 0) (the equivalent of select()) did not work, because libreadline does not handle all console events and then blocks, for instance when a key-up event is in the queue.

I managed to make it work by inspecting the libreadline code, and filtering all events but those that are actually handled. I pushed another change to my repo, however, it only partially works: The EOF detection does not return NULL, so an explicit quit is required in the input. Saving this for some other time.

https://github.com/mariusgreuel/termreadline

@stefanrueger
Copy link
Collaborator Author

I pushed another change to my repo

Thanks, but I am not seeing a change in your termreadline port?

@mariusgreuel
Copy link
Contributor

Thanks, but I am not seeing a change in your termreadline port?

Ah, forgot to push it ;-)

@stefanrueger
Copy link
Collaborator Author

Wow, great prototyping and good progress.

I am sure you are aware that now that readytoread() returns -1 on error, and no longer just 0 and 1, this requires a change of how it is called in https://github.com/mariusgreuel/termreadline/blob/fc05ee8082d90aaa42f541df1d823499bc45f13c/termreadline.c#L153-L154

I predict (but don't know) that this works when the input comes from a file, and even the EOF detection will work there. And, yes, suspect, this needs some further work if input is from a pipeline: https://github.com/mariusgreuel/termreadline/blob/fc05ee8082d90aaa42f541df1d823499bc45f13c/termreadline.c#L74-L78

Reading from a pipeline can make the process sleep. One needs a peek ahead there...

@stefanrueger
Copy link
Collaborator Author

The EOF detection does not return NULL

Is that in the terminal, when reading from file or when reading from a pipeline? In the terminal it's usually a certain character at the beginning of a line that signifies EOF. Is that still ctrl-Z in Windows?

@mariusgreuel
Copy link
Contributor

and even the EOF detection will work there

I think the problem is the WIN32 implementation of rl_getc(): On EOF, it does not return EOF but zero.
https://github.com/mariusgreuel/readline/blob/vs/src/readline/5.0/readline-5.0-src/input.c#L618-L620

It seems that the library needs some work, but it appears that it can be done with a reasonable amount of effort. I will fix the library when I find some time.

@mcuee
Copy link
Collaborator

mcuee commented Oct 21, 2022

@stefanrueger

Just wondering if it is possible to support BSD libedit (https://thrysoee.dk/editline/) under macOS without using readline.

Of course the easier solution is to use readline with some CMake magic that hopefully @mariusgreuel can help out.

@stefanrueger
Copy link
Collaborator Author

@mcuee Have you tried compiling PR #1132 with libedit instead of libreadline? Really, the important bit is the ability to do callbacks:

  • rl_callback_handler_install("avrdude> ", term_gotline);
  • rl_callback_read_char();
  • rl_callback_handler_remove();

If these three functions exist, and add_history(cmdstr), then all should be good.

@mcuee
Copy link
Collaborator

mcuee commented Oct 22, 2022

@mcuee Have you tried compiling PR #1132 with libedit instead of libreadline?

Ah, maybe I did not make it clear enough. Under macOS, PR #1132 will use libedit. I have not figured how to force it to use readline.

mcuee@mcuees-Mac-mini avrdude_bin % otool -L avrdude_pr1132_libedit 
avrdude_pr1132_libedit:
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1311.100.3)
	/opt/homebrew/opt/libusb-compat/lib/libusb-0.1.4.dylib (compatibility version 9.0.0, current version 9.4.0)
	/opt/homebrew/opt/libusb/lib/libusb-1.0.0.dylib (compatibility version 4.0.0, current version 4.0.0)
	/opt/homebrew/opt/libhid/lib/libhid.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/opt/homebrew/opt/hidapi/lib/libhidapi.0.dylib (compatibility version 0.0.0, current version 0.12.0)
	/opt/homebrew/opt/libftdi0/lib/libftdi.1.dylib (compatibility version 22.0.0, current version 22.0.0)
	/opt/homebrew/opt/libftdi/lib/libftdi1.2.dylib (compatibility version 2.0.0, current version 2.5.0)
	/usr/lib/libedit.3.dylib (compatibility version 2.0.0, current version 3.0.0)

The result avrdude is not working very well -- I need to hit RETURN after every commands. So there are some subtle differences between libedit and readline.

Really, the important bit is the ability to do callbacks:

  • rl_callback_handler_install("avrdude> ", term_gotline);
  • rl_callback_read_char();
  • rl_callback_handler_remove();

If these three functions exist, and add_history(cmdstr), then all should be good.

Yes all these functions exist in libedit. Take note libedit does not support Windows. It does support Linux, macOS and BSDs.
Reference:
https://thrysoee.dk/editline/
http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libedit/readline/readline.h?rev=1.53&content-type=text/x-cvsweb-markup&sortby=date

@mcuee
Copy link
Collaborator

mcuee commented Oct 22, 2022

I have not figured how to force it to use readline.

I have figured out a way to force it to use readline, but it is a manual process (similar to what I do under Windows using MSYS2). Once using readline and not libedit, PR #1132 works well under macOS.

@mcuee
Copy link
Collaborator

mcuee commented Oct 22, 2022

@stefanrueger
If you want to giev libedit a try, you can easily do that under Linux by install libedit development package (`sudo apt install libedit-dev" under Ubuntu/Debian).

 2043  cmake -D CMAKE_BUILD_TYPE=RelWithDebInfo 
-D HAVE_LIBREADLINE:FILEPATH=/usr/lib/x86_64-linux-gnu/libedit.so  -B build_linux_libedit
 2044  cmake --build build_linux_libedit
 2045  ldd ./build_linux_libedit/src/avrdude

mcuee@UbuntuSwift3:~/build/avr/avrdude_sr$ ldd ./build_linux_libedit/src/avrdude
	linux-vdso.so.1 (0x00007ffcb1fd9000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fbc8c12f000)
	libelf.so.1 => /lib/x86_64-linux-gnu/libelf.so.1 (0x00007fbc8c113000)
	libusb-0.1.so.4 => /usr/local/lib/libusb-0.1.so.4 (0x00007fbc8c10b000)
	libusb-1.0.so.0 => /usr/local/lib/libusb-1.0.so.0 (0x00007fbc8c0eb000)
	libhidapi-libusb.so.0 => /usr/local/lib/libhidapi-libusb.so.0 (0x00007fbc8c0e0000)
	libftdi.so.1 => /lib/x86_64-linux-gnu/libftdi.so.1 (0x00007fbc8c0d4000)
	libftdi1.so.2 => /usr/local/lib/libftdi1.so.2 (0x00007fbc8c0c3000)
	libedit.so.2 => /lib/x86_64-linux-gnu/libedit.so.2 (0x00007fbc8c08b000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fbc8be99000)
	/lib64/ld-linux-x86-64.so.2 (0x00007fbc8c38e000)
	libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x00007fbc8be7d000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fbc8be5a000)
	libudev.so.1 => /lib/x86_64-linux-gnu/libudev.so.1 (0x00007fbc8be2b000)
	libtinfo.so.6 => /lib/x86_64-linux-gnu/libtinfo.so.6 (0x00007fbc8bdfb000)
	libbsd.so.0 => /lib/x86_64-linux-gnu/libbsd.so.0 (0x00007fbc8bde1000)

Edit to add: but there are no issues with libedit version of PR #1132 under Ubuntu 20.04. So this looks like an issue specic to libedit under macOS. I will check out the Homebrew version of libedit to see if that is okay or not.

Edit to add: unfortunately libedit Homebrew version has the same issue as the system version.

@MCUdude
Copy link
Collaborator

MCUdude commented Nov 8, 2022

Sorry for bringing this up, but is libreadline now required for terminal to work properly on macOS?

I'm building using make, and when in terminal mode, I'll have to hit enter twice to get the avrdude> prompt.
When typing, the text appears on the line below.

There was never an issue with terminal mode before?

avrdude: AVR device initialized and ready to accept instructions
avrdude: device signature = 0x1e9705 (probably m1284p)
avrdude> ?
>>> ? 
Valid commands:
  dump    : dump <memory> [<addr> <len> | <addr> ... | <addr> | ...]
  read    : alias for dump
  write   : write <memory> <addr> <data>[,] {<data>[,]}
          : write <memory> <addr> <len> <data>[,] {<data>[,]} ...
  flush   : synchronise flash & EEPROM writes with the device
  abort   : abort flash & EEPROM writes (reset the r/w cache)
  erase   : perform a chip erase
  sig     : display device signature bytes
  part    : display the current part information
  send    : send a raw command: send <b1> <b2> <b3> <b4>
  parms   : display adjustable parameters
  vtarg   : set <V[target]>
  varef   : set <V[aref]>
  fosc    : set <oscillator frequency>
  sck     : set <SCK period>
  verbose : change verbosity
  quell   : set quell level for progress bars
  help    : show help message
  ?       : same as help
  quit    : quit after writing out cache for flash & EEPROM

Note that not all programmer derivatives support all commands. Flash and
EEPROM type memories are normally read and written using a cache via paged
read and write access; the cache is synchronised on quit or flush commands.
The part command displays valid memory types for use with dump and write.


avrdude> 
quit
avrdude> quit
>>> quit 

avrdude done.  Thank you.

@mcuee
Copy link
Collaborator

mcuee commented Nov 9, 2022

Sorry for bringing this up, but is libreadline now required for terminal to work properly on macOS?

I'm building using make, and when in terminal mode, I'll have to hit enter twice to get the avrdude> prompt. When typing, the text appears on the line below.

There was never an issue with terminal mode before?

Yes now libreadline is required. libedit is not compatible with #1132 unfortunately.

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

Successfully merging a pull request may close this issue.

5 participants