-
Notifications
You must be signed in to change notification settings - Fork 14
imxrt-multi: add GPIO interrupt handling #581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
JIRA: RTOS-1119
JIRA: RTOS-1119
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @julianuziemblo, 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!
This pull request significantly enhances the iMX RT multidriver by integrating robust GPIO interrupt handling. The primary goal is to move away from inefficient polling methods for detecting GPIO state changes, providing a more responsive and resource-efficient way to interact with hardware. This new functionality will be particularly useful for network stacks like lwip, allowing them to react to events more effectively.
Highlights
- GPIO Interrupt Handling: Introduced comprehensive GPIO interrupt handling capabilities, allowing the system to react to external pin changes without constant polling.
- Asynchronous Interrupt Waiting: Implemented an optional asynchronous interrupt waiting mechanism, enabling non-blocking operations for GPIO interrupts, which is beneficial for applications like lwip.
- New API Functions: Added new functions
gpio_setIrqConf,gpio_getIrqConf, andgpio_waitIrqto configure and wait for GPIO interrupts, enhancing the GPIO driver's functionality. - Platform-Specific Interrupt Definitions: Included platform-specific GPIO interrupt definitions for iMX RT 1060 and 1170, ensuring proper interrupt registration and handling across supported devices.
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. 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.
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. ↩
268e729 to
ec840e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces GPIO interrupt handling, a valuable feature for replacing polling mechanisms. However, the current implementation has several critical issues related to thread safety and correctness that must be addressed. These include a deadlock in the interrupt handler thread due to improper mutex usage, a race condition when accessing shared data structures without locks, and a bug in linked list manipulation that can lead to missed interrupts. I have provided detailed comments on these issues with suggestions for fixes. Addressing these is crucial for the stability and correctness of the new functionality.
ec840e8 to
09ea8d8
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds GPIO interrupt handling capabilities to the i.MX RT multidriver, including both synchronous (polling) and asynchronous (interrupt-driven) mechanisms. The changes are well-structured, using a feature flag GPIO_SUPPORT_ASYNC_IRQ_WAIT to control the inclusion of the asynchronous logic. The implementation introduces new functions for configuring and waiting for interrupts, along with a dedicated thread for handling asynchronous requests.
My review has identified a few issues. There is a critical bug in the interrupt service routine that would prevent the asynchronous mechanism from working correctly. Additionally, there are a couple of high-severity issues, including a logic error in input validation and a resource leak in an initialization function's error path. I've also included some medium-severity comments to improve code clarity and maintainability. Addressing these points will improve the robustness and correctness of the new functionality.
| err = beginthread(gpio_intrThread, GPIO_IRQ_THREAD_PRIO, gpio_common.irq.stack, sizeof(gpio_common.irq.stack), NULL); | ||
| if (err < 0) { | ||
| resourceDestroy(gpio_common.irq.req_list.lock); | ||
| resourceDestroy(gpio_common.irq.lock); | ||
| resourceDestroy(gpio_common.irq.cond); | ||
| return err; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If beginthread fails, the allocated mutexes and condition variable are destroyed, but the interrupt handlers registered in gpio_initInterrupts are not unregistered. This is a resource leak. A cleanup function should be implemented to unregister these handlers, and it should be called from this error path.
ebafbeb to
49a3dfb
Compare
JIRA: RTOS-1119
add new GPIO operations: - gpio_[set|get]_irq_conf: get or set interrupt configuration (active low/high/rising edge/falling edge) for a pin - gpio_wait_irq: if GPIO_SUPPORT_ASYNC_IRQ_WAIT=1, adds request to queue and responds with a bitmask of all interrupts that occured only when the requested interrupts occur. else, returns immediatelly either msg.o.err=-EWOULDBLOCK if the interrupt did not occur or with a bitmask of all requested IRQs that occured. JIRA: RTOS-1119
49a3dfb to
dcac1bb
Compare
add GPIO interrupt handling to iMX RT 1050/1060/1170 multidriver
Description
Motivation and Context
will be useful in lwip to replace GPIO polling
Types of changes
How Has This Been Tested?
Checklist:
Special treatment