Add drivers and shields for Arduino Modulino Buttons and Smart LEDs #89685
Conversation
fabiobaltieri
force-pushed
the
modulino
branch
7 times, most recently
from
d96cb8e to
77e8016
Compare
github-actions
bot
added
area: Input
github-actions
bot
requested review from
Mani-Sadhasivam,
aasinclair,
avisconti,
erwango,
jfischer-no,
kartben,
simonguinot and
thedjnK
| static int modulino_buttons_leds_set_brightness(const struct device *dev, | ||
| uint32_t led, uint8_t value) | ||
| { | ||
| return modulino_buttons_leds_set(dev, led, value > 0); |
There was a problem hiding this comment.
I'd suggest you don't implement it (it's considered optional as per the documentation) in this case
There was a problem hiding this comment.
interesting, the problem is that one could use the set brightness API and have it working on both pwm and normal LEDs, if I don't implement it that would not work, but then this would be better handled in the set brightness LED implementation
I'll leave it here for now if you don't mind and follow up with an API clean uproposal
There was a problem hiding this comment.
It is OK to have ->set_brightness implemented here. To be honest, I never understood why we have ->on and ->off methods in Zephyr. I think drivers should only implement ->set_brightness, as in Linux. And eventually some led_on and led_off macros should be wrapped around.
There was a problem hiding this comment.
Not sure either but #89718 is landing in your inbox soon. I'm all for ditching on/off, though I bumped into a driver that was hooking into those differently for enabling just "on" or "pwm on", that it would be particularly hard to handle that...
boards/shields/arduino_modulino_buttons/doc/img/arduino_modulino_buttons.png
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR adds new drivers and device-tree bindings for Arduino Modulino Buttons and Smart LEDs, along with associated Kconfig, CMakeLists, shields, and documentation files.
- Added LED and input drivers for smart LEDs and buttons
- Added DT bindings, configuration files, and documentation overlays for the new hardware modules
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| dts/bindings/led/arduino,modulino-buttons-leds.yaml | New DT binding for Modulino smart LED driver |
| dts/bindings/input/arduino,modulino-buttons.yaml | New DT binding for Modulino buttons input driver |
| drivers/led_strip/modulino_smartleds.c | New LED strip driver for smart LEDs |
| drivers/led_strip/Kconfig.arduino | Kconfig for smart LEDs (note help text mismatch) |
| drivers/led/modulino_buttons_leds.c | New LED driver for Modulino buttons LEDs |
| drivers/input/input_modulino_buttons.c | New input driver for Modulino buttons |
| boards/shields/... | Added shields overlays and documentation for smart LEDs and buttons |
Comments suppressed due to low confidence (1)
drivers/led/modulino_buttons_leds.c:15
- [nitpick] Consider revising the logging configuration to use a LED-specific log level (e.g. CONFIG_LED_LOG_LEVEL) instead of CONFIG_INPUT_LOG_LEVEL, since this driver is for LED control.
LOG_MODULE_REGISTER(modulino_buttons_leds, CONFIG_INPUT_LOG_LEVEL);
Wouldn't say it's obvious, but I would reckon that users of this would just see them like any other i2c device without thinking too much as to how the protocol is implemented. That said, the firmware is open source and the code was the reference I used for the protocol, so yeah I think you are right that adding a reference in the commit message is a good idea, let me rework those. |
|
Okay same code, added some more details in the commit messages and references to the library, firmware and a mention to the fact that they can be updated. That was a good comment @simonguinot, unfortunately GitHub tend to hide commit messages in a tab and for how much I'm preaching about how important they are I realize I'm tending to write poor ones myself. |
simonguinot
left a comment
simonguinot
left a comment
There was a problem hiding this comment.
The LED drivers are looking good to me !
|
Thanks for updating the commit message. I think it is clearer. |
Add an input driver for the modulino buttons module. This is a pluggable I2C board implementing three buttons and three LEDs, the I2C protocol is implemented on a microcontroller on the modulino board itself, the firmware for that is open source and can be updated using an Arduino sketch: Link: https://github.com/arduino/node_modulino_firmware Link: https://github.com/arduino-libraries/Modulino Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
Add an LED driver for the modulino buttons module, this has three LEDs that are controllable independently of the buttons. Link: https://github.com/arduino/node_modulino_firmware Link: https://github.com/arduino-libraries/Modulino Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
Add an led_strip driver for the modulino smartleds module. This is a pluggable I2C board with 8 addressable RGB LEDs The I2C protocol is implemented on an microcontroller on the modulino board itself, the firmware for that is open source and can be updated using an Arduino sketch: Link: https://github.com/arduino/node_modulino_firmware Link: https://github.com/arduino-libraries/Modulino Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
Add two shield definitions for the the modulino buttons and smartleds modules. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
fabiobaltieri
dismissed stale reviews from soburi, simonguinot, and pillo79
via
ba47ee2
Add modulino drivers path to the Arduino area. Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
|
Hey @sonarqubecloud, if you don't have anything to say, could you just not say anything? |
6 participants
Add drivers and shields for Arduino Modulino Buttons and Smart LEDs, these are QWIIC modules to provide buttons and LED strip functionalities.