-
Notifications
You must be signed in to change notification settings - Fork 362
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
Fix simulator issues #344
base: develop
Are you sure you want to change the base?
Fix simulator issues #344
Conversation
… the result of the simulation flipped.
…his help preparing the SVGs and FPZs files.
<views> | ||
<iconView layer="icon"> | ||
<geometry x="-1" y="-1" z="-1"/> | ||
</iconView> | ||
</views> | ||
</instance> | ||
--> | ||
<!-- | ||
<instance modelIndex="33" moduleIdRef="a59353bd0db0225dc22770e292c622f3" path="lm358.fzp"><views><iconView layer="icon"><geometry x="-1" y="-1" z="-1"/></iconView></views></instance><instance modelIndex="34" moduleIdRef="1001AAAB555Timer" path="555timer.fzp"><views><iconView layer="icon"><geometry x="-1" y="-1" z="-1"/></iconView></views></instance><instance modelIndex="35" moduleIdRef="1001AAACOptocoupler" path="optocoupler.fzp"><views><iconView layer="icon"><geometry x="-1" y="-1" z="-1"/></iconView></views></instance><instance modelIndex="36" moduleIdRef="a4f38a10-dd92-11dd-a3e2-001f5b3a17a4" path="h-bridge.fzp"><views><iconView layer="icon"><geometry x="-1" y="-1" z="-1"/></iconView></views></instance><instance modelIndex="36" moduleIdRef="1a7828b6466fac9fd042221e931124c4" path="74HC595.fzp"><views><iconView layer="icon"><geometry x="-1" y="-1" z="-1"/></iconView></views></instance><instance modelIndex="36" moduleIdRef="HEF4094" path="HEF4094.fzp"><views><iconView layer="icon"><geometry x="-1" y="-1" z="-1"/></iconView></views></instance><instance moduleIdRef="RevEd_FRM050"><views><iconView layer="icon"><geometry x="-1" y="-1" z="-1"/></iconView></views></instance><instance moduleIdRef="RevEd_ULN2003A"><views><iconView layer="icon"><geometry x="-1" y="-1" z="-1"/></iconView></views></instance><instance modelIndex="38" moduleIdRef="CrystalModuleID" path="crystal.fzp"><views><iconView layer="icon"><geometry x="-1" y="-1" z="-1"/></iconView></views></instance><instance modelIndex="38" moduleIdRef="37asdf3c843a2127a0e499f8a0b3ef6aCrystalModuleID" path="resonator-3pin.fzp"><views><iconView layer="icon"><geometry x="-1" y="-1" z="-1"/></iconView></views></instance><instance modelIndex="38" moduleIdRef="4001_4_x_2_input_NOR_gate_single"><views><iconView layer="icon"><geometry x="-1" y="-1" z="-1"/></iconView></views></instance><instance modelIndex="38" moduleIdRef="4001_4_x_2_input_NOR_gate_multipart"><views><iconView layer="icon"><geometry x="-1" y="-1" z="-1"/></iconView></views></instance>--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the line breaks intentionally removed? Since the section is commented out, maybe it can entirely be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My tool for auto-indent removes the line breaks for the commented lines. These lines are commented as these parts do not have SPICE fields currently, but they could be added more or less easily. It is a kind of TODO list for me, but we can remove them if you prefer it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, great. I am working with someone to add more SPICE fields right now. The it would make sense to start with those parts, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Stating with these ones makes sense. However, there are a few that will not be useful until we extend the simulator to work with transitory analysis, such as the 555, the HEF4094 or the 74HC595. Maybe they could be a second step.
I have a functional example of an NAND gate, but I would like to add some LEDs at the input and output to show the signals. But my model does not handle well the LEDs (the NAND gate cannot drive the LEDs). Maybe I could send it to you and the person can use it as a starting point.
c-3.966,0-7.184-2.252-7.184-5.029v3.148v1.547c1.5,1.672,4.153,2.789,7.184,2.789c4.687,0,8.48-2.658,8.48-5.938v-1.546 | ||
C20.432,15.167,19.95,14.089,19.133,13.176z"/> | ||
<path opacity="0.24" d="M19.133,13.176v3.146c0,2.777-3.217,5.029-7.185,5.029 | ||
c-3.966,0-7.184-2.252-7.184-5.029v3.148c1.5,1.674,4.153,2.789,7.184,2.789c4.687,0,8.48-2.658,8.48-5.938 | ||
C20.432,15.167,19.95,14.089,19.133,13.176z"/> | ||
<ellipse opacity="0.5" fill="#CCCCCC" cx="11.95" cy="16.323" rx="7.183" ry="5.029"/> | ||
<ellipse id="color_path2" opacity="0.5" fill="#CCCCCC" cx="11.95" cy="16.323" rx="7.183" ry="5.029"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The id color_path2 is used twice. Was this intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this was a mistake. If the id starts by "color_" them it will change by the simulator code based on the results of the simulation. We can change this to "color_path3". Does this cause a bug? Simulator worked properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of things at least have bug potential. Instead of a single item, the xml parser might return a list.
This might be some kind of todo. Instead of a magic id, we could use data fields (similar to javascript patterns) to mark dynamic svg content. (The alternative of using CSS I think is overkill, and would probably be unreliable since CSS parsing is hard)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it would be nice to have an easy way to define dynamic svg properties. Will you fix this or should I commit the change to this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you separate the PR for the schematic symbols from the simulator improvements? Or even if fixes are very related, I'd prefer smaller reviews. More than 100 files is a bit hard to handle at once. Since the merge is "all" or "nothing" otherwise good changes might delay because of unrelated minor issues.
Yes, I can do that. I will do it as soon as you reply my explanations to see if I need to change something. I thought that you could cherry-pick commits if necessary... |
I will cherry-pick the commits. I think all questions have been resolved (and lets see how the ID works for now, refactoring to use something like "data-*" can be done in a separate ticket. |
I just noticed a small issue. The led-rgb-6pin.fzb has the wrong moduleID. I changed it to "LEDModuleID", but it needs to be "ColorLEDModuleID" to trigger the LED code ("LEDModuleID" just triggers the capacitor code). I am attaching another commit to fix this. It is a minor thing as led-rgb-6pin.fzb has not yet SPICE info, but with this change is prepared to be simulated in the future. |
…ed (instead of using a high resistance) to be more realistic
I rebased this branch, and the led rgb 6 pin fix was not yet integrated: https://github.com/fritzing/fritzing-parts/tree/fix_simulator_issues |
Adds spice fields for RGB leds and photocells
Fixes potentiometer symbols
Fixes a few bugs and small issues