Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ayoopierre, 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!
I've been working on a significant update to port the libCSP library to the Phoenix RTOS environment. This involves integrating core CSP functionalities with Phoenix RTOS primitives and adding support for the GRCANFD driver, which enables robust CAN communication. I've also updated the build system with a new Makefile and included several examples to demonstrate the new capabilities. This change broadens the compatibility of libCSP, making it available for use in Phoenix RTOS-based embedded systems.
Highlights
- Phoenix RTOS Port: I've integrated libCSP with Phoenix RTOS, providing architecture-specific implementations for semaphores and system hooks, ensuring CSP can run natively on this RTOS.
- GRCANFD Driver Integration: I've added a new GRCANFD driver, enabling libCSP to communicate over CAN networks using the GRLIB CAN controller. This includes functions for transmitting and receiving CAN frames.
- Updated Build System: I've introduced a new Makefile specifically for Phoenix RTOS, which streamlines the build process for libCSP and includes targets for various test applications like loopback, UART, and RDP client/server over CAN.
- RDP Timeout Adjustment: I've adjusted the RDP connection timeout in src/csp_rdp.c from 10,000 ms to 100,000 ms. This change accounts for the potential overhead of context switching on single-core systems, ensuring more reliable RDP connection establishment.
- New Example Applications: I've included several new example applications (loopback, UART, RDP client/server) that demonstrate and test the newly integrated libCSP functionalities on Phoenix RTOS, providing clear usage patterns for developers.
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 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 or fill out our survey 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.
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 introduces significant new functionality by porting libCSP to Phoenix RTOS and adding a GRLIB CAN driver interface. The changes are extensive, including new drivers, examples, and a Makefile for the new target. My review has identified several critical issues related to thread safety (use of strtok, unprotected global variables), memory management (memory leaks, incorrect error handling on malloc failure), and incorrect logic in driver implementations (dropping CAN frames, flawed semaphore implementation). There are also several high and medium severity issues that should be addressed to improve correctness and maintainability. Overall, this is a great contribution, but these critical issues must be fixed before merging.
| if (detectedDevices == 0) | ||
| return EXIT_FAILURE; | ||
|
|
||
| device = &devices[1]; |
There was a problem hiding this comment.
This line accesses devices[1], but there is no guarantee that two or more devices were detected. The check on line 101 only ensures detectedDevices > 0. If only one device is detected, this will result in an out-of-bounds access, leading to undefined behavior. You should add a check to ensure detectedDevices > 1.
| if (ctx == NULL) { | ||
| printf("%s: Malloc failed\n", __func__); | ||
| } |
There was a problem hiding this comment.
If malloc fails, the function prints an error message but continues execution. This will lead to a null pointer dereference when ctx is used on subsequent lines, causing a crash. The function should return an error code immediately if malloc returns NULL.
if (ctx == NULL) {
printf("%s: Malloc failed\n", __func__);
close(fd);
return CSP_ERR_NOMEM;
}
examples/csp_phoenix_test_loopback.c
Outdated
| void csp_print_func(const char * format, ...) { | ||
| // va_list ap; | ||
| // int argno = 0; | ||
|
|
||
| // va_start(ap, format); | ||
|
|
||
| printf(format); | ||
| } |
There was a problem hiding this comment.
The implementation of csp_print_func is vulnerable to a format string attack. It calls printf(format) which is unsafe if format contains format specifiers. The commented-out code using va_list and vprintf was the correct and safe approach. You should restore and complete that implementation.
void csp_print_func(const char * format, ...) {
va_list ap;
va_start(ap, format);
vprintf(format, ap);
va_end(ap);
}| usart_context_t * ctx = malloc(sizeof(usart_context_t)); | ||
| if (ctx == NULL) { | ||
| printf("%s: Malloc failed\n", __func__); | ||
| } |
There was a problem hiding this comment.
The usart_context_t struct allocated with malloc is never freed, leading to a memory leak. A corresponding csp_usart_close function should be implemented to free this context and clean up other resources (like the RX thread).
| csp_usart_callback_t rx_callback; | ||
| void * user_data; | ||
| int fd; | ||
| handle_t lock; |
There was a problem hiding this comment.
The lock member of usart_context_t is used in csp_usart_lock and csp_usart_unlock but it is never initialized. This will lead to undefined behavior when the lock is used. You need to initialize it, for example with mutexCreate, after allocating the context.
| csp_iface_t iface; | ||
| csp_can_interface_data_t ifdata; |
There was a problem hiding this comment.
The global variables iface and ifdata are declared but appear to be unused. They should be removed to avoid confusion.
| csp_iface_t iface; | ||
| csp_can_interface_data_t ifdata; |
There was a problem hiding this comment.
The global variables iface and ifdata are declared but appear to be unused. They are shadowed by local variables in main. These should be removed to avoid confusion.
| for (;;) { | ||
| sleep(10); | ||
| } |
There was a problem hiding this comment.
This for loop is unreachable because the preceding while(1) loop never terminates. This dead code should be removed.
| for (;;) { | ||
| sleep(10); | ||
| } |
There was a problem hiding this comment.
This for loop is unreachable because the preceding while(1) loop never terminates. This dead code should be removed. The same issue exists in the client and task_router functions.
| LOCAL_CFLAGS += \ | ||
| -DCSP_REPRODUCIBLE_BUILDS=$(CSP_REPRODUCIBLE_BUILDS) \ | ||
| -DCSP_HAVE_STDIO=$(CSP_HAVE_STDIO) \ | ||
| -DCSP_ENABLE_CSP_PRINT=$(CSP_ENABLE_CSP_PRINT) \ | ||
| -DCSP_PRINT_STDIO=$(CSP_PRINT_STDIO) \ | ||
| -DCSP_USE_RDP=$(CSP_USE_RDP) \ | ||
| -DCSP_USE_HMAC=$(CSP_USE_HMAC) \ | ||
| -DCSP_USE_PROMISC=$(CSP_USE_PROMISC) \ | ||
| -DCSP_USE_RTABLE=$(CSP_USE_RTABLE) \ | ||
| -DCSP_BUFFER_ZERO_CLEAR=$(CSP_BUFFER_ZERO_CLEAR) \ | ||
| -DCSP_PHOENIX=$(CSP_PHOENIX) \ | ||
| -DCSP_QFIFO_LEN=$(CSP_QFIFO_LEN) \ | ||
| -DCSP_PORT_MAX_BIND=$(CSP_PORT_MAX_BIND) \ | ||
| -DCSP_CONN_RXQUEUE_LEN=$(CSP_CONN_RXQUEUE_LEN) \ | ||
| -DCSP_BUFFER_SIZE=$(CSP_BUFFER_SIZE) \ | ||
| -DCSP_BUFFER_COUNT=$(CSP_BUFFER_COUNT)\ | ||
| -DCSP_CONN_MAX=$(CSP_CONN_MAX) \ | ||
| -DCSP_RDP_MAX_WINDOW=$(CSP_RDP_MAX_WINDOW) \ | ||
| -DCSP_RTABLE_SIZE=$(CSP_RTABLE_SIZE) |
There was a problem hiding this comment.
The LOCAL_CFLAGS for the test binaries (libcsp-test-loopback, uart-test, csp-rdp-test-server, csp-rdp-test-client) are largely duplicated. This makes the Makefile harder to maintain. Consider defining a common set of flags and reusing it for each target to reduce repetition and improve readability.
…maphore implementation, modify RDP timeouts
ayoopierre
force-pushed
the
ayopierre/grlibCan_interface
branch
from
4fcc711 to
1827215
Compare
1 participant
Description
Motivation and Context
Types of changes
How Has This Been Tested?
Checklist: