Dynamic card reading in I650

[Rhialto]

This is a first go at dynamically reading cards for the use of the carddeck command. Discussion on how to do this better is invited.

There are some ugly bits in this. To read a card you need a UNIT and a UNIT needs a DEVICE. So we allocate and free those dynamically. But it seems to be a bit too intimate in the implementation of devices. Also they don't seem to be meant to be very dynamic, so maybe there are bugs because of that.

Possibly some of this code better belongs in sim_card but then it may need some generalisation.

The original code already used a CDR (card reader) to punch cards, even though there is also a CDP device available (however it has no cdp_attach() function, so you cannot currently create more of them). I'm not sure how to use a CDP to write files or if it would even work; sim_card internally has no problems to write via a CDR.

The test code used overlapping input and output file names when splitting or joining card decks. I am forbidding that (and issue an error message). I don't think this is a great limitation since there is a command to rename files.

I adjusted the ini files used for the self test to correct for this.

I left the binary .doc file as it is; binary files are not very suited for tracking in version systems like git.

[Rhialto]

The diffs for I650/i650_sys.c are unfortunately difficult to read if you show them "inline" or "unified" as github calls it. Side-by-side (or "split") works better but needs a wider window.

[rcornwell]

1. I would change sim_ascii_to_hol to ascii_to_hol since this is not part of SCP.
2. deck_card_read could be replaced with a call to sim_read_card. There is no need to clear upper bits of the card.
3. At 898 move this to beginning of function. Inline defines are not supported by all C compilers.
   3a) I see this at various places, these should be moved to beginning of the block.
