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

ublox Sara U2xx Symbol #1158

Merged
merged 9 commits into from
Dec 30, 2018
Merged

ublox Sara U2xx Symbol #1158

merged 9 commits into from
Dec 30, 2018

Conversation

jneiva08
Copy link
Contributor

Added symbol of ublox Sara U2xxx

Datasheet: https://www.u-blox.com/sites/default/files/SARA-U2_DataSheet_(UBX-13005287).pdf
Integration Manual: https://www.u-blox.com/sites/default/files/SARA-G3-U2_SysIntegrManual_%28UBX-13000995%29.pdf

sara

Footprint: #1099


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

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

poeschlr commented Dec 4, 2018

I am not sure that power out is the correct electrical type for sim_det

Otherwise this looks good

@jneiva08
Copy link
Contributor Author

jneiva08 commented Dec 5, 2018

you are right, in the datasheet says input. thanks for catch

@poeschlr poeschlr added the Pending footprint Pending footprint acceptance before merging label Dec 5, 2018
@antoniovazquezblanco antoniovazquezblanco modified the milestones: Backlog, 5.1.0 Dec 5, 2018
@myfreescalewebpage
Copy link
Collaborator

myfreescalewebpage commented Dec 12, 2018

Hi @jneiva08, thanks for contributing,

A few comments additionally to @poeschlr :

  • Indicate the datasheet in datasheet field, not the integration manual
  • According to the datasheet, RESET should be named RESET_N
  • According to the datasheet ANT is Bidirectional, not Passive
  • You are violating KLC S6.3 (travis output)
  • Do you plan to submit footprint "RF_Module:ublox_SARA-G3_LGA-96" ?

Cheers,
Joel

@poeschlr
Copy link
Collaborator

@myfreescalewebpage the footprint is found under KiCad/kicad-footprints#1099. And yes this link was missing from this pull request.

@antoniovazquezblanco
Copy link
Collaborator

I am sorry another merge caused a conflict in this PR due to KiCad updating the file version.

@myfreescalewebpage about:

According to the datasheet, RESET should be named RESET_N

I think that the _N is to indicate that the logic of the pin is inverted and we can do that with the upper line.

@antoniovazquezblanco
Copy link
Collaborator

I resolved the conflict for you @jneiva08

Please make a pull before commiting any other changes!

@myfreescalewebpage
Copy link
Collaborator

Hi @antoniovazquezblanco

I think that the _N is to indicate that the logic of the pin is inverted and we can do that with the upper line.

I agree. In the past someone ask me to add _N on a symbol to be consistent with the datasheet. I think he was right because when many pins have a similar name, this is a source of troubles. I think we should keep symbol closed to the datasheet.

@myfreescalewebpage
Copy link
Collaborator

myfreescalewebpage commented Dec 13, 2018

@antoniovazquezblanco and @jneiva08 just read KLC S4.7 (http://kicad-pcb.org/libraries/klc/S4.7/) and removed my previous comment about _N. Also make a second review of all the other PR I have commented these days, still some modifications to do to conform with this KLC.

@jneiva08
Copy link
Contributor Author

jneiva08 commented Dec 13, 2018

@antoniovazquezblanco Thanks for resolved the conflict.
Did find already the problem why didn't open and have already fixed.

Add missing enddraw and enddef
Added missing "$endcmp"
Replace Integration Manual to datasheet
@poeschlr poeschlr removed the Pending footprint Pending footprint acceptance before merging label Dec 13, 2018
@antoniovazquezblanco
Copy link
Collaborator

Did find already the problem why didn't open and have already fixed.

Sorry, I resolved the conflict through Github interface and missed those two lines! 🤦‍♂️

@antoniovazquezblanco
Copy link
Collaborator

Is there anything missing in this PR?

@myfreescalewebpage
Copy link
Collaborator

myfreescalewebpage commented Dec 19, 2018

@antoniovazquezblanco there is question about the ANT pin, also on other PR (#748). Librarians haven't status actually.

@jneiva08 I also just seen you should cross reference and name to be consistent with other devices : reference in top left corner, name in top right corner.

Else, sounds good.

@poeschlr
Copy link
Collaborator

Looks good now.

@poeschlr poeschlr merged commit 063c31c into KiCad:master Dec 30, 2018
@jneiva08 jneiva08 deleted the SARA-U201 branch December 30, 2018 15:30
@jneiva08
Copy link
Contributor Author

thanks for all your time

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.

4 participants