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

BQ78350 #757

Merged
merged 5 commits into from
Sep 22, 2020
Merged

BQ78350 #757

merged 5 commits into from
Sep 22, 2020

Conversation

jneiva08
Copy link
Contributor

@jneiva08 jneiva08 commented Jul 15, 2018

Did the following changes:

  • Added alias for BQ78350-R1
  • Updated description
  • Fix KLC S3.6 and S4.1
  • Symbol is a little smaller

Datasheets BQ78350: http://www.ti.com/lit/ds/symlink/bq78350.pdf
Datasheets BQ78350-R1: http://www.ti.com/lit/ds/symlink/bq78350-r1.pdf

Symbol:
bq78


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

jneiva08 added 2 commits July 15, 2018 14:39
Updated description
Fix KLC S3.6
Fix KLC S4.1
Symbol is a little smaller
@DanSGiesbrecht DanSGiesbrecht self-assigned this Oct 1, 2018
@DanSGiesbrecht DanSGiesbrecht added In review Enhancement Improves existing symbol in the library labels Oct 2, 2018
@DanSGiesbrecht
Copy link
Collaborator

Hi @jneiva08

Changes that can be merged:

I do have one suggestion: Consider adding more keywords than just BMS (e.g. battery, management, Li-Ion, Gauge, controller).

Moving the RefDes, part name and package label looks good, as does changing the pin name offset from 40 to the recommended 20.


Changes that can't be merged right now:

Unfortunately, changing the symbol pin positions breaks existing design compatibility. This improvement - while valid - cannot be merged at this time. It will likely have to wait until the release of KiCad 5.1.


If you would like to have the acceptable improvements merged, you can revert the pin positions. Otherwise, this PR can wait until such time that it's deemed acceptable to merge. @poeschlr , what do you think?

@antoniovazquezblanco
Copy link
Collaborator

Offtopic: I use the 'In review' label to filter issues. The intended use is to tag the issue as long as it is assigned to someone. Given that you already started the review I would not remove the tag unless you abandon the review and mark it as pending review. Sorry for the interruption :)

@DanSGiesbrecht
Copy link
Collaborator

DanSGiesbrecht commented Oct 4, 2018

@antoniovazquezblanco thanks for the information! What's the difference between assigning and using the in review label?

My thought was that I would assign myself when I plan to review a PR, and add the label when I began reviewing it.

I removed the label to show that I was not currently reviewing it (my review of the current state of the PR was complete). Is there a better way to show that I'm done reviewing and awaiting further action?

@evanshultz
Copy link
Collaborator

A couple notes about pin names:

  1. VEN should be Open Collector.
  2. This appears to be an I2C master device so should SCL be Output type? I vote yes.

@antoniovazquezblanco antoniovazquezblanco added this to the 5.1.0 milestone Nov 19, 2018
@antoniovazquezblanco
Copy link
Collaborator

All the comments seem to have been addressed here.

I would like to ask @poeschlr to address if this would be acceptable enough to be merged in 5.1.0 or if should be postponed to 6.0.0 due to compatibility changes (pin positions have been changed due to symbol downsizing).

@poeschlr
Copy link
Collaborator

poeschlr commented Dec 4, 2018

I would prefer the symbols to stay as stable as possible (with regards to pin positions)

But if somebody can give a good reason as to why this change is necessary then we can include it.
A good first step would be to add a screenshot of the old symbol such that we can discuss the benefits of the new one more easily.

@jneiva08
Copy link
Contributor Author

jneiva08 commented Dec 5, 2018

This is the actual symbol:
bq78350_actual

This is the new symbol:
bq78350_pr
Also adds the following:

  • Added alias for BQ78350-R1
  • Updated description
  • Fix KLC S3.6 and S4.1

@DanSGiesbrecht
Copy link
Collaborator

Closed/reopened to re-initiate CLA-Assistant.

@DanSGiesbrecht
Copy link
Collaborator

I think this is a good improvement. Users of the libraries would notice the change, unlike something like a pin-swap, for example.

@poeschlr
Copy link
Collaborator

My decision stands. There are not enough benefits from this change to allow it in. So please revert the pin positions to what they where before. (or we need to leave this for after 5.1)

@DanSGiesbrecht
Copy link
Collaborator

Sounds good. I'll modify the milestone to 6.0.0 for now. @jneiva08 , if you'd like to break this into two PRs, feel free.

@DanSGiesbrecht DanSGiesbrecht modified the milestones: 5.1.0, 6.0.0 Dec 11, 2018
@antoniovazquezblanco antoniovazquezblanco mentioned this pull request Dec 11, 2018
5 tasks
@myfreescalewebpage
Copy link
Collaborator

@DanSGiesbrecht what is the status here ? I can continue the review if you want

@myfreescalewebpage myfreescalewebpage self-assigned this Sep 22, 2020
@myfreescalewebpage
Copy link
Collaborator

myfreescalewebpage commented Sep 22, 2020

@jneiva08 I try to continue the review:

  • Name should be BQ78350DBT and alias "BQ78350DBT-R1"
  • Descriptions of the symbol and the alias should be terminated by the package name, please add , TSSOP-30 at the end
  • Pins 11 and 13 should be Bidirectional

Else looks good
Joel

Add package name and size in description,
Change pin 11 and 13 to bidirectional
@jneiva08
Copy link
Contributor Author

@myfreescalewebpage, have made the changes requested

@myfreescalewebpage
Copy link
Collaborator

Thanks for this quick fix on a so old pull request !! Just want to merge it now :)

@myfreescalewebpage myfreescalewebpage merged commit ccea839 into KiCad:master Sep 22, 2020
@jneiva08 jneiva08 deleted the bq78350 branch September 22, 2020 12:19
@jneiva08
Copy link
Contributor Author

Thanks for your time

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Enhancement Improves existing symbol in the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants