Skip to content
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

Add ESP-C3-M1-I-Kit board #6938

Merged
merged 16 commits into from
Dec 14, 2022
Merged

Add ESP-C3-M1-I-Kit board #6938

merged 16 commits into from
Dec 14, 2022

Conversation

wmjohnanderson
Copy link
Contributor

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!

@CLAassistant
Copy link

CLAassistant commented Jul 4, 2022

CLA assistant check
All committers have signed the CLA.

@ajsb85
Copy link

ajsb85 commented Jul 4, 2022

Hi, @wmjohnanderson
Thank you for the contribution.
Could you please rename the folder to create a proper slug name.
Example: from ESP-C3-M1-I-Kit is esp_c3_m1_i_kit
Thank you in advance.

@atanisoft
Copy link
Collaborator

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.

static const uint8_t SS = 23;
static const uint8_t MOSI = 21;
static const uint8_t MISO = 22;
static const uint8_t SCK = 28;
Copy link
Collaborator

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.

Copy link
Collaborator

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;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

static const uint8_t A2 = 4;
static const uint8_t A3 = 5;
static const uint8_t A4 = 6;
static const uint8_t A5 = 13;
Copy link
Collaborator

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.

Copy link
Collaborator

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;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

#define digitalPinHasPWM(p) (p < EXTERNAL_NUM_INTERRUPTS)

static const uint8_t LED_BUILTIN = 18;
static const uint8_t LED_BUILTIN = 19;
Copy link
Collaborator

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?

Copy link
Collaborator

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@SuGlider
Copy link
Collaborator

@wmjohnanderson @ajsb85

Please verify the suggestions and review from @atanisoft in order to move forward.
Thanks!

@VojtechBartoska
Copy link
Collaborator

@wmjohnanderson Any updates so we can move forward with this PR?

@SuGlider SuGlider self-assigned this Aug 8, 2022
@SuGlider SuGlider added Status: Pending Resolution: Awaiting response Waiting for response of author labels Aug 8, 2022
@VojtechBartoska
Copy link
Collaborator

@wmjohnanderson Can you help process suggested changes?

static const uint8_t LED_BLUE = 5;

static const uint8_t TX = 18;
static const uint8_t RX = 19;
Copy link
Collaborator

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;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

static const uint8_t RX = 19;

static const uint8_t SDA = 4;
static const uint8_t SCL = 5;
Copy link
Collaborator

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;

Copy link
Collaborator

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.

@SuGlider
Copy link
Collaborator

SuGlider commented Sep 6, 2022

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.

@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.

@SuGlider SuGlider self-requested a review September 6, 2022 01:42
Copy link
Collaborator

@SuGlider SuGlider left a 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.

@P-R-O-C-H-Y
Copy link
Member

@wmjohnanderson Hi, is this PR still valid as there is no reaction from you for a long time.

Copy link
Collaborator

@SuGlider SuGlider left a 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.

#define digitalPinHasPWM(p) (p < EXTERNAL_NUM_INTERRUPTS)

static const uint8_t LED_BUILTIN = 18;
static const uint8_t LED_BUILTIN = 19;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

static const uint8_t LED_BLUE = 5;

static const uint8_t TX = 18;
static const uint8_t RX = 19;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

static const uint8_t RX = 19;

static const uint8_t SDA = 4;
static const uint8_t SCL = 5;
Copy link
Collaborator

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.

static const uint8_t SS = 23;
static const uint8_t MOSI = 21;
static const uint8_t MISO = 22;
static const uint8_t SCK = 28;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

static const uint8_t A2 = 4;
static const uint8_t A3 = 5;
static const uint8_t A4 = 6;
static const uint8_t A5 = 13;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@SuGlider
Copy link
Collaborator

SuGlider commented Dec 7, 2022

@wmjohnanderson Any updates so we can move forward with this PR?

@VojtechBartoska @P-R-O-C-H-Y - I have searched this board specification and fixed all the pinout definition.
Please review it. I think we can go ahead and get it merge it

It still misses modifications to board.txt file.

@SuGlider SuGlider added this to the 2.0.6 milestone Dec 7, 2022
@SuGlider
Copy link
Collaborator

SuGlider commented Dec 7, 2022

@P-R-O-C-H-Y - Please double check it and if it is ready, please merge it. Thanks!

@P-R-O-C-H-Y P-R-O-C-H-Y self-assigned this Dec 8, 2022
@SuGlider
Copy link
Collaborator

SuGlider commented Dec 8, 2022

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.

This is done now.

@me-no-dev
Copy link
Member

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

@me-no-dev
Copy link
Member

Also this module does not differ in any functional way from the default ESP32-C3 Dev Module

@me-no-dev
Copy link
Member

@ajsb85 @wmjohnanderson have you tested USB connected to IOs 18 and 19? Does it work OK with the LEDs that you have added?

@SuGlider
Copy link
Collaborator

SuGlider commented Dec 13, 2022

@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.
Maybe it can just disable menu option for CDC.

@me-no-dev
Copy link
Member

@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

@SuGlider
Copy link
Collaborator

@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.

@me-no-dev me-no-dev merged commit 972c3bb into espressif:master Dec 14, 2022
@ajsb85
Copy link

ajsb85 commented Dec 15, 2022

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.

Human pushed a commit to Human/arduino-esp32 that referenced this pull request Dec 17, 2022
* 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 <rodrigo.garcia@espressif.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

8 participants