-
Notifications
You must be signed in to change notification settings - Fork 371
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
Flush buffered standard steams #489
Comments
You could suggest to your user to try stdbuf. It is apparently included for Windows in msys2. It should work if the program was built with MINGW, but not if MS build tools were used.
|
Thanks for the quick response. Could you share somewhere simavr built with |
My understanding is that Msys2 is just MINGW with pre-built GNU utilities. So you may already have it. As far as I can see, there is no simavr documentation on building for Windows, but I got the impression that MINGW is the easy, perhaps the only, way. I have not tried myself. |
@adbancroft, could you try this instruction https://github.com/buserror/simavr/blob/master/README.mingw ? Does this work for you? |
I had forgotten those build instructions, as they are in a separate file. But I suggest looking at the fork by maxgerhardt or merging PR #460 as the MINGW instructions are much newer. |
@gatk555 could you merge PR #460? The @maxgerhardt is from PlatformIO Community. @maxgerhardt , could you help us with the new binaries for SimAVR? We can remove the warning from https://docs.platformio.org/en/latest/advanced/unit-testing/simulators/simavr.html Thanks in advance! |
I can rebase my branch (which just fixes building) against the latest version of this repo and then build + test Windows x86 + x64 binaries, sure. |
The only place I could merge that would be in my own fork, but the target file (README.md) is completely different there. It could be used to update README.mingw. I think that would be a better target for the change and that may be the reason it has not yet been merged here. But MP usually comments with a reason for turning down a PR. |
I rebuilt the PlatformIO tool-simavr package for Win x64 with the latest branch plus my build fixes plus gatk555's logging fixes and execution on the commandline looks good. A simple Arduino firmware that prints 10 times per seconds executes nicely with it and with correct timing (when invoked via the commandline). I also see that the firmware output now goes to stdout instead of stderr. (Redirecting stderr to
However, when PlatformIO calls into the binary (and simavr opens the GDB server), then
And in general, simavr seems to postfix firmware output with " See https://github.com/maxgerhardt/pio-simavr-testing for reproduction. Any ideas here? :/ |
I solved this problem in PICSimLab using a modified avr_callback_run_gdb, I believe it can be applied as PR in simavr, what's your opinion @gatk555 ? |
@maxgerhardt wrote:
This is meant to represent the p->stdio_out[p->stdio_len++] = v < ' ' ? '.' : v; |
A PR to fix the "real-time" behaviour with gdb sounds like a good idea to me. You may run into a conflict with merged PR #482. I did not worry about timing behaviour there, except to fix buzzing, as it seemed to be already broken. It also looks a good idea to remove those trailing dots. I think there are two places where they can occur, for UART and "console" output. |
I think simavr is more usefull if it outputs the firmware's intended output 1:1. Then you can even do things like having a firmware produce some binary output and pipe the simavr (= firmware output) into another program for processing or logging etc. I'll try and patch that in plus try and copy/paste the modified avr_callback_run_gdb. Ah and, I've started setting up Msys2 MinGW64 + MinGW32 github action builds in https://github.com/maxgerhardt/simavr/actions/runs/2303871484, it already produces binaries. Edit: Linux x64 and x86 builds were also added in https://github.com/maxgerhardt/simavr/actions/runs/2310166991 |
I think the conversion to dots is very useful when running interactively and the firmware is misbehaving. That is a common case, so "raw mode" should be a configurable option. If it also made stdout unbuffered it would fix the original issue here. Windows binaries ought to be popular, but how will you advertise their availability? A PR to the README would be best, but slow. A Wiki entry perhaps. |
Best case, the CI build things get merged back and thus every commit will have downloadable artefacts for every major OS and architecture, plus the simavr release page will have the downloadable artefacts as well (see e.g. here). Advertising my fork doesn't make sense, it has to go back into mainline.
I agree. Though I still think for a common text-things such as |
The last round of merges was a month ago, with the one before 8 months previous. So I think it would make sense to advertise the fork until the auto-build configuration is merged. |
If anyone want to make a PR for a RAW mode, more than welcome. Either as a function call, an IOCTL, or even an environment variable. The 'ascii filter' was made at at time my test program /was/ outputting binary strings over the UART, so it made perfect sense then. |
I made a crude addition of a I tried to backport @lcgamboa fixes for execution speed in GDB server mode in maxgerhardt@e036e96, which restored nominal execution speed indeed (👍), but broke GDB's ability to pause the running code execution. Once I do Old
After patch
Any ideas there? (Binaries for each patch state are in here). |
It seems you merged a change from a fork that has not merged PR #482 into one that has, and that has partially reverted the PR. Control-C handling was modified in the PR, so it is no surprise that it is now broken. In fact I am surprised gdb works at all. If I understood correctly, you are trying to fix the timing of cpu_sleep() when running with gdb. I hacked-up one of simavr's test firmwares to make a program that writes to the UART every two seconds. With today's build of simavr upstream, I see no difference in timing with gdb. I am running like this: ../simavr/run_avr --gdb atmega88_timer16.axf & Here is a link to the file: sleep_test.c |
That's weird, I'm basically running it the same way you are, but with my firmware at https://github.com/maxgerhardt/pio-simavr-testing. Can you test with for ATMega328P and the ELF file in firmware.zip? I do
goes fast, and
with With exactly the current upstream repo I'm getting the ultra slow behavior. Print time is 10 times per second. |
CC @ivankravets I got CI working for all previously supported operating systems in here, I plan to add |
In my modification I only call the |
Yes, that one is more than 10 times slower when run with gdb. And my guess is also that making a system call for every AVR instruction is what slows it down. The Arduino library is busy-waiting so it suffers badly, but mine spent almost all of its time in cpu_sleep() and executes very few instructions. (I got 72Mb of simavr tracing output in a few seconds and there is no sleep instruction.) |
Calling gdb_network_handler() once every 100 instructions makes the speed reduction about 30%, which seems acceptable. |
@maxgerhardt tested on macOS - works great! P.S: You are the monster with this CI build config => https://github.com/maxgerhardt/simavr/blob/combined_fixes_and_ci/.github/workflows/build.yml I even forked your repo to keep it as a gist 🙏 |
@maxgerhardt does it make sense to update SimAVR in the PlatformIO Regsitry by your build? We could remove this warning before PlatformIO Core 6.0 which is planned to be released this Monday. |
checking for input from GDB. Discussed in buserror#489.
I tidied up my initial change and pushed to by fork as branch "gdb_burst". I would make a PR but the only validation is firmware.zip, which seems meagre. If anyone here has test material or wants to develop it further, that will be welcome. |
@ivankravets I have the CI building all PlatformIO |
Do you mean to not hurry with updating the SimAVR package in the registry? |
Exactly. I'll update once I can verify that binaries are working well with PlatformIO+VSCode/GDB debugger. |
Is this work still in progress? |
I haven't actively worked on this since the last post, had to do other things. The issue may still persist in the current mainline. |
There are several useful changes that may come from this thread. Going into some detail they are: make stdout unbuffered, preferably controlled by an option; remove translation of non-printable firmware output to dots, again should probably be optional; drop, rather than translate, CR/LF characters in firmware output; improve execution speed when the gdb server is active; CI configuration so that new binaries are built when the source code changes. The first three could be combined in a single PR. Mine is now PR#499, faster running with gdb. |
checking for input from GDB. Discussed in issue buserror#489.
Hi,
Firstly, a big THANK YOU for the amazing project! Glad to see that @platformio users found it useful.
See initial platformio/platformio-core#3965 posted by @adbancroft.
There is a problem with data buffering on the SimAVR/Windows side. Possible solutions:
Please note that it works great on Unix.
How to reproduce?
I've just created a simple Python script and tried to redirect standard streams to the PIPE. Subsequent reading of 1 byte hangs Python script. See code
simavr.py
Call SimAVR using Python bridge/script:
The text was updated successfully, but these errors were encountered: