PDP11: RP11-C (RP02/03) disk implementation

[al20878]

Re-do of PR#127...

[markpizz]

I haven't looked at everything closely yet, but so far I have four comments:

1. pdp11_rr.c has LF line endings in the repo. All other source and text files in the repo have CRLF line endings.

2. The 3 commits here should be squashed

3. The simh convention for most arrays of structures is that the array ends with an extra entry which is all 0's. This applies to MTAB, REG, and even drv_typ.

4. pdp11_rr.c is back using assert again. Recall the prior discussion about assert. You should use simh defined ASSURE. You argue that the results will be faster for non-debug builds since assert disappears. Well, if you can measure that while the simulator is actually using a disk connected to this device has an amazing speed increase measured to be more than .5%, better, then we can have a discussion. I suspect that you will not be able to measure any difference. Additionally, since at least some of the values you are validating with assert are data passed in from the simulator, such validation should always be present, not just on debug builds!

[pkoning2]

Anthony,
Could you address the comments from Mark? This is a nice contribution but it would be good to have it updated so it conforms to the coding conventions. Along those lines, Bob Supnik comments that CSRs should be uint16 or uint32, not int32, and that plain "int" should also not be used. He also observed that you have "PR" instead of "RP" in a number of places.

[al20878]

@markpizz and @pkoning2 :

1. I suppose the line endings are the function of a git client that you're using. All my files checked out of the open-simh repo have LFs in them, as they are the standard UNIX text files.

2. I don't understand why they should be "squashed" -- the pull request is a unit of commits; they are not applied individually but all together, anyways.

3. I modeled my code based on the rk11/rl11 implementation, and all the NULL / 0 terminated arrays are there just the same in my code as well. The dev_tab need not to be NULL terminated because it is not used outside the module. But I am adding the trailing NULL (which then distorts the number of available disk types as a calculated value based on the size of the array -- and I am compensating for that as well).

4. assert() is a standard C feature, and I don't understand the "requirement" not to use it. All the presented gotchas against using it were just laughable / moot. You know what you are doing using the language features -- it's just the same as not to divide by 0.

5. The device registers in the Bob's rk source code are all int32 -- again, I modeled my code based on that. It was rather weird to me, TBH (as I myself would rather use "unsigned" for them) but I followed the trend. OTOH, signedness of a 32-bit value is irrelevant for a 16-bit architecture, which PDP11 is, so I assumed the choice of int32 was to underscore the fact (and also to simplify the compiler optimization because the underlying type was plain int, i.e. the native unchecked type).

6. I fixed only one occurrence where RP was typo'd (in a comment). I didn't find any other such places.

7. The bare int's were used only for extremely small integral values or Booleans (0/1) -- so they were perfectly safe in any context (or any architecture that simh can ever work on). Anyways, I replaced them with int32 / t_bool (which BTW is a bare int, as the underlying type).

[markpizz]

1. I suppose the line endings are the function of a git client that you're using. All my files checked out of the open-simh repo have LFs in them, as they are the standard UNIX text files.

What is stored in the repo is NOT a consequence of the client or all of the source code in the simh codebase would look like it has LF line endings. There is only one source file in the whole project that has LF line endings, As I pointed out this is pdp11_rr.c.

The simh standard for some 20 years has been that source code has CRLF line endings. This goes back to at least the simh 2.x timeframe and way before I had anything to do with managing anything to do with the project.

2. I don't understand why they should be "squashed" -- the pull request is a unit of commits; they are not applied individually but all together, anyways.

All of the commits that add a particular feature to the repo should be squashed. What you may have done along the way during your development process can be anything you want, but the interesting contribution of the PR is the totality of the polished results (after accommodating input like this).

3. I modeled my code based on the rk11/rl11 implementation, and all the NULL / 0 terminated arrays are there just the same in my code as well. The dev_tab need not to be NULL terminated because it is not used outside the module. But I am adding the trailing NULL (which then distorts the number of available disk types as a calculated value based on the size of the array -- and I am compensating for that as well).

Nice.

4. assert() is a standard C feature, and I don't understand the "requirement" not to use it. All the presented gotchas against using it were just laughable / moot. You know what you are doing using the language features -- it's just the same as not to divide by 0.

It is indeed a standard C, but that is not the point. Laugh all you want, but please change these to ASSURE or provide precise detailed measurements that the use of assert provides a distinct performance gain. Additionally, I pointed out that the various things you were validating with assert were data values provided from what ever is running within the simulator. You only have control of the data you happen to be using during your personal debugging. If it is ever important to validate values such as these it is more important to always do it since you don't otherwise have control of the source of the input data that is being validated.

5. The device registers in the Bob's rk source code are all int32 -- again, I modeled my code based on that. It was rather weird to me, TBH (as I myself would rather use "unsigned" for them) but I followed the trend. OTOH, signedness of a 32-bit value is irrelevant for a 16-bit architecture, which PDP11 is, so I assumed the choice of int32 was to underscore the fact (and also to simplify the compiler optimization because the underlying type was plain int, i.e. the native unchecked type).

6. I fixed only one occurrence where RP was typo'd (in a comment). I didn't find any other such places.

At least one of the commits that should be squashed also had PR in its subject. Squashing should fix this.

7. The bare int's were used only for extremely small integral values or Booleans (0/1) -- so they were perfectly safe in any context (or any architecture that simh can ever work on). Anyways, I replaced them with int32 / t_bool (which BTW is a bare int, as the underlying type).

Fantastic.

[markpizz]

1. I suppose the line endings are the function of a git client that you're using. All my files checked out of the open-simh repo have LFs in them, as they are the standard UNIX text files.

If you look closely, all of the source files you have checked out absolutely have LF in them. Except for the one you created with bare LF in it, the rest all have CRLF in them. LF is a proper subset of CRLF.

[al20878]

BTW, commit squashing is a function of simh-repo -- you can absolutely squash all my commits in this PR into a single commit for open-simh... I don't have to redo the whole thing for this only reason.

But all in all, the amount of nitpicking in this thread is becoming increasingly annoying. I am coding in C for my entire professional life, for a very sizeable company with a large team of developers, so the resistance I am facing on here makes me re-think my any future efforts, TBH.

That including the line endings. Even MSVC is totally groking the bare LFs for at least a decade now (MS$ finally realized that just as well), so what's the deal with simh?

[markpizz]

BTW, commit squashing is a function of simh-repo -- you can absolutely squash all my commits in this PR into a single commit for open-simh...

It is an option which the person doing the merge may or may not remember to do. This PR certainly is a candidate for the requester (you) to do since it has already been mentioned multiple times and the list of details I enumerated was specifically requested of you.

I don't have to redo the whole thing for this only reason.

This sounds like you're thinking this is a ridiculously manual process. The ability to squash a set of commits is innately part of git. You squash it locally and then do a forced push to your github repo into the branch you've already been working in (rp11), and that's it. A little Googling will help you learn the details.

[markpizz]

By the way, since you're adding a new file to the PDP11 build, you should update the descrip.mms file as well.

[al20878]

By the way, since you're adding a new file to the PDP11 build, you should update the descrip.mms file as well.

Duh, what a timely reminder!

From the quick glance, however, I see that pdp11_mb.c was not added in there (yet it's what the new code follows in makefile), and I don't understand the logic behind the division into two object libraries in there: what's the criteria? Some devices are in 1, the other are in 2, pdp11_mb.c is missing altogether... Any suggestions?

[markpizz]

I think the multiple libraries relates to a line length limitation...

[al20878]

a line length limitation...

which is ... ?

I don't have any such system to build simh on to check that. So either you add the file to one of the libraries, or please let me know where should it go to...

Besides, pdp11_mb.c defines a global mb_dev, which is referenced by pdp11_sys.c, yet it is not included in either library -- making me think that nobody builds the simulator using this .mms file because such a build would have been a failure due to at least one undefined symbol.

[pkoning2]

While technically it's a good idea for descrip.mms to be kept up, it doesn't work right now anyway so I suggest ignoring that. Let's get it fixed as a separate effort.
The new "community license" out of VSI is a nice way to get a system that can be used for this; I have one. VMS on x86 is pretty good and fast.
On squashing: as my favorite Git textbook points out in a discussion of the closely related subject of merge vs. rebase, there are arguments on both sides. Not squashing means you get to see the history; squashing means that you're recording the delivery of completed functionality. In our case, I lean towards the latter: the details of how a PR was created or refined by its author are not really relevant to SIMH.
As for who should do it: if you do it, you can make sure the commit message describes things the way you want them to be shown. If we do it then that may not happen. Also, if you squash and rebase to the current head of "master" then you show up as the author; if I do it then the commit is recorded with both our names which is a bit confusing.
Anthony, on "nit picking", one point of working on multiple projects is that they may follow different coding conventions. For example, the LF vs. CRLF thing is an old convention. Yes, it works either way. Yes, you could argue that for an entire new file it doesn't really matter. But within a file it certainly does, and it's reasonable for things to be consistent. It's unfortunate that Git exposes this stuff -- unlike Subversion where the question simply doesn't arise at all. But it any case, CRLF is our convention. SImilarly, as an experienced C programmer you undoubtedly have a favorite code layout style, and chances are that what you see in SIMH is not that style. But it's good to match the conventions of the project and accommodate your style to the project, rather than the other way around.

[markpizz]

Before deeply considering the squash subject, don't forget the assert subject.

Getting back to the detail of that, since you are using assert to validate some of the data you get directly from within the running simulator, please explain why it is a good idea for someone running a simulator compiled with or without debug will get different data dependent results.

[al20878]

If you were following this PR, I already did the squashing, which included the line endings converted to CRLFs, and all other "suggestions" that I received (even though changing drv_tab was totally uncalled for as it is a local variable).

However, I am not going to change assert(), as I strongly believe that it is a debugging technique that validates and also documents the code yet that is unnecessary in any production build. There are no "different data", because asserts are not supposed to have any side effects -- and that's how they are used. Code should run uninterrupted in production (with all the bugs therein), which is why they are excluded, even if they would not be checking out correctly -- because that's the purpose of debugging by fixing the code or and / or modifying the "wrong" assert()s, in particular. But they are no-op, they are comments, they don't add any value to the algorithm itself. ASSURE is the reason your phone is running out of battery, or your computer is slow, or you are contributing to the increase of the global carbon footprint. Yes, small things create big consequences.

[pkoning2]

Ok, I see the squash, thanks.
But I'm puzzled by wanting to use assert when the project coding style has that sort of thing wrapped in a macro. Projects have their coding conventions, many of which include specific types, or wrapping of standard library mechanisms. It's no different from, say, the GCC project saying that you don't use malloc directly.

[al20878]

@pkoning2 there is no "coding style" in ASSURE. It's a way to allow side-effects in what would have been a legitimate and standard assert(). And it does not disappear from code once the build is final. That's both evil. That's as if you re-define all your pointer dereferences, for example, with "if pointer not NULL then proceed, else issue a warning and do nothing" -- it's totally uncalled for, if the algorithm is correct.

I use assert() in contexts like this:

    assert(cyl < drv_tab[n].cyl  &&  head < RP_NUMSF);
    da = GET_DA(cyl, head, sect);

so it documents that, in this particular case, the drive parameters (cyl and head) are set correctly (previously), so that the disk address can be properly calculated based on their values. It also verifies that at run-time while I'm debugging and, thus, prevents upstream logic errors. Should I have changed something that affected cyl and head in the way not expected, the code aborts and I fix it, yet the assert() stays guarding the statement. But once the debugging is done, the assert expressions is totally unnecessary in production, where it is a mere comment.

Or this one:

    assert(da < drv_tab[n].size);
    n = drv_tab[n].size - da;

Guards the subtraction to prevent an overflow. Again, it's only necessary in debugging (verifying your math, basically, but once that checks out -- it again becomes a comment).

GCC's malloc() analogy can't be applied here because they are actually verifying the allocations and setting up guards in their code to catch all the misuse. With this technique, it's much easier to "inject" heap debugging, if necessary, without having to fish out all malloc calls and change them into something different. Yet, they are able to just drop all the checks and to go with the standard heap routines once the debugging is done, resulting in no overhead, just like assert(). It's a different matter than ASSURE, which always computes.

[markpizz]

Besides declaring that assert() is a C standard, you haven't explained why ASSURE wouldn't equally serve your purpose.

[al20878]

you haven't explained why ASSURE wouldn't equally serve your purpose.

You haven't been reading carefully, because yes I did. ASSURE does not get removed in optimized builds, and it allows side-effects. Because of the latter, I cannot assume that ASSURE is going to be defined as assert() one day, without a thorough code review to find all those occurrences with the side-effects. So no, it does not provide the functionality that assert() does. At least not for the purposes I was using assert()s in my code. And no, I don't want any code that is not necessary to get included in the modules I create -- that's not by my book.

[al20878]

Can I ask what is exactly the problem with this PR getting merged in? It's been sitting here for almost 3 months now.

[pkoning2]

I'll check. I thought there were open questions.

[al20878]

@pkoning2 did you find anything? From my end, Bob Supnik, in a private conversation over email, only stated:

Nice job on the RP11.

Bob Supnik

So I am not sure what's holding this up... Please re-state your suggestions, if any left. This PR has been in limbo for unreasonably long now.

TY

[pkoning2]

I just did a test build and tried to run RSTS/E. It doesn't work, because RSTS/E expects an RP11-C, with the extra 4 unused registers preceding the first real one. So the base address has to be (by default) 17776700. I looked at the code to see how to do that but the answer wasn't immediately obvious.
The other problem is that it conflicts with RHA, the first Massbus controller. I can manually move its CSR and vector, but for some strange reason I can't disable RHA even though it's not used in the config I was trying.

[al20878]

I tested it with DOS extensively and it all worked without a hitch.

For RSTS/E you don't need to change anything -- the fact that the actual base is at 17776710 by default means that 17776700 should be given to RSTS/E, as the base for the range for RP11C. RSTS/E wants 16-word-aligned ranges (and this device only has 12 words). They want the "aligned" range. For that RSX was doing better as they only wanted to know where the CSR was (and it's at 17776714).

To remove the conflict with RHA, you just disable it (by disabling the RP device that it auto-configures for), or move it to some other address. Or you can move RP11C to some other address, alternatively. That is only if you want to have both the Massbus disk and RP11C together in the same config.

sim> SET CPU 11/20 56K
sim> SET RP DIS
sim> SET RR ENA
sim> ATT RK dos_rk.dsk
sim> ATT RP test.dsk
sim> SH RK
RK      address=17777400-17777417, vector=220, BR5, 8 units
  RK0   1247KW, attached to dos_rk.dsk, write enabled
        RK05, autosize, SIMH format
...
sim> SH RR
RR      RP11-C, address=17776710-17776737, vector=254, BR5, 8 units
  RR0   20MW, attached to test.dsk, write enabled
...
sim> SH RR0
RR0     20MW, attached to test.dsk, write enabled
        RP03, autosize, SIMH format
sim> B RK
DOS/BATCH V9-20C
DATE: 21-SEP-90
TIME: 21:05
DIALOGUE? N

$LO 1,1
DATE:-21-SEP-90
TIME:-21:05:01
$R PIP
PIP  V10-02
#DP:/DI

DIRECTORY DP0: [  1,1  ]

21-SEP-90

BADB  .SYS     1  20-NOV-99 <233>
MONLIB.CIL    88  20-NOV-99 <233>
MTBLD .BAT     5  20-NOV-99 <233>
CL0   .SYS     1  20-NOV-99 <233>
VERIFY.LDA    34  20-NOV-99 <233>
EDIT  .LDA     7  20-NOV-99 <233>
CILUS .LDA    20  20-NOV-99 <233>
LINK  .LDA    29  20-NOV-99 <233>
FILDMP.LDA     4  20-NOV-99 <233>
DSKINT.LDA     5  20-NOV-99 <233>
LIBR  .LDA     5  20-NOV-99 <233>
FILCOM.LDA     7  20-NOV-99 <233>
EBASCI.LDA     3  20-NOV-99 <233>
MACRO .LDA    20  20-NOV-99 <233>
PIP   .LDA    18  20-NOV-99 <233>
CREF  .LDA     5  20-NOV-99 <233>
CL1   .SYS     1  20-NOV-99 <233>
CL2   .SYS     1  20-NOV-99 <233>
CL3   .SYS     1  20-NOV-99 <233>
CL4   .SYS     1  20-NOV-99 <233>
CL5   .SYS     1  20-NOV-99 <233>
CL6   .SYS     1  20-NOV-99 <233>
CL7   .SYS     1  20-NOV-99 <233>
SYSMAC.SML    14  20-NOV-99 <233>
ODT   .OBJ     6  20-NOV-99 <233>
LDUMP .LDA    15  20-NOV-99 <233>
MTBLDF.BAT     5  20-NOV-99 <233>
FTNLIB.OBJ    84  20-NOV-99 <233>
TRCLIB.OBJ     7  20-NOV-99 <233>
FORCOM.DGN    11  20-NOV-99 <233>
FORTRN.LDA    17  20-NOV-99 <233>
FORRUN.DGN     8  20-NOV-99 <233>
FORTRN.OVR    60  20-NOV-99 <233>
BBB   .BBB    89  20-NOV-99 <233>
CCC   .CCC   396  20-NOV-99 <233>
DDD   .DDD   396  20-NOV-99 <233>
GGG   .GGG   376  21-NOV-99 <233>
FFF   .FFF   376  21-NOV-99 <233>

TOTL BLKS:  2119
TOTL FILES:   38
#^C
.KI
<Ctrl/E>
Simulation stopped, PC: 001016 (MOV (R1)+,R2)
sim>

[pkoning2]

Yes, I can move RHA to another address, but it doesn't want to be disabled:
sim> set rp dis
sim> show rha
RHA address=17776700-17776753, vector=254, BR5
How did you manage to make it work differently?
As for the RP11 address, the trouble is that RSTS/E (V6B or later) autoconfigures devices by looking for the controller at the documented base address. In the case of RP11, that is 176700, not 176710, so RSTS/E concludes the device doesn't exist.

[al20878]

Thanks for the feedback, I have adjusted the controller code for RP11C to actually respond on the address range beginning 17776700 (by default) instead of x710. (I guess neither DOS nor RSX cared as long as CSR was properly responding to bus accesses.) Now I also checked with RSTS/E V6C that I could find.

You mentioned V6B -- where can I get it to try that too please?
(Before I push the changes in.)

RSTS V06C (DK0)


Option: HA

  HARDWR suboption? LIST

  Name  Address Vector  Comments
  TT:    177560   060
  RK:    177400   220   RK05F units: none
  RP:    176700   254   RP03 units: 0 2 3 4 5 6 7
  TM:    172520   224
  TC:    177340   214
  PR0:   177550   070
  PP0:   177554   074
  LP0:   177514   200
  RX0:   177170   264
  DZ0:   160100   300
  DZ1:   160110   310

  KW11L  177546   100
  SR     177570
  DR     177570

In the above I specifically changed the type of DP1: to RP02 (with sim> set rr1 rp02), and so it was correctly skipped in the list. Before the drive type change, it was showing as

  RP:    176700   254   RP03 units: 0 1 2 3 4 5 6 7

Which was correct as well.

How did you manage to make it work differently?

