Implementing udev monitoring for rM1 #314
Conversation
Eeems
changed the title
|
I'm currently having a ton of issues avoiding deadlocks with this. It doesn't help that I can't find good documentation on libudev. Either I'm bad at threading, or it's not thread safe. |
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce significant updates to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (5)
applications/system-service/powerapi.h (2)
67-67: Approve timer initialization and suggest documentationThe initialization of
timertonullptris a good practice that improves code safety. However, the purpose of this timer is not immediately clear from the header file.Consider adding a brief comment explaining the purpose of this timer and where it's used (e.g., in the .cpp file). This will improve code readability and maintainability.
Line range hint
1-85: Consider enhancing error handling and thread safetyThe overall structure of the PowerAPI class looks good, with clear separation of concerns and appropriate use of Qt's property system. However, given the PR objectives mentioning difficulties with deadlocks, consider the following improvements:
Error Handling: Implement robust error handling mechanisms, especially for operations that might fail (e.g., battery or charger state updates).
Thread Safety: Ensure that all methods that might be called from different threads are thread-safe. This might involve using mutex locks or other synchronization primitives.
Documentation: Add comments explaining the thread safety guarantees of each method, especially those that might be called from different threads.
These enhancements will improve the robustness and reliability of the PowerAPI class, especially in a multi-threaded environment.
shared/liboxide/liboxide.pro (1)
Line range hint
1-146: Overall assessment: Changes are appropriate for implementing udev monitoringThe modifications to
liboxide.proare consistent with the PR objective of implementing udev monitoring for rM1. The additions ofudev.cppandudev.hto the SOURCES and HEADERS variables, respectively, along with the inclusion of the libudev library in the LIBS variable, provide the necessary framework for integrating udev functionality into the liboxide library.Given the reported difficulties with deadlocks, it's crucial to ensure that the udev-related code is implemented with proper consideration for thread-safety. As the project file changes themselves don't directly impact this, the focus should be on the implementation in the
udev.cppandudev.hfiles.To address the thread-safety concerns:
- Implement a thread-safe wrapper around libudev calls if one doesn't already exist.
- Use appropriate synchronization primitives (mutexes, condition variables) in the udev monitoring code.
- Consider using a thread-safe queue for passing udev events between threads if applicable.
- Implement comprehensive error handling and logging to help diagnose any deadlock issues that may occur during runtime.
These measures should help mitigate the risk of deadlocks while implementing the udev monitoring functionality.
shared/liboxide/udev.h (1)
1-5: Close the Doxygen group in the comment blockThe Doxygen group started with
\addtogroup Oxideand\{should be closed with a corresponding\}to ensure proper documentation generation.Add the closing
\}at the end of the file:+/*! + * \} + */Or modify the existing comment block to include it:
/*! * \addtogroup Oxide * @{ * \file + * \} */applications/system-service/powerapi.cpp (1)
24-29: Review thread safety in UDev monitor callbacks.Since you're using UDev monitor callbacks to trigger the
update()method, ensure thatupdate()and any accessed shared resources are thread-safe. Ifupdate()manipulates shared data or interacts with the UI, consider usingQMetaObject::invokeMethodwithQt::QueuedConnectionto safely execute the method in the main thread.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- applications/system-service/powerapi.cpp (2 hunks)
- applications/system-service/powerapi.h (2 hunks)
- shared/liboxide/liboxide.pro (3 hunks)
- shared/liboxide/udev.cpp (1 hunks)
- shared/liboxide/udev.h (1 hunks)
🧰 Additional context used
🪛 cppcheck
applications/system-service/powerapi.cpp
[error] 35-35: There is an unknown macro here somewhere. Configuration is required. If Q_ENUM is a macro then please configure it.
(unknownMacro)
shared/liboxide/udev.cpp
[error] 51-51: There is an unknown macro here somewhere. Configuration is required. If Q_PROPERTY is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (5)
applications/system-service/powerapi.h (1)
5-5: Approve udev include and verify its usageThe addition of
#include <liboxide/udev.h>aligns with the PR objective of implementing udev monitoring for rM1. However, there are no visible changes in this file that directly use udev functionality.Please verify that udev functionality is implemented in the corresponding .cpp file or other related files. Run the following script to check for udev usage:
shared/liboxide/liboxide.pro (3)
37-37: LGTM: Addition of udev.cpp to SOURCESThe addition of
udev.cppto the SOURCES variable is consistent with the PR objective of implementing udev monitoring for rM1. This change ensures that the new udev-related source file will be compiled as part of the liboxide library.
60-60: LGTM: Addition of udev.h to HEADERSThe addition of
udev.hto the HEADERS variable is a necessary complement to theudev.cppsource file. This ensures that the udev-related declarations are properly included in the library's public interface.
80-80: LGTM with a note: Addition of libudev dependencyThe addition of
-ludevto the LIBS variable is necessary for implementing udev monitoring. This change will correctly link the libudev library to the project.However, given the mentioned difficulties with deadlocks in the PR comments, it's important to ensure that the usage of libudev in the codebase is thread-safe. Consider the following:
- Review the libudev documentation for any thread-safety guarantees or limitations.
- Implement appropriate synchronization mechanisms in the udev-related code if necessary.
- Consider running static analysis tools to detect potential race conditions or deadlocks.
To help identify potential thread-safety issues, you can run the following command to search for concurrent programming constructs in the udev-related code:
This will help identify areas where thread synchronization is being used and may need careful review.
✅ Verification successful
To effectively address the absence of
udev.cppandudev.h, let's locate all udev-related files in the codebase.
To assess potential thread-safety issues in the udev-related code, let's search for concurrent programming constructs within
shared/liboxide/udev.cppandshared/liboxide/udev.h.
LGTM with a note: Addition of libudev dependency
The addition of
-ludevto the LIBS variable is necessary for implementing udev monitoring. This change correctly links the libudev library to the project.Furthermore, thread-safety mechanisms are implemented in the udev-related code using
QMutexandQThread, addressing the concerns about potential deadlocks.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for concurrent programming constructs in udev-related files rg -t cpp -i '(mutex|lock|thread|atomic|condition_variable)' udev.cpp udev.hLength of output: 174
Script:
#!/bin/bash # Find all files with 'udev' in their name fd udev --type fLength of output: 66
Script:
#!/bin/bash # Search for concurrent programming constructs in identified udev-related files rg -t cpp -i '(mutex|lock|thread|atomic|condition_variable)' shared/liboxide/udev.cpp shared/liboxide/udev.hLength of output: 1056
shared/liboxide/udev.cpp (1)
25-33: Potential Issues with Object Ownership and ThreadingThe
UDevobject (this) is moved to another thread usingmoveToThread(&_thread)after connecting signals and slots. This can cause unexpected behavior if the object has parent-child relationships or if other threads access it.Ensure that moving
thisto another thread is safe and that all member accesses and signal-slot connections respect thread boundaries.
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- applications/system-service/powerapi.cpp (3 hunks)
- applications/system-service/powerapi.h (1 hunks)
- shared/liboxide/udev.cpp (1 hunks)
- shared/liboxide/udev.h (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- applications/system-service/powerapi.h
🧰 Additional context used
🪛 cppcheck
applications/system-service/powerapi.cpp
[error] 34-34: There is an unknown macro here somewhere. Configuration is required. If Q_ENUM is a macro then please configure it.
(unknownMacro)
shared/liboxide/udev.cpp
[error] 51-51: There is an unknown macro here somewhere. Configuration is required. If Q_PROPERTY is a macro then please configure it.
(unknownMacro)
🪛 GitHub Check: CodeFactor
shared/liboxide/udev.cpp
[notice] 185-257: shared/liboxide/udev.cpp#L185-L257
Complex Method
🔇 Additional comments (13)
shared/liboxide/udev.h (7)
1-14: LGTM: Appropriate file header and includesThe file header with doxygen-style comment and the included headers are appropriate for the implementation. Good practice is followed by not using
using namespace std;.
15-22: LGTM: Well-structured class declaration and singleton implementationThe UDev class is well-structured, inheriting from QObject and implementing the singleton pattern correctly. The use of the Q_OBJECT macro is appropriate for a Qt-based class.
62-70: LGTM: Well-designed public methodsThe public methods provide a comprehensive set of functionalities for device management, including monitoring, starting, stopping, and retrieving device information. The method signatures are clear and consistent.
72-73: LGTM: Appropriate signal declarationThe 'event' signal is well-defined and appropriate for notifying about device events. It correctly emits a Device object, which should contain all necessary information about the event.
75-86: LGTM: Well-structured private and protected membersThe private and protected members are well-designed for managing the UDev state. The use of QMutex for thread safety and QSharedPointer in the monitors map shows good attention to concurrency and memory management. The protected 'monitor()' method seems appropriately placed for potential derived classes.
87-89: LGTM: Appropriate global operator and metatype declarationThe global operator<< for QDebug and UDev::Device is well-declared, allowing for easy logging of Device objects. The use of Q_DECLARE_METATYPE for UDev::Device is correct and will enable its use with Qt's meta-object system.
58-61: 🛠️ Refactor suggestionConsider renaming overloaded methods to avoid ambiguity
The overloaded
subsystemanddeviceTypemethods might cause ambiguity, especially when passing lambdas or function pointers. To improve clarity and prevent potential issues, consider renaming these methods to differentiate them clearly.Apply this diff to rename the methods:
-static void subsystem(const QString& subsystem, std::function<void(const Device&)> callback); -static void subsystem(const QString& subsystem, std::function<void()> callback); +static void subsystemWithDevice(const QString& subsystem, std::function<void(const Device&)> callback); +static void subsystem(const QString& subsystem, std::function<void()> callback); -static void deviceType(const QString& subsystem, const QString& deviceType, std::function<void(const Device&)> callback); -static void deviceType(const QString& subsystem, const QString& deviceType, std::function<void()> callback); +static void deviceTypeWithDevice(const QString& subsystem, const QString& deviceType, std::function<void(const Device&)> callback); +static void deviceType(const QString& subsystem, const QString& deviceType, std::function<void()> callback);Likely invalid or redundant comment.
shared/liboxide/udev.cpp (1)
258-263: LGTM: QDebug operator overloadThe implementation of the
operator<<overload forQDebuglooks correct and follows Qt conventions. It provides a convenient way to debugDeviceobjects.applications/system-service/powerapi.cpp (5)
3-4: Inclusion of UDev header is appropriate.Including
<liboxide/udev.h>is necessary for UDev functionality and monitoring.
14-14: Proper initialization ofm_chargerState.Initializing
m_chargerStatetoChargerUnknownensures the charger state has a defined starting value.
26-39: Conditional monitoring setup based on device type is well-implemented.The code correctly sets up UDev monitoring for RM1 devices and uses a timer for other devices, ensuring device-specific power management functionality.
🧰 Tools
🪛 cppcheck
[error] 34-34: There is an unknown macro here somewhere. Configuration is required. If Q_ENUM is a macro then please configure it.
(unknownMacro)
45-52: Destructor logic issue remains unresolved.The concern regarding the destructor logic from previous reviews is still valid. Please refer to the prior comment for details.
55-63:setEnabledmethod correctly manages monitoring based on device type.The updated
setEnabledmethod properly starts and stops the UDev monitor or timer depending on the device type and theenabledstate.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- shared/liboxide/udev.cpp (1 hunks)
🧰 Additional context used
🪛 cppcheck
shared/liboxide/udev.cpp
[error] 107-107: Unmatched '('. Configuration
(syntaxError)
[error] 107-107: Unmatched '('. Configuration
(syntaxError)
[error] 107-107: Unmatched '('. Configuration
(syntaxError)
[error] 107-107: Unmatched '('. Configuration
(syntaxError)
[error] 107-107: Unmatched '('. Configuration
(syntaxError)
[error] 107-107: Unmatched '('. Configuration
(syntaxError)
[error] 107-107: Unmatched '('. Configuration
(syntaxError)
🪛 GitHub Check: CodeFactor
shared/liboxide/udev.cpp
[notice] 185-257: shared/liboxide/udev.cpp#L185-L257
Complex Method
🔇 Additional comments (2)
shared/liboxide/udev.cpp (2)
210-283: 🛠️ Refactor suggestionConsider refactoring the
monitor()method to improve efficiencyThe
monitor()method is complex and uses aQTimerwith a short interval to poll for events, which may not be the most efficient approach.Consider using a
QSocketNotifierto monitor the file descriptor returned byudev_monitor_get_fd(mon). This allows the application to react to events as they occur without the need for polling.Here's an outline of how you could modify the
monitor()method:void UDev::monitor() { running = true; O_DEBUG("UDev::Monitor starting..."); // Setup udev monitor (mon) // ... int fd = udev_monitor_get_fd(mon); QSocketNotifier* notifier = new QSocketNotifier(fd, QSocketNotifier::Read, this); connect(notifier, &QSocketNotifier::activated, [this, mon](int) { udev_device* dev = udev_monitor_receive_device(mon); if (dev != nullptr) { Device device; device.action = getActionType(dev); device.path = QString(udev_device_get_devnode(dev) ? udev_device_get_devnode(dev) : ""); device.subsystem = QString(udev_device_get_subsystem(dev) ? udev_device_get_subsystem(dev) : ""); device.deviceType = QString(udev_device_get_devtype(dev) ? udev_device_get_devtype(dev) : ""); udev_device_unref(dev); O_DEBUG("UDev::Monitor UDev event" << device); emit event(device); } else if (errno && errno != EAGAIN) { O_WARNING("UDev::Monitor error checking event:" << strerror(errno)); } }); // Handle exit request and cleanup // ... }This approach eliminates the need for a timer and reduces CPU usage by processing events only when they are available.
223-233: Check logical condition in device type filterEnsure that the logical condition for adding device type filters is correct. The current logic may lead to unintended behavior if
deviceTypeis null or empty.Run the following script to confirm that the filters are correctly applied:
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
shared/liboxide/udev.h (1)
85-87: Consider adding a brief comment for the monitor() methodThe purpose of the
monitor()method is not immediately clear from its declaration. Consider adding a brief comment explaining its role in the class, which would improve the code's self-documentation.For example:
protected: // Handles the main monitoring loop for udev events void monitor();shared/liboxide/udev.cpp (1)
123-133: Remove redundant code inremoveMonitormethodIn the
removeMonitormethod, there is redundant code that removes thedeviceTypefrom themonitors[subsystem]list twice. This duplication is unnecessary and can be removed to simplify the code.Apply this diff to clean up the method:
void UDev::removeMonitor(QString subsystem, QString deviceType){ O_DEBUG("UDev::Removing" << subsystem << deviceType); if(monitors.contains(subsystem)){ monitors[subsystem]->removeAll(deviceType); update = true; } - auto it = monitors.find(subsystem); - if(it != monitors.end() && *it){ - (*it)->removeAll(deviceType); - update = true; - } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- shared/liboxide/udev.cpp (1 hunks)
- shared/liboxide/udev.h (1 hunks)
🧰 Additional context used
📓 Learnings (1)
shared/liboxide/udev.h (2)
Learnt from: Eeems PR: Eeems-Org/oxide#314 File: shared/liboxide/udev.h:58-61 Timestamp: 2024-10-03T01:43:10.809Z Learning: Eeems is okay with overloading functions based on `std::function` parameter types, even if it may cause ambiguity when passing lambdas or function pointers.Learnt from: Eeems PR: Eeems-Org/oxide#314 File: shared/liboxide/udev.h:0-0 Timestamp: 2024-10-03T01:42:43.130Z Learning: In the `UDev::Device` struct, the `debugString()` method intentionally returns `std::string` instead of `QString`, and this is intentional even though the rest of the class uses `QString`.
🪛 GitHub Check: CodeFactor
shared/liboxide/udev.cpp
[notice] 185-257: shared/liboxide/udev.cpp#L185-L257
Complex Method
🪛 cppcheck
shared/liboxide/udev.cpp
[error] 51-51: There is an unknown macro here somewhere. Configuration is required. If Q_PROPERTY is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (7)
shared/liboxide/udev.h (7)
1-14: LGTM: Appropriate file header and includesThe file header with doxygen-style comment and the included headers are appropriate for the implementation. Good practice to avoid
using namespace std;in the header file.
15-71: Consider differentiating overloaded method namesThe overloaded methods
subsystemanddeviceTypewith differentstd::functionparameter types might cause ambiguity when passing lambdas or function pointers. Consider renaming them to clearly differentiate their functionality, e.g.,subsystemWithDeviceandsubsystem.However, I note from the previous review that you're comfortable with this ambiguity. If you're confident in handling potential issues that may arise, you can keep the current implementation.
32-57: Acknowledged: Intentional use of std::string in debugString()The
debugString()method returnsstd::stringinstead ofQString, which is different from the rest of the class. As per the previous discussion, this is intentional and serves a specific purpose in your implementation.
72-74: LGTM: Well-defined signalsThe signals
event(const Device& device)andstopped()are well-defined and follow Qt's naming conventions. They provide appropriate notifications for device events and monitoring status changes.
76-84: LGTM: Well-structured private membersThe private members are well-structured:
- Using
QSharedPointerformonitorsensures proper memory management.- The
statelockmutex provides thread safety.- The boolean flags (
running,exitRequested,update) allow for proper state management.This implementation demonstrates good practices in managing resources and ensuring thread safety.
88-90: LGTM: Proper operator overload and metatype declarationThe QDebug operator overload for UDev::Device and the metatype declaration are well-implemented:
- The QDebug operator will facilitate logging and debugging of Device objects.
- Declaring UDev::Device as a metatype allows it to be used with Qt's signal-slot mechanism and other features of Qt's meta-object system.
These additions enhance the usability of the UDev class within a Qt environment.
1-90: Overall: Well-implemented UDev class with minor suggestionsThe UDev class is well-structured and follows good C++ and Qt practices. It provides a comprehensive interface for device management using libudev. The implementation includes proper memory management, thread safety considerations, and integration with Qt's meta-object system.
A few minor suggestions for improvement:
- Consider adding a brief comment for the
monitor()method to clarify its purpose.- While the overloaded methods for
subsystemanddeviceTypeare intentional, be aware of potential ambiguities when using lambdas or function pointers.Overall, the code is of high quality and ready for use, with these small enhancements potentially improving its maintainability and clarity.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- shared/liboxide/udev.cpp (1 hunks)
🧰 Additional context used
🪛 cppcheck
shared/liboxide/udev.cpp
[error] 51-51: There is an unknown macro here somewhere. Configuration is required. If Q_PROPERTY is a macro then please configure it.
(unknownMacro)
🪛 GitHub Check: CodeFactor
shared/liboxide/udev.cpp
[notice] 185-257: shared/liboxide/udev.cpp#L185-L257
Complex Method
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/build.yml (1 hunks)
- shared/liboxide/udev.cpp (1 hunks)
- shared/liboxide/udev.h (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- shared/liboxide/udev.h
🧰 Additional context used
📓 Learnings (1)
shared/liboxide/udev.cpp (3)
Learnt from: Eeems PR: Eeems-Org/oxide#314 File: shared/liboxide/udev.cpp:0-0 Timestamp: 2024-10-03T03:25:43.142Z Learning: In the `UDev` class, starting the thread during singleton creation is acceptable, and separating thread initialization from singleton creation is unnecessary.Learnt from: Eeems PR: Eeems-Org/oxide#314 File: shared/liboxide/udev.h:0-0 Timestamp: 2024-10-03T01:42:43.130Z Learning: In the `UDev::Device` struct, the `debugString()` method intentionally returns `std::string` instead of `QString`, and this is intentional even though the rest of the class uses `QString`.Learnt from: Eeems PR: Eeems-Org/oxide#314 File: shared/liboxide/udev.cpp:75-88 Timestamp: 2024-10-03T03:11:49.555Z Learning: In the `UDev::start()` method, unlocking `statelock` after `monitor()` runs is intentional to prevent running two instances of `monitor()`.
🪛 cppcheck
shared/liboxide/udev.cpp
[error] 51-51: There is an unknown macro here somewhere. Configuration is required. If Q_PROPERTY is a macro then please configure it.
(unknownMacro)
🪛 GitHub Check: CodeFactor
shared/liboxide/udev.cpp
[notice] 185-257: shared/liboxide/udev.cpp#L185-L257
Complex Method
🔇 Additional comments (2)
.github/workflows/build.yml (2)
Line range hint
114-115: Approve job dependency and condition changes.The changes to the 'release' job are well-implemented:
- Explicit dependencies on 'build', 'test', and 'lint' jobs ensure that releases are only created after all checks pass.
- The condition to run only on the 'master' branch maintains the integrity of the release process.
These modifications align with CI/CD best practices and improve the overall workflow structure.
62-62: Approve Python version update with a compatibility check.The update to Python 3.12 is a good practice for leveraging the latest features and security improvements. However, it's important to ensure that all dependencies and code in the project are compatible with this version.
To verify compatibility, please run the following script:
This script will help identify any Python files in the project that might use syntax specific to Python 3.12 or potentially incompatible constructs.
✅ Verification successful
Python version update to 3.12 verified successfully.
The compatibility check confirmed that the current codebase is compatible with Python 3.12. No potential incompatibilities were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for Python version-specific syntax or imports in the codebase # Search for Python files echo "Searching for Python files..." python_files=$(fd -e py) if [ -z "$python_files" ]; then echo "No Python files found in the repository." exit 0 fi # Check for Python 3.12 specific syntax or potential incompatibilities echo "Checking for potential Python 3.12 incompatibilities..." rg -n '(match.*case|type\s+\w+\s*=|def.*->\s*\w+\[)' $python_files echo "Please review any matches found above for potential incompatibilities with Python 3.12." echo "If no matches are found, it's likely that the codebase is compatible with Python 3.12."Length of output: 4676
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
applications/system-service/powerapi.cpp (3)
Line range hint
14-39: LGTM: Device-specific power monitoring setupThe implementation of device-specific power monitoring setup is well-structured and aligns with the PR objective. The use of UDev for RM1 devices and the existing timer-based approach for other devices maintains compatibility while introducing the new functionality.
Consider extracting the device-specific setup into separate private methods for improved readability:
private: void setupRM1Monitoring() { Oxide::UDev::singleton()->addMonitor("platform", NULL); Oxide::UDev::singleton()->subsystem("power_supply", [this]{ update(); }); } void setupOtherDeviceMonitoring() { timer = new QTimer(this); timer->setSingleShot(false); timer->setInterval(3 * 1000); // 3 seconds timer->moveToThread(qApp->thread()); connect(timer, &QTimer::timeout, this, QOverload<>::of(&PowerAPI::update)); timer->start(); }Then, in the constructor:
if(deviceSettings.getDeviceType() == Oxide::DeviceSettings::RM1) { setupRM1Monitoring(); } else { setupOtherDeviceMonitoring(); }This refactoring would improve code organization and make the constructor more concise.
🧰 Tools
🪛 cppcheck
[error] 34-34: There is an unknown macro here somewhere. Configuration is required. If Q_ENUM is a macro then please configure it.
(unknownMacro)
45-52: LGTM: Updated destructor for proper resource cleanupThe destructor now correctly handles cleanup for both timer-based and UDev-based monitoring, which is essential for proper resource management.
For consistency and clarity, consider using the device type check similar to the constructor:
PowerAPI::~PowerAPI() { if(deviceSettings.getDeviceType() == Oxide::DeviceSettings::RM1) { qDebug() << "Stopping UDev monitor"; Oxide::UDev::singleton()->stop(); } else if(timer != nullptr) { qDebug() << "Stopping timer"; timer->stop(); delete timer; } }This approach makes the code more explicit about the device-specific cleanup and aligns with the initialization logic in the constructor.
55-63: LGTM: Updated setEnabled method for device-specific handlingThe
setEnabledmethod now correctly handles both RM1 and other device types, which is consistent with the changes in the constructor and destructor.To improve consistency and reduce nesting, consider restructuring the method as follows:
void PowerAPI::setEnabled(bool enabled) { if(deviceSettings.getDeviceType() == Oxide::DeviceSettings::RM1) { enabled ? Oxide::UDev::singleton()->start() : Oxide::UDev::singleton()->stop(); } else { enabled ? timer->start() : timer->stop(); } }This structure maintains the device-specific logic while reducing the number of nested conditions, making the code more readable and consistent with the other methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/build.yml (2 hunks)
- applications/system-service/powerapi.cpp (3 hunks)
- shared/liboxide/udev.cpp (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build.yml
🧰 Additional context used
📓 Learnings (2)
applications/system-service/powerapi.cpp (1)
Learnt from: Eeems PR: Eeems-Org/oxide#314 File: applications/system-service/powerapi.cpp:37-37 Timestamp: 2024-10-03T03:12:01.946Z Learning: When `cppcheck` reports an unknown macro (e.g., `Q_ENUM`), guide the user to define the macro in a configuration file or via command-line options to suppress the warning.shared/liboxide/udev.cpp (3)
Learnt from: Eeems PR: Eeems-Org/oxide#314 File: shared/liboxide/udev.cpp:0-0 Timestamp: 2024-10-03T03:25:43.142Z Learning: In the `UDev` class, starting the thread during singleton creation is acceptable, and separating thread initialization from singleton creation is unnecessary.Learnt from: Eeems PR: Eeems-Org/oxide#314 File: shared/liboxide/udev.h:0-0 Timestamp: 2024-10-03T01:42:43.130Z Learning: In the `UDev::Device` struct, the `debugString()` method intentionally returns `std::string` instead of `QString`, and this is intentional even though the rest of the class uses `QString`.Learnt from: Eeems PR: Eeems-Org/oxide#314 File: shared/liboxide/udev.cpp:75-88 Timestamp: 2024-10-03T03:11:49.555Z Learning: In the `UDev::start()` method, unlocking `statelock` after `monitor()` runs is intentional to prevent running two instances of `monitor()`.
🪛 cppcheck
applications/system-service/powerapi.cpp
[error] 34-34: There is an unknown macro here somewhere. Configuration is required. If Q_ENUM is a macro then please configure it.
(unknownMacro)
shared/liboxide/udev.cpp
[error] 51-51: There is an unknown macro here somewhere. Configuration is required. If Q_PROPERTY is a macro then please configure it.
(unknownMacro)
🪛 GitHub Check: CodeFactor
shared/liboxide/udev.cpp
[notice] 185-257: shared/liboxide/udev.cpp#L185-L257
Complex Method
🔇 Additional comments (13)
applications/system-service/powerapi.cpp (3)
3-4: LGTM: UDev header inclusionThe addition of
#include <liboxide/udev.h>is appropriate for implementing udev monitoring functionality for the rM1 device, as mentioned in the PR objectives.
37-37: Note: Static analysis warning for Q_ENUM macroThe static analysis tool (cppcheck) reports an unknown macro warning for
Q_ENUM. As per our previous discussion, we've decided to ignore this warning for now. If you want to suppress this warning in the future, you can consider defining the macro in a configuration file or via command-line options when running cppcheck.
Line range hint
1-255: Overall assessment: Solid implementation of udev monitoring for rM1The changes implemented in this file successfully introduce udev monitoring for the rM1 device while maintaining compatibility with other device types. The code is well-structured and aligns with the PR objectives.
Key points:
- Device-specific initialization in the constructor
- Proper resource cleanup in the destructor
- Updated
setEnabledmethod to handle both device typesThe suggested refactorings aim to improve code organization, readability, and consistency. These are minor suggestions and do not impact the overall functionality of the implementation.
Great job on addressing the challenges mentioned in the PR comments, particularly regarding the implementation of udev monitoring and avoiding deadlocks.
🧰 Tools
🪛 cppcheck
[error] 34-34: There is an unknown macro here somewhere. Configuration is required. If Q_ENUM is a macro then please configure it.
(unknownMacro)
shared/liboxide/udev.cpp (10)
13-21: Proper Implementation of Thread-Safe Singleton PatternThe
singleton()method correctly usesstd::call_onceandstd::once_flagto ensure thread-safe initialization of theUDevinstance, adhering to modern C++ standards.
23-34: Thread Initialization and Object Ownership Managed CorrectlyThe constructor initializes the udev library, starts the thread with low priority, and moves the
UDevobject to the thread. Signal-slot connections for thread started and finished signals are set up appropriately.
43-65: Callback Registration Methods Are Well-DefinedThe overloaded
subsystemanddeviceTypemethods provide flexible ways to register callbacks for device events, handling both cases where device details are needed and where they are not.🧰 Tools
🪛 cppcheck
[error] 51-51: There is an unknown macro here somewhere. Configuration is required. If Q_PROPERTY is a macro then please configure it.
(unknownMacro)
74-88: Start Method Correctly Manages State and Prevents Duplicate MonitorsThe
start()method locks thestatelock, verifies whether the monitoring is already running, and starts themonitor()method appropriately, ensuring that only one instance runs.
101-108: Efficient Waiting Mechanism Using Event LoopThe
wait()method uses aQEventLoopto wait for thestoppedsignal, efficiently blocking until the monitoring has stopped without consuming unnecessary CPU resources.
110-128: Monitor Management Methods Correctly Update Monitoring FiltersThe
addMonitorandremoveMonitormethods efficiently manage themonitorsmap and set theupdateflag appropriately to refresh filters when needed.
130-178: Proper Error Handling and Resource Management ingetDeviceList()The
getDeviceList()method includes necessary null checks and error handling. It ensures that udev enumerations and devices are unreferenced properly to prevent memory leaks.
180-205: Accurate Mapping of Action TypesThe
getActionType()methods accurately map action strings to theActionTypeenum, handling unknown actions gracefully.
207-281: Device Monitoring Loop Implemented CorrectlyThe
monitor()method sets up the udev monitor, applies filters based on themonitorsmap, and uses aQTimerfor periodic checking. It handles events, emits signals appropriately, and manages cleanup on exit.
283-288: Overloaded QDebug Operator Enhances DebuggingThe
operator<<overload forQDebugallows for convenient logging ofDeviceinstances, improving debuggability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
applications/system-service/powerapi.cpp (3)
Line range hint
14-39: LGTM: Device-specific power monitoring setupThe constructor now correctly implements device-specific power monitoring:
- For RM1 devices, UDev monitoring is set up for the "platform" and "power_supply" subsystem.
- For other devices, the existing timer-based approach is retained.
This implementation aligns well with the PR objective of introducing udev monitoring for rM1 devices.
Consider extracting the device-specific setup into separate private methods for improved readability:
void PowerAPI::setupRM1Monitoring() { Oxide::UDev::singleton()->addMonitor("platform", NULL); Oxide::UDev::singleton()->subsystem("power_supply", [this]{ QMetaObject::invokeMethod(this, "update", Qt::QueuedConnection); }); } void PowerAPI::setupNonRM1Monitoring() { timer = new QTimer(this); timer->setSingleShot(false); timer->setInterval(3 * 1000); // 3 seconds timer->moveToThread(qApp->thread()); connect(timer, &QTimer::timeout, this, QOverload<>::of(&PowerAPI::update)); timer->start(); }Then, in the constructor:
if (deviceSettings.getDeviceType() == Oxide::DeviceSettings::RM1) { setupRM1Monitoring(); } else { setupNonRM1Monitoring(); }This refactoring would improve code organization and make the constructor more concise.
🧰 Tools
🪛 cppcheck
[error] 34-34: There is an unknown macro here somewhere. Configuration is required. If Q_ENUM is a macro then please configure it.
(unknownMacro)
45-52: LGTM: Updated destructor for proper resource cleanupThe destructor now correctly handles resource cleanup for both RM1 (UDev) and non-RM1 (timer) devices. This change ensures proper shutdown of monitoring mechanisms.
For improved clarity, consider explicitly checking the device type:
PowerAPI::~PowerAPI() { if (deviceSettings.getDeviceType() == Oxide::DeviceSettings::RM1) { qDebug() << "Stopping UDev monitor"; Oxide::UDev::singleton()->stop(); } else if (timer != nullptr) { qDebug() << "Stopping timer"; timer->stop(); delete timer; } }This approach makes the intent clearer and reduces reliance on the implicit assumption about the timer's state.
55-63: LGTM: Updated setEnabled method for device-specific handlingThe
setEnabledmethod now correctly handles both RM1 (UDev) and non-RM1 (timer) devices, ensuring proper enabling/disabling of the respective monitoring mechanisms.Consider restructuring the method to improve readability:
void PowerAPI::setEnabled(bool enabled) { if (deviceSettings.getDeviceType() == Oxide::DeviceSettings::RM1) { enabled ? Oxide::UDev::singleton()->start() : Oxide::UDev::singleton()->stop(); } else { enabled ? timer->start() : timer->stop(); } }This structure eliminates the early return and presents a more unified approach to handling both device types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- applications/system-service/powerapi.cpp (3 hunks)
🧰 Additional context used
📓 Learnings (1)
applications/system-service/powerapi.cpp (1)
Learnt from: Eeems PR: Eeems-Org/oxide#314 File: applications/system-service/powerapi.cpp:37-37 Timestamp: 2024-10-03T03:12:01.946Z Learning: When `cppcheck` reports an unknown macro (e.g., `Q_ENUM`), guide the user to define the macro in a configuration file or via command-line options to suppress the warning.
🪛 cppcheck
applications/system-service/powerapi.cpp
[error] 34-34: There is an unknown macro here somewhere. Configuration is required. If Q_ENUM is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (3)
applications/system-service/powerapi.cpp (3)
3-4: LGTM: UDev header inclusionThe addition of
#include <liboxide/udev.h>is appropriate for implementing udev monitoring functionality for the rM1 device, as mentioned in the PR objectives.
37-37: Note: Static analysis warning for Q_ENUM macroThe static analysis tool (cppcheck) reports an unknown macro warning for
Q_ENUM. This is likely a false positive asQ_ENUMis a Qt macro.As per your previous preference to ignore this warning, we'll skip addressing it for now. If you decide to suppress this warning in the future, you can add the following line to your cppcheck configuration or command:
--suppress=unknownMacro:*Q_ENUM*This will prevent cppcheck from reporting this specific warning for Q_ENUM macros.
Line range hint
1-231: Overall assessment: Well-implemented udev monitoring for rM1The changes in this file successfully implement udev monitoring for rM1 devices while maintaining compatibility with other device types. Key points:
- Proper inclusion of UDev header.
- Device-specific initialization in the constructor.
- Correct resource cleanup in the destructor.
- Updated
setEnabledmethod to handle both device types.The implementation aligns well with the PR objectives. Minor suggestions for code improvement have been provided, but they are not critical for functionality.
Great job on implementing this feature, especially considering the challenges with deadlocks and lack of comprehensive libudev documentation mentioned in the PR comments.
🧰 Tools
🪛 cppcheck
[error] 34-34: There is an unknown macro here somewhere. Configuration is required. If Q_ENUM is a macro then please configure it.
(unknownMacro)
Summary by CodeRabbit
New Features
UDevclass for managing device events, enabling monitoring of device changes without blocking the main application.Bug Fixes
Documentation
UDevclass and its methods for better user understanding.Chores