I just look in an archive that I downloaded on 01 DEC 2023 and this is the header that it has.
/**
* A 16-bit counter with load and reset control bits.
* if (reset[t] == 1) out[t+1] = 0
* else if (load[t] == 1) out[t+1] = in[t]
* else if (inc[t] == 1) out[t+1] = out[t] + 1 (integer addition)
* else out[t+1] = out[t]
*/
I downloaded it to confirm some unrelated issues and the authors then updated the file set, which I downloaded on 04 DEC 2023. The header in the PC.hdl file at that time was:
/**
* A 16-bit counter with increment, load, and reset modes.
* if (inc(t) == 1) out(t+1) = out(t) + 1
* else if (load(t) == 1) out(t+1) = in(t)
* else if (reset(t) == 1) out(t+1) = 0
* else out(t+1) = out(t).
*
* To select a mode, assert the relevant control bit,
* and de-assert the other two bits.
*/
I just downloaded the current files (which changed a couple months ago in response to a few issues) and the header in that file is slightly different:
/**
* A 16-bit counter with increment, load, and reset modes.
* if (inc(t)) out(t+1) = out(t) + 1
* else if (load(t)) out(t+1) = in(t)
* else if (reset(t)) out(t+1) = 0
* else out(t+1) = out(t)
*
* To select a mode, assert the relevant control bit,
* and de-assert the other two bits.
*/
One of the issues I brought to their attention was a file that used both '==' and '=' for an equality comparison. The seem to have moved away from that, since not all programming languages use the '==' operator and so some students may be confused by it. The mixed use appeared to just be a case of missing something in the editing process, so apparently they went through all of the files and scrubbed them for that.
As for the issue you brought up, it looks to me like someone (probably not the authors) tried to make things clearer by putting the signals in the order of most frequent use. In the process, they appear to be aware that their reordering only produces the same results if the instruction at the bottom of the comment if followed.
I think that this might have been the implementation approach that was intended all along, and because they just changed some comments and didn't intend to change the effective behavior, they didn't vet the test files very well.
But, as you noted, the test file DOES assert multiple signals at the same time.
I will bring this to the authors' attention. They are generally pretty responsive to these kinds of things, but with the situation in their part of the world being what it is, it could take some time.
On a side note, I've always thought the PC definition was too cumbersome to begin with. There is no need for an 'inc' control signal -- that should be the default behavior. In fact, in my implementation of the CPU, I have it tied HI (my PC adhered to the old specification). All you need is a 'load' signal that is controlled by the instruction decode logic that overrides the default behavior, and a 'reset' signal that goes out to an input pin on the CPU that overrides everything. There is no situation in which you want the PC to simply hold the current value, so there is no need to support that feature, other than to satisfy the specification (and pass the test). This makes the decode logic SO much cleaner -- you assert the 'load' line if and only if you want to execute a jump. Done.
Unfortunately, the authors are constrained by the fact that different people are going to be using different versions of the books and the tools (once they are upgraded), so they are trying to keep the project specs and tool behavior agnostic with regards to which version is being used. Thus, while they could revise the test to only assert one of the three lines at a time, and to even always assert one of them (removing the 'hold' behavior), they are probably going to have to keep all three signals for compatibility purposes.
Worse, it's probably not a good idea to only test the cases when exactly one of the three signals is asserted, even if the spec requires that this be the case, because you know that some people ARE going to implement their decode logic to assert multiple signals at the same time, so the behaviors under those conditions really need to be specified and tested. If undefined behavior is allowed/specified, it is only going to cause problems down the road.
Locked
