-
Notifications
You must be signed in to change notification settings - Fork 15
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
Added support for fetching schematic symbols, too. #4
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for the functionality; it is highly appreciated. Before I merge it, I have several proposals (in the code) and questions:
- how are the retrieved symbols organized? I cannot understand it from the code.
- would you consider writing a simple usage guide?
kicadFilename = os.path.join(tmpDir, "board.kicad_pcb") | ||
with open(easyFilename, "w") as easyFile: | ||
with open(symbolFilename, "w") as schFile: | ||
schFile.write(json.dumps(symbolJson, indent=2)) |
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.
Is there a particular reason to not be consistent with the boards and save the schematics with indentation of 2 spaces instead of 4?
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, there is a reason. The LC2KiCAD
tool is not very robust and the parser looks compatible only with exactly the same format as the one exported from the EasyEDA.
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.
Then I would prefer to leave a note about it in the code.
easyFile.write(json.dumps(boardJson, indent=4)) | ||
subprocess.check_call(["easyeda2kicad", easyFilename, kicadFilename]) | ||
subprocess.check_call(["easyeda2kicad", boardFilename, kicadFilename]) | ||
os.system("./LC2KiCad/build/lc2kicad -v " + symbolFilename + " -a ENL:1") |
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.
Please, don't use os.system
(especially, when the project already uses subprocess.*
) - you need to only execute a program; not execute it from the shell. It makes no difference here, but it is good to be consistent with the project and also, do not invoke a shell if not needed (as a shell can perform, e.g., expansion. Also, users use various shells with inconsistent behavior).
Also, based on my proposal above; I think it makes sense to invoke LC2KiCAD from PATH, not from a directory.
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.
OK, thanks for the explanation of the difference.
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.
Then, the right way for calling the subprocess is this one?
subprocess.check_call(["lc2kicad", "-v", "symbolFilename", "-a", "ENL:1"])
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.
@@ -103,16 +142,20 @@ def buildPackageBoard(packageInfo): | |||
board["shape"] = [shape] | |||
return board | |||
|
|||
def easyEdaToKicad(boardJson): | |||
def easyEdaToKicad(symbolJson, boardJson): |
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.
You changed the prototype and meaning of the function, but you didn't update the docstring.
Also, you made this function "impure" and inconsistent in what it does:
- first, it takes EasyEDA board and returns KiCAD board (in memory)
- second, it takes EasyEDA schematics and writes KiCAD sch file into the current working directory. I would consider this as an unclear side-effect.
This looks really confusing when invoked: kicadBoard = easyEdaToKicad(schSymbol, packageBoard)
- the users might think "Ok, so to create a KiCAD board, I also need a schematics?". Also, he has no clue from the invocation about the newly created sch file.
Why not have two functions, easyEdaBoardToKicad(boardJson)
and easyEdaSchematicsToKicad(schematicsJson)
?
Fixed script for kicad 6
No description provided.