-
Notifications
You must be signed in to change notification settings - Fork 141
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
Comments
MacOS is a UNIX (even a certified one, IIRC). Other remarks (I know it's not production code, but nevertheless):
|
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. |
If you replace For WinAPI, we need someone knowledgable about it, to propose something that can then be |
Here's the diff for poll vs. select, it's really pretty trivial.
(Edit: I forgot the > 0 comparison before.) |
Maybe revents should be checked for one of the exceptional events as well. |
@stefanrueger For example, I can build under MSYS2 mingw64 of your code with a quick patch but the result exe does not work properly.
|
@dl8dtl and @stefanrueger Not so sure if simpler one works or not. |
Maybe it can be used with minor modifications.
termreadline_jw.c.txt For Linux, both versions work fine.
|
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:
@mcuee, It'll be sufficient to @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 |
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 [edit: AVRDUDE 7.0 has 78k lines] |
@stefanrueger 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. 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. |
@stefanrueger
|
Under macOS homebrew, they seem to prefer you to use pkg-config or export the compiler flags.
|
@stefanrueger |
Since it is only used to match a simple pattern on the part name, it's probably easy enough to write a replacement. |
Indeed.
Maybe the following can work.
|
@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. |
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. Lines 1479 to 1484 in affe4cb
|
@mariusgreuel |
@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 So, would it help to have a simple timer calling you on a separate thread? |
Not exactly very new (actually very old) but at least part of vcpkg. |
no TLDR; requirements for Windows:
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.
Failing 2. I can write a poor man's substitute for |
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. |
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. |
@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 :-(.
@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.
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 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 |
See #1097 |
termreadline demo using Windows and vcpkg readline-win32 for Visual Studio 2022. vcpkg install readline-win32
vcpkg install readline-win32:x64-windows Adjust |
@mariusgreuel
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. |
I have to admit, your solution is dirt-simple.
I agree. No matter how the Windows solution is going to look like, I will not require a redesign of your code. |
Quick question @mariusgreuel In your code does |
Reading up on
works in Windows. |
I see what you mean. |
Hm, this was a little more complicated than expected. Checking for an event via 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 |
Thanks, but I am not seeing a change in your termreadline port? |
Ah, forgot to push it ;-) |
Wow, great prototyping and good progress. I am sure you are aware that now that 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... |
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? |
I think the problem is the WIN32 implementation of 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. |
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. |
@mcuee Have you tried compiling PR #1132 with libedit instead of libreadline? Really, the important bit is the ability to do callbacks:
If these three functions exist, and |
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.
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.
Yes all these functions exist in libedit. Take note libedit does not support Windows. It does support Linux, macOS and BSDs. |
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. |
@stefanrueger
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. |
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 There was never an issue with terminal mode before?
|
Yes now libreadline is required. libedit is not compatible with #1132 unfortunately. |
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: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?
The text was updated successfully, but these errors were encountered: