pfalstad / circuitjs1

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

Rewrite SeqGenElm.java #14

Closed nulldg closed 3 years ago

nulldg commented 3 years ago

Various major DRY violations are now gone. The class now inherits ChipElm's edit info. As such, the chip can now be flipped on both X and Y axes. Flip Y isn't particularly useful, but flip X should be very useful for this element. "One shot" is now stored in the element flag instead of a boolean. This should be backwards compatible but I've only tested this in-vitro and not fully from an example circuit. This also adds a reset pin, which had a function (hasReset) but was never properly implemented. Old circuits should not feature the reset pin.

pfalstad commented 3 years ago

check out EditInfo.changeFlag(flags, bit) also Boolean.parseBoolean(), Integer.parseInt(), etc.

I wouldn't change the dump format unnecessarily. If we already are dumping a boolean for oneShot, we might as well keep it.

pfalstad commented 3 years ago

Looks like a big improvement though.

pfalstad commented 3 years ago

I always thought that we should just have a single "sequence" text entry field that you could fill in with an arbitrary length string of bits, rather than having 8 checkboxes. Any interest in doing that?

pfalstad commented 3 years ago

So what does reset do.. I thought it would restart the sequence if we were in one-shot mode but instead it stops it? I don't know what chip this is based on, is that what they do?

nulldg commented 3 years ago

The new Boolean(st.nextToken()).booleanValue() stuff is Edward's code. I'll clean that up as well - thanks! I strongly urge using a flag for one shot, a boolean string never should have been written directly to exported files to begin with. Anyway, it's simple to work around and it'll be cleaner if we decide to make the seqgen contain arbitrary amounts of data.

As for sequences, I would absolutely love to add that to match the way the SRAM works. Being limited to 8 bits and having to enter them as checkboxes is rather unusual, especially if I want to provide a stream of data to my circuit.

nulldg commented 3 years ago

So what does reset do.. I thought it would restart the sequence if we were in one-shot mode but instead it stops it? I don't know what chip this is based on, is that what they do?

This just resets the circuit as if the user pressed the global reset button (for the entire circuit). It puts it in an inactive state (output low), and the generator waits until the first CLK tick before displaying the first bit. I was rather puzzled by this as well but assumed this was intentional. If it's not, we can just fix it.

pfalstad commented 3 years ago

Ok it was hard to tell which code was there before because of all the whitespace changes, and I don't use this element in any of the samples so I'm unfamiliar with it. I guess a flag is fine for oneshot.

pfalstad commented 3 years ago

If you don't need reset for something we should probably just leave it alone until someone complains.

nulldg commented 3 years ago

check out EditInfo.changeFlag(flags, bit)

I considered using this, however the previous code resets the data pointer whenever the edit values are changed. I'm not entirely sure why it does this, and I don't know why the reset position is 0 or 8 depending on oneShot. I didn't want to break it, so I left it alone.

nulldg commented 3 years ago

I don't think the sequence generator has any equivalent real-world ICs, besides maybe a PISO shift register with pins variously fixed high or low (but even that technically can be reset, by raising the latch). It seems like a tool for producing a byte one bit at a time, or for producing a simple repeating pattern. I've never used it in practice because it's basically entirely useless to me given how non-configurable the circuit element is in its current state.

The sequence generator is fairly useless right now due to these restrictions. Hopefully, with this branch, we/I can make it a lot better.

nulldg commented 3 years ago

I've been thinking more about the sequence generator and I realize now is the opportune time to make substantial changes which would be considerably harder to do later.

nulldg commented 3 years ago

I've tested importing and it seems to work, but I'll need a old circuit featuring the sequence generator to be sure.

nulldg commented 3 years ago

Component footprint is identical, but the options are now different. image The default sequence is 00000000. I would also like to add an easy way to import files (eg text files, raw PCM files, etc) because right now the only way to do it is to convert the file into binary and then copy+paste the binary in, or to turn the file into signed bytes (two's complement) and add it onto dump and change the bit count to match. I think this should also be done for SRAM, because I often need EEPROM for my digital circuits and the closest thing is the SRAM component.

pfalstad commented 3 years ago

I would just store the sequence internally as a string personally. Then we don't have to parse it and then do a bunch of work to un-parse it when dumping.

We definitely do not need to pack the sequence bit by bit into an array of bytes. This is not running on a VIC-20. It has the side effect of adding a bunch of pad bits to the end of the sequence.. If you set the sequence to 111 then you get 5 zeroes at the end before the sequence starts up again.

Also this is all converted to javascript so using types like byte, short, etc. instead of int accomplishes nothing.

nulldg commented 3 years ago

Whoops, using data.length instead of bitCount was a massive blunder on my behalf.

pfalstad commented 3 years ago

What's one shot doing? It's acting weird...

$ 1 0.000005 30.13683688681966 50 5 50 5e-11 188 400 448 448 448 6 3 5 M 496 480 592 480 0 2.5 R 400 448 352 448 1 2 100 2.5 2.5 0 0.5 o 1 64 0 4098 5 0.1 0 1

nulldg commented 3 years ago

I wrote the code in such a way that changing the internal data type is relatively trivial. I can change it to store data as an int array rather than as a byte array. The reason packing/unpacking is done rather than storing as chars is because it makes the exported file considerably smaller for large sequences (which will be one of the use-cases of this sequence generator). There's also the fact that if it were a string, there would be 15 wasted bits for every bit of information, which is rather unnecessary. The old code used to do the same thing (bitwise rather than char-by-char) except with a single primitive rather than an array of primitives.

pfalstad commented 3 years ago

Ok I see that one shot is just as broken before as it is now.

nulldg commented 3 years ago

Heh, the one-shot has a metric ton of bugs in it which need ironing out. Hopefully we'll be able to squash them in this branch.

pfalstad commented 3 years ago

Ok I didn't see how you were dumping it, I glanced at it and thought you were unpacking and dumping as a string. That's fine. Making the dump file small is a worthy goal.

nulldg commented 3 years ago

There are still a few more things to do. The baud rate (of the one-shot), for instance, needs to be configurable and included in the dump. The one-shot in that circuit certainly needs to be fixed, but I'm not sure why it's acting that way. Finally, I need to remove any leftover debug stuff!

pfalstad commented 3 years ago

Importing old circuits seems fine. I don't have any old circuits to check against.

nulldg commented 3 years ago

What part of this component should the scope monitor? The input or the output?

pfalstad commented 3 years ago

the output

so what's going on with the baud rate, what does that do? I thought it just output one bit for each clock transition.

nulldg commented 3 years ago

I think I found the bug in the one-shot which was present in the old version. It occurs when the user resets, because the component stores a value of when the last tick occurred (lastchangetime), but the value does not get reset when the circuit is reset.

so what's going on with the baud rate, what does that do?

The baud rate is used when the component is set to "one shot" mode. It determines how quickly to automatically write out the bits, in bits per second. In normal mode, the value does nothing because the sequence is progressed manually.

the output

The scope is currently reading the input-ground voltage rather than the output-ground voltage, and I'm not sure how to fix this.

pfalstad commented 3 years ago

I don't like adding a baud rate.. just use a clock to specify the baud rate. Doesn't matter if the old code did it, the old code was broken.

create getVoltageDiff() to return the voltage used by the scope.

nulldg commented 3 years ago

I don't like adding a baud rate.. just use a clock to specify the baud rate. Doesn't matter if the old code did it, the old code was broken.

I completely agree with you. People should use a CLK instead. This would drastically simplify the component. As much as it was a pain to debug and finally get working, I'm not opposed to simply cutting "one-shot" out entirely. One-shot was completely unusable (unless you never reset the simulation) because of that major bug with the internal timer, so cutting it isn't going to affect any old circuits.

pfalstad commented 3 years ago

Well, we should make one-shot do what I assumed it did, which is keep the sequence from restarting when it's finished. Everything else should be the same.. It shouldn't ignore the clock, have a hard-coded baud rate, etc...

nulldg commented 3 years ago

I've cut out all the old code. We can add that option separately. I think we should name the option to something clear. "One-shot" can be somewhat ambiguous. Maybe "Loop sequence" or "Play once"?

pfalstad commented 3 years ago

I thought one shot was pretty clear but Play Once (capital O) is fine. renaming it might be wise to distiguish from the old broken behavior.

pfalstad commented 3 years ago

If you set the sequence to 1101111100, click OK and then edit again, the sequence is now 1101111111

nulldg commented 3 years ago

Good catch I must have missed that when changing the export primitive from bytes to integers