Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @tavdog, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request adds support for the Adafruit Matrix Portal ESP32-S3 device. The changes involve adding a new environment configuration in platformio.ini for the matrixportal-s3 board, defining pin mappings in src/display.cpp specific to the Matrix Portal, and creating a new sdkconfig file sdkconfig.matrixportal-s3 with the configuration settings for the device. A minor change was also made to extra_scripts/reset.py to avoid errors.
Highlights
- Board Support: Adds support for the Adafruit Matrix Portal ESP32-S3 device.
- Pin Mapping: Defines pin mappings in
src/display.cppspecific to the Matrix Portal. - Configuration: Creates a new sdkconfig file
sdkconfig.matrixportal-s3with the configuration settings for the device.
Changelog
Click here to see the changelog
- extra_scripts/reset.py
- Added
matrixportal-s3to thePRODUCTION_VERSIONdictionary to prevent errors during the build process.
- Added
- platformio.ini
- Added a new environment
env:matrixportal-s3for the Adafruit Matrix Portal ESP32-S3 board. - Defined board, partition, build flags, and library dependencies for the new environment.
- The build flags include disabling GFX and fast functions, defining
MATRIXPORTALS3, disabling clock phase inversion, and setting HTTP buffer sizes. - The library dependencies include
libwebpandESP32-HUB75-MatrixPanel-DMA.
- Added a new environment
- sdkconfig.matrixportal-s3
- Created a new sdkconfig file with configuration settings for the Adafruit Matrix Portal ESP32-S3 device.
- This file contains numerous configuration options related to the ESP-IDF, including settings for the SOC, build type, bootloader, security features, application manager, and various component configurations.
- src/display.cpp
- Added pin definitions specific to the Matrix Portal S3, including RGB data lines, address lines, clock, latch, and output enable pins.
- Added a conditional compilation block for
MATRIXPORTALS3to use the new pin definitions. - Added
NO_INVERT_CLOCK_PHASEto disable clock phase inversion.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A portal of matrix,
S3's power takes hold,
Pixels start to dance.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request adds support for the Matrix Portal S3 device. The changes include modifications to the reset.py script, platformio.ini and the display.cpp file, as well as adding a new sdkconfig.matrixportal-s3 file. Overall, the changes seem reasonable, but there are a few areas that could be improved.
Summary of Findings
- Missing Documentation: The pull request lacks documentation explaining the changes and how to use the new Matrix Portal S3 device. A README update would be beneficial.
- Inconsistent Naming: In
display.cpp, the naming convention for defining pins is inconsistent between the existing devices and the new Matrix Portal S3. Using a consistent style improves readability. - Comment Clarity: The comment in
extra_scripts/reset.pyis vague. It should explain why the line is needed to shut up the errors.
Merge Readiness
The pull request introduces support for a new device, which is a valuable addition. However, the identified issues, particularly the lack of documentation and the comment clarity, should be addressed before merging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. At a minimum, the high severity comments should be addressed before merging.
| "pixoticker": "v10/35833", | ||
| "tidbyt-gen2": "v11/35369" | ||
| "tidbyt-gen2": "v11/35369", | ||
| "matrixportal-s3": "v11/35369" # just to shut up the errors. |
There was a problem hiding this comment.
Can you elaborate on what errors this line is intended to suppress? Adding more context here would be helpful for future maintainers.
| "matrixportal-s3": "v11/35369" # just to shut up the errors. | |
| "matrixportal-s3": "v11/35369" # Suppress errors due to missing device definition during initial build. |
| #define R1 42 | ||
| #define G1 41 | ||
| #define BL1 40 | ||
| #define R2 38 | ||
| #define G2 39 | ||
| #define BL2 37 | ||
| #define CH_A 45 | ||
| #define CH_B 36 | ||
| #define CH_C 48 | ||
| #define CH_D 35 | ||
| #define CH_E 21 | ||
| #define CLK 2 | ||
| #define LAT 47 | ||
| #define OE 14 |
There was a problem hiding this comment.
For consistency with the other device definitions, consider using BL1 instead of BL1 for the blue pin definition. Also, consider using _PIN suffix for these definitions.
| #define R1 42 | |
| #define G1 41 | |
| #define BL1 40 | |
| #define R2 38 | |
| #define G2 39 | |
| #define BL2 37 | |
| #define CH_A 45 | |
| #define CH_B 36 | |
| #define CH_C 48 | |
| #define CH_D 35 | |
| #define CH_E 21 | |
| #define CLK 2 | |
| #define LAT 47 | |
| #define OE 14 | |
| #define R1_PIN 42 | |
| #define G1_PIN 41 | |
| #define BL1_PIN 40 | |
| #define R2_PIN 38 | |
| #define G2_PIN 39 | |
| #define BL2_PIN 37 | |
| #define CH_A_PIN 45 | |
| #define CH_B_PIN 36 | |
| #define CH_C_PIN 48 | |
| #define CH_D_PIN 35 | |
| #define CH_E_PIN 21 | |
| #define CLK_PIN 2 | |
| #define LAT_PIN 47 | |
| #define OE_PIN 14 |
1 participant
No description provided.