GideonZ / 1541ultimate

Official GIT archive of 1541 ultimate II sources
GNU General Public License v3.0
178 stars 45 forks source link

Virtual printer fixes and enhancements #345

Closed rgc2000 closed 9 months ago

rgc2000 commented 1 year ago

Fixes:

Enhancements:

rgc2000 commented 1 year ago

Maybe you can see in this merge request that the first 21 commits are not related with the printer enhancements I have done these last weeks. I don't know why git is showing them as they are not my own and they are already part of the Gigeon's master branch. If you browse the file changed list, you can see that no files are changed by those commits. @GideonZ if this disturbs you I can try to create a new branch from your master and to commit only printer changes but I would lose the changes history.

The most noticeable changes are :

radius75 commented 1 year ago

In my opinion, the {Files changed} tab is the most important here. Here are shown what changes to "GideonZ/1541ultimate:master" will result from the Request #345 Merge option. Only this should be carefully analyzed before Merge :) https://github.com/GideonZ/1541ultimate/pull/345/files

And all those "weird" commits in your PR are due to you updating your branch by fetching new changes from GideonZ/1541ultimate:master.

Take a look at rgc2000/1541ultimate:master in Network, you can move the view with the mouse. https://github.com/GideonZ/1541ultimate/network

radius75 commented 1 year ago

Here. You updated your Master without doing a Fetch with Gideon/master. That's why you have these Commits at your place. You must analyze all changes in the {Files changed} tab to confirm that you have not accidentally included a Commit that is not yet Printer (eg from other Repos / Branches) and should not be added to Gideon:master.

If you see commits unrelated to "Printer", if they have already been added to Gideon/master then, That's no problem, you just won't see them in the {Files changed} tab. obraz

rgc2000 commented 1 year ago

I have carefully reviewed all the changes in the "Files Changed" tab and everything is correct as expected. No extra or unwanted changes reported. I guess the those mysterious commits are the consequence of manual local fetches from remote origin repository to sync my branch with Gideon's master instead of using pull requests in the web site. Now I use the pool request feature to sync with latest changes.

rgc2000 commented 1 year ago

pull request updated to solve conflicts if merged with 3.10i tested successfully on U2_RiscV and U2+

rgc2000 commented 10 months ago

New enhancements added to this pull request to answer request #383 :

GideonZ commented 10 months ago

I created a merge with the latest Wifi feature branch in "printer_enh". I think it won't work yet, because the initialization is not done in the right order. The IEC is now initialized by means of an InitFunction, and the reference is now a pointer. However, the constructor of the IEC already references the printer pointer, which is probably still NULL.

rgc2000 commented 10 months ago

I'm going to take a look at the printer_enh branch and check if init has to be changed. It should not be a big issue as the printer class instance is a singleton. It should be created at first access if not already done. I know that I have done something that was not dependent of the IEC object init order.

GideonZ commented 10 months ago

I am sure you did it all correctly.. but in my merge operation, I screwed it up. I didn't have time anymore to fix it as I needed to do other things.

But... that said; I'd like to make it a bit cleaner. I'd like to separate the IecInterface from the drive completely. The drive is, just like the printer, a slave to the IecInterface. As you can see, in the IecInterface class the channels are created. These are not a property of the IecInterface but a property of the drive. Which brings me to the first question: Does a printer have channels at all? Could a printer have a status channel? (e.g. to report out of paper, out of ink, paper jam, printer type, whatever...).

Separating the IecInterface from the drive will also allow two instances of the drive to be attached to the interface. Or a third, completely different type of device.

rgc2000 commented 10 months ago

Mmm, I was too confident. I need to understand how your InitFunction class works and then found a solution to make the IEC and Printer objects work together, regardless of initialization order.

GideonZ commented 10 months ago

InitFunction: I think there is a parameter that can be passed to give relative order. The list is first sorted, then executed.

Clean: If the drive and printer "register" themselves to the IEC driver, then the IEC driver could create itself with a singleton construction. In this case, the order is always guaranteed. Plus... this way, the IEC driver is never instantiated when there are no clients.

rgc2000 commented 10 months ago

Trying to compile your code out of the git depot : It compiles with no error for u64, u2+ and u2+L But I am unable to compile for u2 :

markusC64 commented 10 months ago

I have to confirm that "printer_enh" does not compile for the U2 RiscV.

I cannot tell about microblaze because I don't care about microblaze. AFAIK microblaze has passed EOL.

GideonZ commented 10 months ago

Just don't flash these builds as they are now, as the init will crash and thus brick the unit.

markusC64 commented 10 months ago

