Device controls support #41
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 @pacjo, 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 introduces a significant new feature by integrating Android 11's Device Controls API. This allows users to conveniently control their WLED devices, specifically power and brightness, directly from the system's quick access controls, enhancing usability and accessibility for Android 11 and higher users.
Highlights
- Android Device Controls Integration: I've added support for Android 11+ Device Controls, allowing users to manage WLED devices directly from their quick settings or power menu.
- Power and Brightness Control: The implementation provides controls for toggling device power and adjusting brightness for connected WLED devices.
- Dynamic Device State: The controls dynamically reflect the online/offline status and current power/brightness state of WLED devices, updating in real-time.
- New Service Implementation: A new
DeviceControlsProviderServicehas been introduced to handle the communication between the Android system's Device Controls UI and the application's device management logic.
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.
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 adds support for Android 11's Device Controls. I've found a critical issue in the control action handling that could lead to incorrect responses being sent to the system. I've also identified several areas for improvement regarding code duplication, use of hardcoded strings, and the generation of unique IDs for PendingIntents. My suggestions aim to improve the code's robustness and maintainability.
app/src/main/java/ca/cgagnier/wlednativeandroid/service/DeviceControlsProviderService.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/ca/cgagnier/wlednativeandroid/service/DeviceControlsProviderService.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/ca/cgagnier/wlednativeandroid/service/DeviceControlsProviderService.kt
Outdated
Show resolved
Hide resolved
| */ | ||
| private fun createAppIntentForDevice(device: Device): PendingIntent { | ||
| // TODO: we can just remove everything but numbers and be left with a string of a unique integer | ||
| val integerId = device.macAddress.hashCode() |
There was a problem hiding this comment.
Using String.hashCode() for the PendingIntent request code is not ideal. Hash codes are not guaranteed to be unique, which could lead to collisions where different devices get the same PendingIntent, causing unexpected behavior.
A more robust approach is to generate a unique integer from the device's MAC address.
val integerId = try {
// Use the last 6 characters of the MAC address, which should be unique enough for this purpose.
device.macAddress.replace(":", "").takeLast(6).toInt(16)
} catch (e: Exception) {
// Fallback to hashCode if parsing fails for some reason (e.g. invalid MAC format)
device.macAddress.hashCode()
}| private fun toggleDevicePower(device: Device, isOn: Boolean) { | ||
| val deviceSetPost = JsonPost(isOn = isOn) | ||
| scope.launch(Dispatchers.IO) { | ||
| stateFactory.getState(device).requestsManager.addRequest( | ||
| StateChangeRequest(device, deviceSetPost) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
This method is copied from DeviceListViewModel, which introduces code duplication. This should be refactored by extracting the device action logic into a shared component (e.g., a DeviceActionManager or a UseCase) that can be injected into both the ViewModel and this Service.
Additionally, this function is called from a coroutine that already runs on Dispatchers.IO. The inner scope.launch(Dispatchers.IO) is redundant and creates an unnecessary nested coroutine. You can remove the inner launch.
private fun toggleDevicePower(device: Device, isOn: Boolean) {
val deviceSetPost = JsonPost(isOn = isOn)
stateFactory.getState(device).requestsManager.addRequest(
StateChangeRequest(device, deviceSetPost)
)
}There was a problem hiding this comment.
duplicated scope.launch removed, I'm leaving those functions in for now
| private fun setDeviceBrightness(device: Device, brightness: Int) { | ||
| val deviceSetPost = JsonPost(brightness = brightness) | ||
| scope.launch(Dispatchers.IO) { | ||
| stateFactory.getState(device).requestsManager.addRequest( | ||
| StateChangeRequest(device, deviceSetPost) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
This method is copied from DeviceListViewModel, which introduces code duplication. This should be refactored by extracting the device action logic into a shared component (e.g., a DeviceActionManager or a UseCase) that can be injected into both the ViewModel and this Service.
Additionally, this function is called from a coroutine that already runs on Dispatchers.IO. The inner scope.launch(Dispatchers.IO) is redundant and creates an unnecessary nested coroutine. You can remove the inner launch.
private fun setDeviceBrightness(device: Device, brightness: Int) {
val deviceSetPost = JsonPost(brightness = brightness)
stateFactory.getState(device).requestsManager.addRequest(
StateChangeRequest(device, deviceSetPost)
)
}There was a problem hiding this comment.
duplicated scope.launch removed, I'm leaving those functions in for now
(remove from the toggleDevicePower and setDeviceBrightness functions, now only present in performControlAction)
|
I addressed some of the gemini's review. Duplicated functions are left (but modified) and I didn't touch interesting wording used by AIWhen skimming through gemeni's messages I saw this beauty:
you haven't added anything. This wording should have never shown up. Waiting for a non-AI review. |
|
I haven't checked out the code yet but I just compiled your branch and it works great! Speaking about the number, I would prefer it using percent (0%-100%) instead of the brightness value that you see in WLED because I think it's a cleaner approach for this part of the Android UI. I am further of the opinion that getting straight into the details page by long pressing the tile is an absolute must for this feature. Thanks for now, great idea! 😁 |
Glad to hear.
it's just 128, but I might have set it to display as float. This will change when moving to 0-100% scale.
I'll make an attempt too. It's not an absolute must for me, but it sure would be nice to have. |
|
Alright, I've fixed the format in device controls view and added navigation to specific device. Other then that there is still some duplicated code in |
|
Wow, this is great! I will have to test it myself (might take a while, sorry) but I didn't think this was possible for local controls! It's pretty cool. What happens when you are away from home, I assume it says unavailable or something like that? Related to the AI, feel free to ignore it, but I find it nice to have a summary and initial look, even though it's not super accurate for reviews. |
Awesome. Take as much time as you need.
Yea, Offline to be specific.
Well, it got things right. It's just that most of those already had a note attached. |
|
I personally approve of the 0-100% range. It's funny because I did almost the exact same changes on my local branch, but yours are a bit cleaner. |
❤️
I'm having the same issues on my main strip, but that one is running on ESP01 so not exactly a recommended configuration. I did setup another one with ESP32 to test multiple devices and I don't recall having problems there. Not sure how to troubleshoot this either. |
3 participants
Hi,
this PR adds support for Device Controls introduced with Android 11.
Code is mostly just modified example from https://developer.android.com/develop/ui/views/device-control
It's pretty much ready to merge, a few things though:
there are some hardcoded strings in theDeviceControlsProviderService, I'll remove them later todaystrings-fr.xmlsince I don't know the language well enough to do thatI'm not sure weather to show 0-255 or 0%-100% in the Device controls (there's aTODOfor this in the code) - asking for your opinion herelong-pressing the device in the device controls view doesn't open device specific screen (I'm not sure how to achieve that in a simple way and didn't want to mess with UI code much, maybe can be implemented later) (there are twoTODOs for this in the code)MainActivity, but it only shows up for a split second before turning transparent. this happens before and after changes in this PS and I don't know whats causing itprobably closes #21 (at least for people on Android 11 and higher) and helps with #17