Generate QFN packages - #182
Conversation
* MO-220.K01 * MO-288B Only a few variants are defined and not all combinations are generated.
3D-Models have their own UUID
| -not -name 'generate_dfn.py' \ | ||
| -not -name 'generate_dip.py' \ | ||
| -not -name 'generate_so.py' \ | ||
| -print0 | sort -z | xargs -t0L1 -I {} python {} --3d |
There was a problem hiding this comment.
I switched here to generating with --3d, because the assembly has its own UUID which should be cached as well.
There was a problem hiding this comment.
The 3D UUIDs are generated even without the --3d flag, so it's really not required.
There was a problem hiding this comment.
I didn't review the QFN generator itself yet. Just one thing already which I wonder after flying over: Is that complexity really required (multiple files, generating variants from JEDEC designators, looping over height codes)? Couldn't we just add a single table, where every row leads to one package, and all the required numbers are defined in that row (rather than looked up somewhere)? It will also make the table much more readable (seeing the number '2.0' is much more expressive than just an identifier 'C') and allows to define nonstandard QFN packages (i.e. with arbitrary dimensions) just the same way.
Honestly I just feel like the current concept is hard to understand and to maintain for long term. Simpler concepts may lead to a bit more typing when adding a new QFN packages, but are easy to understand and to maintain for long term (keep in mind that we will have to update the generator regularly, e.g. when a new package file format is introduced, or due to some refactoring of the entity classes or common helpers).
Also keeping all variants right in the generator (instead of separate files) would help to reduce the mess of files in this repository. I mean, I agree dividing things into separate files is generally good, but at the moment this repository is not organized that way. Some day we should clean up the repository structure (e.g. every generator in its own subdirectory, together with their corresponding cache, and with data separated from code) but at the moment IMHO we should follow the current concept for consistency. Unfortunately the DFN generator is a bad example as it is the only one not following the concept 🙈 But there we also made the big mistake to generate tons of packages instead of just the required ones...
| -not -name 'generate_dfn.py' \ | ||
| -not -name 'generate_dip.py' \ | ||
| -not -name 'generate_so.py' \ | ||
| -print0 | sort -z | xargs -t0L1 -I {} python {} --3d |
There was a problem hiding this comment.
The 3D UUIDs are generated even without the --3d flag, so it's really not required.
| -print0 | sort -z | xargs -t0L1 python | ||
| stat --format="%s %n" *.csv | sort -k2 > sizes_after.txt | ||
| diff sizes_before.txt sizes_after.txt | ||
| run: ./check_stale_cache.sh |
There was a problem hiding this comment.
Hmm honestly I don't like to factor this out in a script. This thing is (currently) not meant for local use, for several reasons:
- It is kind of dangerous since UUID cache files will be deleted without confirmation
- It may create new UUID entries without the user expecting it (it's not obvious that a "checker script" actually runs all the generators)
- It will leave behind new files not ignored in
.gitignore
And also it is not related to this PR. If you like to make this script usable locally, please do it in a separate PR. Honestly I would just keep it as-is, IMHO it's good enough if CI catches those errors, no need to check it locally.
There was a problem hiding this comment.
I factored it out because the CI threw an error and I was not able to reproduce it locally.
I can revert it on this branch.
The best solution IMHO would be to check for stale entries in the python script itself. If we use the UuidCache as suggested below, the class can check on exit whether all entries have actually been queried during its lifetime (I can add a flag to suppress it for uses where not all UUIDs might be accessed). This would also allow to get more detailed information about which uuids are stale and will directly tie into the development cycle without a separate script.
There was a problem hiding this comment.
I factored it out because the CI threw an error and I was not able to reproduce it locally.
The thing is, usually you can just delete the UUID cache file, then checkout the original version from Git again (if it was already tracked) and run the generator again. Then you will have no stale entries anymore. It's basically what CI does, but it's more explicit when you do it by hand, and you do it only for one specific script.
(of course this only works if you don't use the generated packages anywhere yet, but usually this shouldn't be the case until they are committed to our libraries)
However, your idea with UuidCache also sounds interesting. There might even be an "auto-clean" functionality which automatically removes stale entries so you don't need to modify the CSV by hand...
| return uuid_cache | ||
|
|
||
|
|
||
| class UuidCache: |
There was a problem hiding this comment.
Very nice to factor out this functionality into a common class 👍 However, to keep all scripts consistent (not leaving behind inconsistent concepts which drift apart more and more), honestly I'd highly prefer if you also adjust all the existing generators to make use of this new UuidCache. This will make future maintenance much easier.
Also I guess the functions init_cache() and save_cache() would be part of that class then, which would make the code cleaner.
In addition, since this change is not related to the new QFN generator, could you please move this into a separate PR?
If you don't want to go this (harder) way for now, I'd propose to just use init_cache() and save_cache() in the QFN generator.
About footprint sizes:
Maybe the best solution would be to add all the information from the table. Then we have both an expressive list and we can have sanity checks. About the heights:
Quess that depends on the intention of the reader. My intention was to allow to review whether the sizes are consistent with the standard.
That was actually one of the implicit goals: Right now, there is no need to touch any of the mo_xxx files if the generator changes. Generation of the packages is handled in one file. The mo_xxx files are basically just a database for the JEDEC values, so they are generator-agnostic and can be added without knowing anything about the package format or footprints. The JEDEC standards are a bit confusing when it comes to naming things. Example: Assuming you place the index area in the top left according to our library guidelines, Maybe it would be a good idea to add a subdirectory for QFN alone? |
Having said that: Should the name of the package reflect the body length declaration of the JEDEC standard or the x/y coordinates in the library? I would use width=x-size in library. |
Only a few variants are defined and not all combinations are generated.
TODOs: