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

Isolator: add TI ISOW784x series #1791

Closed
wants to merge 1 commit into from
Closed

Conversation

karlp
Copy link
Contributor

@karlp karlp commented Apr 27, 2019

Based on existing ADuM54xx and ADuM64xx series digital isolators with
built in DC/DC regulators.

Signed-off-by: Karl Palsson [email protected]

Datasheet http://www.ti.com/lit/gpn/isow7842
Screenshot from 2x2.
Screenshot from 2019-04-27 12-15-55

@myfreescalewebpage myfreescalewebpage added Addition Adds new symbols to library Pending reviewer A pull request waiting for a reviewer labels Apr 29, 2019
@myfreescalewebpage myfreescalewebpage self-assigned this May 2, 2019
@myfreescalewebpage myfreescalewebpage removed the Pending reviewer A pull request waiting for a reviewer label May 2, 2019
@myfreescalewebpage
Copy link
Collaborator

myfreescalewebpage commented May 2, 2019

@karlp thanks for this contribution,

I have not yet performed a full review, but from the screenshot:

  • Isolation barrier is now represented with a single dash line in the convention, you can have a look to ISO1540 device for example
  • GND pins should be stacked on each side

Cheers,
Joel

@karlp
Copy link
Contributor Author

karlp commented May 2, 2019

Sure. I can go and fix this. I've filed a ticket for the bad existing parts that mislead contributors.

@karlp
Copy link
Contributor Author

karlp commented May 2, 2019

Do you want a ticket that the KLC doesn't document this required style of marking isolation? This is the second time I've filed a PR for an isolated device, that copied an existing isolated device, and have been told it needs to be different. It's not mentioned on http://kicad-pcb.org/libraries/klc/ at all, where is that document controlled?

@karlp karlp force-pushed the pulls/iso-isow784x branch from 7d60dd5 to 1257f51 Compare May 2, 2019 22:42
@karlp
Copy link
Contributor Author

karlp commented May 2, 2019

New screenshot. If this style is ok, I'll update the ADuM64xx PR as well.
Screenshot from 2019-05-02 22-42-20

@karlp karlp force-pushed the pulls/iso-isow784x branch from 1257f51 to c7cb63d Compare May 3, 2019 01:02
@karlp
Copy link
Contributor Author

karlp commented May 3, 2019

KLC ticket on updating the isolation is KiCad/kicad-website#284 since may 2018

@myfreescalewebpage
Copy link
Collaborator

myfreescalewebpage commented May 3, 2019

Thanks, making a full review from this point.

  • Names should be "ISOW7841DWE", "ISOW7842DWE", "ISOW7843DWE", "ISOW7844DWE" to avoid conflict if the manufacturer add other package option in the future
  • GND pin stacking are not correct. The rule is first pin Power Input, visible, other pin Passive, invisible. See also travis log.
  • Name of pins 9 and 15 is GND2
  • Name of pin 1 is VCC
  • Name of pin 10 is SEL
  • Can you rename inputs/outputs according to names defined in the datasheet page 5 (INA, OUTA, INB, OUTB etc).
  • Can you have positive and negative power pins always at the same place, and VCC aligned with GND1, VISO aligned with GND2

Also, maybe you can submit ISOW7840 too ?

Thanks,
Joel

@karlp
Copy link
Contributor Author

karlp commented May 3, 2019

  1. No, the names shouldn't be. That excludes the F parts, active high/low default outputs, and if they add packages, the footprint filter should be updated, not the symbol. You're still not including the full orderable name there anyway.

  2. Re-reading http://kicad-pcb.org/libraries/klc/S4.3/ I can see that now, but your travis check is giving a green tick on this anyway. (I'm not reading things that are passing)

3.4.5,6 If we're going to blindly copy datasheets with no regard to consistency, then sure, but then it should have the double lines for isolation too... :) At the very least I strongly feel that the datasheet is unhelpful with VCCISO + GND2, VCCISO+GNDISO is far more self consistent. The other names will be extended causing even more problems for item 7.

  1. was a problem with the 7844 because of the fixed placement of the part name. I can't see a way around this without making a whole new shape making it inconsistent with all the others. Is this a must or a nice to have? It's only in the 7844 part that it didn't have the space for it.

@evanshultz
Copy link
Collaborator

Pardon me for popping in. Just want to help clarify.

First, you are correct in various spots that KLC could use a number of updates. In some cases the issue is open and in other cases the issue is roughly defined but the text hasn't been transferred to KLC. That falls on all the librarians.

  1. We don't include the full orderable MPN anyway, since we cut off packaging info and other non-functional info. That's in KLC.

Capturing the "F" suffix does make sense, and it looks to me like this could be an ALIAS. Right? So the symbols Joel mentioned above are good, and the "F" suffix versions can be added as an ALIAS.

We have been gravitating towards unique symbols for each package, so if there are other packages available in the future we would add a new symbol and not a new footprint filter for this symbol.

3-6. We do want to copy pin names because that's easiest to match up with the datasheet for users. "Triangle symbol" (opamp, comparator) power pins are the only places where we intentionally deviate, because the symbol size is small, at least that I can recall right now. These pins are understood. Elsewhere, like here, please do use the datasheet pin names. We want high confidence in the symbol.

Using a common isolation style is reasonable, and somewhat driven by the graphic tools available in KiCad. You have chosen to conflate pin names with isolation graphics when they need not be.

@myfreescalewebpage
Copy link
Collaborator

@evanshultz thanks to answer, @karlp thanks to update your contribution.

@karlp karlp force-pushed the pulls/iso-isow784x branch from c7cb63d to 6e8939b Compare June 30, 2019 23:00
@karlp
Copy link
Contributor Author

karlp commented Jun 30, 2019

Updated. Included the 7840, included the *F variants, lined up all power pins, renamed all pins to match datasheet rather than any (un)"common" interpretation.

@karlp
Copy link
Contributor Author

karlp commented Jun 30, 2019

New picture of representative sample:
Screenshot from 2019-06-30 23-02-26

@myfreescalewebpage
Copy link
Collaborator

myfreescalewebpage commented Jul 2, 2019

Thanks, making a full review from this point.

  • Names should be "ISOW7840DWE" with alias "ISOW7840FDWE", "ISOW7841DWE" with alias "ISOW7841FDWE", etc to avoid conflict if the manufacturer add other package option in the future
  • GND pin stacking are not correct. The rule is first pin Power Input, visible, other pin Passive, invisible. See also travis log.

Thanks,
Joel

@karlp
Copy link
Contributor Author

karlp commented Jul 2, 2019

I can't work out why the visibility isn't rpesenting properly. The editor clearly shows pin 9 as visible and pin 15 as invisible. I've tried moving and replacing them with pin 15 first, and pin 9 dragged "on top" and also vice versa. The properties on the pins are correct at least. I'm using rolling nightlies, is this possibly just a bug in the symbol viewer/editor? The travis logs have passed, so I'm not looking at them, and even now, they just seem to be warnings that 8/15 should be visible, unless they're the second pins, which, they are.

As for the part names, no. I'm not going to name them with DWE suffices. It's not the name TI uses, it's not the way any of the other isolators are named, and given that the ISOW784x series is explicitly for the 5kv isolation parts, the chance of there ever being another footprint than soic wide is infinitesimally small.

IF they add another package, the symbol can be updated. Things aren't set in stone, the git history is full of changes. But it's a wildly big if.

@myfreescalewebpage
Copy link
Collaborator

I can't work out why the visibility isn't rpesenting properly. The editor clearly shows pin 9 as visible and pin 15 as invisible

The invisible pin must be Passive, this is the error.

As for the part names, no. I'm not going to name them with DWE suffices. It's not the name TI uses

See datasheet of TI page 45. This is it. I have reviewed many TI devices and we do like that so when several packages are available, we can distinguish the parts. It's not because other devices are not properly define in the lib that you should not do it.

IF they add another package, the symbol can be updated.

No, it's always a problem later to modify the symbols because it breaks users schematics. That's not the way we are working.

Joel

@evanshultz
Copy link
Collaborator

@karlp
I agree with your assessment that other packages are probably unlikely.

However, this library is moving towards "fully specified symbols". You can see the PR to update KLC at KiCad/kicad-website#407. While that update hasn't been merged yet, the delay is mostly over minor wording details and initiative as this practice is already being done in practice.

KLC also already includes the guideline to not use the complete, orderable MPN. That is seen at http://kicad-pcb.org/libraries/klc/S2.2/.

Putting those together explains why the existing symbols you see don't fit into this paradigm. Updating them is disruptive to users right now so the symbol library doesn't change much between minor releases of KiCad. I can understand why this inconsistency is questionable and frustrating, but to try out new concept we've been doing "rolling changes", such as this, where new contributions will use the current paradigm while older symbols/footprints/etc. aren't touched. Perfect? No.

Lastly, I'm unclear what you meant by "It's not the name TI uses", unless you meant the packaging (T&R, tube, etc.) wasn't included. http://www.ti.com/product/ISOW7842/samplebuy shows that "DWE" is included as a suffix for this part when buying it.

So all considered, Joel and I would like to see this merged, and it's quite close to meeting the guidelines for merging, but we would like you to add DWE to the end of each symbol name. Please.

If you think either of us have misunderstood your points please let us know, but I feel quite confident we get what you're saying and we're reply with the style we want to see to merge this PR.

@karlp karlp force-pushed the pulls/iso-isow784x branch from 6e8939b to bb1e4f6 Compare July 3, 2019 21:37
@karlp
Copy link
Contributor Author

karlp commented Jul 3, 2019

Updated the pin visibilities after another re-reading of the KLC. I know what point you're trying to make, I just simply disagree with it, and particularly with your interpretation of it as it applies to this part. The package is even listed in the feature set. I'm in that other "camp" you refer to in the documentation issue, and feel you're heading in very much the wrong direction with this approach to symbols, though I do understand the appeal of it. Mostly, the argument of "doing the change would hurt users" becomes rather invalidated by the pain users now have by having diverging sets of parts, with different naming styles. But that's a project direction discussion that is rather out of scope for this ticket.

Importantly to me, is that you're not actually applying these rules consistently even on new additions despite how important you claim this to be. Just #1868 from after this PR was submitted has no package sufixxing, and was merged by Joel. That part even has package variants, unlike these parts here. a724525 from last week, and these are just from the first page of recent commits.

@myfreescalewebpage
Copy link
Collaborator

@karlp thanks for the pin stacking update.

Regarding to the name of the devices, and because you take #1868 as an example, what appends now if we consider to add ADN4650 SOIC version to the library ? All devices should have a unique name, it's not possible to have two devices with the same name in the library. The package in the description is just to describe the device. So when creating a device, the name should be chosen according to the manufacturer order guides.

I have merged #1868 and thanks to indicate I have forgotten to check the device name at this moment (we are all humans and sometimes we forgot something). If you wish to make a new PR to fix that before we release a new version of KiCad, that's a pleasure. According to the datasheet page 24, the SSOP version should be called ADN4650BRS. If the SOIC version is added in the future, this will be the ADN4650BRW.

Joel

@myfreescalewebpage
Copy link
Collaborator

No news from the original author, indicate this PR is abandoned.

@myfreescalewebpage myfreescalewebpage added the Abandoned Original author has stopped working on the PR label Dec 10, 2019
@karlp
Copy link
Contributor Author

karlp commented Mar 17, 2020

Yep, not going to spend more effort on this.

@karlp karlp closed this Mar 17, 2020
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 symbols to library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants