Re: [PR] Add more I/O options for brickmatch example [nuttx-apps]
acassis merged PR #3010: URL: https://github.com/apache/nuttx-apps/pull/3010 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add more I/O options for brickmatch example [nuttx-apps]
acassis commented on PR #3010: URL: https://github.com/apache/nuttx-apps/pull/3010#issuecomment-2683010662 @eren-terzioglu the 220 ohms as pull-down is too small, it is better to use 1K or even better 4k7 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add more I/O options for brickmatch example [nuttx-apps]
eren-terzioglu commented on code in PR #3010:
URL: https://github.com/apache/nuttx-apps/pull/3010#discussion_r1970343920
##
games/brickmatch/bm_main.c:
##
@@ -300,24 +347,117 @@ void draw_board(FAR struct screen_state_s *state,
usleep(10);
}
}
+#else
+
+/
+ * Name: dim_color
+ *
+ * Description:
+ * Dim led color to handle brightness.
+ *
+ * Parameters:
+ * val - RGB24 value of the led
+ * dim - Percentage of brightness
+ *
+ * Returned Value:
+ * Dimmed RGB24 value.
+ *
+ /
+
+uint32_t dim_color(uint32_t val, float dim)
+{
+ uint16_t r = RGB24RED(val);
+ uint16_t g = RGB24GREEN(val);
+ uint16_t b = RGB24BLUE(val);
+
+ float sat = dim;
+
+ r *= sat;
+ g *= sat;
+ b *= sat;
+
+ return RGBTO24(r, g, b);
+}
+
+/
+ * Name: draw_board
+ *
+ * Description:
+ * Draw the user board without the edges.
+ *
+ * Parameters:
+ * state - Reference to the game screen state structure
+ * area - Reference to the valid area structure
+ * screen - Reference to the information structure of game screen
+ *
+ * Returned Value:
+ * None.
+ *
+ /
+
+void draw_board(FAR struct screen_state_s *state,
+FAR struct fb_area_s *area,
+FAR struct game_screen_s *screen)
+{
+ int result;
+ uint32_t *bp = state->fbmem;
+ int x;
+ int y;
+ int rgb_val;
+ int tmp;
+
+ for (x = 1; x <= BOARDX_SIZE; x++)
+{
+ for (y = 1; y <= BOARDY_SIZE; y++)
+{
+ rgb_val = RGB16TO24(pallete[board[x][y]]);
+ tmp = dim_color(rgb_val, 0.15);
+ *bp++ = ws2812_gamma_correct(tmp);
+}
+}
+
+ lseek(state->fd_fb, 0, SEEK_SET);
+
+ result = write(state->fd_fb, state->fbmem, 4 * N_LEDS);
+ if (result != 4 * N_LEDS)
+{
+ fprintf(stderr,
+ "ws2812_main: write failed: %d %d\n",
+ result,
+ errno);
+}
+}
+#endif /* CONFIG_GAMES_BRICKMATCH_USE_LED_MATRIX */
/
* Name: print_board
*
* Description:
* Draw the board including the user non-visible border for debugging.
+ *
+ * Parameters:
+ * None
+ *
+ * Returned Value:
+ * None.
+ *
/
#ifdef DEBUG_BRICKMATCH_GAME
void print_board(void)
{
int row;
int col;
+ int tmp;
for (row = 0; row < ROW; row++)
{
- printf("+---+---+---+---+---+---+---+---+\n");
+ for (tmp = 0; tmp < ROW; tmp++)
Review Comment:
You are right, thanks for notifying.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add more I/O options for brickmatch example [nuttx-apps]
eren-terzioglu commented on PR #3010: URL: https://github.com/apache/nuttx-apps/pull/3010#issuecomment-2682444349 > GAMES_BRICKMATCH_USE_SPI_SCREEN I did not change anything on previous implementation, additions only made to LED Matrix. I build it without any error but did not test it with an LCD screen. I don't think that it will create a problem. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add more I/O options for brickmatch example [nuttx-apps]
acassis commented on PR #3010: URL: https://github.com/apache/nuttx-apps/pull/3010#issuecomment-2682606433 > Thanks @acassis, updated. Sorry, I found more some small issues -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add more I/O options for brickmatch example [nuttx-apps]
acassis commented on code in PR #3010:
URL: https://github.com/apache/nuttx-apps/pull/3010#discussion_r1970145506
##
games/brickmatch/bm_main.c:
##
@@ -300,24 +347,117 @@ void draw_board(FAR struct screen_state_s *state,
usleep(10);
}
}
+#else
+
+/
+ * Name: dim_color
+ *
+ * Description:
+ * Dim led color to handle brightness.
+ *
+ * Parameters:
+ * val - RGB24 value of the led
+ * dim - Percentage of brightness
+ *
+ * Returned Value:
+ * Dimmed RGB24 value.
+ *
+ /
+
+uint32_t dim_color(uint32_t val, float dim)
+{
+ uint16_t r = RGB24RED(val);
+ uint16_t g = RGB24GREEN(val);
+ uint16_t b = RGB24BLUE(val);
+
+ float sat = dim;
+
+ r *= sat;
+ g *= sat;
+ b *= sat;
+
+ return RGBTO24(r, g, b);
+}
+
+/
+ * Name: draw_board
+ *
+ * Description:
+ * Draw the user board without the edges.
+ *
+ * Parameters:
+ * state - Reference to the game screen state structure
+ * area - Reference to the valid area structure
+ * screen - Reference to the information structure of game screen
+ *
+ * Returned Value:
+ * None.
+ *
+ /
+
+void draw_board(FAR struct screen_state_s *state,
+FAR struct fb_area_s *area,
+FAR struct game_screen_s *screen)
+{
+ int result;
+ uint32_t *bp = state->fbmem;
+ int x;
+ int y;
+ int rgb_val;
+ int tmp;
+
+ for (x = 1; x <= BOARDX_SIZE; x++)
+{
+ for (y = 1; y <= BOARDY_SIZE; y++)
+{
+ rgb_val = RGB16TO24(pallete[board[x][y]]);
+ tmp = dim_color(rgb_val, 0.15);
+ *bp++ = ws2812_gamma_correct(tmp);
+}
+}
+
+ lseek(state->fd_fb, 0, SEEK_SET);
+
+ result = write(state->fd_fb, state->fbmem, 4 * N_LEDS);
+ if (result != 4 * N_LEDS)
+{
+ fprintf(stderr,
+ "ws2812_main: write failed: %d %d\n",
+ result,
+ errno);
+}
+}
+#endif /* CONFIG_GAMES_BRICKMATCH_USE_LED_MATRIX */
/
* Name: print_board
*
* Description:
* Draw the board including the user non-visible border for debugging.
+ *
+ * Parameters:
+ * None
+ *
+ * Returned Value:
+ * None.
+ *
/
#ifdef DEBUG_BRICKMATCH_GAME
void print_board(void)
{
int row;
int col;
+ int tmp;
for (row = 0; row < ROW; row++)
{
- printf("+---+---+---+---+---+---+---+---+\n");
+ for (tmp = 0; tmp < ROW; tmp++)
Review Comment:
Hmm, I think this should be COL, the original printf() was forming the COL
per ROW
##
games/brickmatch/bm_main.c:
##
@@ -333,7 +473,12 @@ void print_board(void)
printf("|\n");
}
- printf("+---+---+---+---+---+---+---+---+\n\n\n");
+ for (tmp = 0; tmp < ROW; tmp++)
Review Comment:
Same here! It is easy to test: just build it for CONSOLE
##
games/brickmatch/Kconfig:
##
@@ -33,6 +33,39 @@ config GAMES_BRICKMATCH_STACKSIZE
int "Brickmatch Game stack size"
default DEFAULT_TASK_STACKSIZE
+#
+# Output Device Selection
+#
+
+choice
+ prompt "Output Device (LED Matrix, SPI Screen, etc)"
Review Comment:
```suggestion
prompt "Output Device (LED Matrix, LCD Screen, etc)"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Add more I/O options for brickmatch example [nuttx-apps]
eren-terzioglu commented on PR #3010: URL: https://github.com/apache/nuttx-apps/pull/3010#issuecomment-2682538039 Thanks @acassis, updated. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add more I/O options for brickmatch example [nuttx-apps]
acassis commented on code in PR #3010: URL: https://github.com/apache/nuttx-apps/pull/3010#discussion_r1970069918 ## games/brickmatch/Kconfig: ## @@ -33,6 +33,39 @@ config GAMES_BRICKMATCH_STACKSIZE int "Brickmatch Game stack size" default DEFAULT_TASK_STACKSIZE +# +# Output Device Selection +# + +choice + prompt "Output Device (LED Matrix, SPI Screen, etc)" + default GAMES_BRICKMATCH_USE_SPI_SCREEN + +config GAMES_BRICKMATCH_USE_SPI_SCREEN Review Comment: ```suggestion config GAMES_BRICKMATCH_USE_LCD_SCREEN ## games/brickmatch/Kconfig: ## @@ -33,6 +33,39 @@ config GAMES_BRICKMATCH_STACKSIZE int "Brickmatch Game stack size" default DEFAULT_TASK_STACKSIZE +# +# Output Device Selection +# + +choice + prompt "Output Device (LED Matrix, SPI Screen, etc)" + default GAMES_BRICKMATCH_USE_SPI_SCREEN + +config GAMES_BRICKMATCH_USE_SPI_SCREEN + bool "SPI Screen as Output" Review Comment: ```suggestion bool "LCD Screen as Output (Also APA102 Matrix as LCD Interface)" ## games/brickmatch/Kconfig: ## @@ -33,6 +33,39 @@ config GAMES_BRICKMATCH_STACKSIZE int "Brickmatch Game stack size" default DEFAULT_TASK_STACKSIZE +# +# Output Device Selection +# + +choice + prompt "Output Device (LED Matrix, SPI Screen, etc)" + default GAMES_BRICKMATCH_USE_SPI_SCREEN Review Comment: ```suggestion default GAMES_BRICKMATCH_USE_LCD_SCREEN -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add more I/O options for brickmatch example [nuttx-apps]
acassis commented on PR #3010: URL: https://github.com/apache/nuttx-apps/pull/3010#issuecomment-2682386696 @eren-terzioglu nice work! I suggest testing also in a LCD display to confirm that the old code still working -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add more I/O options for brickmatch example [nuttx-apps]
acassis commented on code in PR #3010: URL: https://github.com/apache/nuttx-apps/pull/3010#discussion_r1970019225 ## games/brickmatch/Kconfig: ## @@ -33,6 +33,39 @@ config GAMES_BRICKMATCH_STACKSIZE int "Brickmatch Game stack size" default DEFAULT_TASK_STACKSIZE +# +# Output Device Selection +# + +choice + prompt "Output Device (LED Matrix, SPI Screen, etc)" + default GAMES_BRICKMATCH_USE_SPI_SCREEN Review Comment: Actually it should be GAMES_BRICKMATCH_USE_LCD_SCREEN because by default it works with any LCD screen, not only APA103 MATRIX as LCD -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add more I/O options for brickmatch example [nuttx-apps]
nuttxpr commented on PR #3010: URL: https://github.com/apache/nuttx-apps/pull/3010#issuecomment-2681217815 [**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) The PR, as described, **does not fully meet** the NuttX requirements. While it provides a good starting point, it lacks crucial details in several sections. Here's a breakdown: **Summary:** * **Insufficient Detail:** While the changes are mentioned (GPIO input and LED matrix output), the *why* is missing. Why are these changes necessary? Do they address a specific user need or improve existing functionality? Explain the motivation behind the change. * **Missing How:** How does the GPIO input work? Which GPIOs are used? How is the LED matrix integrated? Which driver is used? The summary should provide a concise technical overview of the implementation. **Impact:** * **Too many "No" answers without justification:** While many impacts might genuinely be "no," it's crucial to provide a brief justification. For example, "Impact on build: No, as no new dependencies or build steps are introduced." This demonstrates that the impact was considered. * **Potential Hardware Impact:** Adding LED matrix support and GPIO input *almost certainly* has a hardware impact. The description mentions specific configurations and hardware. This needs to be clearly documented. Specify which architectures and boards are affected. The provided diagram is helpful, but the PR description must be self-contained. Embed the image directly in the PR description (if supported) or refer to it clearly in the repository. **Testing:** * **Insufficient Testing Logs:** Simply stating "Internal CI testing +" isn't sufficient. Provide snippets of relevant log output demonstrating the functionality before and after the change. Show that the GPIO input works and that the LED matrix displays correctly. * **Unclear Configuration:** While the configuration options are listed, it's not clear what the *default* behavior is or how a user would enable these new features. This information should be included in the documentation update (which should exist). * **Lack of Specific Test Cases:** What specific test cases were executed? Did you test different input patterns with the GPIO? Different display patterns with the LED matrix? Describe the tests performed. * **Missing Build Host Information:** Provide information about the build host used for testing. **Recommendations for Improvement:** * **Expand on the *Why* in the summary.** Justify the changes with a clear explanation of the benefits. * **Provide a concise technical explanation of *How* the changes are implemented.** Mention specific drivers, APIs, and configuration options used. * **Justify "No" impact statements.** Even if there's no impact, briefly explain why. * **Clearly document the hardware impact.** Specify affected architectures, boards, and any required hardware connections. Include the diagram in the PR description itself, if possible. * **Provide more detailed testing logs.** Demonstrate the functionality before and after the change with specific examples. * **Describe specific test cases executed.** Show the different scenarios tested and their expected outcomes. * **Document how to enable the new features.** This might require updates to the documentation, which should be included in the PR. * **Include Build Host information:** Specify OS, CPU, compiler details. By addressing these points, you can significantly improve the quality of your NuttX PR and make it easier for reviewers to understand and approve your changes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] Add more I/O options for brickmatch example [nuttx-apps]
eren-terzioglu opened a new pull request, #3010: URL: https://github.com/apache/nuttx-apps/pull/3010 ##Summary Added gpio input option to brickmatch demo to have more ways input (especially devices which does not have adc support). Additionally, added led matrix (ws2812) support to have more options to output * games/brickmatch: Add led matrix output option * games/brickmatch: Add GPIO input option ## Impact Impact on user:No Impact on build:No Impact on hardware:No Impact on documentation:No Impact on security:No Impact on compatibility:No ## Testing Internal CI testing + `esp32-devkitc:brickmatch` or `esp32c6-devkitc:nsh` config with these options enabled: ``` CONFIG_ESP_RMT=y CONFIG_EXAMPLES_RMTCHAR=y CONFIG_EXAMPLES_RMTCHAR_RX=y CONFIG_EXAMPLES_RMTCHAR_TX=y CONFIG_EXAMPLES_WS2812=y CONFIG_RMT=y CONFIG_RMTCHAR=y CONFIG_RMT_DEFAULT_RX_BUFFER_SIZE=256 CONFIG_WS2812=y CONFIG_WS2812_LED_COUNT=64 CONFIG_WS2812_NON_SPI_DRIVER=y CONFIG_BOARD_LATE_INITIALIZE=y CONFIG_DRIVERS_VIDEO=y CONFIG_ESPRESSIF_I2C0=y CONFIG_ESPRESSIF_SPI2=y CONFIG_EXAMPLES_FB=y CONFIG_GAMES_BRICKMATCH=y CONFIG_LCD=y CONFIG_LCD_APA102=y CONFIG_LCD_FRAMEBUFFER=y CONFIG_LCD_NOGETRUN=y CONFIG_SCHED_LPWORK=y CONFIG_SENSORS=y CONFIG_VIDEO_FB=y CONFIG_GAMES_BRICKMATCH_USE_GPIO=y CONFIG_SENSORS_APDS9960=n #CONFIG_EXAMPLES_APDS9960=y CONFIG_INIT_ENTRYPOINT="brick_main" CONFIG_DEV_GPIO=y CONFIG_EXAMPLES_GPIO=y ``` Note: Added options might not minimal, fyi. Here is the diagram I used:  -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
