PDP11: RF11: Fix DAE computation in service routine; log WC naturally

[al20878]

This PR fixes post-op DAE (disk address extension) computation, and also improves debugging log so that it's more readable

[markpizz]

I'm sure that Bob will suggest that while outputting rf_wc, rather than casting to (short) you just leave it. Anything bigger than 16 bits is an error which would be obscured by the cast.

[al20878]

I think this needs to be redone to avoid using the cast to the "natural" type, as I was told in another PR. I'll do that.

As for the overflow, it will most definitely occur, because the sign extension, so the cast to cut off only the 16 bits is always necessary.

Finally, there's still an issue with the "extended" DAE calculation because a runaway I/O from the last platter should assert the "Overflow bit" 5 of the DSR, the next higher bit to DAE, -- which is currently impossible because the word count is prematurely cut-off (adjusted to the capacity of the drive), and thus this error cannot be properly asserted. I don't know how important that is from the software POV, but that's how the controller's logic is described in the documentation. In fact, I'm having an issue of using the controller with an old DOS/Batch -- along with two DECTapes reels, and I will need to create an issue about that. Not sure if they are at all related. Probably not, though. That's how I came to look at this beast, anyways, -- to tell the full story.

[markpizz]

I'm not sure I understand what you're describing. Are you saying that in the middle of a DMA operation, if the read or write crosses a platter boundary, the controller needs to tell the simulated OS precisely when this happens or merely that it happened somewhere along the way?

[al20878]

Taking it back... Looks like DA was just not properly set at all and that my fix actually cures the issue and shows the overflow through, if that occurs. The DAE mask is proper 077 (which includes the OVR bit), so once that calculates correctly, with this fix, that should be it.

[markpizz]

Since this is Bob's module, has he been engaged about your observations?

[al20878]

has he been engaged about your observations?

I thought he reads the messages... I don't have his phone number, sorry.

[al20878]

rf_da = da & DMASK;                                     /* split da */
rf_dae = (rf_dae & ~RFDAE_DAE) | ((rf_da >> 16) & RFDAE_DAE);

This (original) logic was not good: as it first cuts off the higher bits in the full Disk Address (DA), which include the higher bits of the track number plus the entire field for Drive # plus an overflow bit, then it uses the rf_da value in the next statement. But shifting that to the right by 16 always yields 0, because it was already a 16-bit entity (by the virtue of the previous statement), so the higher track bits and the drive id are always 0 post any I/O -- yet they should reflect the disk address at which the I/O has completed -- RF11 can cross not only the track boundaries but also the platter boundaries as it does the transfers. So my fix is addressing that using the full da instead of rf_da in the second statement.

The logging was very secondary to that -- when I was debugging I found it very hard to follow the 2's complement word count (either set or residual), so I changed those to the "human readable" form.

[pkoning2]

Bob doesn't use Git, but you can reach him by email: [email protected]

As for sign extension, I don't understand. CSRs should be unsigned, so if you want to operate on an extended disk address by combining the disk address CSR (uint16) with the upper bits from the DAE, that's still an unsigned value and it should just work. Did I miss something?

[al20878]

Did I miss something?

Unfortunately, you did. The signed entity was mentioned in the context of the word count, which is a 16 bit value, but which is a two's complement of the original value = a negated value. It's easier for the controllers to operate those because they are doing the increment by 1 until that value reaches 0 (instead of adding 177777, to do an equivalent of 1's subtraction). So when you want to transfer 1 word, you put 177777 into the wc. When it's 512 bytes, the value coded would be 177400... When an I/O stops in the middle, that would result in some value between 177400 and 177777, which you need to convert, if it is printed "as is". I highly praise folks who can do in that their head just by looking at those numbers, but I generally need a tool (like a calculator). So I added the negation to the output, to see the actual word count value, not its two's complement. That's it.

[al20878]

[email protected]

Thanks. I sent him an email asking to take a look at this PR.

[pkoning2]

I see. It never occurred to me that debug messages would to that. My thinking is that, if you're doing work for which those messages need to be enabled, looking at 2's complement values is just one small detail you'd be expected to handle.

[al20878]

2's complement values is just one small detail you'd be expected to handle.

Why not to ask the computer to convert them back and forth? You know they are printed naturally, of their two's complement internal representation, so the values in there are valid. If anything you can take a look at the bare registers with the "examine" command. If you have a slew of debug printouts, converting them all, even in your head if you can, can cause you a headache, for sure.

[rms47]

The only actual bug fix is

rf_da = da & DMASK; /* split da */
rf_dae = (rf_dae & ~RFDAE_DAE) | ((da >> 16) & RFDAE_DAE);

Everything else is window dressing and isn't needed. It will cause merge conflicts from the 3.X branch.

[al20878]

The only actual bug fix is

Thanks for confirming.

It will cause merge conflicts from the 3.X branch.

It can be changed there the same way. The logging pre- and post- I/O is inconsistent (full DA prior, and split DAE+DA after), and is very hard to follow (and to debug). Until I changed that (and the word count), it was not helping at all in understanding of what was going on.

[al20878]

I undid the log mods, so it goes only with one line fix, altogether. Please apply PR squashed

[al20878]

I think I just screwed up, sorry. I'm going to close this and open another one for just that one line that needs to be changed

[al20878]

See PR #297