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

MCU Module: added NodeMCU modules ESP-12E and ESP-32S #1426 #2073

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nerdyscout
Copy link
Contributor

@nerdyscout nerdyscout commented Aug 14, 2019

This pull request is a follow up of #1426 as it seems to be abandon.
Several modifications mentioned by @myfreescalewebpage have been done.
For the better understanding I did change some names.

MCU symbol footprint
ESP32 ESP32-DevKitC-V4 image #1301
ESP32 Adafruit HUZZAH32 - ESP32 Feather image #1657
ESP8266 NodeMCU_DEVKIT_V1.0 image #1301
ESP8266 Adafruit Feather HUZZAH ESP8266 image #1657

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.

@chschlue
Copy link
Contributor

I couldn't find anything in KLC and the existing MCU_* symbols are inconsistent, but:
Is it sensible to include every alternate pin function?
Personally I find it convoluted and it makes for freaking huge symbols.

@nerdyscout nerdyscout marked this pull request as ready for review August 18, 2019 05:45
@ladyada
Copy link

ladyada commented Aug 18, 2019

yay!

@myfreescalewebpage
Copy link
Collaborator

@nerdyscout thanks for this contribution.
I'm reviewing #1426 and the author seems to have stop working on it.
However, I do not understand the behavior of your PR: is it some symbol of the evaluation board (to create some shields) or is it the symbol of the ESP32 itself (what the title of the PR suggests) ?
In the second case, symbol of ESP32 devices, the name of the symbol should be the name of the ESP32 device, not the name of one particular evaluation board on which it is.
Please detail.
Also, @chschlue is right, we try to only indicate the mains functions of the pins, there aretoo much on the symbols you have designed.
Joel

@myfreescalewebpage myfreescalewebpage added the Addition Adds new symbols to library label Aug 24, 2019
@myfreescalewebpage myfreescalewebpage self-assigned this Aug 24, 2019
@nerdyscout
Copy link
Contributor Author

thank you for your quick feedback.
My pull request is about the development boards, therefore the symbol names should be right.
As far as I understand #1426 is also about development boards as it says "Node_MCU", might be different development boards, but for sure not the ESP32 itself. The pull request for the ESP32 is abandon #722 and the MCU ESP8266 is already in the library MCU_Espressif available.

Having all this information at the pins I was just trying to follow the comment. Anyway... I am able to rework all symbols completely, but might take a while as my holiday just started ;)
Reworking the symbols I guess it is best to make them look alike to the already available ESP32-WROOM*, right? The downside of this design, and having just the main functions declared, is a lot of pins logical belonging together will be ripped apart.
Should I keep my development boards in the generic MCU_Module or move them to RF_Module?

@myfreescalewebpage
Copy link
Collaborator

Thanks for your answer, this clarify. Let say first need a review, will do soon. Then will see if many modifications are required, but maybe not. You can keep in MCU_Module, it's fine.

@nerdyscout
Copy link
Contributor Author

is there anything I can do, assist somehow... to speed up this review?

@poeschlr
Copy link
Collaborator

@chschlue we normally do not include alternative functions in pin names. (yes some legacy symbols still do it that way. And yes there is not really a KLC rule as of yet. I opened an issue KiCad/kicad-website#436)

@myfreescalewebpage
Copy link
Collaborator

@nerdyscout sorry for being long on this, was a little it busy on other subjects. I was making the review when @poeschlr made his comment, he is right, we only set the first function, except when an alternative function is important, for example a debug port or JTAG connection for example, because it remains nice to have.

Can you:

  • Remove alternate functions that are too much on the symbol according to the previous comment?
  • Decrease the width of symbols following the previous comment ?

I was making a first review of "Adafruit_Feather_HUZZAH_ESP8266":

  • Name of pin 3 is "VBUS"
  • Pins 16 to 23 +26 are missing (NC pins should be added, invisible, on the border outline of the symbol)
  • Name of pin 28 is ~RESET

Please check also the other symbol and if the comments apply, fix them too, this will speed up the review.

Joel

@nerdyscout
Copy link
Contributor Author

nerdyscout commented Sep 22, 2019

RF Modul Symbol Footprint PR
ESP-WROOM-32 ESP32-DevKitC-V4 image none?
ESP-WROOM-32 Adafruit_HUZZAH32_ESP32_Feather image 1657
ESP-12 NodeMCU_DEVKIT_V1.0 image abandon 1301 reopened 1255 reopened 1254
ESP-12 Adafruit_Feather_HUZZAH_ESP8266 image 1657

notes

  • The symbols of ESP32-DevKitC-V4, Adafruit_HUZZAH32_ESP32_Feather and ESP32-WROOM-32 (which is used by those two modules) are drop in compatible to each other.
  • The symbols of NodeMCU_DEVKIT_V1.0, Adafruit_Feather_HUZZAH_ESP8266 and ESP-12 (which is used by those two modules) are drop in compatible to each other.

issues

  • NodeMCU_DEVKIT_V1.0
    • does still have an issue with ADC pin, I have to fix this.
    • stack of power pins: spread the pins? ignore it?
  • Adafruit_Feather_HUZZAH_ESP8266
    • travis dislikes the NC stack pin. what should I do? delete those completely, spread them, ignore it?

@chschlue chschlue added the Pending footprint Pending footprint acceptance before merging label Sep 22, 2019
@chschlue
Copy link
Contributor

Re CI errors:
One power pin per stack must be visible, preferably the lowest numbered. The others must be invisible and passive.
NC pins must not be connected together, meaning yes, spread them out.

@myfreescalewebpage
Copy link
Collaborator

Hello @nerdyscout some travis error can still be fixed. It's better starting with that. Also, due to the nature of this symbols, it's better to start working on the footprint. I think a pending question is at KiCad/kicad-footprints#1301 Joel

@nerdyscout
Copy link
Contributor Author

nerdyscout commented Nov 11, 2019

hi @myfreescalewebpage, i think the travis errors should be fine now.

As this pull request contains several symbols also several footprint pull request apply.
PR #1301 by @farkasgzoltan seems to be abandon to me, this is why I reopened #1255 and #1254.
I agree it is better to work on those, but it seems there is no documentation available for the outstanding review, therefore I currently see no way to go on...
At the same time I see no reason why the PR #1657 by @matthijskooijman has not been merged yet, this would allow me to insert atleast two of my symbols.

@CLAassistant
Copy link

CLAassistant commented Jul 28, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ farkasgzoltan
✅ nerdyscout
❌ Stefan Herold


Stefan Herold seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@matthijskooijman
Copy link
Contributor

Just noticed this PR. It intersects a bit with my #1933, which adds various feather symbols. Originally it had just the M0 Basic proto, but after some discussion about pin layout, it was extended with some additional ones and I'm about to add a few more feathers.

In that PR, we are considering using a common layout for all feather boards, to make them easy to drop in. The ESP-based feathers in this PR do not follow the same layout, so would not end up being compatible. However, it seems the versions in this PR are compatible with other ESP-based boards, which might also have some value. I wonder which should be preferred, or maybe there should just be two symbols for the feather (one compatible with other feather boards, one with other ESP boards)?

In any case, I'm planning to make a version of these ESP feathers for my PR, even if just to explore how compatible such a symbol could actually be.

One additional note for this PR: In the feather footprint PR, I recently renumbered the pins, so I think these symbols must be updated to the new numbering.

@nerdyscout
Copy link
Contributor Author

nerdyscout commented Sep 27, 2020

Hi @matthijskooijman, you got a lot nice looking symbols there!
I guess for all MCU modules we should watch/work on #3059 as this seems like the way to go for both our PR I think.
Currently I am working on a symbol generator for NAND flash symbols, maybe we can find a similar solution for all the MCUs

@myfreescalewebpage
Copy link
Collaborator

@nerdyscout the PR #1933 has been merged, Adafruit Feather boards added. I suggest you should rebase on top of master and get the inspiration on the Adafruit Feather boards forth remaining ones, impressive work has been done by @matthijskooijman

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Addition Adds new symbols to library Pending footprint Pending footprint acceptance before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants