-
Notifications
You must be signed in to change notification settings - Fork 742
Conversation
This seems to be a duplicate of #2308 edit: fixed the link to the other PR. |
Oops, I seem to have missed #2130.
|
3d86021
to
94135c0
Compare
I will most likely review and hopefully merge this soon. |
Sure thing! In past repos people have preferred force-pushing so I got in the habit of that. I just realized my pin lengths are 200mil when they should be 100mil, I'll fix that in the next couple hours. |
@cpresser fixed the pin lengths |
bd953d4
to
a008773
Compare
@cpresser any updates? |
Sorry for the huge delay. I am quite busy with moving our Hackerspace to a new location. This takes a insane amount of time out of my budget. |
I will update this post with my review results. So please check if there are any edits. I am not sure if we should include aliases for the device variants
Those are the new symbols:
I did not check each alias since most of the descriptions need improvement. |
@0xdec The review is done for now. I will do another round once you do some fixes. Overall this is a really good contribution. There are only minor issues. Keep up the good work! |
I feel as though the variants have fairly specific changes that would require reading the datasheet, but don't really change the core functionality of the device. The symbol is unchanged for most of the variants (L excluded), so it seems to me that there's no reason to make an entirely new identical symbol. The descriptions for each variant are rather verbose:
Do you think it's worth it to try to simplify this description for the variant description? Or just include the variant name in the description?
This is because the
Done, although it looks kinda ugly. Maybe the symbol body should be widened... |
@cpresser oops looks like I made those changes while you updated your review...will look at the new stuff now. And thank you, I will! |
@cpresser I think I addressed all the issues |
As for the discussion about variant B, it seems that variants B, C, and D are simply silicon revisions that add RWW support to sections of the flash memory. I would argue that this is the same level of functional difference as different SRAM or FLASH sizes, so it shouldn't warrant a new symbol. The L variant is a bit different, it seems to contain the same silicon changes that add RWW support, as well as change which peripherals are available, and the device pinout. |
Added the SAM DA1 family as well, since they're almost identical |
This reverts commit 0160a1a.
TIL: approving one commit out of 11 marks the whole thing as approved. Thanks for the changes, they are good:
To be honest, I would rather have the DA1 parts in a dedicated PR. Now this one is insanely big. Which makes it pretty hard to properly review. Also the force-push is something that makes the review harder - now I have to check which ones of the individual commits was forced. |
I figured DA1 might be a separate PR, even though they're identical. Do you think it would be okay to just have them as aliases? I literally just copied them and changed the descriptions... As for force-push, sorry about that. In past repos I've been requested to do that whenever I rebase, to avoid a merge commit. All my force-pushes have been only for rebasing on master, but I see your point. Will no longer force push from now on. |
I don't see a reason to not have them as alias. Am I missing something? Would you undo the last commit in this branch? Then I will merge this one and we can work on the DA1 parts in a dedicated PR. That PR will most likely be trivial since its only aliases.
Actually I prefer re-basing over merging since merging produces ugly commits. |
Alias seems fine to me! I'll make a new PR for SAM DA1. Went ahead and un-did the last commit. And sounds good on rebasing, will remember for future. |
Thanks for the good work! 👍 |
Add Microchip SAM D21/DA1 family (everything except the chip-scale packages).
Datasheet
All contributions to the kicad library must follow the KiCad library convention
Thanks for creating a pull request to contribute to the KiCad libraries! To speed up integration of your PR, please check the following items: