Ayopierre/grlib can by ayoopierre · Pull Request #569 · phoenix-rtos/phoenix-rtos-devices

ayoopierre · 2025-07-21T11:37:59Z

Description

Motivation and Context

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

How Has This Been Tested?

Already covered by automatic testing.
New test added: (add PR link here).
Tested by hand on: (list targets here).

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing linter checks and tests passed.
My changes generate no new compilation warnings for any of the targets.

gemini-code-assist

Summary of Changes

Hello @ayoopierre, 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 introduces a new GRCANFD (CAN with Flexible Data-Rate) driver for the GRLIB CAN controller. It provides the necessary components to interact with the hardware, including a core driver, a user-space interface library, and a test application. This integration is a foundational step for future developments, specifically for LibCSP.

Highlights

New GRCANFD Driver Core: Implemented the grlib-can-core static library, providing low-level hardware interaction, interrupt handling, circular buffer management for TX/RX, and functions for device configuration and baud rate calculation.
GRCANFD Driver Binary: Introduced the grlib-can-driver binary, which acts as the main driver application, handling message-based communication from user-space applications to control the CAN hardware.
User-Space Interface Library: Added grlib-can-if, a static library that provides a simplified API for user applications to open, close, configure, send, and receive CAN messages from the driver.
CAN Test Application: Included can-test-app, a simple binary demonstrating the usage of the grlib-can-if library to interact with the GRCANFD device, performing basic send/receive and configuration operations.
Build System Integration: Updated _targets/Makefile.riscv64-noelv to include the new grlib-can-driver and can-test-app components, and added a dedicated Makefile for building all GRCANFD-related components.

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.

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 a new GRCANFD driver, including the core logic, a driver binary, an interface library, and a test application. The implementation is comprehensive but has several critical and high-severity issues related to correctness, resource management, and robustness. These issues should be addressed to ensure the driver is reliable.

gemini-code-assist · 2025-07-21T11:40:59Z

+			return 0;
+		case can_reset:
+			grlibCan_resetDevice(device);
+			msg->o.err = EOK;


This case statement is missing a break. This will cause execution to fall through to the can_writeSync case, which is incorrect behavior.

Suggested change

msg->o.err = EOK;

msg->o.err = EOK;

break;

gemini-code-assist · 2025-07-21T11:40:59Z

+	grlibCan_cleanupResources(driverInstance.devices);
+	portDestroy(driverInstance.port);
+	grlibCan_unregisterDevices(&oid, driverInstance.devices, driverInstance.num);


This block of code is unreachable because grlibCan_messageThread has an infinite loop and will never return. This is a significant issue because the cleanup logic will never be executed, potentially leading to resource leaks. If the driver is intended to run forever, this cleanup code should be removed. If shutdown is a possibility, it must be handled correctly (e.g., via signal handlers), and the cleanup logic must be fixed.

gemini-code-assist · 2025-07-21T11:40:59Z

+	device->ctrlReg = (1 << 1);
+
+	/* Sleeping until reset finished */
+	while ((device->ctrlReg & (1u << 1)) != 0) { }


This busy-wait loop polls a hardware register, which will consume 100% of a CPU core and can starve other threads. Consider adding a timeout and a sleep mechanism to make the driver more robust and efficient.

gemini-code-assist · 2025-07-21T11:41:00Z

+
+	/* Configure ids to listen to */
+	device->confReg = 0;
+	device->confReg |= (1 << 2) | (1 << 1) | (1 << 1) | (1 << 6);


The flag (1 << 1) is duplicated in this bitwise-OR operation. This is likely a typo and could lead to incorrect device configuration. Please review the intended flags.

device->confReg |= (1 << 2) | (1 << 1) | (1 << 6);

gemini-code-assist · 2025-07-21T11:41:00Z

+	dev->rxBufAdd = mmap(NULL, _PAGE_SIZE,
+			PROT_WRITE | PROT_READ, MAP_UNCACHED | MAP_ANONYMOUS, -1, 0);
+	if (dev->rxBufAdd == MAP_FAILED) {
+		(void)munmap((void *)dev->txBufAdd, sizeof(grlibCan_msg_t) * dev->txBufSz);


The size passed to munmap is incorrect. mmap was used to allocate _PAGE_SIZE bytes. Therefore, _PAGE_SIZE bytes should be unmapped. Using sizeof(grlibCan_msg_t) * dev->txBufSz will likely unmap less memory than was allocated, causing a memory leak.

(void)munmap((void *)dev->txBufAdd, _PAGE_SIZE);

gemini-code-assist · 2025-07-21T11:41:00Z

+	}
+
+	if (dev->rxBufAdd != MAP_FAILED) {
+		(void)munmap((void *)dev->rxBufAdd, sizeof(grlibCan_msg_t) * dev->rxBufSz);


The size passed to munmap is incorrect. The rxBufAdd buffer was allocated with a size of _PAGE_SIZE. Therefore, _PAGE_SIZE bytes should be unmapped to avoid a memory leak.

(void)munmap((void *)dev->rxBufAdd, _PAGE_SIZE);

gemini-code-assist · 2025-07-21T11:41:00Z

+			if (msg->i.size < sizeof(uint32_t)) {
+				msg->o.err = -1;
+				return -1;
+			}


The size check for can_getStatus is incorrect. It checks msg->i.size, but this operation writes to an output buffer and takes no input data. The check should be on msg->o.size to ensure the output buffer provided by the caller is large enough to hold the status.

Suggested change

if (msg->i.size < sizeof(uint32_t)) {

msg->o.err = -1;

return -1;

}

case can_getStatus:

if (msg->o.size < sizeof(uint32_t)) {

msg->o.err = -EINVAL;

return -1;

}

gemini-code-assist · 2025-07-21T11:41:00Z

+NAME := grlib-can-driver
+LOCAL_SRCS := grlib-can-driver.c
+DEP_LIBS := grlib-can-core
+LOCAL_CFLAGS = -Wno-error


Using -Wno-error is discouraged as it can hide potential issues by preventing warnings from being treated as errors. It's better to fix the underlying warnings to improve code quality. If this is a temporary measure for development, a comment explaining the reason would be helpful.

Yup, PR isn't draft, so this should be removed.

gemini-code-assist · 2025-07-21T11:41:00Z

+	while (lookup(canDevice, NULL, &canDev) < 0) {
+		printf("Device does not exist\n");
+		return EXIT_FAILURE;
+	}


This while loop is logically equivalent to an if statement. If lookup() fails, the program exits; otherwise, it continues. Using an if statement would represent the code's intent more clearly. If the goal is to wait for the device, the loop should include a sleep mechanism to avoid busy-waiting and high CPU usage.

Suggested change

while (lookup(canDevice, NULL, &canDev) < 0) {

printf("Device does not exist\n");

return EXIT_FAILURE;

}

if (lookup(canDevice, NULL, &canDev) < 0) {

printf("Device does not exist\n");

return EXIT_FAILURE;

}

It's ok as is.

gemini-code-assist · 2025-07-21T11:41:00Z

+
+	printf("Successfully checked CAN device oid\n");
+
+	printf("Can device open: %d\n", grlibCan_open(canDev));


The return value of grlibCan_open() is printed but not checked for errors. If the device fails to open, the program continues, and subsequent calls will likely fail. It's important to add error handling here.

agkaminski · 2025-07-21T11:44:05Z

@@ -0,0 +1,76 @@
+#include <errno.h>


Missing file header

agkaminski · 2025-07-21T11:44:15Z

+LOCAL_SRCS := can-test-app.c
+DEP_LIBS := grlib-can-if
+LOCAL_CFLAGS = -Wno-error
+include $(binary.mk)


No new line at the end of the file

agkaminski · 2025-07-21T11:44:46Z

+NAME := grlib-can-driver
+LOCAL_SRCS := grlib-can-driver.c
+DEP_LIBS := grlib-can-core
+LOCAL_CFLAGS = -Wno-error


Yup, PR isn't draft, so this should be removed.

agkaminski · 2025-07-21T11:45:04Z

+	while (lookup(canDevice, NULL, &canDev) < 0) {
+		printf("Device does not exist\n");
+		return EXIT_FAILURE;
+	}


It's ok as is.

agkaminski · 2025-07-21T11:46:14Z

+{
+	grlibCan_dev_t *dev = (grlibCan_dev_t *)arg;
+	volatile grlibCan_hwDev_t *device = dev->device;
+	volatile uint32_t pending = device->penIsr;


This volatile does nothing - device->penIsr is volatile, local copy on stack is not.

agkaminski · 2025-07-21T11:58:28Z

+		msg.type = mtCreate;
+		msg.oid = dir;
+		msg.i.create.type = otDev;
+		msg.i.create.mode = 0;
+		msg.i.create.dev.port = oid->port; /* Port number assigned by portCreate */
+		msg.i.create.dev.id = i;           /* Id assigned by the driver itself */
+		msg.i.data = buf;                  /* Filename */
+		msg.i.size = strlen(msg.i.data);
+		msg.o.data = NULL;
+		msg.o.size = 0;


Use createdev()

agkaminski · 2025-07-21T11:59:00Z

+	while (lookup("/dev", NULL, &dir) < 0) {
+		usleep(100000);
+	}


Is there a way for /dev on be present at this point?

agkaminski · 2025-07-21T11:59:37Z

+			}
+		}
+	}
+


Perhaps some handling of abnormally big diff?

agkaminski · 2025-07-21T12:00:28Z

+
+/* Core driver functions */
+/* Create devices */
+int grlibCan_initDevices(grlibCan_dev_t *dev, int num);


Keep two empty lines

agkaminski · 2025-07-21T12:01:49Z

+			}
+			msg->o.err = 0;
+			*(uint32_t *)(msg->o.data) = device->device->statReg;
+			msg->o.size = sizeof(uint32_t);


Server cannot modify these fields, it will be ignored.

github-actions · 2025-07-21T13:57:46Z

+#include <sys/mman.h>
+#include <posix/utils.h>
+
+/* Platform specific incldues */


[codespell] _{reported by reviewdog 🐶}
incldues ==> includes

github-actions · 2025-07-21T13:57:46Z

+
+	uint32_t nomBdRate;    /* Nominal baud-rate */
+	uint32_t dataBdRate;   /* Data transfer baud-rate */
+	uint32_t transDelComp; /* Tranmission delay compensation */


[codespell] _{reported by reviewdog 🐶}
Tranmission ==> Transmission

github-actions · 2025-07-21T14:14:06Z

Unit Test Results

10 890 tests ±0 10 220 ✅ ±0 53m 21s ⏱️ +15s
680 suites ±0 670 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 4c80706. ± Comparison against base commit e09d4dc.

♻️ This comment has been updated with latest results.

github-actions · 2025-07-31T14:17:24Z

+			msg->o.err = ret;
+			return ret;
+		case can_readSync:
+			/* Places number of pending frames in RX cirucular buffer, if buffer cannot fit whole CAN frame */


[codespell] _{reported by reviewdog 🐶}
cirucular ==> circular

github-actions · 2026-07-07T09:35:55Z

+		}
+	}
+	return detectedDevices;
+}


[clang-format-pr] _{reported by reviewdog 🐶}
suggested fix

Suggested change

}

gemini-code-assist Bot reviewed Jul 21, 2025

View reviewed changes

ayoopierre self-assigned this Jul 21, 2025

gemini-code-assist Bot reviewed Jul 21, 2025

View reviewed changes

agkaminski reviewed Jul 21, 2025

View reviewed changes

ayoopierre force-pushed the ayopierre/grlib-can branch 2 times, most recently from 8f0c524 to 55f1456 Compare July 21, 2025 13:57

github-actions Bot reviewed Jul 21, 2025

View reviewed changes

ayoopierre force-pushed the ayopierre/grlib-can branch from 55f1456 to 365435a Compare July 21, 2025 13:58

ayoopierre force-pushed the ayopierre/grlib-can branch from 365435a to b2e7c9c Compare July 31, 2025 14:15

github-actions Bot reviewed Jul 31, 2025

View reviewed changes

ayoopierre force-pushed the ayopierre/grlib-can branch 2 times, most recently from 3b23cc5 to b030c67 Compare July 31, 2025 14:34

ayoopierre force-pushed the ayopierre/grlib-can branch 2 times, most recently from cfde493 to a4242b2 Compare July 7, 2026 09:35

github-actions Bot reviewed Jul 7, 2026

View reviewed changes

Add grlib can core, driver, driver interface, test app and stress test

4c80706

ayoopierre force-pushed the ayopierre/grlib-can branch from a4242b2 to 4c80706 Compare July 7, 2026 09:37


		printf("Successfully checked CAN device oid\n");

		printf("Can device open: %d\n", grlibCan_open(canDev));

Uh oh!

Conversation

ayoopierre commented Jul 21, 2025

Description

Motivation and Context

Types of changes

How Has This Been Tested?

Checklist:

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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 Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

github-actions Bot commented Jul 21, 2025 •

edited

Loading