-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Add ESP-C3-M1-I-Kit board #6938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hi, @wmjohnanderson |
This creates a pins variant but doesn't define any board to use it. Please update boards.txt to have a reference to this new board. |
esp_c3_m1_i_kit/pins_arduino.h
Outdated
static const uint8_t SS = 23; | ||
static const uint8_t MOSI = 21; | ||
static const uint8_t MISO = 22; | ||
static const uint8_t SCK = 28; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the above four pins are not recommended for use or do not exist on the ESP32-C3. These should be remapped to valid pins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wmjohnanderson - suggestion:
static const uint8_t SS = 5;
static const uint8_t MOSI = 10;
static const uint8_t MISO = 6;
static const uint8_t SCK = 7;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
esp_c3_m1_i_kit/pins_arduino.h
Outdated
static const uint8_t A2 = 4; | ||
static const uint8_t A3 = 5; | ||
static const uint8_t A4 = 6; | ||
static const uint8_t A5 = 13; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GPIO 0 - 5 are the only ADC pins, GPIO 13 does not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wmjohnanderson - suggestion:
static const uint8_t A0 = 0;
static const uint8_t A1 = 1;
static const uint8_t A2 = 2;
static const uint8_t A3 = 3;
static const uint8_t A4 = 4;
static const uint8_t A5 = 5;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
esp_c3_m1_i_kit/pins_arduino.h
Outdated
#define digitalPinHasPWM(p) (p < EXTERNAL_NUM_INTERRUPTS) | ||
|
||
static const uint8_t LED_BUILTIN = 18; | ||
static const uint8_t LED_BUILTIN = 19; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate pin define... which is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wmjohnanderson - suggestion:
static const uint8_t LED_C = 18;
static const uint8_t LED_W = 19;
static const uint8_t LED_BUILTIN = 19;
#define BUILTIN_LED LED_BUILTIN // backward compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Please verify the suggestions and review from @atanisoft in order to move forward. |
@wmjohnanderson Any updates so we can move forward with this PR? |
@wmjohnanderson Can you help process suggested changes? |
esp_c3_m1_i_kit/pins_arduino.h
Outdated
static const uint8_t LED_BLUE = 5; | ||
|
||
static const uint8_t TX = 18; | ||
static const uint8_t RX = 19; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the correct pins used in this boards. GPIO18 and 19 are LEDs and USB JTAG/CDC.
static const uint8_t TX = 21;
static const uint8_t RX = 20;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
esp_c3_m1_i_kit/pins_arduino.h
Outdated
static const uint8_t RX = 19; | ||
|
||
static const uint8_t SDA = 4; | ||
static const uint8_t SCL = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion based on the boards layout would be:
static const uint8_t SDA = 1;
static const uint8_t SCL = 2;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with standard ESP32-C3 definition.
@wmjohnanderson - As @atanisoft already said, we also need that you make proper changes to boards.txt in order to add this boards to the list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wmjohnanderson - Please check all the pending requested changes.
@wmjohnanderson Hi, is this PR still valid as there is no reaction from you for a long time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed all pending issue in order to move forward and get it merged.
esp_c3_m1_i_kit/pins_arduino.h
Outdated
#define digitalPinHasPWM(p) (p < EXTERNAL_NUM_INTERRUPTS) | ||
|
||
static const uint8_t LED_BUILTIN = 18; | ||
static const uint8_t LED_BUILTIN = 19; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
esp_c3_m1_i_kit/pins_arduino.h
Outdated
static const uint8_t LED_BLUE = 5; | ||
|
||
static const uint8_t TX = 18; | ||
static const uint8_t RX = 19; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
esp_c3_m1_i_kit/pins_arduino.h
Outdated
static const uint8_t RX = 19; | ||
|
||
static const uint8_t SDA = 4; | ||
static const uint8_t SCL = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with standard ESP32-C3 definition.
esp_c3_m1_i_kit/pins_arduino.h
Outdated
static const uint8_t SS = 23; | ||
static const uint8_t MOSI = 21; | ||
static const uint8_t MISO = 22; | ||
static const uint8_t SCK = 28; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
esp_c3_m1_i_kit/pins_arduino.h
Outdated
static const uint8_t A2 = 4; | ||
static const uint8_t A3 = 5; | ||
static const uint8_t A4 = 6; | ||
static const uint8_t A5 = 13; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@VojtechBartoska @P-R-O-C-H-Y - I have searched this board specification and fixed all the pinout definition. It still misses modifications to |
@P-R-O-C-H-Y - Please double check it and if it is ready, please merge it. Thanks! |
Fixes board.txt file to add the new ESP32_C3_M1_I_KIT variant
This is done now. |
Specs say that the module comes with 4MB Flash, so I suggest (strongly) that the unnecessary options are removed (flash size, flash mode, flash freq and partitions that would not fit) 4MB 80Mhz QIO should be default and only option |
Also this module does not differ in any functional way from the default ESP32-C3 Dev Module |
@ajsb85 @wmjohnanderson have you tested USB connected to IOs 18 and 19? Does it work OK with the LEDs that you have added? |
The board has a CH340. I think it doesn't mind about USB pins. |
@SuGlider you still have the options for Flash Frequency and Flash Mode, which I find redundant as well. 4MB 80Mhz QIO is all that is needed |
I think it is fine now. Let me know. |
Hi, @me-no-dev I will take a look all the features and review the PR in deep with that board. Thank you very much for the merged into the repo. |
* feat: add ESP-C3-M1-I-Kit board to variants * docs: rename file to slug format * Fixes GPIO definitions * Adds ESP32 C3 M1 I Kit to the board list Fixes board.txt file to add the new ESP32_C3_M1_I_KIT variant * fixes extra board separator * Keeps only 4MB flash option * Fix it to Flash 80Mhz QIO only * Undo a change by mistake Co-authored-by: Rodrigo Garcia <[email protected]>
Summary
PR for ESP-C3-M1-I-Kit board in variants
I am current summer intern learning as much as I can and using this as practice. This is my first time doing this so if there is anything wrong, or any issues let me know and I can work on it. Thanks!