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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

farkasgzoltan
Copy link

@farkasgzoltan farkasgzoltan commented Jan 14, 2019

Replace this line with your commit message! Please provide a description of your pull request


Thanks for creating a pull request to contribute to the KiCad libraries! To speed up integration of your PR, please check the following items:

@CLAassistant
Copy link

CLAassistant commented Jan 14, 2019

CLA assistant check
All committers have signed the CLA.

@myfreescalewebpage myfreescalewebpage added Pending reviewer A pull request waiting for a reviewer Addition Adds new symbols to library Re-push This is a new attempt of an abandoned PR labels Jan 14, 2019
@myfreescalewebpage
Copy link
Collaborator

@farkasgzoltan you have made several additional commits, please indicate if ready to be reviewed.

Thanks.
Joel

@farkasgzoltan
Copy link
Author

@myfreescalewebpage yes ready. sorry my first kicad library ;)

@myfreescalewebpage myfreescalewebpage self-assigned this Jan 17, 2019
@myfreescalewebpage myfreescalewebpage removed the Pending reviewer A pull request waiting for a reviewer label Jan 17, 2019
@myfreescalewebpage
Copy link
Collaborator

myfreescalewebpage commented Jan 17, 2019

Hi @farkasgzoltan , thanks for contributing,

A few comments I have during my review:

NodeMCU ESP-12E:

  • PDF datasheet is the must, or at least an HTML documentation online, we can link to the schematic of the module itself
  • The device is atomic, missing footprint and only one footprint filter is expected
  • What is the WEMOS ? is it really an alias of this module (=the same module with the same pinout) ?
  • For consistency with the ESP-12E symbol, keywords 802.11 Wi-Fi module is great
  • The description can be modified to ESP-12E 802.11 b/g/n Wi-Fi Module
  • Pin length should be 100mil only (symbol < 100 pins)
  • Reference should be U?, not U??
  • Name of pin 1 is ADC_EX/A0, name of pin 4 is GPIO10/SD_D3/D12, name of pin 5 is GPIO9/SD_D2/D11 etc etc (logic to be applied for the naming with first/second/third function). It's good to reverse for pin on the right so you get the first function near to the pin itself.
  • Name of pin 13 should be ~RST to get the over-bar indicating it is active low
  • Not Connected pin 3 must be placed on the outline border of the symbol
  • You should group the pins by functionality, not according to the position of the pin on the module. For example, GND pins must be stacked at the bottom of the symbol, +3V3 pins must be moved at the top of the symbol, you should also group UART pins etc etc

NodeMCU ESP-12S:
Please apply the same comments to this device too.

You should check the travis result to help you.

Then will need a complete review.

Cheers,
Joel

@myfreescalewebpage myfreescalewebpage added the Pending footprint Pending footprint acceptance before merging label Jan 17, 2019
@farkasgzoltan
Copy link
Author

farkasgzoltan commented Jan 17, 2019

@myfreescalewebpage

  • PDF datasheet is the must, or at least an HTML documentation online, we can link to the schematic of the module itself

    pdf link datasheet it was in it.

  • The device is atomic, missing footprint and only one footprint filter is expected

  • What is the WEMOS ? is it really an alias of this module (=the same module with the same pinout) ?

  • For consistency with the ESP-12E symbol, keywords 802.11 Wi-Fi module is great

  • The description can be modified to ESP-12E 802.11 b/g/n Wi-Fi Module

  • Pin length should be 100mil only (symbol < 100 pins)

  • Reference should be U?, not U??

  • Name of pin 1 is ADC_EX/A0, name of pin 4 is GPIO10/SD_D3/D12, name of pin 5 is GPIO9/SD_D2/D11 etc etc (logic to be applied for the naming with first/second/third function). It's good to reverse for pin on the right so you get the first function near to the pin itself.

  • Name of pin 13 should be ~RST to get the over-bar indicating it is active low

  • Not Connected pin 3 must be placed on the outline border of the symbol

  • You should group the pins by functionality, not according to the position of the pin on the module. For example, GND pins must be stacked at the bottom of the symbol, +3V3 pins must be moved at the top of the symbol, you should also group UART pins etc etc

@myfreescalewebpage
Copy link
Collaborator

Thanks for the modifications. You commit messages can be a little bit more descritive, like "fix description", "fix pinout of the device", "add something", that's better for the reviewer than just a list of fix1 fix2 fix3 etc

Also, can you indicate when the review can restart if you think you have corrected everything.

Thanks,
Joel

@farkasgzoltan
Copy link
Author

@myfreescalewebpage please verify

@myfreescalewebpage
Copy link
Collaborator

Thanks @farkasgzoltan for your modification, it's better. Starting a new review from here.

NodeMCU_ESP-12E:

  • Need to find better for the documentation link, this one is not acceptable.
  • Footprint filter should be NodeMCU*ESP*12E*
  • GND pin must be stacked (first pin Power Input visible, other pins Passive not visible -> http://kicad-pcb.org/libraries/klc/S4.3/)
  • Name of pin 4 is GPIO10/SD_D3/D12, name of pin 5 is GPIO9/SD_D2/D11, name of pin 6 is SPI_INT/SD_D1 etc. I just simply read pin name on the page 5 of the provided documentation, but maybe I'm wrong, if you want to have something else, we need a full documentation. It should exists.
  • Name of pin 15 is VDD5V, name of the 3.3V pins is VDD3V3
  • Pins 11/16/25 are Power Output

NodeMCU_ESP-32S:

  • Footprint filter should be NodeMCU*ESP*32S* (travis error)
  • Description should be ESP-WROOM-32 802.11 b/g/n Wi-Fi+BT+BLE Module
  • Keywords should be 802.11 Wi-Fi bluetooth module
  • GND pin must be stacked (first pin Power Input visible, other pins Passive not visible -> http://kicad-pcb.org/libraries/klc/S4.3/)
  • Name of pin 3 is GPIO36/ADC0, name of pin 4 is GPIO39/ADC3, name of pin 5 is GPIO34/ADC6 etc. I just simply read pin name on the page 2 of the provided documentation, but maybe I'm wrong, if you want to have something else, we need another documentation.
  • Name of pin 1 is 3.3V and type must be Power Output
  • Name of pin 2 is EN/~RESET

Cheers,
Joel

@farkasgzoltan
Copy link
Author

farkasgzoltan commented Jan 18, 2019

NodeMCU_ESP-12E:

  • Need to find better for the documentation link, this one is not acceptable.

  • Footprint filter should be NodeMCUESP12E*

  • GND pin must be stacked (first pin Power Input visible, other pins Passive not visible -> http://kicad-pcb.org/libraries/klc/S4.3/)

  • Name of pin 4 is GPIO10/SD_D3/D12, name of pin 5 is GPIO9/SD_D2/D11, name of pin 6 is SPI_INT/SD_D1 etc. I just simply read pin name on the page 5 of the provided documentation, but maybe I'm wrong, if you want to have something else, we need a full documentation. It should exists.

  • not used D12, D11 because flash memory uses only gpio9 pin can be used as input but also like sensor for reading... i couldn't find a perfect documentation...

  • SD1 I think it's clearer flash memory uses FLASH_SD1 etc. ?

  • Name of pin 15 is VDD5V, name of the 3.3V pins is VDD3V3
    VDD5V is not entirely true because VIN max 12V lm1117 VIN used arduino boards

  • Pins 11/16/25 are Power Output

NodeMCU_ESP-32S:

  • Footprint filter should be NodeMCUESP32S* (travis error)
  • Description should be ESP-WROOM-32 802.11 b/g/n Wi-Fi+BT+BLE Module
  • Keywords should be 802.11 Wi-Fi bluetooth module
  • GND pin must be stacked (first pin Power Input visible, other pins Passive not visible -> http://kicad-pcb.org/libraries/klc/S4.3/)
  • Name of pin 3 is GPIO36/ADC0, name of pin 4 is GPIO39/ADC3, name of pin 5 is GPIO34/ADC6 etc. I just simply read pin name on the page 2 of the provided documentation, but maybe I'm wrong, if you want to have something else, we need another documentation.
  • Name of pin 1 is 3.3V and type must be Power Output
  • Name of pin 2 is EN/~RESET

@myfreescalewebpage
Copy link
Collaborator

myfreescalewebpage commented Jan 18, 2019

not used D12, D11 because flash memory uses only gpio9 pin can be used as input but also like sensor for reading... i couldn't find a perfect documentation...

SD1 I think it's clearer flash memory uses FLASH_SD1 etc. ?

Regarding to the pin naming the logic is very simple to not lost people using the module in the future. Take the documentation (schematic of the module at the moment) and look at 1st/2nd/3rd functions of the pins, and put name according to this. Example below with GPIO10:

capture

There is nothing to invent with FLASH or something else, symbol should just refer to the documentation.

Joel

Edit: can you also check why you have a modification of MCU_Microchip_SAMD library, this is not expected here. Thanks

@myfreescalewebpage
Copy link
Collaborator

Hi @farkasgzoltan are you still alive on this subject ? Thanks.

@farkasgzoltan
Copy link
Author

Yes, sorry too busy... I will have a little time weekend...

@myfreescalewebpage
Copy link
Collaborator

No worries, do it when you have some times :) That what just to get news from you !

@antoniovazquezblanco antoniovazquezblanco added the Pending changes User is expected to perform fixes before merging label Jun 27, 2019
@myfreescalewebpage
Copy link
Collaborator

@farkasgzoltan ping :)

@myfreescalewebpage myfreescalewebpage added Abandoned Original author has stopped working on the PR and removed Pending changes User is expected to perform fixes before merging labels Aug 24, 2019
@HamzaHajeir
Copy link

Any updates to ESP8266 and NodeMCU files ?

@myfreescalewebpage
Copy link
Collaborator

@HamzaHajeir this PR is no more updated by the original author, but you can contribute to if you want. I requested some changes in a previous comment.

@myfreescalewebpage
Copy link
Collaborator

@HamzaHajeir there is also #2073

@HamzaHajeir
Copy link

@HamzaHajeir this PR is no more updated by the original author, but you can contribute to if you want. I requested some changes in a previous comment.

I appreciate your management and quality assurance role in keeping every library perfect.

Hope author would make appropriate changes.

Thanks

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 Pending footprint Pending footprint acceptance before merging Re-push This is a new attempt of an abandoned PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants