pfalstad / circuitjs1

Electronic Circuit Simulator in the Browser
GNU General Public License v2.0
1.65k stars 280 forks source link

Rewrite both shift registers #15

Closed nulldg closed 3 years ago

nulldg commented 3 years ago

The SIPO and PISO are cleaner now, DRY violations are gone. The SIPO is now more efficient because it's no longer wasting resources storing and manipulating redundant information. Of course, I've also enabled changing the number of bits (since not every shift register IC is 8-bit)

I've also added defaultBitCount() to ChipElm.java to allow elements to use a different default bit count: the number of bits to use when creating a new element, or when the bit count has been omitted in the dump for some reason (eg loading old circuits from a time when "# of bits" was hard coded). There used to be a hard-coded exception for the Ring Counter to give it a default of 10, but the shift registers now need an exception too, because the default bit count of the shift registers needs to be 8 to not break old circuits, whereas the default is 4.

Although I've unit tested these shift registers and tested loading an old dump, I don't have a good circuit to properly test them with.

pfalstad commented 3 years ago

looks great. I don't have any old circuits that uses these. One thing I noticed is that, like the old version, it doesn't save its state when exporting/important. For the Sipo you just need to set state=true for all the output pins. For Piso it would be more work.

nulldg commented 3 years ago

The states are now exported and imported as integers. I've ensured that even if bits are missing from the data, the registers will still load successfully.

nulldg commented 3 years ago

Hmm, I'm concerned the pins are backwards. In real shift registers, Q1 feeds into Q2, which feeds into Q3. But in Edward's implementation, the latches feed the wrong way round. Fixing this would be trivial, though.

nulldg commented 3 years ago

The shift registers now work the same as the 74HC164 and 74HC165. But unlike the 74HC165, the PISO has an extra output register. This isn't obvious. This new labeling should hopefully allude to this. But by fixing the labelling, we've now got a cosmetic problem with symmetry, where the PISO and SIPO are both oriented in opposite directions. image The SIPO is perfect, so if this is enough of a problem, we should change the PISO.

pfalstad commented 3 years ago

The PISO pins should be renamed so D0 is on the left, shouldn't they?

The bigger problem is that the PISO doesn't work like the 74HC165. If I'm reading this datasheet correctly, whatever you load into D7 is immediately reflected in the output (Q7).

pfalstad commented 3 years ago

Also the code to dump/undump an array of bits should be shared by moving it into ChipElm.

Using StringBuilder is fine but it's basically the same as string concatenation in GWT, so I wouldn't go out of my way to use it. Here is how it is implemented: https://github.com/gwtproject/gwt/blob/master/user/super/com/google/gwt/emul/java/lang/StringBuilder.java

nulldg commented 3 years ago

The PISO pins should be renamed so D0 is on the left, shouldn't they?

I compared the simulated behavior against the datasheet's examples and it seems to check out. Of course, I could be wrong. I'll test it again in a sec.

The bigger problem is that the PISO doesn't work like the 74HC165

Yeah, that's what I said. This PISO has an extra output register for some reason, but I don't think we can change this behavior now.

Using StringBuilder is fine but it's basically the same as string concatenation in GWT

Wow, that's disappointing. StringBuilders are normally significantly faster than standard concatenation. I could have sworn javascript had buffers or something which would have let the GWT developers retain the performance boosts. Still, it does allow me to make those Integer.toString calls implicitly.

pfalstad commented 3 years ago

oh extra output REGISTER. gotcha.

nulldg commented 3 years ago

Also the code to dump/undump an array of bits should be shared by moving it into ChipElm.

I'm having difficulty with this. The code may look duplicate, but there are cardinal differences between the import/export functions of both shift registers. Without anonymous functions in Java, these differences are difficult to reconcile. When exporting: The PISO needs to read its data as a circular array (not a direct index!), and the SIPO reads its data directly from its pins' values. When importing: The PISO reads bits into the data array, but the SIPO needs to assign the values to its pins' value.

pfalstad commented 3 years ago

You can leave it like it is if you want and I'll take a look at it. I will probably copy the bits into a temporary array.

nulldg commented 3 years ago

I worry that that'll end up resulting in more duplicate code just to mutate the data into a format the common function can work with.

nulldg commented 3 years ago

Thinking more about the PISO, maybe it won't be necessary to treat the data like a circular array, considering how the PISO shifts destructively. Data prior to the circular array's index is just garbage anyway. As long as enough zero-padding is added to the start, this would simplify it.

pfalstad commented 3 years ago

Either D0 should be on the left, or the output should be renamed Q-1 (which would be weird). If we didn't have the extra output register, then the output would be named after the input right next to it, since it is the same. But since we do have the extra register, it should be one larger/smaller.

nulldg commented 3 years ago

I've been thinking more about this problem.

The pins are 0-indexed numbers referring to the index of the internal shift register it's connected to. Pins prefixed with D are inputs, and pins prefixed with Q are outputs.

D0 is the most significant bit which means it's the first to go out, and Dh is the least significant digit, so it's the last to go out. The reason the final pin is labeled Q8 (for an 8-bit SR) is because it represents the final output register, after D7.

When clocking out, the first bit to go out is H (which would correspond to pin D7) and A is the last (which would correspond to D0). In other words, the data moves from A to B to C to D, etc until it reaches the output at the end. image image

Edit: Whoops. We can flip the indexes and it'll be fine.

pfalstad commented 3 years ago

I'm confused. It looks like the bits are going the right way (to the right). Here is a test circuit.

https://tinyurl.com/yflqttv5

Click load, and you will get 10000111 on the output.

nulldg commented 3 years ago

Whoops, I think I confused myself by mistaking branches. You're completely correct. Ignore the points about the data going the wrong direction, sorry!

nulldg commented 3 years ago

Yeah, Edward still got the indexes the wrong way round, likely because he was unnecessarily manipulating the PISO's state in bitwise, but the visual direction actually is correct. Everything is good now.

nulldg commented 3 years ago

Do you think we should add a SER pin? It's often used to combine several PISO shift registers to increase the number of bits, but it's useful for other applications too (eg using the parallel-in-serial-out as a serial-in-serial-out shift register, or providing overflow data)

pfalstad commented 3 years ago

If you want, but then won't we have to remove the extra register? If we want to combine several PISO's, that is. We could certainly do that with an extra flag bit.

nulldg commented 3 years ago

SER would fill the gap at Q0 as the data is shifted up. Right now we're just filling the gap implicitly with 0s. Unless I'm mistaken, I don't think the extra register would affect this. The hidden register would be a nuisance for linking, though, and I agree we should have a flag to enable/disable it.

pfalstad commented 3 years ago

Yes, adding SER and/or removing the hidden register would be good if you're willing.

nulldg commented 3 years ago

How should we deal with the hidden register? Should all new shift registers simply omit it? Or should there be an option for enabling/disabling it?

pfalstad commented 3 years ago

omit it I think. there's no reason to enable it. the only reason I would add the option is to make it more obvious that there is a difference between old shift registers and new ones. but I think that is clear by looking at the output, which will be labelled differently, right?

nulldg commented 3 years ago

Yep, it will be.

pfalstad commented 3 years ago

I didn't merge this because it sounds like you're still working on it, right?

nulldg commented 3 years ago

Yep, I still would like to add the SER pin. Sorry for the delay, I've been quite busy recently.

pfalstad commented 3 years ago

no rush at all, don't worry.

nulldg commented 3 years ago

Looks like I'm done! As the commit name suggests, I've added the SER pin, removed the hidden register, and simplified writeBits since the extra arguments are not needed anymore. Although I've done my best to preserve the old behavior for old circuits, I think more testing is needed. I would use an old circuit of mine, but none of my circuits use these shift registers because of the limitations circuitjs's shift registers had. (I used subcircuits instead, but subcircuits can be rather slow for large circuits)

pfalstad commented 3 years ago

Excellent!! Looks great! I don't have any old test circuits either, unfortunately.

nulldg commented 3 years ago

I'll try make a digital circuit using the master branch, and then import it using this branch to make sure there aren't any bugs I didn't catch by simply unit testing it.

nulldg commented 3 years ago

Good news, it works. This is the old circuit I tested it with. The circuit takes 4 milliseconds to start working because I wanted to make sure the registers' initial conditions are preserved across dumps and the version change.

Here is an updated version which is slightly cleaner because it no longer has the hidden register.