+ TFT attempts to bug-fix #7

DnCraptor · 2025-10-14T16:43:30Z

small improvements

gemini-code-assist · 2025-10-14T16:43:53Z

Summary of Changes

Hello @DnCraptor, I'm Gemini Code Assist¹! 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!

This pull request implements "bug-fixes" and "small improvements" primarily centered around the TFT display functionality. The core change introduces a user-configurable inversion setting for TFT displays, which is integrated into the system's persistent configuration and accessible through the menu. This involves refactoring the display initialization logic, adjusting CMake build options, and modifying the application's startup sequence to properly apply display settings and handle reboots when inversion is changed.

Highlights

TFT Inversion Control: Introduced a user-configurable setting for TFT display inversion, allowing dynamic adjustment of display colors.
CMake Build Configuration: Updated default build settings for platform and board, and streamlined TFT-related options by removing a redundant INVERSION CMake option.
Display Initialization Refactoring: The TFT display initialization sequence was moved and made dynamic, incorporating the new inversion setting directly into the display commands.
Configuration Integration: The new inversion setting is now saved and loaded with other system settings and is accessible via the in-game menu.
Build System Adjustments: Conditional compilation directives were added to ensure VGA pin testing is skipped when TFT is enabled, and the core initialization order was adjusted for better startup flow.

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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

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.

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

gemini-code-assist

Code Review

This pull request introduces several changes to fix bugs and improve TFT display support. The main change is refactoring the display inversion handling from a compile-time macro to a runtime setting, which is a good improvement for user experience. The PR also includes a critical fix for a buffer overflow when initializing the palette.

My review focuses on a few areas for improvement:

There's a significant issue with a redefined RGB888 macro that could lead to incorrect colors.
The new runtime inversion logic seems incomplete for ILI9341 displays.
Some minor issues include a magic number, a variable naming style inconsistency, and a typo in a UI string.

Overall, the changes are positive, but the identified issues, especially the macro redefinition and incomplete ILI9341 logic, should be addressed to ensure correctness and maintainability.

gemini-code-assist · 2025-10-14T16:47:03Z

drivers/st7789/st7789.c

+    #ifdef ILI9341
+        // ILI9341
+        #ifdef INVERSION
+        // ILI9341: переворот и инверсия цветов
+        2, 0, 0x36, MADCTL_MY | MADCTL_MX | MADCTL_ROW_COLUMN_EXCHANGE | MADCTL_BGR_PIXEL_ORDER, // flip + BGR
+        #else
+        2, 0, 0x36, MADCTL_ROW_COLUMN_EXCHANGE | MADCTL_BGR_PIXEL_ORDER, // Set MADCTL
+        #endif
+    #else


The refactoring from a compile-time INVERSION macro to a runtime variable seems incomplete for the ILI9341 display. The #ifdef INVERSION block will now always take the #else path since the INVERSION macro is no longer defined. This means display flipping for ILI9341 is now disabled and cannot be enabled at runtime. The INVERSION variable is also not used to control the MADCTL register setting for ILI9341.

To fix this, you'll need to dynamically construct the init_seq array or parts of it based on the INVERSION variable's value. Since init_seq is a const array, you cannot modify it. One approach is to remove const, making it a mutable local array, and then set the MADCTL parameter for the 0x36 command based on the INVERSION variable before calling lcd_init().

gemini-code-assist · 2025-10-14T16:47:03Z

drivers/graphics/graphics.h

+#ifndef RGB888
+#define RGB888(r, g, b) (((r) << 16) | ((g) << 8 ) | (b))
+#endif


While adding #ifndef prevents a compilation error, it introduces a more subtle issue. The RGB888 macro is also defined in drivers/st7789/st7789.h to perform a 24-bit to 16-bit color conversion. When TFT is enabled, graphics.h includes st7789.h, and due to this #ifndef, the 16-bit conversion macro from st7789.h is used everywhere RGB888 is called. This is problematic in files like src/main.cpp, where NesPalette888 is initialized with RGB888 and expects 24-bit color values, not 16-bit. This can lead to incorrect colors.

A better approach is to have uniquely named macros for different functionalities. I recommend the following changes:

In drivers/st7789/st7789.h, rename RGB888 to something like RGB888_TO_565.

In drivers/st7789/st7789.c, update the textmode_palette initialization to use the new macro name.

In this file (drivers/graphics/graphics.h), you can then safely define RGB888 for 24-bit color without the #ifndef guard.

This will make the code clearer and prevent subtle color bugs.

gemini-code-assist · 2025-10-14T16:47:03Z

drivers/st7789/st7789.c

 #define MADCTL_ROW_COLUMN_EXCHANGE (1<<5)
 #define MADCTL_COLUMN_ADDRESS_ORDER_SWAP (1<<6)

+uint8_t INVERSION = 0;


Using all-caps for a variable name like INVERSION can be misleading, as this convention is typically reserved for constants and macros. To improve clarity and adhere to common C/C++ style, consider renaming it to g_inversion to denote its global scope, or simply inversion. This would require updating its extern declaration and all usages across the project.

gemini-code-assist · 2025-10-14T16:47:03Z

drivers/st7789/st7789.c

+    #else
+        // ST7789
+    //    2, 0, 0x36, MADCTL_COLUMN_ADDRESS_ORDER_SWAP | MADCTL_ROW_COLUMN_EXCHANGE, // Set MADCTL
+        2, 0, 0x36, MADCTL_COLUMN_ADDRESS_ORDER_SWAP | 32, // Set MADCTL


Using the magic number 32 reduces code readability. It's better to use the MADCTL_ROW_COLUMN_EXCHANGE macro, which is defined in this file and represents the same value (1 << 5). This makes the code self-documenting and easier to maintain.

2, 0, 0x36, MADCTL_COLUMN_ADDRESS_ORDER_SWAP | MADCTL_ROW_COLUMN_EXCHANGE, // Set MADCTL

gemini-code-assist · 2025-10-14T16:47:04Z

src/main.cpp

 #endif
+#if TFT
+    { "" },
+    { "Investion %s", ARRAY, &settings.inversion, nullptr, 1, { "NO ", "YES" } },


There's a typo in the menu item text. 'Investion' should be 'Inversion'.

{ "Inversion %s", ARRAY, &settings.inversion, nullptr, 1, { "NO ", "YES" } },

DnCraptor added 3 commits October 14, 2025 17:02

+ TFT inversion from menu

1e527af

+ attempts to fix it

f884ff5

Merge branch 'xrip:main' into main

93f19c7

gemini-code-assist bot reviewed Oct 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

+ TFT attempts to bug-fix #7

+ TFT attempts to bug-fix #7

Uh oh!

DnCraptor commented Oct 14, 2025

Uh oh!

gemini-code-assist bot commented Oct 14, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Oct 14, 2025

Uh oh!

gemini-code-assist bot Oct 14, 2025

Uh oh!

gemini-code-assist bot Oct 14, 2025

Uh oh!

gemini-code-assist bot Oct 14, 2025

Uh oh!

gemini-code-assist bot Oct 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

+ TFT attempts to bug-fix #7

Are you sure you want to change the base?

+ TFT attempts to bug-fix #7

Uh oh!

Conversation

DnCraptor commented Oct 14, 2025

Uh oh!

gemini-code-assist bot commented Oct 14, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant