Skip to content
This repository has been archived by the owner on Oct 2, 2020. It is now read-only.

Added atomic part and dcm data for PIC18(L)F25/26K83 in the narrow DI… #1120

Merged
merged 6 commits into from Mar 10, 2019

Conversation

ghost
Copy link

@ghost ghost commented Nov 4, 2018

Commit message:
"Added atomic part and dcm data for PIC18(L)F25/26K83 in the narrow DIP-28 package to MCU_Microchip_PIC18.lib and MCU_Microchip_PIC18.dcm via KiCad's library editor."

Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/40001943A.pdf

image

image

image

image

image

Commits for other packages would follow after feedback for this one (since a lot of copy paste would be involved).
I feel overwhelmed by the "Check the output of the Travis automated check scripts" since I could not find much information online and seems to require login to https://travis-ci.org/ ?

Thanks for all your hard work on KiCad!


Thanks for creating a pull request to contribute to the KiCad libraries! To speed up integration of your PR, please check the following items:

  • Provide a URL to a datasheet for the symbol(s) you are contributing
  • An example screenshot image is very helpful
  • Ensure that the associated footprints match the official footprint library
  • If there are matching footprint PRs, provide link(s) as appropriate
  • Check the output of the Travis automated check scripts - fix any errors as required

…P-28 package to MCU_Microchip_PIC18.lib and MCU_Microchip_PIC18.dcm via KiCad's library editor.
@CLAassistant
Copy link

CLAassistant commented Nov 4, 2018

CLA assistant check
All committers have signed the CLA.

…_xxx: Moved pin locations to 100 mil grid; set component outline thickness to 10 mil; set default pin offset to 20 mil; The component should now also be perfectly in center.
@ghost
Copy link
Author

ghost commented Nov 4, 2018

Got the Travis part now, last commit resulted in no errors or comments. Newest commit message:
"Fixed erros found by the travis check for the PIC18(L)F25/26K83x-x_SP_xxx: Moved pin locations to 100 mil grid; set component outline thickness to 10 mil; set default pin offset to 20 mil; The component should now also be perfectly in center."

@ghost
Copy link
Author

ghost commented Nov 4, 2018

Screenshot of changed component:
image

@antoniovazquezblanco antoniovazquezblanco added Pending reviewer A pull request waiting for a reviewer Addition Adds new symbols to library labels Nov 5, 2018
@antoniovazquezblanco antoniovazquezblanco added this to the Backlog milestone Dec 5, 2018
@diegoherranz diegoherranz self-assigned this Feb 5, 2019
@diegoherranz diegoherranz removed the Pending reviewer A pull request waiting for a reviewer label Feb 5, 2019
@diegoherranz
Copy link
Collaborator

diegoherranz commented Mar 3, 2019

Thanks for your submission.

Commits for other packages would follow after feedback for this one (since a lot of copy paste would be involved).

Yes, good idea. Let's wait until until everything has been agreed on the symbol, and then create the required copies.

I feel overwhelmed by the "Check the output of the Travis automated check scripts" since I could not find much information online and seems to require login to https://travis-ci.org/ ?

You don't need any login. Simply click on "Details" on the Travis build and that will show the ouput of the automated analysis that has been performed on this Pull Request:
travis

A few comments:

  • Can you stack the VSS pins? See S4.3, especially the last paragraph.

  • Could MCLR be place on the left hand side, ideally near the top? That's the location that feels the most natural to me for a reset.

  • Can you remove the trailing _xxx of the part numbers? It's just for the pattern which is a special requirement. It won't be used most of the time, and we don't have it on any other PIC as far as I can see.

Thanks!

@diegoherranz diegoherranz added the Pending changes User is expected to perform fixes before merging label Mar 3, 2019
@ghost
Copy link
Author

ghost commented Mar 3, 2019

Thank you diegoherranz, will implement the changes probably within a week.

Do you know if there is any problems with the license/cla? I do not seem to be able to sign it. Clicking on Details brings me to a page where I am able to sign the cla, however, waiting a few seconds and I get "You have signed the CLA for KiCad/kicad-symbols". I remember having successfully singed the cla after my first commit.

@diegoherranz
Copy link
Collaborator

No idea about the license. The times I had to sign it, it was just a matter of logging with my github account into it and it worked. Does it ask you to "Sign in with Github to agree"?

@poeschlr poeschlr closed this Mar 3, 2019
@poeschlr poeschlr reopened this Mar 3, 2019
@poeschlr
Copy link
Collaborator

poeschlr commented Mar 3, 2019

CLA was simply stuck (Further up there was already a post "all contributors have signed". Closing and reopening helps in such cases.)

@ghost
Copy link
Author

ghost commented Mar 3, 2019

Thanks poeschlr!

@diegoherranz
Copy link
Collaborator

Thanks, @poeschlr. I'll have it in mind for further occasions.

fragraider added 2 commits March 9, 2019 17:07
…rom the part numbers. Moving the MCLR pin abuttet text, widened the symbol for this reason.
…usted the Component designator to pertrude less outside the outline.
@ghost
Copy link
Author

ghost commented Mar 9, 2019

Got a bit unsure regarding the pin stacking, does it matter which pin number is visible?

What are the thoughts on positioning the reference/component designators? I saw that other pic18*f2?k components have the reference on the top left, the component name on the top or bottom right. I find the visual gravity that the current solution offers, convenient. It allows me to find the reference faster. It uses more spave away form the component outline though. Would love to hear your thoughts on this.

billede

Renaming aliases form the options menu deleted the descriptions entries. That should be fine again, here is a screenshot of the descriptions. The whole series offers 1K of EEPROM, thought for a second that was an oversight.
billede

@diegoherranz
Copy link
Collaborator

diegoherranz commented Mar 9, 2019

Got a bit unsure regarding the pin stacking, does it matter which pin number is visible?

Not really. I normally leave the one with the lower number visible and the rest hidden, but it doesn't really matter.

  • It is important though, in case of power pins, to set the hidden ones to passive to avoid creating problems. Can you set it as passive? See the last paragraph of S4.3 for the explanation.

What are the thoughts on positioning the reference/component designators? I saw that other pic18*f2?k components have the reference on the top left, the component name on the top or bottom right. I find the visual gravity that the current solution offers, convenient. It allows me to find the reference faster. It uses more spave away form the component outline though. Would love to hear your thoughts on this.

I think I prefer the reference designator on the top left and the part name on the top right. But I don't think we are strict with this on the KLC, so it is OK as is.

I've checked every pin as well and they match the datasheet. I think there was a discussion on STM32 parts about not showing all the alternative functions of the pins. On this case, it's only key pins (oscillators and ICSP) which in my opinion helps being explicit. So happy with that.

  • Could you, however, move PORTA to the left hand side, to have PORTB and PORTC on the right hand side? That seems to be the most common for other PICs and to me having the main oscillator on the left looks better.

So, if you fix the hidden power pin to passive, and the PORT locations, it would be good to go.
Probably best to merge this and have a separate pull request with the rest of packages unless you have them ready.

Thanks!

@ghost
Copy link
Author

ghost commented Mar 9, 2019

It is important though, in case of power pins, to set the hidden ones to passive to avoid creating problems. Can you set it as passive?
Overlooked that!

fragraider added 2 commits March 9, 2019 20:30
… closely to existing PIC symbols, adjusted possitioning of reference and component designator.
@ghost
Copy link
Author

ghost commented Mar 9, 2019

Ok, have a bit of a fight with my internet, but it seems it worked. I elected to also change the component/reference designators since I feel better having them positioned as per your preference, getting more text out of the way after placing the component:
billede

Most importantly, the pin stack should be fixed now:
billede

Thank you for keeping a sharp eye and the great feedback! I am happy to oblige if there is more.

@diegoherranz
Copy link
Collaborator

Thanks for all the changes. This looks good to me.
I will merge this. Can you create another pull request for the other packages?

Thanks!

@diegoherranz diegoherranz merged commit 217ec92 into KiCad:master Mar 10, 2019
@diegoherranz diegoherranz removed the Pending changes User is expected to perform fixes before merging label Mar 10, 2019
@diegoherranz
Copy link
Collaborator

Aggh, I've just noticed that the first 'x' in the part numbers doesn't apply for the DIP package since there is no tape and reel option. Could you fix that as well on the second PR or a separate one?
Sorry for not noticing before. Thanks!

@ghost
Copy link
Author

ghost commented Mar 10, 2019

Will do!
Since I am a bit new to all of the Github stuff (as well as Git itself), I would now create a new branch and simply repeat the process (Adding the "new" symbols, fix the just contributed symbol)?

I may need to create some footprints too, should I wait with the new pull request for the set of symbols until all footprints are ready?

Thanks again for the friendly guidance :)

@diegoherranz
Copy link
Collaborator

would now create a new branch and simply repeat the process (Adding the "new" symbols, fix the just contributed symbol)?

Yes, make sure you create the branches from an up to date master branch.
You could go for only one branch/PR or two separate ones (name fix + new symbols). Whatever you prefer.

I may need to create some footprints too, should I wait with the new pull request for the set of symbols until all footprints are ready?

Do you think that not all the footprints are available (honest question)?
If you need to create footprints, I would recommend using scripted ones. See https://github.com/pointhi/kicad-footprint-generator

If that's the case (footprints needed), the process of merging the PR can take longer (sometimes you wait until both symbol and footprints are ready), so in that case I would go for a separate PR for the symbol fix, to get that trhought quickly.

Thanks again!

@ghost
Copy link
Author

ghost commented Mar 10, 2019

All right, I would then prefer to do the symbol fix as a separate PR first, just to get a quick iteration and more used to git, finding efficient ways of working (hope that is OK?). Should be ready again next weekend :)

Do you think that not all the footprints are available (honest question)?

I was a bit unsure about the UQFN, but as it seems, it looks very much like a normal QFN. Will have a closer look when the time comes. I definitely need to have a look at the footprint generator too! It seems to be a long desired blessing.

Thank you!

@diegoherranz
Copy link
Collaborator

I would then prefer to do the symbol fix as a separate PR first[...] (hope that is OK?).

Perfect with me.

@ghost
Copy link
Author

ghost commented Mar 11, 2019

Follow up pull request regarding the part number: #1624

@antoniovazquezblanco antoniovazquezblanco modified the milestones: 6.0.0, 5.1.1 Apr 12, 2019
DaToBSn pushed a commit to DaToBSn/kicad-symbols that referenced this pull request Jul 2, 2019
* 'master' of github.com:KiCad/kicad-symbols: (228 commits)
  Added alias for MS5607 pressure sensor
  Fixed pin stacking
  Fix pin names
  Changes as requested
  Shrink pin length and number for small gnd crystals
  Save Device lib
  Update LT3010 with proper naming (KiCad#1458)
  Move CH376 from Memory_Controller to to Interface (KiCad#1144) (KiCad#1176)
  Add SSOP to fp-filter
  Add PIC18(L)F25/26K83 in DIP-28 (KiCad#1120)
  Address review comments for PocketBeagle
  - uploaded dcm for fixing description
  Fixed footprint filter
  - fixed footprint name in filter and description
  Added 2N7002K (KiCad#1615)
  Add PocketBeagle
  Implemented changes
  Implemented changes
  - added description to aliases
  - fixed naming - changed package from LQFP-64 to TQFP-64_10x10mm_P0.5mm - changed pin name offset to 20mil - changed pin length to 1000mil - removed stacking of positive power pin stacking - changed name of pin 21 from VDDIO to VDDIOB - fixed RESET pin name - moved reset pin to bottom left corner - VDDCORE -> Power Input - VSW -> Power Input (moved to the top of the symbol)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Addition Adds new symbols to library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants