New libqsgepaper and initial rMPP support #395
Conversation
📝 WalkthroughWalkthroughThis pull request updates several components related to display and device settings. In the display server, the rectangle data used for GUI updates now comes from an auxiliary buffer rather than the main framebuffer, and framebuffer update methods have been simplified. The EPAPER feature has been removed from the main configuration. Additionally, dynamic screen dimensions are now used across multiple modules, and support for a new device type ("reMarkable Paper Pro") has been added. The ePaper library’s build configuration and EPFramebuffer API have been restructured to accommodate these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant DBusInterface
participant GUIThread
participant EPFramebuffer
participant Driver
%% Display update flow initiated from a DBus event
DBusInterface->>GUIThread: Trigger exitExclusiveMode/exclusiveModeRepaint
GUIThread->>EPFramebuffer: Retrieve auxBuffer rectangle data
EPFramebuffer-->>GUIThread: Return rectangle dimensions
GUIThread->>Driver: Call swapBuffers(Color, QualityMode, CompleteRefresh)
Poem
✨ Finishing Touches
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 (
|
asivery
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
shared/liboxide/devicesettings.cpp (1)
167-172: Fix inconsistent indentation.The indentation in the
getTouchHeight()method is inconsistent after adding the RMPP case. Thedefault:case and subsequent lines should be aligned with the other cases.case DeviceType::RMPP: return 2832; - default: - return 0; - } - } + default: + return 0; + } + }shared/liboxide/eventfilter.cpp (1)
14-15: Consider replacing macros with inline functions.
Using macros forDISPLAYWIDTHandDISPLAYHEIGHTcan reduce type safety and hinder debugging. Inline functions or static constants can serve the same purpose while offering clearer semantics.-#define DISPLAYWIDTH (deviceSettings.getScreenWidth()) -#define DISPLAYHEIGHT (deviceSettings.getScreenHeight()) +static inline int displayWidth() { + return deviceSettings.getScreenWidth(); +} +static inline int displayHeight() { + return deviceSettings.getScreenHeight(); +}applications/display-server/guithread.cpp (1)
76-78: Offer to help with memfd_create implementation.
The TODO suggests a plan to switch from/dev/nullto a dynamically created file descriptor. Let me know if you'd like assistance implementing this feature.shared/epaper/epframebuffer.h (4)
42-42: Factory-method pointer return.
Returning a raw pointer is typical in singletons/factory methods, but consider returning by reference or using a smart pointer if you need stronger lifetime guarantees.
47-48: Add documentation for newly introduced methods.
Theseinitialize()andsync()methods are self-explanatory, but additional doc comments clarifying their usage and preconditions might help maintainability.
52-68: Check layout consistency inEPFramebufferAcep2.
This class relies on carefully placed buffers and the size ofQImage. Changes in Qt versions or compilers may cause alignment or size-differences, leading to potential memory corruption.If you’d like, I can provide a shell script to search for any references that might incorrectly cast or offset these buffer members.
70-85: Ensure robust handling ofEPFramebufferFusionmemory layout.
Similar toEPFramebufferAcep2, this class uses partially opaque data and members relying on specific offset sizes. Confirm thatsizeof(QImage)is stable on ARM builds to avoid out-of-bounds access.Would you like me to generate a web search query to investigate
QImagesize consistency across Qt builds?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
shared/epaper/libqsgepaper.sois excluded by!**/*.so
📒 Files selected for processing (13)
applications/display-server/dbusinterface.cpp(2 hunks)applications/display-server/guithread.cpp(5 hunks)applications/display-server/main.cpp(0 hunks)applications/settings-manager/main.cpp(1 hunks)applications/system-service/apibase.cpp(1 hunks)applications/system-service/screenapi.h(1 hunks)shared/epaper/epaper.pro(1 hunks)shared/epaper/epframebuffer.h(3 hunks)shared/liboxide/devicesettings.cpp(4 hunks)shared/liboxide/devicesettings.h(2 hunks)shared/liboxide/eventfilter.cpp(1 hunks)shared/qpa/oxideeventfilter.cpp(2 hunks)shared/qpa/oxideintegration.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- applications/display-server/main.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (22)
shared/liboxide/devicesettings.h (2)
29-30: LGTM: Device type enum updated correctly.The addition of the RMPP device type to the enum with proper documentation looks good.
78-87: LGTM: New screen dimension methods well documented.The new methods for retrieving screen dimensions are properly documented and follow the existing code style pattern.
applications/system-service/screenapi.h (1)
20-23: Good improvement: Dynamic screen dimensions.Replacing hardcoded screen dimensions with dynamic values from deviceSettings improves flexibility and allows for better support of different device types.
shared/qpa/oxideeventfilter.cpp (2)
9-9: LGTM: Required header added.Adding the deviceSettings header is appropriate for accessing the screen dimension methods.
88-89: Good improvement: Dynamic screen dimensions.Replacing hardcoded dimensions with dynamic values from deviceSettings enhances device compatibility and maintainability.
shared/liboxide/devicesettings.cpp (5)
128-129: LGTM: Device name added appropriately.Adding the proper display name for the new device type is good.
154-155: LGTM: Touch dimensions added for new device.Touch input dimensions added correctly for the RMPP device.
174-196: LGTM: Screen dimension methods implemented correctly.The implementations for
getScreenWidth()andgetScreenHeight()look good, providing appropriate values for each device type.
106-110:Details
❓ Verification inconclusive
Verify device identification method.
Using "reMarkable Ferrari" to identify the RMPP device seems like a development codename rather than a production identifier. Consider if this is the final identification string that will be used in production.
🏁 Script executed:
#!/bin/bash # Check if there are any references to the final device name in the codebase echo "Searching for references to reMarkable Ferrari and other device identifiers..." rg -A 2 -B 2 "reMarkable Ferrari" --glob "!*.md"Length of output: 563
Action: Confirm the Device Identifier
In
shared/liboxide/devicesettings.cpp(lines 106–110), the code checks if the model name contains"reMarkable Ferrari". This string appears to be a development codename rather than a finalized production identifier for the RMPP device. Please verify whether"reMarkable Ferrari"is indeed the intended identifier for production or if it should be updated to reflect the correct final device name.
141-142:Details
❓ Verification inconclusive
Verify touch environment setting for RMPP.
The RMPP device has an empty touch environment setting. Confirm if this is intentional or if specific settings are needed for proper touch input orientation.
🏁 Script executed:
#!/bin/bash # Look for any rotation or inversion settings for RMPP elsewhere in the codebase echo "Searching for touch orientation settings for RMPP..." rg -A 3 -B 3 "RMPP.*rotate|rotate.*RMPP" --glob "!*.md" rg -A 3 -B 3 "touchscreen.*orientation|orientation.*touchscreen" --glob "!*.md"Length of output: 254
I'll need to gather a bit more context before finalizing the review comment. Could you please run an additional verification script to further search for any RMPP-specific touch or orientation configurations in the codebase? For example, a script that searches for general mentions of "RMPP" and entries that might involve "touch" or "orientation" might shed more light on whether the empty setting for RMPP is intentional.
Below is a proposed script for re-verification:
Once you have the output from this script, we can better determine if the empty touch environment setting for the RMPP device is deliberate or if further adjustments are needed.
🏁 Script executed:
[run_scripts] #!/bin/bash echo "Searching for any mention of RMPP in the codebase..." rg "RMPP" --glob "!*.md" echo "Searching for RMPP touch related settings..." rg -A 5 -B 5 "RMPP.*touch" --glob "!*.md"Length of output: 1060
Action Required: Verify RMPP Touch Configuration
After searching the codebase, we found several references to
RMPPinshared/liboxide/devicesettings.cppandshared/liboxide/devicesettings.h, but no evidence of any dedicated touch orientation or inversion settings for this device type. This suggests that returning an empty string for the RMPP touch environment might be intentional.
- The code snippet in question:
case DeviceType::RMPP: return "";- Our searches for keywords like
rotate,touch, andorientationin relation to RMPP did not reveal any additional configurations.- Next Step: Please verify against the product specifications or with the relevant stakeholders whether the empty touch environment setting for RMPP is by design or if specific parameters (e.g., rotation or inversion settings) should be applied.
shared/liboxide/eventfilter.cpp (1)
12-12: No issues with new include.
This header inclusion is necessary for accessingdeviceSettings.applications/display-server/guithread.cpp (4)
64-64: Validate auxiliary buffer geometry.
Before creatingGUIThreadwithauxBuffer.rect(), consider handling edge cases where the auxiliary buffer might not be initialized or has a zero-size rectangle. This helps prevent potential runtime errors.
79-79: Ensure correct usage of /dev/null.
Opening/dev/nullis likely a temporary placeholder and won't provide meaningful read/write operations for a framebuffer. Confirm that this usage is intentional and doesn't break any downstream logic.
179-185: Clear function logic looks consistent.
Clearing the auxiliary buffer with white and issuing a complete refresh appears correct. Verify whether additional error handling is needed forswapBuffers()failures.
230-230: Aux buffer usage confirmed.
Switching toauxBufferis aligned with the new approach. No issues here.shared/epaper/epaper.pro (4)
4-4: TEMPLATE changed to aux.
Switching fromlibtoauxis unorthodox. Ensure this aligns with the intended build artifacts and doesn't break downstream usage.
6-7: Copying .so and header together could be fragile.
Copying the library file in the same command as the header may cause unexpected issues if either path changes or if the shared object isn't built yet. Confirm build order or consider separate targets.
11-11: Installing multiple files.
Including both the header and the.soinepframebuffer_h_install.filesbroadens deployment scope. Double-check that this matches your packaging requirements and doesn't omit versioned library handling.
16-17: Review new dependencies and clean rules.
PRE_TARGETDEPSandQMAKE_CLEANnow include theraw_copytarget. Ensure these additions align with your build steps and that cleaning removes all generated artifacts.shared/epaper/epframebuffer.h (3)
11-13: Validate newly introduced macros for reMarkable 2.
These macros are derived from disassembly and may change in future library updates. Consider maintaining them in a single source of truth or verifying them dynamically to avoid misalignment.If you’d like, I can provide a shell script or a web search query to confirm these offsets match the actual structure size.
15-19: Confirm architecture detection logic.
The definition ofDEFAULT_EPFR_RETURNdiffers across__aarch64__and the fallback. Ensure that reMarkable 2 builds consistently match the intended macro.
40-40: Signature looks good.
The revised parameter list and default argument appear consistent with usage patterns. No issues spotted.
| return nullptr; | ||
| } | ||
| image = new QImage(data, 1404, 1872, stride, QImage::Format_RGB16); | ||
| image = new QImage(data, deviceSettings.getScreenWidth(), deviceSettings.getScreenHeight(), stride, QImage::Format_RGB16); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded dimensions with dynamic screen size
This change improves device compatibility by using dynamic screen dimensions from the device settings rather than hardcoded values. This is particularly important for supporting different device types with varying screen resolutions.
| EPFramebuffer::instance()->auxBuffer.rect(), | ||
| Blight::HighQualityGrayscale, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Switch from main framebuffer to auxiliary buffer
The code now uses the auxiliary buffer's rectangle data instead of the main framebuffer. This change is consistent with the broader refactoring of the display update system in this PR.
Verify that this change correctly coordinates with the rest of the framebuffer system changes to ensure proper rendering.
| EPFramebuffer::instance()->auxBuffer.rect(), | ||
| Blight::HighQualityGrayscale, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use auxiliary buffer for exclusive mode repaint
Similar to the previous change, this modification switches from using the main framebuffer to the auxiliary buffer for repaint operations in exclusive mode. This ensures consistency with the other display buffer changes in this PR.
| ? EPScreenMode::QualityFull | ||
| : EPScreenMode::QualityFast; | ||
| auto flag = rect == m_screenRect | ||
| ? EPFramebuffer::UpdateFlag::CompleteRefresh | ||
| : EPFramebuffer::UpdateFlag::NoRefresh; | ||
| EPFramebuffer::instance()->swapBuffers( | ||
| m_screenGeometry, | ||
| EPContentType::Mono, | ||
| mode, | ||
| flag | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine update modes for partial refresh.
Currently, partial updates always use EPScreenMode::QualityFast. This might lead to ghosting or performance trade-offs on ePaper devices. Consider a more granular approach or user-controlled waveforms to optimize refresh quality.
There was a problem hiding this comment.
@asivery with the c-abi changes, you should now have more information for what refresh mode was requested at this point, so you can use that instead of hardcoding a single refresh mode value.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| return nullptr; | ||
| } | ||
| image = new QImage(data, 1404, 1872, stride, QImage::Format_RGB16); | ||
| image = new QImage(data, deviceSettings.getScreenWidth(), deviceSettings.getScreenHeight(), stride, QImage::Format_RGB16); |
There was a problem hiding this comment.
Stride will need to take into account rMPP screen sizes here too
|
|
||
| epframebuffer_h.target = epframebuffer.h | ||
| epframebuffer_h.commands = cp $$PWD/epframebuffer.h $$OUT_PWD | ||
| epframebuffer_h.target = raw_copy |
There was a problem hiding this comment.
I don't see raw_copy defined here. Shouldn't it be what was here before?
|
|
||
| linux-oe-g++{ | ||
| epframebuffer_h_install.files = epframebuffer.h | ||
| epframebuffer_h_install.files = epframebuffer.h libqsgepaper.so |
There was a problem hiding this comment.
Libqsgepaper should be it's own install that places it in the correct location, right now this is placing it in /opt/include instead of /opt/lib/plugins/scenegraph
There was a problem hiding this comment.
I would love to remove this from the repo and pull it from the toolchain instead.
| _deviceType = DeviceType::RM2; | ||
| return; | ||
| } | ||
| if (modelName.contains("reMarkable Ferrari")) { |
There was a problem hiding this comment.
This should be startswith as Not reMarkable Ferrari should not match. You could also do direct comparisons assuming there are no newlines in the value.
|
@asivery I just merged the c abi branch, luckily there are no conflicts! |
2 participants
The pen input device is flipped, and refreshes are slow, but other than that, it works :)
Summary by CodeRabbit
New Features
Bug Fixes
These improvements provide a more reliable and visually optimized experience across different devices.