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

Reverb Belton #1372

Merged
merged 15 commits into from Sep 26, 2020
Merged

Reverb Belton #1372

merged 15 commits into from Sep 26, 2020

Conversation

McColson
Copy link
Contributor

@McColson McColson commented Feb 4, 2019

Add Digital Reverb Belton BTDR-1H and BTDR-1V footprints

http://www.belton.co.kr/inc/downfile.php?seq=17&file=pdf

image

image

Symbol request

KiCad/kicad-symbols#1486


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 footprint(s) you are contributing
  • An example screenshot image is very helpful
  • If there are matching symbol or 3D model pull requests, 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.

@CLAassistant
Copy link

CLAassistant commented Feb 4, 2019

CLA assistant check
All committers have signed the CLA.

McColson referenced this pull request in KiCad/kicad-symbols Feb 4, 2019
@DanSGiesbrecht DanSGiesbrecht added Addition Adds new footprint to library Pending reviewer A pull request waiting for a reviewer labels Feb 4, 2019
Update keywords
@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

Thanks for your submission.

This footprint has been added to a new library called Reverb_Belton.pretty. I think that that is too specific and we should go for something a bit more generic like Module_Audio.pretty.
Can you change it to that if the rest of @KiCad/librarians don't have anything to object?
Because it's a new library, it has to be added to fp-lib-table. Can you add an entry there?

I'll continue reviewing in the meantime.

Thanks!

@McColson
Copy link
Contributor Author

Hello,
Thanks for your review, I understand for the name of the library. I will try to change that tomorrow and wait the validation by the librarians.
For the fp-lib-table, I add the new library and I need to do a PR for this file ?

Thks

@diegoherranz
Copy link
Collaborator

For the fp-lib-table, I add the new library and I need to do a PR for this file ?

Add that change to this same PR.

Thanks!

@McColson
Copy link
Contributor Author

Hello,
In the footprint, I've the link for the 3D model
${KISYS3DMOD}/Reverb_Belton.3dshapes/Reverb_BTDR-1H.wrl
If I anticipate, I need to rename the 3dshapes library too, for Module_Audio.3dshapes or something like that ?

@McColson
Copy link
Contributor Author

Hello,
To match with the symbol library (if it stay like that), may we create just an Audio.prettty library in place of Module_Audio.pretty ?

@diegoherranz
Copy link
Collaborator

In the footprint, I've the link for the 3D model
${KISYS3DMOD}/Reverb_Belton.3dshapes/Reverb_BTDR-1H.wrl
If I anticipate, I need to rename the 3dshapes library too, for Module_Audio.3dshapes or something like that ?

That is correct. It needs to be changed to whichever name is agreed.

To match with the symbol library (if it stay like that), may we create just an Audio.prettty library in place of Module_Audio.pretty ?

@evanshultz, @poeschlr: what do you think about this one?
On the symbols side, we've got audio-related stuff on at least: Audio, Amplifier_Audio, Connector, Sensor_Audio.
On the footprints side we've got at least: Connector_Audio, Sensor_Audio.

Sensor_Audio maps clearly between symbols and footprints.
The audio connector symbols in Connector maybe could move to a new Connector_Audio symbol library at some point in the future (e.g. v6)?
Some symbols in Audio maybe could be moved into Amplifier_Audio?
Having said all that, for this topic at hand, do we go for a generic Audio.prettty?

For the rest of stuff I can open an issue for v6 if you think so.

Thanks!

@diegoherranz diegoherranz added the Librarian Subject need attention of an experienced librarian label Feb 19, 2019
@diegoherranz
Copy link
Collaborator

The audio connector symbols in Connector maybe could move to a new Connector_Audio symbol library at some point in the future (e.g. v6)?

Regarding this, I've just noticed that it's the same for USB connectors: symbol on Connector, footprint in Connector_USB. So maybe that is deliberate? Or something to change too?

@diegoherranz
Copy link
Collaborator

@evanshultz, @poeschlr: any preference on what library name to use for this one? Thanks.

@poeschlr
Copy link
Collaborator

poeschlr commented Mar 3, 2019

What is the symbol library called? Might be best to go with a similar name as this device is highly specialized. (Both symbol and footprint are specialized to this exact device -> atomic pairing)

@diegoherranz
Copy link
Collaborator

The symbol, on KiCad/kicad-symbols#1486, is simply using Audio.lib.

@McColson
Copy link
Contributor Author

McColson commented Apr 5, 2019

Hello,
Have we found a library for this part finally ?
If the last problem it's just the name of the library maybe we can find a solution to merge this PR shortly, isn't it ?

@diegoherranz
Copy link
Collaborator

Personally, I would go for Audio_Module for both the symbol and the footprint.

On the symbol side, we've got Audio which feels too broad and many of the symbols there could be in more specific libraries like Amplifier_Audio and this device feels a bit different to most of the devices there which are mostly chips/ICs rather than a module. However, I wouldn't be opposed to simply have it in Audio.

On the footprint side, Reverb_Belton feels too specific, and I think that Audio_Module may be a good compromise.

Could you make those changes?

Thanks!

@chmorgan chmorgan self-requested a review March 16, 2020 00:56
@chmorgan chmorgan self-assigned this Mar 16, 2020
@chmorgan
Copy link
Collaborator

Hi @McColson, are you available to make the renames for the module name @diegoherranz requested? I saw the 3D modules PR that is linked to this and wanted to see if you were interested in pursuing getting them merged in after review. Apologizes for the delay, we have lots of requests and not so many people reviewing changes.

@McColson
Copy link
Contributor Author

McColson commented Mar 16, 2020

Hello,
I'm agree for the changes, but i've not enough time for that now. I completly busy (newborn at home).
If you can make the changes, i authorize you to pursuit my work, if needed.
Sorry
Matthieu colson

@chmorgan chmorgan removed their assignment Mar 21, 2020
@chmorgan chmorgan removed their request for review March 21, 2020 17:43
Add new Audio_Module library for Belton Reverb Module
Remove old naming
Change the 3d model path
Add Audio_Module.pretty to receive the Belton Reverb Module
@McColson
Copy link
Contributor Author

McColson commented May 2, 2020

Hello, I've tried to finished the job for this PR.

  1. Change the lib name to Audio_Module
  2. Remove all the Reverb_Belton lib
  3. Change the path of the footprint in the symbol description to match the lib name.
  4. Idem for the 3D package and 3D source
  5. Add the Audio_Module lib in fp-lib-table

Hope that all this mods can complete the PR.

Thanks.

@diegoherranz
Copy link
Collaborator

Thanks, @McColson, I'll review as soon as possible.

@diegoherranz
Copy link
Collaborator

diegoherranz commented May 16, 2020

I've reviewed these footprints. A few comments and questions:

  • Typo on fplib. {$KISYMOD} should be ${KISYSMOD}
  • I'm slightly confused with the X coordinates for the pins. I would think that the horizontal and vertical versions should have the same X coordinates for the pins but the drawing says differently? Example (43.2 vs 20+23). Do you have any more info on that?

Reverb_BTDR-1H

  • F.Fab should include the rounded parts around the screws
  • F.CrtYd vertical lines should be at (they are very close to that, but just to be strict)
    -9.38-4.5-.25=-14.13 and 64.62+4.5+.25 = 69.37 (same as BTDR-1V)
  • F.CrtYd could be something like this. There's lots of area there which can be used for something else.
    courtyard

Reverb_BTDR-1V

  • Why is this distance 1.3mm? I can only find a 0.9mm dimension on the datasheet.
    distance

Thanks!

@McColson
Copy link
Contributor Author

Hello,
Thanks, i will change that asap

@McColson
Copy link
Contributor Author

McColson commented May 31, 2020

Hello,

  • I've made the change on fplib.

  • For the X coordinates, I've use this datasheet :
    http://www.uk-electronic.de/PDF/BTDR-1.pdf
    The X coordinates are the same for the horizontal and vertical module.
    If we think the pins are in a grid of 2.54mm, yes the X coordinates should be 2.54 *17 = 43.18mm.
    But it's impossible for me to mesure it, the pins are a little bit flexible.
    So I follow the datasheet and use 20+23 of space.

BTDR1-H

  • F.Fab Layer is corrected
  • F.CrtYd is corrected too

BTDR1-V

  • The 0.9mm in datasheet is for the space between the body and the edge of the pins, not the center ! The pin mesure 0.8mm of diameter, so I add 0.8mm/2 = 0.4mm + 0.9mm = 1.3mm for the center of the pin, and the pad.

I hope, that will be good this time, sorry for the time you spent on this PR.

@diegoherranz
Copy link
Collaborator

diegoherranz commented Aug 26, 2020

Thanks for all the changes. There will be a lockdown and move of the library repos to gitlab, so I'd like to get this PR completed before that happens (see the README on https://github.com/KiCad/kicad-symbols for more info).

The 0.9mm in datasheet is for the space between the body and the edge of the pins, not the center ! The pin mesure 0.8mm of diameter, so I add 0.8mm/2 = 0.4mm + 0.9mm = 1.3mm for the center of the pin, and the pad.

That sounds OK

For the X coordinates, I've use this datasheet :
http://www.uk-electronic.de/PDF/BTDR-1.pdf
The X coordinates are the same for the horizontal and vertical module.
If we think the pins are in a grid of 2.54mm, yes the X coordinates should be 2.54 *17 = 43.18mm.
But it's impossible for me to mesure it, the pins are a little bit flexible.
So I follow the datasheet and use 20+23 of space.

That drawing makes more sense (both vertical and horizontal having the same X coordinates.
We normally only add one footprint link per footprint, and we prefer that to be an "official" link, but in this case I think it can be useful to have both.

  • Could you add it to the description too? Also add "Digital Reverberation Unit" at the beginning? For example: "Digital Reverberation Unit, http://www.belton.co.kr/inc/downfile.php?seq=17&file=pdf (footprint from http://www.uk-electronic.de/PDF/BTDR-1.pdf)".

  • Travis complains about the Courtyard not being a closed path. And this vertical line is not completely vertical (I think both small mistakes are related).
    fab

  • Something I didn't notice before, the "REF**" in the silkscreen should be outside of the body.

I think that would be it. Thanks again!

@McColson
Copy link
Contributor Author

Hello, i try to do this asap. Thx

@myfreescalewebpage
Copy link
Collaborator

@McColson ping

@McColson
Copy link
Contributor Author

Sorry no more time for the moment to work on it 😥

@myfreescalewebpage
Copy link
Collaborator

No problem, thanks for the answer. I indicate that for the moment it is Abandoned but you or anyone else can comme back to this PR later.

@myfreescalewebpage myfreescalewebpage added the Abandoned Original author has stopped working on the PR label Sep 24, 2020
@diegoherranz
Copy link
Collaborator

There was so little pending fixing that I've just done a quick couple of commits since I didn't want the work on this pull request to get lost. Merging.
Thanks!

@diegoherranz
Copy link
Collaborator

I missed one of my own review comments:

Something I didn't notice before, the "REF**" in the silkscreen should be outside of the body.

So I've created #2493 to fix it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Abandoned Original author has stopped working on the PR Addition Adds new footprint to library Librarian Subject need attention of an experienced librarian
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants