Re: [PR] drivers/leds: Add support for KTD2052 [nuttx]
acassis merged PR #16217: URL: https://github.com/apache/nuttx/pull/16217 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/leds: Add support for KTD2052 [nuttx]
acassis commented on PR #16217: URL: https://github.com/apache/nuttx/pull/16217#issuecomment-3032674404 > @acassis done. Sorry about that, selected the wrong file from a merge conflict. Thought I selected the one that had those fixes. Don't worry, it happens with me all the time! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/leds: Add support for KTD2052 [nuttx]
stbenn commented on PR #16217: URL: https://github.com/apache/nuttx/pull/16217#issuecomment-3032648660 @acassis done. Sorry about that, selected the wrong file from a merge conflict. Thought I selected the one that had those fixes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/leds: Add support for KTD2052 [nuttx]
acassis commented on PR #16217: URL: https://github.com/apache/nuttx/pull/16217#issuecomment-3032637423 @stbenn please fix these issues: Error: /home/runner/work/nuttx/nuttx/nuttx/drivers/leds/ktd2052.c:85:79: error: Long line found Error: /home/runner/work/nuttx/nuttx/nuttx/drivers/leds/ktd2052.c:89:81: error: Long line found Error: /home/runner/work/nuttx/nuttx/nuttx/drivers/leds/ktd2052.c:135:79: error: Long line found Error: /home/runner/work/nuttx/nuttx/nuttx/drivers/leds/ktd2052.c:208:80: error: Long line found Error: /home/runner/work/nuttx/nuttx/nuttx/drivers/leds/ktd2052.c:240:81: error: Long line found Error: /home/runner/work/nuttx/nuttx/nuttx/drivers/leds/ktd2052.c:241:80: error: Long line found Error: /home/runner/work/nuttx/nuttx/nuttx/drivers/leds/ktd2052.c:249:78: error: Long line found Error: /home/runner/work/nuttx/nuttx/nuttx/drivers/leds/ktd2052.c:384:79: error: Long line found Error: /home/runner/work/nuttx/nuttx/nuttx/drivers/leds/ktd2052.c:458:81: error: Long line found Error: /home/runner/work/nuttx/nuttx/nuttx/drivers/leds/ktd2052.c:524:79: error: Long line found Error: /home/runner/work/nuttx/nuttx/nuttx/drivers/leds/ktd2052.c:553:78: error: Long line found Error: /home/runner/work/nuttx/nuttx/nuttx/drivers/leds/ktd2052.c:572:80: error: Long line found Error: /home/runner/work/nuttx/nuttx/nuttx/drivers/leds/ktd2052.c:639:78: error: Long line found Error: /home/runner/work/nuttx/nuttx/nuttx/drivers/leds/ktd2052.c:640:78: error: Long line found Error: /home/runner/work/nuttx/nuttx/nuttx/drivers/leds/ktd2052.c:648:80: error: Long line found -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/leds: Add support for KTD2052 [nuttx]
stbenn commented on PR #16217: URL: https://github.com/apache/nuttx/pull/16217#issuecomment-3032622425 @acassis documentation has been added for this driver -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/leds: Add support for KTD2052 [nuttx]
stbenn commented on PR #16217: URL: https://github.com/apache/nuttx/pull/16217#issuecomment-2807685399 @acassis, sounds good to me 👍 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/leds: Add support for KTD2052 [nuttx]
acassis commented on PR #16217: URL: https://github.com/apache/nuttx/pull/16217#issuecomment-2807683774 @stbenn we can merge it, but I think it is better wait for your commit with Documentation and apps example, it avoids we losing track of it. I will move this PR to draft, ok? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/leds: Add support for KTD2052 [nuttx]
stbenn commented on PR #16217: URL: https://github.com/apache/nuttx/pull/16217#issuecomment-2807474176 @acassis I can add those, but it may be a little bit until I get around to it. If you would prefer to wait on this PR until those are implemented, that is understandable. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/leds: Add support for KTD2052 [nuttx]
xiaoxiang781216 commented on code in PR #16217: URL: https://github.com/apache/nuttx/pull/16217#discussion_r2045132768 ## include/nuttx/leds/ktd2052.h: ## @@ -0,0 +1,142 @@ +/ + * include/nuttx/leds/ktd2052.h + * + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + / + +#ifndef __INCLUDE_NUTTX_LEDS_KTD2052_H +#define __INCLUDE_NUTTX_LEDS_KTD2052_H + +/ + * Included Files + / + +#include +#include + +/ + * Pre-processor Definitions + / + +/* Set RGB value for a single module */ +#define KTDIOSETRGB _PWMIOC(1) Review Comment: can we use ioctl number from include/nuttx/leds/userled.h -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/leds: Add support for KTD2052 [nuttx]
acassis commented on PR #16217: URL: https://github.com/apache/nuttx/pull/16217#issuecomment-2806819297 @stbenn please include a Documentation to https://nuttx.apache.org/docs/latest/components/drivers/character/leds/index.html and an app testing application, this way users could use it without facing issues -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] drivers/leds: Add support for KTD2052 [nuttx]
nuttxpr commented on PR #16217: URL: https://github.com/apache/nuttx/pull/16217#issuecomment-2805483571 [**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR description does not fully meet the NuttX requirements. Here's why and how to fix it: * **Summary:** While it states *what* was changed, it lacks crucial details: * **Why?** What problem does this solve? Why is this driver needed? (e.g., "Enables control of LEDs connected to a KTD2052 driver, allowing for..." or "Adds support for a commonly used LED driver, expanding hardware compatibility.") * **How?** "Driver implementation" is too vague. Briefly explain the mechanism. (e.g., "Uses I2C to communicate with the KTD2052, setting brightness levels through registers...") * **Issues:** Are there any related NuttX or NuttX Apps issues this addresses? Even if not, explicitly stating "N/A" is helpful. * **Impact:** While some impacts are addressed, others are missing or incomplete: * **New Feature:** Explicitly say "YES". * **User Impact:** Even if the answer is NO, briefly explain why. (e.g., "NO, this adds a new driver and does not affect existing LED functionality.") * **Build Impact:** YES - You added a Kconfig option. Describe how the build process is affected. (e.g., "YES, a new Kconfig option `CONFIG_LED_KTD2052` enables the driver.") * **Hardware Impact:** Be specific about which architectures and boards are supported. (e.g., "YES, supports the KTD2052 on [architecture] platforms, tested on a custom board.") * **Documentation:** If no documentation was added, state that and why. (e.g., "NO, documentation will be added in a follow-up commit.") or "YES, a README.md file describing the driver usage has been added." * **Security, Compatibility:** Explicitly state "NO" (if applicable) with a brief explanation. (e.g., "NO, this driver does not introduce any new security concerns." / "NO, this is a new driver and maintains backward compatibility.") * **Testing:** Severely lacking detail. * **Build Host:** Provide specifics. (e.g., "Linux Ubuntu 22.04, x86_64, GCC 11.2") * **Target:** Provide specific architecture, board, and configuration. (e.g., "ARM Cortex-M4, Custom KTD2052 board, `stm32f4discovery:nsh` config") * **Logs:** The "before" logs might not be relevant for a new driver, but "after" logs demonstrating the driver's functionality are essential. Include examples showing successful LED control. If the logs are extensive, consider attaching a file instead of pasting them directly. **Example of Improved PR Description:** ``` This commit adds support for the KTD2052 LED driver chip, expanding hardware compatibility for LED control within NuttX. ## Summary This change adds a driver for the KTD2052 LED driver chip, allowing users to control LEDs connected to this chip. The driver communicates with the KTD2052 via I2C, setting brightness levels through specific registers. This addresses the need to support this commonly used LED driver chip. Related NuttX Issue: N/A Related NuttX Apps Issue/PR: N/A ## Impact * New feature added? YES * Impact on user: NO, this adds a new driver and does not affect existing LED functionality. Users wishing to use this new driver will need to configure their .config file appropriately. * Impact on build: YES, a new Kconfig option `CONFIG_LED_KTD2052` enables the driver. * Impact on hardware: YES, supports the KTD2052 on ARM platforms, tested on a custom KTD2052 board. * Impact on documentation: NO, documentation will be added in a follow-up commit. * Impact on security: NO, this driver does not introduce any new security concerns. * Impact on compatibility: NO, this is a new driver and maintains backward compatibility. ## Testing I confirm that changes are verified on a local setup and work as intended: * Build Host(s): Linux Ubuntu 22.04, x86_64, GCC 11.2 * Target(s): ARM Cortex-M4, Custom KTD2052 board, `stm32f4discovery:nsh` config (adapted for the custom board) Testing logs after change: ``` nsh> ktd2052_test # Example test application Initializing KTD2052... Success! Setting LED 1 to 50% brightness... Success! Setting LED 2 to 100% brightness... Success! ... ``` ``` By providing more detail and addressing all the required points, your PR will be much easier to review and merge. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