BTW: Branch cli fails on the U2 with the same error.

GideonZ commented 10 months ago

The CLI branch is stale... it has been merged in the wifi branch, which has been merged into printer_enh:

image

markusC64 commented 10 months ago

I doubt building the wifi branch without a newer FPGA blob is a really good idea... but the cli branch does not necessarily require an fpga never than the one contained in the last release. And for the platforms it builds, it works quite well (haven't tested too much).

GideonZ commented 10 months ago

For the U64 FPGA, I think it is better to leave it out of the git repository and use a mechanism to fetch it somewhere else.. OR maybe as an intermediate solution use git LFS. Do you have any ideas about this, Markus?

markusC64 commented 10 months ago

If I would have to decide, I would do it in the following way:

1) Remove the external/* file (there is only one) 2) Add "external/README" with some content, just that the folder is non empty. 3) Change the top level makefile to use "curl" to fetch the latest fpga and place it inside the external folder if and only if the fpga blob does not exist.

Note: That way, one can also uses an archived fpga.

4) For the U2+ / U2+L builds: If there is an FPGA blob within the /external directory for that device, use that instead of building one. So builds are much faster when they do not change the fpga - assuming that the user of the compiler has placed his last fpga output inside the "external" directory. If it simplifies things, the two cases could be separate make targets.

5) If possible, another top level target could fetch a prebuild fpga for U2+ / U2+L using curl - to speed things up even more.

rgc2000 commented 10 months ago

I fixed the IEC and printer object initialization dependency. IEC no longer needs to call the init_done() print method. This method has been removed because the printer now has another way to know that initialization has been completed for IEC and the printer.

I have tested this on my own master branch, not a clone of the printer_enh branch. I don't know if the whole firmware is stable enough to upgrade my U2+. Can it brick the device ? I guess that wifi is only for U64. I prefer testing it on the U2 as the ultimate.bin loading facility on boot makes the tests much easier.

@GideonZ to answer about printer channel : the printer never talks on IEC, it is only receiving data from the IEC master. There is no status channel to inform the C64 that the printer is out of paper/ink, etc... IEC can send 2 type of information to the printer : data and commands. Data are the bytes sent to printer by the program running on the C64 they are interpreted by the printer emulation according to the EPSON/CBM/IBM instruction set selected by the user. Commands are only 4 possible : open printer, close printer, cursor up and cursor down. I am using open and close to open/close output file when printing in RAW/TXT mode. Cursor up/down are used to communicate to the printer the secondary device address used to select upper/lower case charset when printer is working with CBM emulation.

markusC64 commented 10 months ago

U2+ (not U2+L) is the only device that is safe for testing firmware builds you aren't sure. If they do not work, you can use the integrated recovery firmware of the U2+ by pressing the middle button of the U2+ while powering on. Even when the firmware has bricked your U2+, that should work (yes, I have bricked my U2+ several times, and that always worked). Well, unless you modify the installer to update the recovery firmware, too.

So if you think you want to test, I'd advise to use the U2+ for that. For any other ultimate (U2, U2+L, U64) you need additional hard- and software for recovery.

markusC64 commented 10 months ago

I prefer testing it on the U2 as the ultimate.bin loading facility on boot makes the tests much easier.

Well, testing on the U2+ is even easier if you have an usb blaster. Then you can trasfer the new application by wire and have it started any time. Without having to powercycle the C64. So in case I expect several attempts before my change works, I prefer U2+ (or U64, it's the same on U64) over U2 and U2+L.

rgc2000 commented 10 months ago

I have just tested my changes on U2+ with 3.11⍺ and printer seem to work fine. I have noticed a small bug, disabling IEC drive from F5 action menu has no effect. I am going to clone that branch in my git repo to be able to send you the fixes.

rgc2000 commented 10 months ago

Pull request #385 created to fix init sequence (Now I remember why I prefer U2 against U2+ to test and improve the printer : because printer is much faster on U2 !)

rgc2000 commented 10 months ago

A small bug fix added to the pull request : when turning off/on the IEC or Printer from the action F5 menu, the F2 Setting page may not show the current status (Enabled/Disabled) for IEC or Printer.

Bad news : I think I will not be able to do more dev/tests for a while. I have fried my C64, I have to find a new one. I am still able to access the Ultimate setting but it is not stable and the C64 side does not work. Something wrong with VIC-II or CIA, I don't know.

rgc2000 commented 9 months ago

All code changes have already been merged to Gideon master branch. The only changes remaining have been added later for the doc directory to explain how to build the standalone virtual printer on a Linux system.