4. r = deck_read_card... could be replaced with r = sim_card_read(&dev0.units[0], CardImage); if (r == CDSE_EOF || r == CDSE_EMPTY) {...

Other than these changes I like it.

[Rhialto]

1. I guess it was given that name because there is also already an array called ascii_to_hol... so this will be multiple renames over several files (expanding the scope of the MR a bit). But I can surely do this.
2. Good to know about the clearing of the bits. What's left are two little bits of abstraction where we start with the device and deduce the unit from it, and making EMPTY the same as EOF. That may or may not be worth it to keep the function. My intention was to just take as argument whatever deck_open() gives you and allow the caller to be ignorant of what it is. Over time I had different types for this but I ended up with a DEVICE *.
3. Please allow me a little rant here: declarations don't have to be at the start of a block any more since C99. Any compiler that doesn't understand this literally 25 years later is not a C compiler. Yes I know that this implies that MS didn't make a C compiler for many many years (although I understand that their current attempt does support this). I have very little patience for MSVC's deliberate brokenness (since I don't believe that they were unable to implement C99 if they had wanted). I am not aware of any other C compiler for a relevant system that doesn't reach the level of C99. (Yes of course I can change it but I really hate kowtowing to Microsoft)
4. This follows from item 2. And I suppose that if we expand the body of deck_card_read we should equally do this with deck_card_write (but similar arguments are against it).

Further opinions and/or arguments?

[rcornwell]

For 2 I think it might make things clearer if you move the check for EOF and EMPTY to where you call deck_read_card... also now that I think of it, I would rename this to read_card since you are not reading in a deck but a single card.
For 3 I think it is good practice in C to declare all variables at beginning of block. If they are declared in line it might make code harder to read. It does make sense in C++ since you might be allocating this and not want to have it created unless needed.
For 4 I can agree with keeping deck_read_card that just used the device, but you probably already have the unit pointer/structure.

[Rhialto]

Oh, what I wanted to ask: In deck_split_cmd() I commented a line with XXX. We need to count the number of cards in the input deck, and then we need to read them for real. (Strictly speaking we only need to count them if the user gives a negative number cards, because then we need to leave the last nCards1 cards for the second output file, but the first copy loop is a bit easier if we always know the right number).
So currently I close the deck and re-open it. But is there some way to use the caching nature of sim_card, and just reset the deck to the beginning? Or using some function from sim_card to do something like that?

[rcornwell]

There is already a function in sim_card to return the number of cards in the deck.
sim_hopper_size returns size of hopper.
sim_card_input_hopper_count, returns the number of cards yet to be read.

[Rhialto]

Ah very good! I changed the code to use that.

[Rhialto]

On the other hand, if there is a card with EOF marking in the input, the original code would use only the cards before the EOF, but when using sim_card_input_hopper_count(), it would count the EOF card and the following ones too (and expect to copy them). What would be expected behaviour for such a case?

[rcornwell]

When you attach to a deck, if you don't specify a EOF option you will not get one added to the deck.

[Rhialto]

What I meant was the following. Suppose you have the following deck:

6I1954195C      0000240000800?6019540009   0   rau 1954 1f          0134-
6I1954195C      0000240001800?8809990976   1   racreada psudo       0135-
6I1954195C      0000240002800?6500310085   2   ral8f    1b          0136-
6I1954195C      0000240003800?6900870097   3   lodaton  putag       0137-
6I1954195C      0000240004800?6500310085   4   ral8f    1b          0139-
6I1954195C      0000240005800?1604040753   5   slon0063 7b          1378-
6I1954195C      0000240006800?3500020016       slt 2                0178-
6I1954195C      0000240007800?2417110197       stdotend             0170-
6I1954195C      0000240008800?2003142219  3    stlinstr  219 a      0452-
6I1954195C      0000240009800?8000000014  1    raa 0                0141-
~
6I1954195C      0000240010800?5990590618       sxc 9059             0644-
6I1954195C      0000240011800?1600640619       slo400p              0150-
6I1954195C      0000240012800?6980050020       lod 8005             0190-
6I1954195C      0000240013800?6063420012       raun0001c            0189-
6I1954195C      0000240014800?8200010019       rab 1    0f          0142-
6I1954195C      0000240015800?3500090085       slt 9    1f          0128-
6I1954195C      0000240016800?1680020228       slo 8002             0179-
6I1954195C      0000240017800?0016500000  obase 00o0101  0          0195-
6I1954195C      0000240018800?6590110226       ral 9011             0401-

I took these cards from one of the *.dck files under the I650 directory, and added the EOF marker ~ after the 10th card.
Suppose you want to split the deck into parts, with the last 5 cards going to the second and the rest to the first output deck.
The command for that would be carddeck split -5 to-split.dck 1.dck 2.dck. This definitely needs to count the cards in the deck, so that it knows how many cards to move to the first output deck before switching the output to the second deck.
The original code for the command would only consider the first 10 cards, those before the EOF, because deck_load() read up to CDSE_EOF.

sim> carddeck split -5 to-split.dck 1.dck 2.dck
%SIM-INFO: CDR0: 20 card Deck Loaded from to-split.dck
%SIM-INFO: CDR0: creating new file: 1.dck
%SIM-INFO: CDR0: creating new file: 2.dck
%SIM-INFO: Deck splitted to 5/5 cards

The report about 20 cards comes from sim_card but deck_load() only loads 10.

However when using sim_card_input_hopper_count() it considers 20 usable cards to be in the hopper, and thus wants to put 15 cards into the first output deck. Which fails because after 10 it receives CDSE_EOF.

sim> carddeck split -5 to-split.dck 1.dck 2.dck
%SIM-INFO: CDR: 20 card Deck Loaded from to-split.dck
%SIM-INFO: CDR: creating new file: 1.dck
%SIM-INFO: CDR: creating new file: 2.dck
%SIM-ERROR: Cannot read enough cards from source deck (to-split.dck)
%SIM-INFO: Deck splitted to 15/5 cards

(the output decks now contain wildly different numbers of cards, not the ones requested, nor the ones reported; 1.dck contains the 10 cards before the EOF and 2.dck contains the remaining 9)

I think the only thing that makes sense is to simply ignore every card after the EOF, just like before. To that end I'm introducing a function similar to sim_card_input_hopper_count() that only counts cards until EOF.

If we wanted to do something more "intelligent" with decks containing EOF markers, there are so many different ways to do that, that a one-size-fits-all approach doesn't really fit with these split and join commands as they are. I propose to leave that to someone who has a concrete need in the future.

[rcornwell]

You probably do not want to add a ~ to end of deck. I could add a function to return number of cards until next EOF or ERR card. Just let me know what you would like it called. It does sound like useful function.

[Rhialto]

I already made one and included it in the changed merge request - have a look at sim_card_input_hopper_count_before_eof() in sim_card.c
https://github.com/open-simh/simh/pull/392/files#diff-71454dcdb5e2e02ae00a229bddecdb326637a0ec52826a747d0f9b2a6d7d1b55R580
Although now that you mention it, it does make sense to stop counting at ERR cards too.

[rcornwell]

I might also stop counting of (card & (CARD_EOF|CARD_ERR) != 0.

Also could you put this in it's own commit under SIM_CARD: And can you also post it to my sims repository. This is not really part of i650.

[Rhialto]

I did some git surgery and there are now 3 commits. Two preparatory ones, where 5b47799 is the one that introduces the new card function. It can be used separately. The last one is the meat of the change. I combined my original version of that with the changes since then, since I changed the order of commits and would not really make sense any more.

[rcornwell]

I am not sure of the use of the card counting routine. I would just use the hopper size function and allocate enough space to cover that. Then read the deck into your space. If you get error or EOF, stop and return number of cards read. Errors will only occur if there is an untranslatable line in input. Since you attaching your file to the reader, you don't have to worry about stacking.

I am also wondering if these card management functions don't belong in a stand alone utility.

[Rhialto]

It has been a while since I looked at this so I have to page everything back in...

I guess that with "the card counting routine" you mean sim_card_input_hopper_ok_count(). Because sim_card_input_hopper_count() already does exactly that (using the hopper size - and the current input position).

The thing is that there is no longer a "space" to "read the deck into your space.". That has been eliminated. If I recall right, in an earlier version of the merge request, I did have a local buffer with a dynamic size.

But it was pointed out to me that all cards are already stored anyway and making another copy is wasteful.

So now the modified functions have just one card in memory at any time which is much nicer.

Unfortunately there is a case where the deck is split in two parts, and the number of cards for the second output is given.
To do that correctly we need an accurate count of the cards to process, so we can put the correct number of cards into the first output deck (nCards1) before switching further output to the second output deck.
sim_card_input_hopper_count() can give an over-estimation, which would leave us with incorrect results.

[Rhialto]

As for whether these functions belong in a stand-alone utility, I offer no opinion other than that I was encouraged to do this refactoring, and that the functionality is used in the self-test scripts.

[rcornwell]

What is unclear to me is why you need to read the complete deck into memory and not just process cards one at a time?

Also what kind of processing are you doing on these decks. I have not looked at code in a long time, so don't remember.

[markpizz]

The potential value of reading all cards at attach time validates th data correctly being good and if there are issues with the data, the user gets to see it directly later than having the running program stumble in who knows what way.

[rcornwell]

This is not the issue here. Sim_Card currently verifies cards as they are loaded, and will print out error messages if something is wrong. Here he is doing processing on the cards to generate decks. It is unclear why he has to process the decks all at once.

[Rhialto]

I am getting very confused. The whole point of this merge request is to do processing using only one card at a time.

That the already pre-existing sim_card code buffers all cards in memory is something I wasn't even aware of, at the start. In any case there is no change in that, so it seems irrelevant to me.

[rcornwell]

Ok, why do you need to add a routine to count number of cards up to first error or end of file?

[Rhialto]

To keep the semantics of the changed commands the same. They were reading the input deck up to that point: https://github.com/open-simh/simh/blob/master/I650/i650_sys.c#L751-L770

code

[Rhialto]

In the github CI, the i650 test fails like this, but for me locally it passes.
CI:

BIN/i650 RegisterSanityCheck /home/runner/work/simh/simh/I650/tests/i650_test.ini </dev/null 
 Running internal register sanity checks on IBM 650 simulator.
*** Good Registers in IBM 650 simulator.

IBM 650 simulator Open SIMH V4.1-0 Current        git commit id: feaf9487
Logging to file "console.txt"


** IBM 650: Basic Instruction Test: 

I/O Error, IC: 01901 ( 7019511902+   RD    1951  1902 )
FDS failed (bad ar)
%SIM-INFO: Debug output disabled
Log file closed
make: *** [makefile:2959: BIN/i650] Error 1

for me locally:

BIN/i650 RegisterSanityCheck /mnt/vol1/rhialto/cvs/other/open-simh/I650/tests/i650_test.ini </dev/null
 Running internal register sanity checks on IBM 650 simulator.
*** Good Registers in IBM 650 simulator.

IBM 650 simulator Open SIMH V4.1-0 Current        git commit id: 9a3be898+uncommitted-changes
Logging to file "console.txt"


** IBM 650: Basic Instruction Test:
00 0000 0001  50 1000 0000  50 1000 0000  00 0000 0000  00 0000 0000  00 0000 0000  00 0000 0000  00 0000 0000
00 0000 0002  50 2000 0000  50 1414 2135  00 0000 0000  00 0000 0000  00 0000 0000  00 0000 0000  00 0000 0000
00 0000 0003  50 3000 0000  50 1732 0508  00 0000 0000  00 0000 0000  00 0000 0000  00 0000 0000  00 0000 0000
00 0000 0004  50 4000 0000  50 2000 0000  00 0000 0000  00 0000 0000  00 0000 0000  00 0000 0000  00 0000 0000
00 0000 0005  50 5000 0000  50 2236 0679  00 0000 0000  00 0000 0000  00 0000 0000  00 0000 0000  00 0000 0000
00 0000 0006  50 6000 0000  50 2449 4897  00 0000 0000  00 0000 0000  00 0000 0000  00 0000 0000  00 0000 0000
00 0000 0007  50 7000 0000  50 2645 7513  00 0000 0000  00 0000 0000  00 0000 0000  00 0000 0000  00 0000 0000
00 0000 0008  50 8000 0000  50 2828 4271  00 0000 0000  00 0000 0000  00 0000 0000  00 0000 0000  00 0000 0000
00 0000 0009  50 9000 0000  50 3000 0000  00 0000 0000  00 0000 0000  00 0000 0000  00 0000 0000  00 0000 0000

Address Error, IC: 09999
FDS Ok
** Test: passed.
...
***
*** Assemble source deck
***
1                   SOAP TEST TAP
1
          EQU   SUB1     1000
1
 SUB1     NOP    0000   SUB2           COMMEN    1000    00  0000  0004
1              SUB2 TAP
 SUB2     NOP    0002              COMMENT       0004    00  0002  0008
          HLT    0002                            0008    01  0002  0012
1
          HLT    0001                            0012    01  0001  0016
          PST
          EQU   SUB1     1000
          EQU   SUB2     0004

I/O Error, IC: 00653 ( 7019991998+   RD1   1999  1998 )
***
*** Read assembled program deck
***

I/O Error, IC: 08000 ( 7019519999+   RD1   1951  9999 )
***
*** Run assembled program
***

Programmed Stop, IC: 00008 ( 0100020012+   HLT   0002  0012 )
Soap4 Ok
** Test: passed.


** clean up temp files generated during tests


!! All Tests Passed !!

open-simh$ echo $?
0

Maybe the compiler used at github has some new exciting code breaking optimisations that need to be disabled?

[rcornwell]

Why do these commands need to load the full deck into memory then do the operations and then dump out the results? Why not process one card at a time?

Also if you are not dealing with binary decks, these can be done with simple commands. Deck Join is also probably no longer needed as sim_card supports stacking of decks.

[Rhialto]

Again:

After incorporating this merge request, the code in I650/i650_sys.c will no longer do that.

The code that loads a full deck into memory is then only still in sim_card.c; at a first glance that would be in the function _sim_read_deck(). According to git blame, that function (in fact, the whole file) is mostly written by a Richard Cornwell. Maybe ask them why they load the full deck into memory.

[rcornwell]

I wrote pretty much all of sim_card. Originally it did not read the decks into memory and only read one card at a time. However @markpizz wanted me to validate the deck. This also allows for support of stacking of decks on the reader. This allows for you to read in several different format decks and process them as one deck.