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

boards/esp32s3-seeedstudio: Seeedstudio xiao esp32s3 board support #20973

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

Conversation

IsikcanYilmaz
Copy link
Contributor

Contribution description

Support for the tiny, barebones board "Seeed Studio Xiao ESP32S3"

Mostly based on the esp32s3-pros3 board

Testing procedure

  • I have not yet tested the SPI/I2C/ADC, as my main goal has been to get my board up and running as soon as possible. I assume the pin definitions etc. are correct due to their similarities to the pros3 board, and what's noted in the vendor's webpage
  • I'm making this PR following the advice on the contributions document 😄(Verify your concept early!) pardon if it's got obvious mistakes that werent obvious to me

@github-actions github-actions bot added Area: doc Area: Documentation Area: boards Area: Board ports Area: Kconfig Area: Kconfig integration labels Nov 11, 2024
…ostly copied from esp32s3-pros3 board files.
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Looks pretty straightforward!
For the naming, I'd go with esp32s3-seeedstudio-xiao, acknowledging that esp somehow has it's own naming scheme going on, but allowing for future esp32s3 boards by seeedstudio.

Comment on lines +94 to +137
/** TODO all gpios are pwm in seeed esp32s3
* @brief Declaration of the channels for device PWM_DEV(0),
* at maximum PWM_CHANNEL_NUM_DEV_MAX.
*/
// #ifndef PWM0_GPIOS
// #define PWM0_GPIOS { GPIO12, GPIO13, GPIO14, GPIO15, GPIO16 }
// #endif

/**
* @brief Declaration of the channels for device PWM_DEV(1),
* at maximum PWM_CHANNEL_NUM_DEV_MAX.
*/
// #ifndef PWM1_GPIOS
// #define PWM1_GPIOS { GPIO6, GPIO7, GPIO21, GPIO38 }
// #endif

/** @} */

/**
* @name SPI configuration
*
* @note The GPIOs listed in the configuration are first initialized as SPI
* signals when the corresponding SPI interface is used for the first time
* by either calling the `spi_init_cs` function or the `spi_acquire`
* function. That is, they are not allocated as SPI signals before and can
* be used for other purposes as long as the SPI interface is not used.
* @{ TODO SPI pins
*/
// #ifndef SPI0_CTRL
// #define SPI0_CTRL FSPI /**< FSPI is used as SPI_DEV(0) */
// #endif
// #ifndef SPI0_SCK
// #define SPI0_SCK GPIO36 /**< FSPI SCK (pin FSPICLK) */
// #endif
// #ifndef SPI0_MISO
// #define SPI0_MISO GPIO37 /**< FSPI MISO (pin FSPIQ) */
// #endif
// #ifndef SPI0_MOSI
// #define SPI0_MOSI GPIO35 /**< FSPI MOSI (pin FSPID) */
// #endif
// #ifndef SPI0_CS0
// #define SPI0_CS0 GPIO34 /**< FSPI CS0 (pin FSPICS0) */
// #endif
/** @} */
Copy link
Contributor

Choose a reason for hiding this comment

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

Either define them or remove them 😉

Copy link
Contributor

@gschorcht gschorcht Nov 12, 2024

Choose a reason for hiding this comment

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

I would prefer to define them at least for the SPI what schouldn't be a problem.

#define SPI0_SCK    GPIO7
...
#define SPI0_MISO   GPIO8
...
#define SPI0_MOSI   GPIO9
...
#define SPI0_CS0    <any available GPIO, e.g. GPIO3 (D2/A2)>

Copy link
Contributor

@gschorcht gschorcht Nov 12, 2024

Choose a reason for hiding this comment

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

Since GPIO3 (D2/A2) is also used for the SD Card Expansion port, it makes sense to use GPIO3 also for SPI0_CS0. This ensures that the SD Card Expansion Board works with with our sdcard_spi driver.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless you define them, the doc.txt would need to be adapted to reflect the pins that are actually usable from RIOT with the default configuration.

* at maximum PWM_CHANNEL_NUM_DEV_MAX.
*/
// #ifndef PWM0_GPIOS
// #define PWM0_GPIOS { GPIO12, GPIO13, GPIO14, GPIO15, GPIO16 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Some or at least the USER LED pin GPIO21 should be defined as PWM channel.

@mguetschow
Copy link
Contributor

For the naming, I'd go with esp32s3-seeedstudio-xiao, acknowledging that esp somehow has it's own naming scheme going on, but allowing for future esp32s3 boards by seeedstudio.

According to https://github.com/RIOT-OS/RIOT/blob/master/doc/memos/rdm0003.md, I'd rather go with seeedstudio-xiao-esp32s3 (I know board naming has been a mess, but at least there is some common scheme now with RDM0003).

Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Thanks for your PR, much appreciated! 🎉 some comments below

* @brief Support for the Seeed Studio Xiao ESP32S3
* @author Isikcan 'Jon' Yilmaz <[email protected]>

\section esp32s3_seeedstudio Seeed Studio Xiao ESP32
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
\section esp32s3_seeedstudio Seeed Studio Xiao ESP32
\section esp32s3_seeedstudio Seeed Studio Xiao ESP32S3

?

Comment on lines +94 to +137
/** TODO all gpios are pwm in seeed esp32s3
* @brief Declaration of the channels for device PWM_DEV(0),
* at maximum PWM_CHANNEL_NUM_DEV_MAX.
*/
// #ifndef PWM0_GPIOS
// #define PWM0_GPIOS { GPIO12, GPIO13, GPIO14, GPIO15, GPIO16 }
// #endif

/**
* @brief Declaration of the channels for device PWM_DEV(1),
* at maximum PWM_CHANNEL_NUM_DEV_MAX.
*/
// #ifndef PWM1_GPIOS
// #define PWM1_GPIOS { GPIO6, GPIO7, GPIO21, GPIO38 }
// #endif

/** @} */

/**
* @name SPI configuration
*
* @note The GPIOs listed in the configuration are first initialized as SPI
* signals when the corresponding SPI interface is used for the first time
* by either calling the `spi_init_cs` function or the `spi_acquire`
* function. That is, they are not allocated as SPI signals before and can
* be used for other purposes as long as the SPI interface is not used.
* @{ TODO SPI pins
*/
// #ifndef SPI0_CTRL
// #define SPI0_CTRL FSPI /**< FSPI is used as SPI_DEV(0) */
// #endif
// #ifndef SPI0_SCK
// #define SPI0_SCK GPIO36 /**< FSPI SCK (pin FSPICLK) */
// #endif
// #ifndef SPI0_MISO
// #define SPI0_MISO GPIO37 /**< FSPI MISO (pin FSPIQ) */
// #endif
// #ifndef SPI0_MOSI
// #define SPI0_MOSI GPIO35 /**< FSPI MOSI (pin FSPID) */
// #endif
// #ifndef SPI0_CS0
// #define SPI0_CS0 GPIO34 /**< FSPI CS0 (pin FSPICS0) */
// #endif
/** @} */
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless you define them, the doc.txt would need to be adapted to reflect the pins that are actually usable from RIOT with the default configuration.

Comment on lines +154 to +158
/**
* @name Camera configurations and pins
* TODO also mic pins
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/**
* @name Camera configurations and pins
* TODO also mic pins
*/

The board doesn't have those, right?

@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 12, 2024
@IsikcanYilmaz
Copy link
Contributor Author

sorry folks, maybe this PR was more premature than i thought.

@riot-ci
Copy link

riot-ci commented Nov 12, 2024

Murdock results

FAILED

1285c4c boards/esp32s3-seeedstudio: seeedstudio xiao esp32s3 board support. mostly copied from esp32s3-pros3 board files.

Success Failures Total Runtime
33 0 553 01m:19s

Artifacts

@benpicco
Copy link
Contributor

sorry folks, maybe this PR was more premature than i thought.

No worries, nothing wrong with getting feedback early

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants