Improve platform code robustness and fix bugs. #61

nchapman · 2025-12-07T23:19:18Z

Add fallback PLAT_detectVariant() for single-device platforms
Fix rgb30 HDMI disabled despite hardware support
Fix rg35xxplus crash when RGXX_MODEL env var unset
Add error handling for I2C, mmap, and input device operations
Replace unsafe sprintf/strcpy with bounds-checked variants
Initialize ioctl structures to prevent kernel garbage
Remove debug logging that spams during gameplay

- Add fallback PLAT_detectVariant() for single-device platforms - Fix rgb30 HDMI disabled despite hardware support - Fix rg35xxplus crash when RGXX_MODEL env var unset - Add error handling for I2C, mmap, and input device operations - Replace unsafe sprintf/strcpy with bounds-checked variants - Initialize ioctl structures to prevent kernel garbage - Remove debug logging that spams during gameplay

Copilot

Pull request overview

This pull request improves platform code robustness across multiple embedded gaming device platforms by adding comprehensive error handling, fixing critical bugs, and replacing unsafe string operations with bounds-checked variants.

Key changes:

Enhanced error handling for device operations (file descriptors, mmap, I2C)
Replaced unsafe sprintf/strcpy with snprintf/strncpy with explicit bounds checking
Fixed critical bugs: rgb30 HDMI support, tg5040 file descriptor validation, rg35xxplus crash on missing env var
Added fallback PLAT_detectVariant() implementation for single-device platforms
Initialized ioctl structures to prevent kernel garbage
Removed debug logging that spammed during gameplay

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
workspace/zero28/platform/platform.c	Added joystick open failure warning
workspace/trimuismart/platform/platform.c	Added error handling for ION mmap, device file opens, and ADC mmap
workspace/tg5040/libmsettings/msettings.c	Fixed file descriptor check from `if (fd)` to `if (fd >= 0)`
workspace/rgb30/platform/platform.c	Enabled HDMI support, added input device error handling, replaced strcpy with strncpy
workspace/rg35xxplus/platform/platform.c	Added null check for RGXX_MODEL environment variable
workspace/rg35xx/platform/platform.c	Added error handling for device files and mmap, replaced sprintf with snprintf, initialized ioctl structure
workspace/my282/platform/platform.c	Added input device error logging, replaced sprintf with snprintf
workspace/miyoomini/platform/platform.c	Added I2C device open error handling, replaced sprintf with snprintf
workspace/miyoomini/keymon/keymon.c	Added I2C device open error handling
workspace/magicmini/platform/platform.c	Added input device error logging, commented out debug logging
workspace/m17/platform/platform.c	Replaced sprintf with snprintf, added input device error logging
workspace/all/common/platform_variant.c	Added weak fallback PLAT_detectVariant() for single-device platforms

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-07T23:24:31Z

workspace/trimuismart/platform/platform.c

 	vid.mem_fd = open("/dev/mem", O_RDWR);
+	if (vid.disp_fd < 0 || vid.fb_fd < 0 || vid.ion_fd < 0 || vid.mem_fd < 0) {
+		fprintf(stderr, "Failed to open device files (disp=%d fb=%d ion=%d mem=%d)\n", vid.disp_fd,
+		        vid.fb_fd, vid.ion_fd, vid.mem_fd);


Resource leak: When file descriptor opening fails after some files have been successfully opened, the successfully opened file descriptors should be closed before returning NULL. For example, if vid.disp_fd opens successfully but vid.fb_fd fails, vid.disp_fd should be closed.

Consider adding cleanup code:

if (vid.disp_fd < 0 || vid.fb_fd < 0 || vid.ion_fd < 0 || vid.mem_fd < 0) { fprintf(stderr, "Failed to open device files (disp=%d fb=%d ion=%d mem=%d)\n", vid.disp_fd, vid.fb_fd, vid.ion_fd, vid.mem_fd); if (vid.disp_fd >= 0) close(vid.disp_fd); if (vid.fb_fd >= 0) close(vid.fb_fd); if (vid.ion_fd >= 0) close(vid.ion_fd); if (vid.mem_fd >= 0) close(vid.mem_fd); return NULL; }

Suggested change

vid.fb_fd, vid.ion_fd, vid.mem_fd);

vid.fb_fd, vid.ion_fd, vid.mem_fd);

if (vid.disp_fd >= 0) close(vid.disp_fd);

if (vid.fb_fd >= 0) close(vid.fb_fd);

if (vid.ion_fd >= 0) close(vid.ion_fd);

if (vid.mem_fd >= 0) close(vid.mem_fd);

Copilot · 2025-12-07T23:24:32Z

workspace/trimuismart/platform/platform.c

 	vid.mem_map =
 	    mmap(0, sysconf(_SC_PAGESIZE), PROT_READ | PROT_WRITE, MAP_SHARED, vid.mem_fd, OVL_V);
+	if (vid.mem_map == MAP_FAILED) {
+		fprintf(stderr, "Failed to mmap overlay registers\n");


Resource leak: When mmap fails, the successfully opened file descriptors (vid.disp_fd, vid.fb_fd, vid.ion_fd, vid.mem_fd) should be closed before returning NULL.

Add cleanup:

if (vid.mem_map == MAP_FAILED) { fprintf(stderr, "Failed to mmap overlay registers\n"); close(vid.mem_fd); close(vid.ion_fd); close(vid.fb_fd); close(vid.disp_fd); return NULL; }

Suggested change

fprintf(stderr, "Failed to mmap overlay registers\n");

fprintf(stderr, "Failed to mmap overlay registers\n");

close(vid.mem_fd);

close(vid.ion_fd);

close(vid.fb_fd);

close(vid.disp_fd);

Copilot · 2025-12-07T23:24:32Z

workspace/trimuismart/platform/platform.c

 	adc.mem_map =
 	    mmap(0, adc.page_size * 2, PROT_READ | PROT_WRITE, MAP_SHARED, adc.mem_fd, addr_start);
+	if (adc.mem_map == MAP_FAILED) {
+		fprintf(stderr, "Failed to mmap ADC registers\n");


Resource leak: When file opening fails, successfully opened file descriptors should be closed before returning NULL. For example, if adc.mem_fd opens successfully but later mmap fails, adc.mem_fd needs to be closed.

Add cleanup:

if (adc.mem_fd < 0) { fprintf(stderr, "Failed to open /dev/mem for ADC\n"); return; }

And similarly for the mmap failure case at line 533, close adc.mem_fd before returning.

Suggested change

fprintf(stderr, "Failed to mmap ADC registers\n");

fprintf(stderr, "Failed to mmap ADC registers\n");

close(adc.mem_fd);

Copilot · 2025-12-07T23:24:33Z

workspace/rg35xx/platform/platform.c

 	vid.fd_mem = open("/dev/mem", O_RDWR);
+	if (vid.fd_fb < 0 || vid.fd_ion < 0 || vid.fd_mem < 0) {
+		LOG_error("Failed to open device files (fb=%d ion=%d mem=%d)\n", vid.fd_fb, vid.fd_ion,
+		          vid.fd_mem);


Resource leak: When file descriptor opening fails after some files have been successfully opened, the successfully opened file descriptors should be closed before returning NULL.

Add cleanup code:

if (vid.fd_fb < 0 || vid.fd_ion < 0 || vid.fd_mem < 0) { LOG_error("Failed to open device files (fb=%d ion=%d mem=%d)\n", vid.fd_fb, vid.fd_ion, vid.fd_mem); if (vid.fd_fb >= 0) close(vid.fd_fb); if (vid.fd_ion >= 0) close(vid.fd_ion); if (vid.fd_mem >= 0) close(vid.fd_mem); return NULL; }

Suggested change

vid.fd_mem);

vid.fd_mem);

if (vid.fd_fb >= 0) close(vid.fd_fb);

if (vid.fd_ion >= 0) close(vid.fd_ion);

if (vid.fd_mem >= 0) close(vid.fd_mem);

Copilot · 2025-12-07T23:24:33Z

workspace/rg35xx/platform/platform.c

+
 	vid.de_mem = mmap(0, DE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, vid.fd_mem, DE);
+	if (vid.de_mem == MAP_FAILED) {
+		LOG_error("Failed to mmap display engine\n");


Resource leak: When mmap fails, the successfully opened file descriptors (vid.fd_fb, vid.fd_ion, vid.fd_mem) should be closed before returning NULL.

Add cleanup:

if (vid.de_mem == MAP_FAILED) { LOG_error("Failed to mmap display engine\n"); close(vid.fd_mem); close(vid.fd_ion); close(vid.fd_fb); return NULL; }

Suggested change

LOG_error("Failed to mmap display engine\n");

LOG_error("Failed to mmap display engine\n");

close(vid.fd_mem);

close(vid.fd_ion);

close(vid.fd_fb);

Copilot · 2025-12-07T23:24:33Z

workspace/trimuismart/platform/platform.c

 	info->padd = (void*)spd.phys_addr;
 	info->vadd = mmap(0, info->size, PROT_READ | PROT_WRITE, MAP_SHARED, info->fd, 0);
+	if (info->vadd == MAP_FAILED) {
+		fprintf(stderr, "ION mmap failed\n");


Resource leak: When mmap fails, info->fd (created by ION_IOC_MAP ioctl at line 103-104) should be closed before returning, as it won't be used.

Add cleanup:

if (info->vadd == MAP_FAILED) { fprintf(stderr, "ION mmap failed\n"); close(info->fd); info->vadd = NULL; return; }

Suggested change

fprintf(stderr, "ION mmap failed\n");

fprintf(stderr, "ION mmap failed\n");

close(info->fd);

Copilot · 2025-12-07T23:24:33Z

workspace/rg35xx/platform/platform.c

 	info->padd = (void*)ipd.phys_addr;
 	info->vadd = mmap(0, info->size, PROT_READ | PROT_WRITE, MAP_SHARED, info->fd, 0);
+	if (info->vadd == MAP_FAILED) {
+		fprintf(stderr, "ION mmap failed\n");


Resource leak: When mmap fails, info->fd (created by ION_IOC_MAP ioctl at line 78-79) should be closed before returning, as it won't be used.

Add cleanup:

if (info->vadd == MAP_FAILED) { fprintf(stderr, "ION mmap failed\n"); close(info->fd); info->vadd = NULL; }

Suggested change

fprintf(stderr, "ION mmap failed\n");

fprintf(stderr, "ION mmap failed\n");

close(info->fd);

Copilot AI review requested due to automatic review settings December 7, 2025 23:19

Copilot started reviewing on behalf of nchapman December 7, 2025 23:19 View session

Copilot AI reviewed Dec 7, 2025

View reviewed changes

Review fixes.

043250a

nchapman merged commit f113b01 into develop Dec 8, 2025
4 checks passed

nchapman deleted the feature/platform-review branch December 8, 2025 00:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve platform code robustness and fix bugs. #61

Improve platform code robustness and fix bugs. #61

Uh oh!

nchapman commented Dec 7, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 7, 2025

Uh oh!

Copilot AI Dec 7, 2025

Uh oh!

Copilot AI Dec 7, 2025

Uh oh!

Copilot AI Dec 7, 2025

Uh oh!

Copilot AI Dec 7, 2025

Uh oh!

Copilot AI Dec 7, 2025

Uh oh!

Copilot AI Dec 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-		        vid.fb_fd, vid.ion_fd, vid.mem_fd);
+		        vid.fb_fd, vid.ion_fd, vid.mem_fd);
+		if (vid.disp_fd >= 0) close(vid.disp_fd);
+		if (vid.fb_fd >= 0) close(vid.fb_fd);
+		if (vid.ion_fd >= 0) close(vid.ion_fd);
+		if (vid.mem_fd >= 0) close(vid.mem_fd);

	fprintf(stderr, "Failed to mmap ADC registers\n");
	fprintf(stderr, "Failed to mmap ADC registers\n");
	close(adc.mem_fd);

-		          vid.fd_mem);
+		          vid.fd_mem);
+		if (vid.fd_fb >= 0) close(vid.fd_fb);
+		if (vid.fd_ion >= 0) close(vid.fd_ion);
+		if (vid.fd_mem >= 0) close(vid.fd_mem);

	fprintf(stderr, "ION mmap failed\n");
	fprintf(stderr, "ION mmap failed\n");
	close(info->fd);

Improve platform code robustness and fix bugs. #61

Improve platform code robustness and fix bugs. #61

Uh oh!

Conversation

nchapman commented Dec 7, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants