Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite SeqGenElm.java #14

Merged
merged 15 commits into from Jun 22, 2021
Merged

Rewrite SeqGenElm.java #14

merged 15 commits into from Jun 22, 2021

Conversation

nulldg
Copy link

@nulldg nulldg commented Jun 11, 2021

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.

"One shot" is now stored as a flag, but the boolean still exists for backcomp. This also adds a reset pin.
@pfalstad
Copy link
Owner

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
Copy link
Owner

Looks like a big improvement though.

@pfalstad
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

nulldg commented Jun 11, 2021

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
Copy link
Author

nulldg commented Jun 11, 2021

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
Copy link
Owner

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
Copy link
Owner

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

@nulldg
Copy link
Author

nulldg commented Jun 11, 2021

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
Copy link
Author

nulldg commented Jun 11, 2021

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.

  • It cannot be flipped, meaning the input MUST be on the left and output MUST be on the right.
  • There is no way to control how fast the "one shot" sequence is played. The rate is hard-coded.
  • The sequence generator cannot be reset, and so the user has no way to restore its internal sequence position to a known state without resetting the entire circuit.
  • The fixed sequence size of 8 bits severely limits the usefulness of the component.

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
Copy link
Author

nulldg commented Jun 12, 2021

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
Copy link
Author

nulldg commented Jun 12, 2021

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

@nulldg
Copy link
Author

nulldg commented Jun 12, 2021

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
Copy link
Owner

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
Copy link
Author

nulldg commented Jun 12, 2021

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

@pfalstad
Copy link
Owner

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
Copy link
Author

nulldg commented Jun 12, 2021

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
Copy link
Owner

pfalstad commented Jun 12, 2021

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

@nulldg
Copy link
Author

nulldg commented Jun 12, 2021

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
Copy link
Owner

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
Copy link
Author

nulldg commented Jun 12, 2021

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
Copy link
Owner

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

@nulldg
Copy link
Author

nulldg commented Jun 12, 2021

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

@pfalstad
Copy link
Owner

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
Copy link
Author

nulldg commented Jun 12, 2021

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
Copy link
Owner

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
Copy link
Author

nulldg commented Jun 12, 2021

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
Copy link
Owner

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
Copy link
Author

nulldg commented Jun 12, 2021

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
Copy link
Owner

pfalstad commented Jun 12, 2021

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.

@nulldg nulldg changed the title Cleanup SeqGenElm.java Rewrite SeqGenElm.java Jun 13, 2021
@pfalstad
Copy link
Owner

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

@nulldg
Copy link
Author

nulldg commented Jun 14, 2021

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

@pfalstad pfalstad merged commit e337817 into pfalstad:master Jun 22, 2021
@nulldg nulldg deleted the seqgen-cleanup branch June 22, 2021 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants