Skip to content
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

RJ45: Add a new Amphenol RJHSE5380-08 footprint #1300

Merged
merged 8 commits into from Jan 20, 2019

Conversation

Projects
None yet
5 participants
@manio
Copy link
Contributor

manio commented Jan 14, 2019

Small parts based on Martin Kröll work (RJHSE5380)

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Jan 14, 2019

CLA assistant check
All committers have signed the CLA.

@manio

This comment has been minimized.

Copy link
Contributor Author

manio commented Jan 14, 2019

Hi, is the 3D model a strict requirement for every footprint?

@myfreescalewebpage

This comment has been minimized.

Copy link
Contributor

myfreescalewebpage commented Jan 14, 2019

Hi @manio

Hi, is the 3D model a strict requirement for every footprint?

No it isn't, however, please indicate path in the footprint (name of 3D = name of footprint, look at other devices) so it is possible to create it later without to come back to the definition of the footprint.

Cheers,
Joel

RJ45: Add a new Amphenol RJHSE5380-08 footprint
Small parts based on Martin Kröll work (RJHSE5380)

@manio manio force-pushed the manio:master branch from 5bdedec to 9571d07 Jan 14, 2019

@manio

This comment has been minimized.

Copy link
Contributor Author

manio commented Jan 14, 2019

@myfreescalewebpage
Thank you Joel, I've updated this PR as requested

@myfreescalewebpage

This comment has been minimized.

Copy link
Contributor

myfreescalewebpage commented Jan 14, 2019

Thanks.

@poeschlr poeschlr self-assigned this Jan 19, 2019

@poeschlr

This comment has been minimized.

Copy link
Contributor

poeschlr commented Jan 19, 2019

For reference the screenshot of the part:

screenshot from 2019-01-19 09-17-47

The datasheet (dimensioned drawing) https://www.amphenolcanada.com/ProductSearch/drawings/AC/RJHSE538X08.pdf

@manio please provide these two in the future.


Review

The courtyard must take all pads into account. (All shield pads in your case)

You have the same pin numbers added multiple times. This will mean you can not connect the connectors independently but only in parallel. I doubt this would be on purpose.

Any dimension must agree to all significant digits given in the datasheet. (No rounding, No tolerance) If a datasheet gives metric and imperial dimensions then the metric ones are preferred.
rj svg

@manio manio force-pushed the manio:master branch 5 times, most recently from fab6494 to cbdd558 Jan 19, 2019

@manio

This comment has been minimized.

Copy link
Contributor Author

manio commented Jan 19, 2019

@poeschlr
Thank you very much for your time reviewing the footprint :)
I think I've fixed all issues you've found.
Please only keep in mind the following:

  1. You've told me to provide the screenshot while when creating a PR the info is saying "An example screenshot image is very helpful". I just assumed it is not a strict requirement. Thank you for providing the screenshot yourself (maybe it will be better to change this info for PR creators?).

  2. Regarding the main pin adjustment (marked 2.04 on your schema): I was just using the Amphenol RJHSE5380 as a base for my footprint. So the dimensions are cloned from there. But you're absolutely right that it has to be 2.03 as in the drawing, so I've fixed this. We only need to note that eventually the RJHSE5380 also has to be fixed the same way.

  3. Regarding the pink "15.74" mark - the problem here is IMHO it was done correctly. I was placing these two bottom shield pads relative to center big "hole" - as in the drawing. But there is another problem regarding this: "[...] the metric ones are preferred". As you can see in the drawing it's main dimensions are inches. But if I took the milimeters in the drawing for this placement you can see that 7.87+7.87 is not equal 15.75. Rounding the inches to mm was not with enough precision, so to fix the error you marked I've just converted the inches myself and I've put those two pads in with better metric precision.

  4. The last thing is the most bottom dimension you've marked (from outer left-right shield pads). On the drawing it is 126.49 and this is exactly as in my footprint, while you have it 126.76. I just believe it is correct in the footprint.

  5. I forgot one more thing: don't you think it would be better to provide multiple rectangular pads in this case. In fact it is a module which consist of 8 single 8P8C, so every single socket could have a rectangular pad to mark which is first. Of course the pin numbering is fixed, but there was a problem with those rectangular pads on each socket:
    "Violating F7.3
    Pad 1 should be denoted by rectangular pad
    Only pad 1 should be rectangular"
    Maybe I am wrong - but it is only my though :)

I hope it is now OK for you - so please review again. Thank you in advance! :)
regards!

@poeschlr

This comment has been minimized.

Copy link
Contributor

poeschlr commented Jan 19, 2019

Please to not use force pushes! I now can not tell what you changed and need to review the whole thing again. (Well will take a while as i am really not motivated for that right now.)

@manio manio force-pushed the manio:master branch from cbdd558 to cd58568 Jan 20, 2019

@manio

This comment has been minimized.

Copy link
Contributor Author

manio commented Jan 20, 2019

@poeschlr
Oh sorry - i just squashed it because I thought that it would be eventually quicker for you to merge it upstream as a single commit. I have the discrete changes in my local branch, so I now recovered it to you to single commits :)

@poeschlr

This comment has been minimized.

Copy link
Contributor

poeschlr commented Jan 20, 2019

We use the squash option to merge pull requests (if there are many commits by the contributor). Github offers an easy interface for that.

We also use that to clean up commit messages by contributors. (Most contributors don't really have any experience with git. Meaning they do not really know how to write proper messages.)

@poeschlr

This comment has been minimized.

Copy link
Contributor

poeschlr commented Jan 20, 2019

The courtyard clearance on the right side is not quite correct.
screenshot from 2019-01-20 18-23-12

@manio

This comment has been minimized.

Copy link
Contributor Author

manio commented Jan 20, 2019

Thanks, fixed now.

@poeschlr

This comment has been minimized.

Copy link
Contributor

poeschlr commented Jan 20, 2019

Thanks

@poeschlr poeschlr merged commit 5d22674 into KiCad:master Jan 20, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

DaToBSn pushed a commit to DaToBSn/kicad-footprints that referenced this pull request Jan 21, 2019

DaToBSn
Merge branch 'master' of github.com:KiCad/kicad-footprints
* 'master' of github.com:KiCad/kicad-footprints: (39 commits)
  Crystal: Add Abracon ABS25 4Pin 8.0x3.8mm (KiCad#1270)
  RJ45: Add a new Amphenol RJHSE5380-08 footprint (KiCad#1300)
  Add DFN-10 with EP 1.2x2 (KiCad#1290)
  Seiko ms621 f (KiCad#1218)
  Sip package link fixes (KiCad#1215)
  RF Module: Add CMWX1ZZABZ (KiCad#1296)
  Add L_Wuerth_WE-PD-Typ-7345. (KiCad#1299)
  Converter_DCDC XP_POWER IA, IH, ITX and ITQ series (KiCad#1058)
  More 3M Textool ZIF sockets, generated with script. (KiCad#495)
  Changed courtyard clearance to 0.25mm.
  Added Kyocera 6 pin tcxo.
  Add JST PHD series of connector footprints (KiCad#915)
  Rename Cherry MX1A to Cherry MX (KiCad#1180)
  RF_Module: Fix and improves HOPERF_RFM9XW_SMD footprint (KiCad#1213)
  Add vertical USB Type-C DX07S024WJ1 (KiCad#1251)
  Add PAT1220 RF attenuator. (KiCad#1288)
  Add RJ45_BEL_SS74301-00x (KiCad#1036)
  Add Texas R-PUQFN-N10 (KiCad#1310)
  Add LairdTech_28C0236-0JW-10 Ferrite bead (KiCad#1040)
  Fix courtyard and silkscreen
  ...
@manio

This comment has been minimized.

Copy link
Contributor Author

manio commented Mar 4, 2019

@poeschlr
I only want to confirm that using this footprint the element fits just perfectly fine! (as well as single-port RJHSE5380). PCB was manufactured at jlcpcb. Thank you!
image

@myfreescalewebpage

This comment has been minimized.

Copy link
Contributor

myfreescalewebpage commented Mar 4, 2019

@manio thanks for the photo ! That's nice to have a return using the footprint :)

@antoniovazquezblanco antoniovazquezblanco added this to the 5.1.0 milestone Mar 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.