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

Add ARJP11A-MASA-B-A-EMU2 RJ45 connector #2267

Merged
merged 8 commits into from
Nov 5, 2019

Conversation

ppielorz
Copy link
Contributor

Added ARJP11A-MASA-B-A-EMU2 symbol
Datasheet: https://abracon.com/Magnetics/lan/ARJP11A.PDF
Screenshot:
image

Footprint PR: KiCad/kicad-footprints#1940


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:

  • 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
    • A new fitting footprint must be submitted if the library does not yet contain one.
  • 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
  • Give a reason behind any intentional library convention rule violation.

@gmsotavio gmsotavio added Pending reviewer A pull request waiting for a reviewer Addition Adds new symbols to library labels Oct 26, 2019
@gmsotavio gmsotavio self-assigned this Oct 30, 2019
@gmsotavio gmsotavio removed the Pending reviewer A pull request waiting for a reviewer label Oct 30, 2019
@gmsotavio
Copy link
Collaborator

gmsotavio commented Nov 4, 2019

Hi @ppielorz,

Thanks for your contribution. One remark:

  • I think your symbol is placed in the wrong library. In my understanding, the correct library is Connector and not Transformer. It is a symbol for an RJ45 jack connector with a buit-in magnetic module.

@gmsotavio gmsotavio added the Pending changes User is expected to perform fixes before merging label Nov 4, 2019
Small changes in symbol look (LEDs, rectifiers).
Moved symbol from Transformer to Connector library.
Changed name co it contains connector type and manafacturer.
@ppielorz
Copy link
Contributor Author

ppielorz commented Nov 5, 2019

You are perfectly right about library, I don't know why I left it in Transformers...

Changed designator to J.
@gmsotavio gmsotavio removed the Pending changes User is expected to perform fixes before merging label Nov 5, 2019
@gmsotavio
Copy link
Collaborator

gmsotavio commented Nov 5, 2019

Closing and reopening to force travis-ci running again.

@gmsotavio gmsotavio closed this Nov 5, 2019
@gmsotavio gmsotavio reopened this Nov 5, 2019
@gmsotavio
Copy link
Collaborator

gmsotavio commented Nov 5, 2019

@ppielorz,

  • It seems the specified footprint is wrong. Replace Connector_RJ:RJ45_Abracon_ARJP11A-MA by Connector_RJ:RJ45_Abracon_ARJP11A-MA_Horizontal
  • In footprint filter, replace RJ45*Abracon*ARJP11A-MA* by RJ45*Abracon*ARJP11A?MA*

I merged the footprint. I need these changes to merge the symbol.

@gmsotavio gmsotavio added the Pending changes User is expected to perform fixes before merging label Nov 5, 2019
Changed footprint and footprint filter.
@ppielorz
Copy link
Contributor Author

ppielorz commented Nov 5, 2019

Done.

@gmsotavio gmsotavio merged commit 8d4dc24 into KiCad:master Nov 5, 2019
@gmsotavio
Copy link
Collaborator

Thanks

@gmsotavio gmsotavio removed the Pending changes User is expected to perform fixes before merging label Nov 5, 2019
@gmsotavio gmsotavio added this to the 5.1.5 milestone Nov 5, 2019
@ghost
Copy link

ghost commented Nov 6, 2019

Hi,

my OCD triggered slightly because the two halves of the transformer are not perfectly symmetrical.
So I took a closer look and found some more issues. They are all not really critical, its just cosmetic:

  • Symmetry of transformers
  • Duplicate lines
    grafik
  • Of the the center-tap connections is missing (to the lower bridge rectifier)
  • The LEDs should have arrows, otherwise they are just diodes
  • Marker-Dot for the transformers is missing
  • scaling of resistors/capactitors/diodes seems off. Here is a screenshot of the symbol with R and C symbols from the lib on top:
    grafik

It seems like travis does not catch the duplicate lines thing. I will investigate that.

@gmsotavio
Copy link
Collaborator

gmsotavio commented Nov 6, 2019

Hi @cp-aquila
Unfortunately, I don't usually be as rigorous with the revision of symbols as I usually do with the revision of footprints, highlighting the simplified internal diagram. For future PR, I will try to be more rigid. For this, I just checked the KLC but missed some elements of the internal drawing. What do you suppose I do here? Revert the PR and ask the author (@ppielorz) to make the modifications?

Additionally

  • I suppose that "left" and "right" LED words should be replaced by the color names of the LEDs.

@ghost
Copy link

ghost commented Nov 6, 2019

Hi @gmsotavio
No worries, my whole post is pretty much nitpicking. All those are cosmetic.
I would not revert the PR. Instead lets make a bugfix commit on top of that. The Pin-Positions and types do not need to change, so it won't be a breaking change.

@ppielorz Is that something that you want to do? If not, I will take care of it instead.

@ppielorz
Copy link
Contributor Author

ppielorz commented Nov 6, 2019

@cp-aquila I can do it. Should I make changes and commit to the same branch as before?

@ghost
Copy link

ghost commented Nov 6, 2019

I think its best if you make a new branch and a new PR. In that new PR, put a link to this one.

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.

2 participants