Board files for LOLIN S3 MINI #8010

tkroo · 2023-05-21T01:31:07Z

My first time creating a new board definition, hopefully everything is ok.

ports/espressif/boards/lolin_s3_mini/board.c

ports/espressif/boards/lolin_s3_mini/mpconfigboard.mk

ports/espressif/boards/lolin_s3_mini/sdkconfig

#8010 (comment)

…olin-s3-mini

dhalbert · 2023-05-22T23:15:49Z

@tkroo Does the latest batch of changes boot? If so I can approve. @bill88t How is it for you?

bill88t

@dhalbert I do not have the board.
I just try to review stuff I am comfortable with.

The optimisation flag is what concerns me most with this definition.

If you want me to stop reviewing stuff, just tell me.

bill88t · 2023-05-22T23:41:46Z

ports/espressif/boards/lolin_s3_mini/sdkconfig

+#
+# SPI RAM config
+#
+# CONFIG_SPIRAM_MODE_QUAD=y


I should have been more specific, sorry.
It's CONFIG_SPIRAM_TYPE_AUTO that is unwanted.
You want CONFIG_SPIRAM_MODE_QUAD or CONFIG_SPIRAM_MODE_OCT enabled.

You can just try out if oct works if you are too lazy to check the schematic.
If oct doesn't boot, quad.

dhalbert · 2023-05-22T23:49:12Z

@bill88t No problem, keep reviewing. THe optimization change may be needed to make it fit. -O2 is the default for the xtensa boards; -Os is needed for the RISC-V boards. (See the Makefile). You probably know this already. I don't see any other boards setting optimization per board in ports/espressif, though.

bill88t · 2023-05-22T23:53:47Z

Isn't it the policy to just disable unused modules in favor of keeping optimisation as is?

It's a 4mb S3 board.
Looking at the adafruit_feather_esp32s3_tft definition, instead of disabling optimisation, CIRCUITPY_ESPCAMERA = 0 is set instead.

That should be added in the .mk.

dhalbert · 2023-05-22T23:59:45Z

Normally we would turn ESPCAMERA off. We don't necessarily preserve optimization in favor of turning of general-purposes functionality. O2 is a lot bigger than Os. That is certainly true on various SAMD boards, for instance. We use Os to have all the useful modules.

tkroo · 2023-05-23T01:25:29Z

@tkroo Does the latest batch of changes boot? If so I can approve. @bill88t How is it for you?

Yes, my board boots with latest changes. I also just did a build without the OPTIMIZATION_FLAG = -0s and it also boots. Should I remove that flag? I don't know what is preferred.

bill88t · 2023-05-23T01:28:23Z

If OPTIMIZATION_FLAG isn't needed, it should absolutely be removed.

tkroo · 2023-05-23T01:53:15Z

If OPTIMIZATION_FLAG isn't needed, it should absolutely be removed.

Thanks! Removed. I had thought the optimization flag was brought over from the existing Lolin S3 board files, but it was me who accidentally pasted it from somewhere else. Sorry about the confusion.

skieast · 2023-05-23T14:34:16Z

Although may not be relevant I would remove all the extraneous pin definitions for those not broken out on the board. I would include a definition for the onboard WS2812 which I believe is on IO47. I can't be more specific on my phone lol
Thanks

dhalbert · 2023-05-23T19:52:03Z

@ktroo I don't understand why there are now merge conflicts noted, since you are just adding files.

Do you see @skieast's suggestion of removing pins that are not accessible?

tkroo · 2023-05-23T20:13:25Z

@ktroo I don't understand why there are now merge conflicts noted, since you are just adding files.

Do you see @skieast's suggestion of removing pins that are not accessible?

I don't understand the merge conflicts either. Strange. But I am new to this whole process, so maybe I did something accidentally.
I did see @skieast's suggestions. On board neopixel is already defined:

circuitpython/ports/espressif/boards/lolin_s3_mini/pins.c

Lines 101 to 103 in 5c49978

    
           { MP_ROM_QSTR(MP_QSTR_IO47), MP_ROM_PTR(&pin_GPIO47) }, 
        
           { MP_ROM_QSTR(MP_QSTR_D47), MP_ROM_PTR(&pin_GPIO47) }, 
        
           { MP_ROM_QSTR(MP_QSTR_NEOPIXEL), MP_ROM_PTR(&pin_GPIO47) },

circuitpython/ports/espressif/boards/lolin_s3_mini/mpconfigboard.h

Line 32 in 5c49978

#define MICROPY_HW_NEOPIXEL (&pin_GPIO47)

re: extraneous pins. I checked pins.c and don't see any pins defined that are not also exposed on the board.

dhalbert · 2023-05-23T20:21:05Z

I don't understand the merge conflicts either. Strange. But I am new to this whole process, so maybe I did something accidentally.

I think the easiest way to fix this would be to make a new branch, copy the new files into it, and make a new PR. We can close this one assuming the new one works.

skieast · 2023-05-23T20:29:23Z

@ktroo I don't understand why there are now merge conflicts noted, since you are just adding files.
Do you see @skieast's suggestion of removing pins that are not accessible?

I don't understand the merge conflicts either. Strange. But I am new to this whole process, so maybe I did something accidentally. I did see @skieast's suggestions. On board neopixel is already defined:

circuitpython/ports/espressif/boards/lolin_s3_mini/pins.c

Lines 101 to 103 in 5c49978

{ MP_ROM_QSTR(MP_QSTR_IO47), MP_ROM_PTR(&pin_GPIO47) },

{ MP_ROM_QSTR(MP_QSTR_D47), MP_ROM_PTR(&pin_GPIO47) },

{ MP_ROM_QSTR(MP_QSTR_NEOPIXEL), MP_ROM_PTR(&pin_GPIO47) },

circuitpython/ports/espressif/boards/lolin_s3_mini/mpconfigboard.h

Line 32 in 5c49978

#define MICROPY_HW_NEOPIXEL (&pin_GPIO47)

re: extraneous pins. I checked pins.c and don't see any pins defined that are not also exposed on the board.

Quite true. I was looking at it on my phone. I can blame my old eyes, better than blaming myself. Thanks again.

tkroo · 2023-05-23T20:58:03Z

I don't understand the merge conflicts either. Strange. But I am new to this whole process, so maybe I did something accidentally.

I think the easiest way to fix this would be to make a new branch, copy the new files into it, and make a new PR. We can close this one assuming the new one works.

Ok. I will create a new branch and PR. Thanks for all your help!

tkroo · 2023-05-23T20:59:29Z

re: extraneous pins. I checked pins.c and don't see any pins defined that are not also exposed on the board.

Quite true. I was looking at it on my phone. I can blame my old eyes, better than blaming myself. Thanks again.
No worries! I am new to this and appreciate as many eyes on this as possible. Thank you!

dhalbert · 2023-05-23T21:07:33Z

@tkroo you don't need to delete your fork. Just deleting the branch would be ifne.

tkroo · 2023-05-23T21:16:33Z

@tkroo you don't need to delete your fork. Just deleting the branch would be ifne.

Oh well, I've already deleted it. I think what caused the merge conflict was that I had merged my branch into my fork's main. Anyway, I'll create a new fork, branch, and PR.
Sorry about the chaos

dhalbert · 2023-05-23T21:20:26Z

had merged my branch into my fork's main.

Yes, try to keep your main the same as upstream's main. Basically, never work in main. Always work in a branch.

tkroo · 2023-05-23T21:30:23Z

new PR: #8023

tkroo added 4 commits May 20, 2023 18:17

Board files for LOLIN S3 MINI

437d8d7

add missing new line at end of file

93d3ef6

remove extra spaces

cafe472

remove space characters

fbf598c

bill88t reviewed May 21, 2023

View reviewed changes

tkroo and others added 5 commits May 21, 2023 20:15

add debug no resets

56bec3a

#8010 (comment)

remove spriram type auto

dc0f1fe

Merge branch 'lolin-s3-mini' of github.com:tkroo/circuitpython into l…

88c5f0f

…olin-s3-mini

revert change in wrong file

03746a2

remove spiram type auto

6ee215e

tannewt requested a review from dhalbert May 22, 2023 20:07

tkroo added 2 commits May 22, 2023 13:59

update copyright

6eff467

update never reset to correct rx and tx pins

324b95a

bill88t reviewed May 22, 2023

View reviewed changes

tkroo added 2 commits May 22, 2023 18:38

specify spiram mode quad

373e955

remove optimization flag

5c49978

bill88t previously approved these changes May 23, 2023

View reviewed changes

update copyright

f671d2b

tkroo dismissed bill88t’s stale review via f671d2b May 23, 2023 19:38

tkroo closed this by deleting the head repository May 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Board files for LOLIN S3 MINI #8010

Board files for LOLIN S3 MINI #8010

tkroo commented May 21, 2023

dhalbert commented May 22, 2023

bill88t left a comment

bill88t May 22, 2023

dhalbert commented May 22, 2023

bill88t commented May 22, 2023 •

edited

Loading

dhalbert commented May 22, 2023

tkroo commented May 23, 2023

bill88t commented May 23, 2023

tkroo commented May 23, 2023

skieast commented May 23, 2023

dhalbert commented May 23, 2023 •

edited

Loading

tkroo commented May 23, 2023

dhalbert commented May 23, 2023

skieast commented May 23, 2023 •

edited by dhalbert

Loading

tkroo commented May 23, 2023

tkroo commented May 23, 2023

dhalbert commented May 23, 2023

tkroo commented May 23, 2023

dhalbert commented May 23, 2023

tkroo commented May 23, 2023

Board files for LOLIN S3 MINI #8010

Board files for LOLIN S3 MINI #8010

Conversation

tkroo commented May 21, 2023

dhalbert commented May 22, 2023

bill88t left a comment

Choose a reason for hiding this comment

bill88t May 22, 2023

Choose a reason for hiding this comment

dhalbert commented May 22, 2023

bill88t commented May 22, 2023 • edited Loading

dhalbert commented May 22, 2023

tkroo commented May 23, 2023

bill88t commented May 23, 2023

tkroo commented May 23, 2023

skieast commented May 23, 2023

dhalbert commented May 23, 2023 • edited Loading

tkroo commented May 23, 2023

dhalbert commented May 23, 2023

skieast commented May 23, 2023 • edited by dhalbert Loading

tkroo commented May 23, 2023

tkroo commented May 23, 2023

dhalbert commented May 23, 2023

tkroo commented May 23, 2023

dhalbert commented May 23, 2023

tkroo commented May 23, 2023

bill88t commented May 22, 2023 •

edited

Loading

dhalbert commented May 23, 2023 •

edited

Loading

skieast commented May 23, 2023 •

edited by dhalbert

Loading