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

Valve rework #1649

Closed
wants to merge 1 commit into from
Closed

Valve rework #1649

wants to merge 1 commit into from

Conversation

myfreescalewebpage
Copy link
Collaborator

Rework of all valve devices:

  • Adding some missing properties
  • Typo fixing
  • Adding missing pins (some stacking pins and NC pins)
  • Fix some pins names/number

Also tried to fix majority of KLC errors, my result is the following:

Capture

PS : no information about the STABI device. It has actually no description, no datasheet, should be reworked too.


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.

@myfreescalewebpage myfreescalewebpage added Bug Fix symbol existing in the library Enhancement Improves existing symbol in the library Pending reviewer A pull request waiting for a reviewer labels Mar 18, 2019
@evanshultz evanshultz self-assigned this Mar 19, 2019
@evanshultz evanshultz removed the Pending reviewer A pull request waiting for a reviewer label Mar 19, 2019
@evanshultz
Copy link
Collaborator

  1. IEEE 315-1975 shows various symbols an symbol elements. Did you look at those? How did you come up with the graphic style used here?
  2. That same doc shows "tube" parts should have a ref des of V. And "valve" should be E. Not U.
  3. Some of the datasheets are pretty bad or missing, but these are relatively odd parts and may be hard to find. Do you want to take a look at that or merge as is?
  4. EABC80 is missing almost all pins and stuff inside the oval when opened in KiCad v5.1. For me, at least. But I see the pins in the symbol so something is busted. In what version did you make the symbol? ECC81 is also missing a lot of stuff for me.
  5. Some pins go to thin lines and others to thick lines. Compare the bottom pin and arc on EC92 and EL34. Maybe all should be 0mil?
  6. Perhaps STABI is for the symbol merged at Replace sensor/Geiger-Muller with sensor/Nuclear-Radiation_Detector #1550, and thus this one can be removed?

A lot needs to be reviewed when the 4th item above is sorted out.

@myfreescalewebpage
Copy link
Collaborator Author

Hello @evanshultz following are the responses to your questions:

1- I haven't, I have keep existing design of the lib, the goal was just to fix KLC errors as reported in #217
2- Same position, I have keep the existing, but I agree it's better and I can change this
3- I have tried to find better datasheets, have done my best at the moment
4- I have still 5.0.1 but it is correct for me. It has 4 units, do you see the 4 ?
5- Same position than question 1 and question 2, I have kept original design, but I agree it's better and I can change this. 0mil for everything ?
6- Agree, this is it, will do

Joel

@evanshultz
Copy link
Collaborator

  1. OK, let's change.
  2. Looks OK now. Your PR didn't change so it must have been me. Weird.
  3. Yeah, I guess 0mil. Somewhat similar to our transistor symbols in this respect.
  4. Let's do it.

Pins should not be duplicated on multiple units, such as pin 3 on CK6418. Please check this out and resolve it. Otherwise, if multiple nets are connected to the pin 3 on multiple units the nets will be shorted and that's bad. We found this on opamps with multiple units and resolved it, so here's another example.

Lastly, some clashing of elements:
image

@myfreescalewebpage
Copy link
Collaborator Author

@evanshultz I think I have addressed everythink of the previous comment, if you can have a look.

Except:

Pins should not be duplicated on multiple units, such as pin 3 on CK6418. Please check this out and resolve it. Otherwise, if multiple nets are connected to the pin 3 on multiple units the nets will be shorted and that's bad.

Well in this case it is wanted, the same pin on the device has two functions, the first on unit A, the second on unit B. Please let me known how this should be fixed because I have no ideas at the moment...

Joel

@evanshultz
Copy link
Collaborator

We can't have duplicate pins. I'm not sure either, but can take a closer look on Monday. If you have any ideas in the interim please share them.

@evanshultz
Copy link
Collaborator

Just came across clause 7.2.3 of IEEE315-1975 which says, as I understand, a tube with multiple internal elements (heater is an example given) shall have one single pin unless the actual device has separate pins. Just giving strength to not duplicating pins.

@myfreescalewebpage
Copy link
Collaborator Author

Agree with you @evanshultz but don't know how to fix this in our library.... Do you remember some examples you already treated in the past ?
Joel

@evanshultz
Copy link
Collaborator

For the one mentioned above, why not collapse it into a single symbol since there's room for the duplicated pin?
image

EBAC80 also has a duplicate pin 7 (use the ladybug toolbar icon to check for this). For this one, perhaps A and B units could be folded together and include pin 7, while units C and D could be folded together as well.

However, we're talking about total redesign at that point. Why not merge what's committed now, which is relatively minor, with more work closer to v6's release. Does that sound like the best way to go to you?

If so, I only want to decide on ref des being V or E and then we can merge.

@myfreescalewebpage
Copy link
Collaborator Author

However, we're talking about total redesign at that point. Why not merge what's committed now, which is relatively minor, with more work closer to v6's release. Does that sound like the best way to go to you?

Probably it's a best option. The purpose of the PR was to get the library enhanced following #217 , but maybe we have not all answers right now.

If so, I only want to decide on ref des being V or E and then we can merge.

Currently changed to E

@Misca1234
Copy link
Collaborator

Ahhh, my old love interest, the valve library,

I was waiting for my push
KiCad/kicad-footprints#1139
to be merged before taking on the symbols

While you are at it, you have new foot prints and 3D models here
KiCad/kicad-footprints#1139

Note that the foot print and 3D models is not oriented anymore toward the device new but instead the socket name.

Perhaps you should already now change the foot print name or wait for
KiCad/kicad-footprints#1139

@myfreescalewebpage
Copy link
Collaborator Author

@Misca1234 thanks to point this footprint pull request :) My opinion is that it is good to add the footprint and footprint filter right now too, I will check your new footprints and try to associate all symbols.
However, I'm not used to review footprints and have lot's of thing to learn about them, particularly when it is scripted. @evanshultz have you got also time to review the footprint pull request KiCad/kicad-footprints#1139 ?
Cheers,
Joel

@myfreescalewebpage
Copy link
Collaborator Author

@Misca1234 just looked at all the valve symbols, all have a footprint. Is your footprint pull request aimed to replace existing footprints ? Thanks.

@Misca1234
Copy link
Collaborator

Misca1234 commented Apr 8, 2019

Yes

My long term project was first to update the footprints and then do an overhaul of the symbols to match them to correct footprints, but you have also notice the extreme shape the valve stuff is in.

I suggest we do it in this order

  1. Merge the foot prints (and 3D models) first because the new foot print does not affect the old ones.
  2. Update the symbols and at the same time remove the old foot prints

@myfreescalewebpage
Copy link
Collaborator Author

@Misca1234 the proposition is ok for me :)
@evanshultz what do you prefer ? Merging valves like that now or waiting for the merge of footprints enhancements ?

@Misca1234
Copy link
Collaborator

@myfreescalewebpage

However, I'm not used to review footprints and have lot's of thing to learn about them, particularly when it is scripted

Just treat the footprints as "normal ones", the scripting stuff is already merged to
easyw and pointhi repositories and those repositories are not officially a part of kicad.

So, if you find stuff in the foot prints that needs to be corrected I will push new one foot prints as usually (and also push the script changes to easyw and pointhi repositories)

You do not need to check the scripts

@myfreescalewebpage
Copy link
Collaborator Author

Humm yeah I know but still not confident to review them at the moment. I still make many issues when I propose my own footprints :D

@Misca1234
Copy link
Collaborator

@myfreescalewebpage
Have you had any time to look into
KiCad/kicad-footprints#1139

@myfreescalewebpage
Copy link
Collaborator Author

@Misca1234 I'm not very confident when reviewing the footprints generally at the moment so I haven't checked them for now. @poeschlr indicated he wanted to review them, maybe I will participate but will not be the main actor on this.

@evanshultz evanshultz mentioned this pull request Jul 8, 2019
8 tasks
@myfreescalewebpage myfreescalewebpage added this to the 6.0.0 milestone Dec 10, 2019
@myfreescalewebpage myfreescalewebpage mentioned this pull request Jun 25, 2020
8 tasks
@myfreescalewebpage
Copy link
Collaborator Author

Will rebase here after #2789 and continue cleaning the Valves.
@evanshultz the proposition to move this library to obsolete folder is great for me, still valid proposition ?

@CLAassistant
Copy link

CLAassistant commented Sep 2, 2020

CLA assistant check
All committers have signed the CLA.

@myfreescalewebpage
Copy link
Collaborator Author

Closing/opening to refresh the CLA.

@myfreescalewebpage
Copy link
Collaborator Author

Merged the Valve libraries all together, according to me it's ready for review @evanshultz and @chschlue
Just one question to answer is moving to the obsolete directory. I'm not so sure because Valves have still high popularity for high quality audio and clocks! We can still buy new parts on internet. Let me know about your opinion.

@chschlue
Copy link
Contributor

chschlue commented Sep 2, 2020

I totally get that tubes are still a thing among enthusiasts (although whether valve amps and Nixie clocks are especially "high quality" is debatable, but I'm not going to get into that).
These can all stay in Valves or you can move the actually obsolete ones to obsolete if you like, but I'm not on board with sub-categories like modern valves, legacy valves and "just valves".

Apart from that, master is still broken and is not going to stay that way until this (rather large PR) is through review. Reverting #2789.

@myfreescalewebpage
Copy link
Collaborator Author

The previous commit was also solving the merge issue with the other PR, you make me working more ;)
Branch conflict fixed again

@myfreescalewebpage
Copy link
Collaborator Author

Apart from that, master is still broken and is not going to stay that way until this (rather large PR) is through review.

I agree with you anyway :)

@myfreescalewebpage
Copy link
Collaborator Author

@evanshultz or @chschlue if you have time to review the valves before end of the month ?

@chschlue
Copy link
Contributor

Probably not.
But you're basically overhauling the whole lib here, so you might as well just do its v6 conversion some time later.

@myfreescalewebpage
Copy link
Collaborator Author

Yes sure !

@evanshultz
Copy link
Collaborator

@myfreescalewebpage
According to #1649 (comment), where I had taken at least a quick look at all symbols, the items left were to prevent duplicate pins and settles the ref des prefix. Have those items been addressed?

@myfreescalewebpage
Copy link
Collaborator Author

@evanshultz I'm not sure this is all covered but I think there was no proper conclusion. Example below are the 4 units of 6AK8 symbol:

unitA
unitB
unitC
unitD

And the table of all pins:

table

Pin 7 is duplicated but it is expected in my point of view. It's a common pin of both diode and triode included in this part.

@evanshultz
Copy link
Collaborator

Did you check this in CvPcb? The part actually has 10 pins, not 9, so it won't match with a correct 9-pin footprint using the footprint pin count filter. For example, we pulled the power pins from opamps into a separate unit for exactly this reason. Can you only show the common pin on one of the units? Or perhaps things need to be re-arranged to use just a single unit?

@myfreescalewebpage
Copy link
Collaborator Author

As detailed above, the same pin (7) is used for two purposes but it's not easy at all to merge two units in only one unit. Moreover the same valve can be used on a schematic at totally opposed parts and 2 units is really better for this kind of symbol.

On the other side the issue was present before. The purpose of the pull request was to solve several KLC issues and rearrange consistency across the valves. I can add other enhancements of course but for this specific subject I have no idea how.

@myfreescalewebpage myfreescalewebpage closed this by deleting the head repository Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Fix symbol existing in the library Enhancement Improves existing symbol in the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants