ra4king / CircuitSim

Basic Circuit Simulator
https://ra4king.github.io/CircuitSim
BSD 3-Clause "New" or "Revised" License
76 stars 27 forks source link

Align Transistors with Patt & Patel #72

Closed ausbin closed 2 years ago

ausbin commented 2 years ago

Today Professor Tom Conte, who is teaching CS 2110 this semester, expressed frustration with CircuitSim transistors, so I've taken a stab at addressing his criticisms.

To my understanding, his main criticism was that the little arrows/chevrons on transistors in CircuitSim do not match the transistor diagrams in Chapter 3 of the CS 2110 textbook (Patt & Patel 3rd ed.). To address that, I have removed the DIRECTION property entirely from TransistorPeers and configured them to effectively point NORTH if they are N-type and SOUTH if they are P-type. (This matches all of the figures in Patt & Patel.) Practically, this means you can't have horizontal transistors anymore, and the source/drain pins flip locations when you go between P-type and N-type.

Additionally, about 4 years ago, I emailed Tom to ask how CircuitSim should handle hooking up P-type transistors to ground and N-type transistors to VDD. I managed to find the thread on my hard drive and have tried to implement the behavior he recommended. I've included the email thread in a giant Javadoc comment below.

These are definitely breaking changes but in my opinion, these make CircuitSim do a better job as a circuit simulator for CS 2110, and I think the blast radius should be pretty small: it's the first week of the semester and only 2110 uses the transistor component (to my knowledge), so old 2110 TA solutions will break, and that's about it. I'm curious what you think, Mr. Roi

ausbin commented 2 years ago

I just remembered: Tom isn't the only one unsatisfied with CircuitSim transistors having a direction. Here's a picture of the current CircuitSim transistor-direction behavior confusing Dan Forsyth for several minutes during a lecture in Spring 2018:

image

ra4king commented 2 years ago

Thanks for the pull request! I prefer avoiding breaking changes, perhaps we could introduce a new "corrected" transistor type and leave the old transistor type around? I wonder if we could use the program version to detect saves with the old transistor, while allowing only the new transistor for newer saves?

ausbin commented 2 years ago

That's a cool idea @ra4king. Do you know any other places in the code that use that trick that I can reference?

In the worst case we can merge this into the 2110 fork (the students will probably install CircuitSim in lab next week, so time is precious), but it would be nice to upstream

sameer-s commented 2 years ago

I think that (as of now) the version of a file that's saved is the current version of CircuitSim, regardless of what's previously in the file. (So loading a file from an earlier version, making a change, and then saving it, would cause it to be saved with the new version number).

So unless we wanted to change that behavior as well, it could lead to the following:

  1. Load an old file
  2. Make some changes, using the old transistor behavior
  3. Save the file and reload
  4. The transistors are now the "new type"

Maybe we could just add the new transistor type alongside it and hide the old one in the sidebar? My only concern with that is that when you load / modify older files, you would get a hodgepodge of new + old transistors.

sameer-s commented 2 years ago

Another easy option would just to make the behavior configurable by a checkbox/dropdown and have it default to the new behavior for newly-dragged components, but have it default to the old behavior if its value isn't specified in the JSON.

That way backwards compatibility would be maintained, and people could extend old circuits with the old behavior if they wanted to, but the new behavior would be the default as far as students are concerned, and it's something we could easily check for in autograders (i.e. no transistors where "new behavior == false")

ausbin commented 2 years ago

@ra4king I pushed up a commit to keep the old transistor supported in existing circuits (with the same behavior) but not show it in the components list in the GUI. Instead, students can only choose the new transistor to place in their circuit. They could edit the .sim file or use a previous version of CircuitSim or paste one in or something, but that's fine with me

ra4king commented 2 years ago

I really like the decision to add that "showInComponents" param, this way old files can still load it but it doesn't show up in the UI anymore. I merged your code in locally and made some fixes to the Simulator to support exception-throwing from within the valueChanged() function. Will test some more and officially merge. Thanks!