Merge develop into main #494
Conversation
📝 WalkthroughWalkthroughUpdates GitHub Actions checkout steps to 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
Devices/lilygo-tdeck/Source/devices/Display.cpp (1)
8-17: Constructor parameters are in correct order, but inline comments would improve readability.Parameter order confirmed as correct:
mirrorY(false),pinReset(GPIO_NUM_NC),pinInterrupt(GPIO_NUM_16). However, with 8 positional arguments, a brief inline comment such as/* mirrorY */ falsewould reduce the risk of misconfigurations during future maintenance, especially since the semantic meaning of each parameter isn't immediately obvious from the values alone.TactilityKernel/Source/drivers/root.cpp (1)
9-12: Parameter name mismatch with header declaration; missing<cstring>include.The header (
root.h) declares the parameter asmodel, but the implementation usesbuffer. Align naming for clarity. Also,strcmpis used without an explicit include — add<cstring>(or<string.h>for C linkage context).Proposed fix
+#include <cstring> + extern "C" { -bool root_is_model(const struct Device* device, const char* buffer) { +bool root_is_model(const struct Device* device, const char* model) { auto* config = static_cast<const RootConfig*>(device->config); - return strcmp(config->model, buffer) == 0; + return strcmp(config->model, model) == 0; }Tactility/Source/Tactility.cpp (1)
161-167: Consider extracting the duplicated root device lookup and model string.The pattern
device_find_by_name("/")+check()+root_is_model(…, "LilyGO T-Deck")is duplicated. A small helper or a shared constant for the model string would reduce repetition and the risk of typo-induced bugs if more T-Deck conditionals are added later.Example helper
// Near top of file or in a shared header static constexpr const char* TDECK_MODEL = "LilyGO T-Deck"; static bool isCurrentDeviceModel(const char* model) { auto* root_device = device_find_by_name("/"); check(root_device); return root_is_model(root_device, model); }Also applies to: 255-260
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
TactilityKernel/Source/drivers/root.cpp (1)
10-13: Parameter name mismatch with header declaration.The header (
root.hLine 21) declares the second parameter asmodel, but the implementation names itbuffer. Usemodelfor consistency and clarity — it better describes the parameter's purpose.Proposed fix
-bool root_is_model(const struct Device* device, const char* buffer) { +bool root_is_model(const struct Device* device, const char* model) { auto* config = static_cast<const RootConfig*>(device->config); - return strcmp(config->model, buffer) == 0; + return strcmp(config->model, model) == 0; }
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
device.py (1)
108-110: Consider making the workaround flag data-driven instead of hardcoded.Hardcoding
"lilygo-tdeck"ties the build script to a specific device string. If another device needs this workaround or the device ID changes, this must be manually updated. Sincedevice.propertiesalready drives all other config emissions, you could add a property liketdeckWorkaround=truein the T-Deck'sdevice.propertiesand read it here withget_boolean_property_or_false.That said, I understand this is a temporary workaround per the Kconfig comments, so this is fine to defer.
Firmware/Kconfig (1)
9-15: Minor indentation inconsistency.The new
config TT_TDECK_WORKAROUNDblock and its children use 5-space indentation (lines 9–14), while the surrounding entries (TT_DEVICE_NAME,TT_DEVICE_ID,TT_SPLASH_DURATION) use 4-space indentation. Consider aligning to keep the file consistent.Proposed fix
- # T-Deck device-related code was directly referenced from Tactility in a pull request. - # This breaks other devices because the code does not exist in those implementations. - # Until we move it out into a proper driver, we have to have pre-processor definition for that. - config TT_TDECK_WORKAROUND - bool "Temporary work-around until we fix the T-Deck keyboard and trackball settings" - default n - config TT_SPLASH_DURATION + # T-Deck device-related code was directly referenced from Tactility in a pull request. + # This breaks other devices because the code does not exist in those implementations. + # Until we move it out into a proper driver, we have to have pre-processor definition for that. + config TT_TDECK_WORKAROUND + bool "Temporary work-around until we fix the T-Deck keyboard and trackball settings" + default n + config TT_SPLASH_DURATIONTactility/Source/Tactility.cpp (2)
114-117: Inconsistent preprocessor guard style forCONFIG_TT_TDECK_WORKAROUND.Three different guard styles are used for the same config symbol:
- Line 114:
#if CONFIG_TT_TDECK_WORKAROUND == 1- Line 163:
#if defined(CONFIG_TT_TDECK_WORKAROUND)- Line 254:
#if defined(CONFIG_TT_TDECK_WORKAROUND)While functionally equivalent in ESP-IDF (bool
nleaves the macro undefined), mixing styles is confusing and error-prone if the generation behavior ever changes. Pick one form and use it consistently.Proposed fix — unify to `#if defined(CONFIG_TT_TDECK_WORKAROUND)`
`#ifdef` ESP_PLATFORM namespace crashdiagnostics { extern const AppManifest manifest; } namespace webserversettings { extern const AppManifest manifest; } -#if CONFIG_TT_TDECK_WORKAROUND == 1 +#if defined(CONFIG_TT_TDECK_WORKAROUND) namespace keyboardsettings { extern const AppManifest manifest; } // T-Deck only for now namespace trackballsettings { extern const AppManifest manifest; } // T-Deck only for now `#endif`Also applies to: 163-166, 254-256
163-166: Extra indentation on guardedaddAppManifestcalls.Lines 164–165 use 8-space indentation while all surrounding
addAppManifestcalls use 4-space indentation. This appears to be an accidental whitespace change.Proposed fix
`#if` defined(CONFIG_TT_TDECK_WORKAROUND) - addAppManifest(app::keyboardsettings::manifest); - addAppManifest(app::trackballsettings::manifest); + addAppManifest(app::keyboardsettings::manifest); + addAppManifest(app::trackballsettings::manifest); `#endif`
1 participant
Summary by CodeRabbit
New Features
Chores