I just disabled the "RP" device. Note that if your configuration is using the first Massbus controller (which is reserved for disks, IIRC), or use any other Massbus controller (RHB, RHC), then RHA will remain enabled (and there's no way to disable it). This is "auto" configuration, which is rather weird (in the sense that RHA remains enabled just because RHB is in use, and conversely, RHB remains enabled even if unused, but RHC is in use).

$ ~/simh/BIN/pdp11.exe

PDP-11 simulator Open SIMH V4.1-0 Current        git commit id: a653bbfc+uncommitted-changes
sim> set cpu 11/45 256
Disabling XQ
sim> sh dev
PDP-11 simulator configuration
CPU     11/45, FPP, MMU, autoconfiguration enabled, idle disabled
SYSTEM
RHA     address=17776700-17776753, vector=254, BR5
RHB     disabled
RHC     disabled
CLK     60Hz, address=17777546-17777547, vector=100, BR6
PCLK    disabled
PTR     address=17777550-17777553, vector=70, BR4
PTP     address=17777554-17777557, vector=74, BR4
TTI     7p, address=17777560-17777563, vector=60, BR4
TTO     7p, address=17777564-17777567, vector=64, BR4
TDC     disabled
CR      disabled
LPT     address=17777514-17777517, vector=200, BR4
DLI     disabled
DLO     disabled
DCI     disabled
DCO     disabled
DZ      address=17760100-17760117*, vector=300-314*, BR5, lines=16
VH      disabled
DH      disabled
RC      disabled
RF      disabled
RK      address=17777400-17777417, vector=220, BR5, 8 units
RL      RL11, address=17774400-17774411, vector=160, BR5, 4 units
HK      address=17777440-17777477, vector=210, BR5, 8 units
RX      address=17777170-17777173, vector=264, BR5, 2 units
RY      disabled
RP      Massbus adapter 0 (RHA), 8 units
RR      disabled
RS      disabled
RQ      address=17772150-17772153, no vector, BR5, RQDX3, 4 units
RQB     disabled
RQC     disabled
RQD     disabled
TC      disabled
TM      address=17772520-17772533, vector=224, BR5, 8 units
TS      disabled
TQ      TK50 (94MB), address=17774500-17774503, no vector, BR5, 4 units
TU      disabled
TA      disabled
XQ      disabled
XQB     disabled
XU      disabled
XUB     disabled
KG      disabled
DMC     disabled
DUP     disabled
DPV     disabled
KDP     disabled
KE      disabled
ROM     4 units
CH      disabled
MB      disabled

sim> set rp dis
sim> set tq dis
sim> set rl dis
sim> set hk dis
sim> set rq dis
sim> set tc ena
sim> set rr ena

sim> show dev
PDP-11 simulator configuration

CPU     11/45, FPP, MMU, autoconfiguration enabled, idle disabled
SYSTEM
RHA     disabled
RHB     disabled
RHC     disabled
CLK     60Hz, address=17777546-17777547, vector=100, BR6
PCLK    disabled
PTR     address=17777550-17777553, vector=70, BR4
PTP     address=17777554-17777557, vector=74, BR4
TTI     7p, address=17777560-17777563, vector=60, BR4
TTO     7p, address=17777564-17777567, vector=64, BR4
TDC     disabled
CR      disabled
LPT     address=17777514-17777517, vector=200, BR4
DLI     disabled
DLO     disabled
DCI     disabled
DCO     disabled
DZ      address=17760100-17760117*, vector=300-314*, BR5, lines=16
VH      disabled
DH      disabled
RC      disabled
RF      disabled
RK      address=17777400-17777417, vector=220, BR5, 8 units
RL      disabled
HK      disabled
RX      address=17777170-17777173, vector=264, BR5, 2 units
RY      disabled
RP      disabled
RR      RP11-C, address=17776700-17776737, vector=254, BR5, 8 units
RS      disabled
RQ      disabled
RQB     disabled
RQC     disabled
RQD     disabled
TC      address=17777340-17777351, vector=214, BR6, 8 units
TM      address=17772520-17772533, vector=224, BR5, 8 units
TS      disabled
TQ      disabled
TU      disabled
TA      disabled
XQ      disabled
XQB     disabled
XU      disabled
XUB     disabled
KG      disabled
DMC     disabled
DUP     disabled
DPV     disabled
KDP     disabled
KE      disabled
ROM     4 units
CH      disabled
MB      disabled

sim> sh rha
RHA     disabled
sim> sh rhb
RHB     disabled
sim> sh rhc
RHC     disabled

sim> sh io
      Address         Vector  BR # Device
-------------------- -------- -- - ------
17760100 - 17760117* 300-314   5 2 DZ
17772200 - 17772277              1 CPU
17772300 - 17772317              1 CPU
17772320 - 17772337              1 CPU
17772340 - 17772357              1 CPU
17772360 - 17772377              1 CPU
17772516 - 17772517              1 CPU
17772520 - 17772533  224       5 1 TM
17776700 - 17776737  254       5 1 RR
17777170 - 17777173  264       5 1 RX
17777340 - 17777351  214       6 1 TC
17777400 - 17777417  220       5 1 RK
17777514 - 17777517  200       4 1 LPT
17777546 - 17777547  100       6 1 CLK
17777550 - 17777553  070       4 1 PTR
17777554 - 17777557  074       4 1 PTP
17777560 - 17777563  060       4 1 TTI
17777564 - 17777567  064       4 1 TTO
17777570 - 17777571              1 CPU
17777572 - 17777577              1 CPU
17777600 - 17777617              1 CPU
17777620 - 17777637              1 CPU
17777640 - 17777657              1 CPU
17777660 - 17777677              1 CPU
17777740 - 17777775              1 CPU
17777776 - 17777777              1 CPU

sim> att rk0 dk0.dsk
%SIM-INFO: RK0: 'dk0.dsk' Contains a RSTS File system
%SIM-INFO: RK0: Pack ID: SYSGEN Revision Level: 0.0 Pack Clustersize: 1
%SIM-INFO: RK0: Last Unallocated Sector In File System: 4799

sim> att rk1 dk1.dsk
%SIM-INFO: RK1: 'dk1.dsk' Contains a RSTS File system
%SIM-INFO: RK1: Pack ID: VIXEN  Revision Level: 0.0 Pack Clustersize: 1
%SIM-INFO: RK1: Last Unallocated Sector In File System: 4799

sim> att rr0 dp0.dsk
%SIM-INFO: RR0: 'dp0.dsk' Contains a RSTS File system
%SIM-INFO: RR0: Pack ID: SAMPLE Revision Level: 0.0 Pack Clustersize: 2
%SIM-INFO: RR0: Last Unallocated Sector In File System: 79999

sim> b rk0



RSTS V06C-03 vixen (DK0)


Option: HA

  HARDWR suboption? LIST

  Name  Address Vector  Comments
  TT:    177560   060
  RK:    177400   220   RK05F units: none
  RP:    176700   254   RP03 units: 0 1 2 3 4 5 6 7
  TM:    172520   224
  TC:    177340   214
  PR0:   177550   070
  PP0:   177554   074
  LP0:   177514   200
  RX0:   177170   264
  DZ0:   160100   300
  DZ1:   160110   310

  KW11L  177546   100
  SR     177570
  DR     177570

  Hertz = 60.

  Other: FPU, SL

[pkoning2]

Great!
I don't have V6B, unfortunately. I normally use V10-1. The mention of V6B was just because that's the first RSTS release that included device autoconfiguration.
So now I understand why RHA won't go away; I have RHB because I configured a TU16. That's obviously an autoconfig mistake, since the "usually for tape" RH and the "usually for RP/RM disks" one should be independent.

[al20878]

So now I understand why RHA won't go away; I have RHB because I configured a TU16

Exactly, the second Massbus controller is for the tapes.

That's obviously an autoconfig mistake

Well, there's no way to manipulate the Massbus controllers (other than changing their I/O assignments): they are enabled and disabled by the simulator automatically depending on the actual devices that they are associated with (and higher controllers require the lower controllers to remain active in the hardware configuration even if they are not actually used for any devices themselves)... I don't think (TBH) there's an easy way to change that simulator behavior, as it's coded very deep down the Massbus logic -- that's from what I understood going through the code, as obviously RHA was in the way for RP11-C...

I don't have V6B, unfortunately. I normally use V10-1.

Yeah, I was asking because I don't think V6B survived at all (from what I read online) -- so your mentioning it sounded intriguing :-) I have V10 too but I am not exactly sure if it was sysgenned to use RP11...

[pkoning2]

We've had some discussion about the RH setup. To match what the hardware does, there should be up to four RH controllers, each one with a configurable set of devices on it. While there are conventions that two are for RP/RM, one is for RS, and one is for TU, the hardware doesn't require that and some OS are more flexible as well. (RSTS isn't, though, other than that it can cope with multiple TU formatters.) In any case, even with the current restrictive setup, the fact that RHA can be disabled only if RHB is disabled is obviously wrong. Oh well, I can work around that.

[pkoning2]

Found another issue: as coded, if the unit is RP02 but not attached, RSTS doesn't see it. The reason is that setting the other status flags (locked, seeking, ready) is dependent on it being attached. It should not be; those status bits are valid even if there is no pack in the drive. I changed my test copy to remove the { } after the test for UNIT_ATT and with that RSTS recognizes both RP03 and RP02.
I can use the "copy" command in INIT to copy the minimal installation files to the RP03 or RP02, and boot it, but as you mentioned the V10.1 SYSGEN isn't configured with RP03.

[al20878]

if the unit is RP02 but not attached, RSTS doesn't see it.

I'm sorry but I'm not sure what you are saying. I showed you earlier in this thread that RSTS/E correctly detects RP02's (no attachment to a file was necessary).

those status bits are valid even if there is no pack in the drive

The type bit is always valid (unless the device is disabled); other bits are dependent on whether the unit is actually ready.

        if (!(uptr->flags & UNIT_DIS)) {                /* not disabled? */
            if (GET_DTYPE(uptr->flags))
                rpds |= RPDS_RP03;
            if (uptr->flags & UNIT_ATT) {               /* attached? */
                rpds |= RPDS_ONLN;
                if (uptr->flags & UNIT_WPRT)            /* write locked? */
                    rpds |= RPDS_WLK;
                if (uptr->SEEKING)                      /* still seeking? */
                    rpds |= RPDS_SEEK;
                else if (!sim_is_active(uptr))          /* idle? */
                    rpds |= RPDS_RDY;
            }
        }

I changed my test copy to remove the { } after the test for UNIT_ATT

Again, I'm not sure what your change was, and why was it necessary.

RSTS/E HA LIST works like this:

Table 3.8 (page 3-25) in http://www.bitsavers.org/pdf/dec/pdp11/rsts_e/V06/AA-2669D-TC_V6Csysgen_Jul78.pdf

[al20878]

I checked in all the changes, please apply them all "squashed" -- I wasn't able to properly do it myself (if you know how to, please let me know), because the repo went ahead too far since I opened this PR.

[pkoning2]

In V10.1, it lists RP11 disks this way:

  Name  Address Vector  Comments
  TT0:   177560   060   
  RL0:   174400   160   Units: 0(RL01) 1(RL02)
  RP0:   176700   254   Units: 0(RP03) 1(RP02) 2(RP03) 3(RP03) 4(RP03) 5(RP03)
                               6(RP03) 7(RP03)

That's with the change I mentioned. With your code, unit 1 (RP02) is not shown, matching the text you quoted. But the issue is that it isn't shown because RSTS treats it as nonexistent, so subsequent attempts to use it fail. This happens because as written I get RPDS == 0 and that is handled as "no drive here". Any non-zero value is ok, then the RP03 bit distinguishes the types. With the change I mentioned the Ready bit is set.

Separate issue: I see this:

sim> bo rr
Command not allowed, PC: 005714 (MOV (SP)+,R1)

but I can boot it just fine from within RSTS, so it isn't the execution of the code in the boot block that is at issue.

[al20878]

sim> bo rr

The boot command I left not implemented so far... If you want it, I need to do it separately.

but I can boot it just fine from within RSTS, so it isn't the execution of the code in the boot block that is at issue.

Indeed it is not, because the boot command is a-la ROM. Software boot (which you are referring to) is different.
Again, I can see into implementing the boot (it's not a big deal, really) -- it wasn't just done there.

RPDS == 0 and that is handled as "no drive here"

That's weird because when the disk pack is not installed, RPDS == 0 is exactly what it should read (if it's RP02) -- according to the documentation. The "ready" bit means the drive is ready for the command, and which is not the case when there's no pack -- so it def cannot be RDY (aka SU RDY). Maybe it can be ONLN (aka SU OL) in this case because the manual reads "the selected drive ENABLE/DISABLE switch is set to ENABLE".

I'll try to address that...

[pkoning2]

I see your point about "ready" that it should not be set without an attach ("pack loaded"). But "online" should indeed be set. We don't really have the equivalent of the RP "disable" switch, but the closest would be the enable/disable commands. So an enabled unit should appear "online". The maintenance manual says that "disable" is basically a quick way to get the equivalent of the drive cabling being removed, so for a disabled drive to appear nonexistent makes sense. Hm, I wonder if an disabled drive will still provide the RP03 signal. The maintenance manual isn't clear (in the bits I have read); perhaps the schematics would say.
On the boot command, a disk that is bootable (as RP11 obviously is) really should include that command. Ok to do it separately but it's definitely needed.

[al20878]

@pkoning2 : I just pushed an update that should address the PR02 type disks, which are not attached, as discussed previously;
and I also implemented the boot command -- I checked it with what I have around here, and it appears to work correctly for me...

Although, I do not have a system RP02/03 disk so I was using an RSTS/E data disk that printed this message for me:

?PLEASE BOOT FROM THE SYSTEM DISK.

which basically proves the concept for me that the bootstrap is getting loaded properly (I also checked with different unit numbers and different bus address, and it worked in all those cases).

Please let me know if you are able to boot the actual system (RSTS/E, I presume).

P.S. BTW I checked the schematics for RP11-C / RP02 / RP03.... From what I could see (and I am not that keen in reading schematics, TBH), since RP02 and RP03 are actually two different drives, a powered-on RP03 would still post the RP03 bit on in the controller, even if the drive switch in the DISABLED position. RP02 won't post that bit, regardless. Since you mentioned that a non-zero RPDS value was all that was required for RSTS/E v10 to "decide" whether a drive was present or not, that logic is not exactly correct, as it would detect RP03 as always present (since they post the type bit) -- irrespective of their actual ONLN bit; yet it wouldn't detect RP02s as present if they were in DISABLED (as RDPS would be 0 for them, in that case). So I left the logic in place that in simh a drive posts the type bit only if it's enabled (which means that the ONLN bit is also set) -- this way, the detection by the OS would be more consistent, IMO.

[pkoning2]

Looks good. I don't have a RSTS/E monitor built with RP support included, but I can boot INIT on both an RP03 and RP02, and it sees the device types correctly , and handles disabled units as you intended.
Can you rebase this? It complains about conflicts. I don't mind doing a rebase that is free of conflicts but resolving conflicts is better done by the author of the code.

[al20878]

Thanks for the confirmation!

I tried to rebase but git seems to be quite happy saying it was dropping my squashed commit (that I tried to create) and saying that everything is fine.

I wonder if you can do something like this on your end, instead: it's really going to be easier for all of us:

I don't see any conflicts on my end and I see it all green and ready to merge.

[pkoning2]

It looks like you're showing stuff from the user's guide. But on the page for this PR I see this instead:

[al20878]

Yes, that instruction was from the user manual, but that's exactly that I see on my end here:

So I don't see any conflicts, both web-based and local git repo... it's all clean and green

[al20878]

BTW, I was able to sysgen RSTS/E V6C for RP03 and it boots and works.

While I was playing with it, I noticed the following:

DF0: disabled - no RF: controller
DS0: disabled - no RS: controller
DS1: disabled - no RS: controller
DM0: disabled - no RM: controller
DM1: disabled - no RM: controller
DR0: disabled - no RR: controller
DR1: disabled - no RR: controller
DB0: disabled - no RB: controller
DB1: disabled - no RB: controller
MM0: disabled - no TU: controller
MM1: disabled - no TU: controller
DT0: disabled - no TC: controller
DT1: disabled - no TC: controller

13 devices disabled

Namely, the DRx: devices and their RR controller.... Do you know what those are?

Obviously they are not at all the RP11-related stuff (which RSTS/E calls as an RP controller and the DPx), but there's definitely a name clash from the user standpoint (whatever the RR controller RSTS/E has in mind is not the RR controller as shown in simh -- because it's just a name substitute for what should have been an RP controller, but the name has been already taken by the next gen RP disks).

Should RR be reconsidered in simh, given this recent finding? Or it's okay to leave it alone? Bob said he was fine with RR. There was a suggestion to use DP but that's usually how the communication devices are named (and actually there was a DP11 interface, which was why I didn't want to hog that abbreviation, in case someone would want to implement that, eventually).

[al20878]

Answering myself (for the record):

So the naming is already inconsistent, and I wouldn't worry about any further confusion. Let us it write off as "historic" :-)
We can have the RR controller in simh, which is responsible the DP disks in the OSes -- it's okay.

[pkoning2]

In RSTS, RP11 disks are called "DP". And it uses "DB" and "DR" for the RP/RM series disks. Two names because it supports two Massbus controllers for those; originally, you had to have either RP or RM disks on a given Massbus but not a mix. In late releases that restriction was removed, but there were still two names and two separate (identical) device drivers, just as a hack to handle the two controller addresses. The "DR" code was used because it was available; DM was not because that was chosen earlier to refer to the RK06/07 disks.
You're getting those "disabled" messages because the SYSGEN monitor is configured with "two of each disk". So at startup, it compares the OS config with the hardware, and marks as disabled any configured unit that doesn't actually exist on the system it's running on.
I'm not all that thrilled with the name "rr" for the RP11 in SIMH, but it's just one of many random names that have no connection to DEC conventions. Consider "vh" (for DMV-11) or "hk" for RK06, not to mention "rq" for MSC disks. So we'll just have it be another one of those things that people just have to get used to.

[pkoning2]

Yes, that instruction was from the user manual, but that's exactly that I see on my end here:

So I don't see any conflicts, both web-based and local git repo... it's all clean and green

What repo are you looking at? I'm looking at the PR, which is in the SIMH repo. You may not have the latest opensimh/simh "master" branch in your fork, and if so you might indeed see different status.

[al20878]

You're getting those "disabled" messages because the SYSGEN monitor is configured with "two of each disk"

I understand why they were disabled, I was just pointing out a potential naming confusion. Since there are many more already, I am closing this question after seeing that.

or "hk" for RK06

Bob explained that it was "next gen" RK, and this HK abbreviation was used in Unix, so it came from there -- it wasn't random.

So we'll just have it be another one of those things that people just have to get used to.

Yeah, that is what it boils down to -- there are too many devices and only a handful of abbreviations

[al20878]

You may not have the latest opensimh/simh "master" branch in your fork, and if so you might indeed see different status.

I updated my branch to be up to speed with opensimh (a few days ago). And this is what it shows for me right now:

The green message shows on my PR -- which is in the opensimh repo -- and that's where the merge is going to be go.
It's not "my" fork.

This is the URL:
#244
https://github.com/open-simh/simh/pull/244

And the all-green is given under this very input box that I'm typing in right now.

[pkoning2]

I don't know why github is giving me this error message. I can merge it just fine in the command line interface.

[pkoning2]

Ok, done, but I got a warning message explaining the issue.
One of your commits is a merge, and the repository policy is not to allow merges (commits with two parents). So if you need to bring your dev branch or dev repository up to date, use "git rebase" and not "git merge".
That keeps the "linear history" which is what we like to have, because git merge histories make some things harder (for example, "git bisect").

[al20878]

One of your commits is a merge
None of my commits was a merge -- whatever github did to bring upstream:master to speed I had no control -- but it looks like they did the merge instead of rebase. All other commits were not related to upstream:master (were new), so could have been rebased (and squashed into a single commit to master, for that matter) -- the PR was perfectly linear w.r.t. to master branch (if done in a single final commit). IDK why github works differently than the command line interface.

Thanks for merging and closing!

If you get a chance, please take a look at #297 -- it's a single-line patch that was long ago confirmed / approved by Bob

[pkoning2]

Will do.

Github does a bunch of things wrong, but it's close enough to be tolerable. On the merge, the issue is the way updates from the source repository are applied to a fork; this is done by a merge and that's what is wrong. It is clearly possible to avoid that by doing a rebase, but I don't know if the Github UI has a way to ask for that. It can clearly be done in a shell, I'll see about documenting that.