Update logging mechanism with LOG macro in multiple classes #250
Conversation
WalkthroughThe changes in this pull request primarily focus on updating the logging mechanism across multiple classes in the codebase. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ItemCommand
participant ItemInput
participant ItemList
participant ItemToggle
participant MenuScreen
User->>ItemCommand: executeCommand()
ItemCommand->>ItemInput: setValue()
ItemInput->>ItemList: process()
ItemList->>ItemToggle: toggle()
ItemToggle-->>ItemList: return
ItemList-->>ItemInput: return
ItemInput-->>ItemCommand: return
ItemCommand-->>User: return
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)
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 (
|
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range comments (9)
src/ItemCommand.h (1)
Line range hint
1-4: Add missing include for LOG macro.The file uses the
LOGmacro but doesn't include the header where it's defined. Based on the AI summary, this should be defined insrc/utils/utils.h.Add this include:
#include "MenuItem.h" +#include "utils/utils.h"src/ItemToggle.h (1)
Line range hint
1-107: Consider enhancing type safety and documentation.While the overall class implementation is solid, here are some suggestions to improve type safety and maintainability:
- Use
std::functioninstead of function pointers for callbacks- Add
[[nodiscard]]attribute to getter methods- Document thread safety guarantees
Example enhancement for the callback:
#include <functional> // Replace function pointer typedef with std::function using ToggleCallback = std::function<bool(bool)>; // Update member variable private: ToggleCallback callback;src/MenuScreen.cpp (1)
Line range hint
71-105: Consistent logging implementation across navigation actions.The logging changes effectively capture navigation events and relevant state information. Consider creating a logging style guide to document these patterns:
- Class::action format for messages
- State variables to include in logs
- Placement of log statements (after state changes)
src/input/KeyboardAdapter.h (1)
Line range hint
224-234: Add error logging for invalid states.Consider adding logging for error conditions to aid debugging:
default: + LOG(F("KeyboardAdapter: Unknown CSI command received")); reset(); // Reset after unknown code set break;Similar logging could be added for other error cases where
reset()is called.src/ItemInput.h (5)
Line range hint
183-195: Consider wrapping callback invocation in try-catch.The callback invocation could throw an exception, which would prevent the logging of the exit action. Consider adding error handling:
void back(MenuRenderer* renderer) { renderer->clearBlinker(); renderer->setEditMode(false); cursor = 0; view = 0; draw(renderer); + try { if (callback != NULL) { callback(value); } + } catch (...) { + LOG(F("ItemInput::exitEditMode"), F("callback failed")); + throw; + } LOG(F("ItemInput::exitEditMode"), value); }
Line range hint
267-291: Fix memory leaks in string manipulation.The current implementation has potential memory leaks in both branches where dynamically allocated memory is not properly managed.
Consider this safer implementation:
void typeChar(MenuRenderer* renderer, const unsigned char character) { uint8_t length = strlen(value); if (cursor < length) { char start[length]; char end[length]; - char* joined = new char[length + 2]; + char* newValue = new char[length + 2]; substring(value, 0, cursor, start); substring(value, cursor, length - cursor, end); concat(start, character, end, joined); - value = joined; + delete[] value; // Clean up old value if heap-allocated + value = newValue; } else { - char* buf = new char[length + 2]; + char* newValue = new char[length + 2]; concat(value, character, buf); - value = buf; + delete[] value; // Clean up old value if heap-allocated + value = newValue; } cursor++; // ... rest of the method LOG(F("ItemInput::typeChar"), character); }
Line range hint
295-301: Fix potential memory leak in clear method.Direct assignment of empty string literal could leak memory if the previous value was heap-allocated.
void clear(MenuRenderer* renderer) { + delete[] value; // Clean up old value if heap-allocated value = (char*)""; draw(renderer); renderer->drawBlinker(); LOG(F("ItemInput::clear"), value); }
Line range hint
1-304: Consider implementing RAII for string management.The class would benefit from using a proper string management class (like
Stringin Arduino) or implementing RAII principles to handle dynamic memory allocation consistently across all methods.This would help prevent memory leaks and make the code more maintainable. Consider:
- Using Arduino's
Stringclass- Creating a custom string wrapper class
- Adding a destructor to clean up allocated memory
- Implementing copy constructor and assignment operator if needed
Based on the code analysis, I can now provide a final response:
Memory leak confirmed in ItemInput class
The code shows multiple instances of heap allocations without proper cleanup:
- In
typeChar()method:char* joined = new char[length + 2]; // ... value = joined; // Previous value is leaked- In
ItemInputCharset::commitCharEdit():char* buf = new char[length + 2]; // ... value = buf; // Previous value is leakedThe
valuemember is reassigned without freeing the previous allocation, and there's no destructor to clean up the final allocation. The original review comment's concern is valid.The suggested fix should be enhanced to:
class ItemInput { char* value; ~ItemInput() { delete[] value; } bool setValue(char* newValue) { if (this->value != newValue) { delete[] this->value; this->value = newValue; LOG(F("ItemInput::setValue"), newValue); return true; } return false; } };Additionally, all methods that allocate new strings (typeChar, commitCharEdit) should be reviewed to ensure proper cleanup of the previous value before assignment.
🔗 Analysis chain
Line range hint
95-106: Consider adding memory management for previous value.If
valueis heap-allocated, the current implementation could lead to memory leaks when setting new values.Consider adding proper cleanup:
bool setValue(char* value) { if (this->value != value) { + // If previous value was heap-allocated + delete[] this->value; this->value = value; LOG(F("ItemInput::setValue"), value); return true; } return false; }
Let me gather more information about how the
valueis used and allocated in the codebase.These commands will help us understand:
- How the char* value is used across the codebase
- Whether there's proper cleanup in the destructor
- If there are any heap allocations for char arrays
- The complete context of the ItemInput class
This will give us a better picture of the memory management requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if value is heap-allocated elsewhere in the codebase rg -l "new char\[.+\].*=.*value"Length of output: 69
Script:
#!/bin/bash # Search for any char* allocations or assignments related to ItemInput rg -A 5 "char\*.*value" # Search for constructors and destructor of ItemInput ast-grep --pattern 'class ItemInput { $$$ ~ItemInput() { $$$ } $$$ }' # Search for any new char[] allocations in the codebase rg "new char\[" -A 2 # Look for the ItemInput header file to understand the member declaration fd "ItemInput.h" --exec cat {}Length of output: 26636
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (12)
- src/ItemCommand.h (1 hunks)
- src/ItemInput.h (8 hunks)
- src/ItemInputCharset.h (3 hunks)
- src/ItemList.h (4 hunks)
- src/ItemRangeBase.h (4 hunks)
- src/ItemSubMenu.h (1 hunks)
- src/ItemToggle.h (1 hunks)
- src/LcdMenu.cpp (1 hunks)
- src/MenuScreen.cpp (2 hunks)
- src/input/KeyboardAdapter.h (2 hunks)
- src/renderer/MenuRenderer.h (1 hunks)
- src/utils/utils.h (2 hunks)
🔇 Additional comments (24)
src/ItemSubMenu.h (1)
35-35: LGTM! Verify logging consistency across codebase.The update to use the
LOGmacro with the F() macro for flash memory storage is correct and follows best practices.Let's verify the consistent usage of the new logging mechanism across the codebase:
✅ Verification successful
Let me generate new shell scripts to better verify the logging consistency.
The previous search failed because the path pattern was incorrect. This new set of commands will help us:
- Find the actual location and naming of Item-related header files
- Check the LOG macro usage across the source files
- Verify no printLog calls remain in the source code
- Understand the LOG macro definition for context
Logging mechanism has been consistently updated across the codebase
The verification shows that:
- No
printLogcalls remain in the source code (src/directory)- The
LOGmacro is consistently used across all menu items with the correct patternLOG(F("ClassName::methodName"), ...)- The
LOGmacro is properly defined insrc/utils/utils.hwith a debug and no-op version- The only remaining
printLogreferences are in example files and keywords.txt, which is acceptable as examples often maintain backward compatibilityThe change in
ItemSubMenu.hfollows the same pattern used throughout the codebase, confirming consistent implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of LOG macro and absence of printLog # Expected: Only LOG macro usage, no printLog calls # Test 1: Check for any remaining printLog calls echo "Checking for remaining printLog calls..." rg "printLog" # Test 2: Verify consistent LOG macro usage pattern in other menu items echo "Verifying LOG macro usage pattern in menu items..." rg "LOG\(F\([\"'].*::" "src/Item*.h"Length of output: 504
Script:
#!/bin/bash # First, let's find all header files in src directory echo "Finding all header files in src directory..." fd -e h -e hpp "^Item.*" src/ # Then, let's check for LOG macro usage in these files echo "Checking LOG macro usage..." rg "LOG\(" src/ # Check for any remaining printLog in src directory echo "Checking for remaining printLog in src..." rg "printLog" src/ # Get a better context of the logging mechanism echo "Checking LOG macro definition..." rg "^#define\s+LOG" -A 2Length of output: 3719
src/LcdMenu.cpp (2)
12-12: LGTM! Logging standardization looks good.The change from
printLogtoLOGis consistent with the codebase-wide logging standardization effort mentioned in the PR summary.
12-12: Verify required includes for LOG and F macros.The code uses both
LOGandFmacros, but their includes are not visible in this file.Let's verify the necessary includes are present:
✅ Verification successful
Let me check the includes in the implementation file and verify if utils.h (which contains LOG macro) is properly included.
Based on the verification results, I can now provide the final response:
LOG and F macros are properly included and available
The necessary includes are properly set up through the include chain:
LcdMenu.cppincludesLcdMenu.hLcdMenu.hincludesutils/utils.hutils/utils.hincludesArduino.hand defines theLOGmacro- The
Fmacro is available through the Arduino.h include- The
LOGmacro is properly defined with debug/release configurations🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if LOG and F macros are properly included echo "Checking for LOG macro definition:" rg -l "^#define\s+LOG\b" echo -e "\nChecking for F macro include or definition:" rg -l "^#include.*Arduino\.h" rg -l "^#define\s+F\b" echo -e "\nChecking LcdMenu.h for the includes:" cat src/LcdMenu.hLength of output: 4494
Script:
#!/bin/bash # Check LcdMenu.cpp includes echo "Checking includes in LcdMenu.cpp:" rg -A 10 "^#include" src/LcdMenu.cpp # Check utils.h content to verify LOG and F macro definitions echo -e "\nChecking utils.h content:" cat src/utils/utils.hLength of output: 1936
src/utils/utils.h (1)
61-61: Well-structured header file.The file maintains a clean organization with proper include guards, consistent inline function declarations, and clear debug/release configuration separation.
src/ItemCommand.h (2)
60-60: LGTM! Good logging practice.The logging implementation:
- Uses F() macro to store string literal in flash memory (good for Arduino memory optimization)
- Provides clear context with class::method name
- Includes the menu item text for better debugging
- Logs after callback execution for accurate command tracking
60-60: Verify logging pattern consistency.Let's verify that:
- The LOG macro is properly defined
- Other files follow the same logging pattern
✅ Verification successful
Based on the results, I can now generate the final response since I have sufficient information about the logging patterns and macro definition.
Logging pattern is consistent across the codebase
The verification shows that:
- The LOG macro is properly defined in
src/utils/utils.hwith two variants:
- A functional version that calls
log()- A no-op version for when logging is disabled
- The logging pattern
LOG(F("ClassName::methodName"))is consistently used across all files, with:
- Proper class scope prefixing (e.g.,
ItemCommand::,MenuScreen::, etc.)- Consistent use of the
F()macro for string literals- Optional parameters correctly appended after the message
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check LOG macro definition echo "Checking LOG macro definition:" rg -p "^#define\s+LOG" src/ # Check logging pattern consistency echo -e "\nChecking logging pattern in other files:" rg "LOG\(F\([\"'].*::" src/Length of output: 2394
src/ItemToggle.h (1)
105-105: LGTM! Good use of F() macro for flash string optimization.The change from
printLogtoLOGmaintains logging functionality while properly using the F() macro to store the string literal in flash memory, which is beneficial for memory optimization on embedded systems.src/MenuScreen.cpp (2)
78-78: LGTM!The log message appropriately captures both the action and the view shift state.
85-85: LGTM!The log message maintains consistency with the RIGHT command logging pattern.
src/renderer/MenuRenderer.h (2)
116-116: LGTM! Good use of memory optimization.The logging change follows the new standardized approach and properly uses the F() macro to store the string in flash memory, which is a good practice for embedded systems.
114-118: Verify logging consistency in derived classes.Since
updateTimer()is virtual, we should ensure derived classes maintain consistent logging if they override this method.✅ Verification successful
Let me try a different approach to verify the derived classes and logging patterns.
This should give us:
- All classes that inherit from MenuRenderer
- All implementations of updateTimer
- The logging pattern usage across the codebase to verify consistency
Based on the results, I can now generate the final response:
Logging pattern is consistent, no derived class overrides found
The verification results show:
updateTimer()is only implemented in the baseMenuRendererclass, with no derived class overrides found- The
LOG(F())pattern is consistently used across the entire codebase for logging- The timeout logging in
MenuRenderer::updateTimer()follows the same pattern as other logging statements🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for derived class implementations of updateTimer ast-grep --pattern 'class $_ : $$$public MenuRenderer$$$ { $$$ void updateTimer() { $$$ } $$$ }'Length of output: 110
Script:
#!/bin/bash # First, let's find all classes that inherit from MenuRenderer ast-grep --pattern 'class $_ : $$$MenuRenderer$$$' # Also search for updateTimer implementations across the codebase rg "void updateTimer\(\)" -A 5 # And check the logging pattern usage rg "LOG\(F\(" --type cppLength of output: 3212
src/ItemList.h (2)
102-102: LGTM! Logging implementation is correct and efficient.The LOG call is properly placed after the state changes and uses F() macro for flash string storage optimization.
118-118: LGTM! Logging is consistent with the exit mode pattern.The LOG call is properly placed after entering edit mode and maintains consistency with other logging calls.
src/ItemRangeBase.h (3)
66-66: LGTM: Logging change in increment method.The logging change maintains the same semantic meaning while using the new
LOGmacro.
78-78: LGTM: Logging change in decrement method.The logging change maintains the same semantic meaning while using the new
LOGmacro.
66-66: Verify logging implementation across codebase.Let's verify that the new
LOGmacro is consistently implemented across the codebase.Also applies to: 78-78, 125-125, 151-151
✅ Verification successful
Based on the shell script results, I can now generate the final response:
LOG macro implementation is consistent with a few exceptions
The verification reveals:
- The LOG macro is properly defined in
src/utils/utils.hwith both debug and no-op variants- Most of the codebase consistently uses
LOG(F(...))pattern for logging- There are 3 remaining legacy
printLogcalls inexamples/RTOS/RTOS.ino, but these are in example code and don't affect the core implementation- Two commented-out LOG calls in
KeyboardAdapter.hdon't follow the F() pattern but they're inactiveThe implementation is largely consistent, with only example code needing updates for full uniformity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining printLog calls and verify LOG macro usage # Test 1: Check for any remaining printLog calls echo "Checking for remaining printLog calls..." rg "printLog\(" # Test 2: Verify LOG macro definition and implementation echo "Checking LOG macro definition..." rg "^#define\s+LOG\b" "src/" # Test 3: Check LOG usage pattern across codebase echo "Checking LOG usage patterns..." rg "LOG\(F\("Length of output: 3309
src/ItemInputCharset.h (3)
60-60: LGTM! Logging placement is appropriate.The LOG call is well-positioned after character selection and provides useful debugging information.
67-67: LGTM! Logging is consistent with UP command implementation.The LOG call maintains consistency in debug information across navigation commands.
Line range hint
60-143: Verify the LOG macro implementation in utils.h.The logging changes look consistent. Let's verify the LOG macro implementation to ensure it handles the parameter types correctly.
✅ Verification successful
Let me gather more information about the logging implementation to make a thorough verification.
LOG macro implementation is correctly handling the parameter types
The verification reveals that the LOG macro implementation in
utils.his well-designed and properly handles all the logging cases inItemInputCharset.h:
- The macro is conditionally compiled (
#ifdef DEBUG) with a no-op fallback- It supports two function overloads:
- Single parameter:
log(const __FlashStringHelper* command)- Two parameters:
log(const __FlashStringHelper* command, T value)where T is templated- All LOG calls in
ItemInputCharset.hfollow these patterns:
LOG(F("ItemInputCharset::up"), charset[charsetPosition])LOG(F("ItemInputCharset::abortCharEdit"))The logging implementation correctly handles both string literals (using Arduino's F macro for flash storage) and various value types through template parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the LOG macro definition and its usage pattern rg -p "LOG\(" "src/utils/utils.h"Length of output: 100
Script:
#!/bin/bash # Let's check the log function implementation and its context rg -p "void log" "src/utils/utils.h" -B 5 -A 10 # Also check if there are any other logging-related functions or macros rg -p "log|LOG" "src/utils/utils.h" # Let's also check how LOG is being used in the codebase for parameter patterns rg "LOG\(.*\)" "src/"Length of output: 3775
src/input/KeyboardAdapter.h (1)
201-204: Verify the intended behavior for End/Home keys.The code logs End and Home key events but doesn't process them. Is this intentional, or should these keys trigger specific menu actions?
✅ Verification successful
Let me try a different search strategy to verify the End/Home key handling.
The first command will help us find any files containing End/Home references, while the second command will show us the context around these specific CSI sequences ('H' for Home, 'F' for End) to understand their implementation better.
Let me search for more context around the keyboard handling implementation to understand if there's any existing pattern for handling navigation keys.
This will help us understand the overall key handling patterns and whether navigation keys like Home/End should have specific actions implemented.
Let me check the menu interface to understand what navigation actions are supported.
This will help us understand what actions are available in the menu system and whether Home/End navigation should be implemented.
Let me try one final search to find the menu-related definitions.
This will help us understand the menu navigation system and available actions to determine if Home/End keys should be implemented.
Based on the comprehensive code analysis, I can now generate the final response.
End/Home key events are intentionally logged but not processed
The codebase shows a clear pattern for handling navigation keys:
- Arrow keys (UP/DOWN/LEFT/RIGHT) are processed and trigger menu navigation actions
- ENTER/BACK/BACKSPACE have specific menu behaviors
- Other special keys like PgUp, PgDn, Insert are also only logged without actions
The End/Home key logging is consistent with the codebase's pattern of logging unimplemented keys for debugging purposes, similar to PgUp and PgDn. This appears to be intentional as these keys are not part of the core navigation scheme used by the menu system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing End/Home key handling in the codebase rg -l "End|Home" --type cpp --type hppLength of output: 68
Script:
#!/bin/bash # Search for End/Home key handling in C/C++ files with broader extension patterns rg -l "End|Home" -g "*.{h,hpp,c,cpp}" # Also search for specific CSI handling patterns that might include End/Home rg -A 5 "case 'H':|case 'F':" -g "*.{h,hpp,c,cpp}"Length of output: 1184
Script:
#!/bin/bash # Look for other key handling patterns in KeyboardAdapter rg -B 5 -A 10 "case.*:" src/input/KeyboardAdapter.h # Check CharacterDisplayRenderer for any navigation-related functionality cat src/renderer/CharacterDisplayRenderer.cppLength of output: 7487
Script:
#!/bin/bash # Check menu interface for supported actions rg -B 2 -A 2 "enum.*Action" -g "*.{h,hpp,c,cpp}" # Look for menu process method implementation ast-grep --pattern 'process($$$) { $$$ }'Length of output: 96
Script:
#!/bin/bash # Search for menu-related files and their content fd -e h -e cpp "Menu" --exec cat {} # Also look specifically for action definitions rg "UP|DOWN|LEFT|RIGHT|ENTER|BACK" -B 2 -A 2 -g "*.{h,hpp,c,cpp}"Length of output: 46763
src/ItemInput.h (4)
103-103: LGTM! Logging placement is appropriate.The LOG call is correctly placed after the value update.
181-181: LGTM! Logging placement is appropriate.The LOG call is correctly placed after all state changes are complete.
211-211: LGTM! Logging placement is appropriate in navigation methods.The LOG calls are correctly placed after all state changes in both left and right navigation methods.
Also applies to: 232-232
253-253: Verify the remove helper function implementation.The logging is correctly placed, but let's verify the implementation of the
removehelper function used for string manipulation.#!/bin/bash # Look for the remove function implementation ast-grep --pattern 'void remove(char* $str, $_pos, $_len) { $$$ }'
In KeyboardAdapter, ItemCommand, ItemInput, ItemInputCharset, ItemList, and ItemRangeBase classes. - Removed redundant printLog calls for various actions. - Updated logging to use the LOG macro for consistency.
forntoh
force-pushed
the
enhancement/improved-logging
branch
from
991cc30 to
92dec87
Compare
Updated logging function calls in toggleRelay functions to use LOG macro instead of printLog for consistency and clarity.
Adjusted logging messages in the MenuScreen class to provide more specific information when navigating up and down within the menu.
- Changed log messages in multiple test files from using single colon to double colon for MenuScreen actions.
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range comments (2)
src/ItemSubMenu.h (1)
Line range hint
1-4: Add missing include for LOG macro.The
LOGmacro is used in the code but its header file is not included. This could lead to compilation errors.Add the required include at the top of the file:
#ifndef ItemSubMenu_H #define ItemSubMenu_H #include "LcdMenu.h" #include "MenuItem.h" +#include "utils/utils.h" // For LOG macrosrc/MenuScreen.cpp (1)
Line range hint
108-112: Consider adding logging to reset() method.For consistency with other navigation methods, consider adding logging to track when the screen is reset.
void MenuScreen::reset(MenuRenderer* renderer) { cursor = 0; view = 0; draw(renderer); + LOG("MenuScreen::reset", cursor); }🧰 Tools
🪛 cppcheck
[style] 108-108: The function 'reset' is never used.
(unusedFunction)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (20)
- examples/RTOS/RTOS.ino (1 hunks)
- src/ItemCommand.h (1 hunks)
- src/ItemInput.h (8 hunks)
- src/ItemInputCharset.h (3 hunks)
- src/ItemList.h (4 hunks)
- src/ItemRangeBase.h (4 hunks)
- src/ItemSubMenu.h (1 hunks)
- src/ItemToggle.h (1 hunks)
- src/LcdMenu.cpp (1 hunks)
- src/MenuScreen.cpp (2 hunks)
- src/input/KeyboardAdapter.h (2 hunks)
- src/renderer/MenuRenderer.h (1 hunks)
- src/utils/utils.h (1 hunks)
- test/Basic.test.yml (1 hunks)
- test/Callbacks.test.yml (1 hunks)
- test/IntFloatValues.test.yml (3 hunks)
- test/ItemBack.test.yml (1 hunks)
- test/List.test.yml (2 hunks)
- test/MenuTimeout.test.yml (1 hunks)
- test/SubMenu.test.yml (1 hunks)
🧰 Additional context used
📓 Learnings (3)
src/ItemInputCharset.h (1)
Learnt from: forntoh PR: forntoh/LcdMenu#250 File: src/ItemInputCharset.h:143-144 Timestamp: 2024-10-23T23:46:58.953Z Learning: In `src/ItemInputCharset.h`, within the `commitCharEdit` method, do not include the full updated value in the log messages.src/ItemList.h (1)
Learnt from: forntoh PR: forntoh/LcdMenu#250 File: src/ItemList.h:141-141 Timestamp: 2024-10-24T00:16:02.760Z Learning: In the `ItemList` class (`src/ItemList.h`), for the `selectNext` method, keep the `LOG` call outside the `if` block to support tests that assert that the value doesn't change when an action is performed. This ensures that logging occurs even when the selection does not change.src/input/KeyboardAdapter.h (1)
Learnt from: forntoh PR: forntoh/LcdMenu#250 File: src/input/KeyboardAdapter.h:210-220 Timestamp: 2024-10-23T23:45:14.170Z Learning: You prefer to keep the logging messages for special keys in their current form and do not wish to add more context to them.
🔇 Additional comments (33)
test/Basic.test.yml (3)
7-7: LGTM! Downward navigation test sequence is properly updated.The test correctly validates the menu navigation bounds by:
- Testing navigation from position 0 to 4
- Verifying that position stays at 4 when trying to go beyond
Also applies to: 9-9, 11-11, 13-13, 15-15
17-17: LGTM! Upward navigation test sequence is properly updated.The test correctly validates the menu navigation bounds by:
- Testing navigation from position 4 back to 0
- Verifying that position stays at 0 when trying to go beyond
Also applies to: 19-19, 21-21, 23-23, 25-25
7-25: Verify test coverage for edge cases.While the basic navigation flow is well tested, consider adding test cases for:
- Rapid button presses
- Long button press behavior (if supported)
- Concurrent up/down button presses (if possible in hardware)
Let's check if these scenarios are covered in other test files:
src/ItemSubMenu.h (1)
35-35: LGTM! Logging change looks good.The update from
printLogtoLOGmacro is consistent with the PR objectives to standardize logging across the codebase. The logging provides good context by including both the class/method name and the menu item text.Let's verify the consistency of LOG macro usage across other menu item classes:
✅ Verification successful
Let me try a different approach to locate the files and verify the logging consistency.
LOG macro usage is consistent across all Item classes
The verification shows that the LOG macro is being used consistently across all Item classes with the proper format of
LOG("ClassName::methodName", value). There are no remainingprintLogcalls in any of the Item files, confirming that the logging standardization has been completed successfully.The logging in
ItemSubMenu::changeScreenfollows the same pattern as other Item classes:
- All LOG calls include the class and method name as context
- All LOG calls include a relevant value being logged
- No legacy
printLogcalls remain🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent LOG macro usage across menu item classes # Expected: All Item* classes should use the LOG macro with similar format # Search for LOG usage in Item* classes rg "LOG\(" "src/Item.*" # Also check for any remaining printLog calls that might have been missed rg "printLog" "src/Item.*"Length of output: 154
Script:
#!/bin/bash # First, let's find all Item* files in the repository fd "Item.*" --type f # Then use the found files to check LOG and printLog usage echo "=== Checking LOG macro usage ===" fd "Item.*" --type f --exec rg "LOG\(" {} echo "=== Checking printLog usage ===" fd "Item.*" --type f --exec rg "printLog" {}Length of output: 1773
test/List.test.yml (1)
7-7: LGTM! Log format updates are consistent.The changes to double colon format align with the new logging mechanism update.
Also applies to: 19-19
test/SubMenu.test.yml (2)
10-26: LGTM! Consistent logging format update.The logging format changes from single to double colon are applied consistently across all menu navigation steps, maintaining a uniform logging pattern.
30-30: LGTM! Logging format consistency maintained across different contexts.The updated logging format is consistently applied in different navigation contexts (after sub-menu entry and back navigation), ensuring uniform logging throughout the test flow.
Also applies to: 38-38
src/utils/utils.h (1)
42-44: Verify the DEBUG flag behavior.The current implementation disables logging when DEBUG is defined, which seems counterintuitive. Typically, debug logging is enabled when DEBUG is defined. Please verify if this is the intended behavior.
Also applies to: 56-57
test/IntFloatValues.test.yml (1)
7-7: LGTM! Log format standardization looks consistent.The update to use double colons (
::) in MenuScreen log messages aligns with the broader standardization effort across test files.Also applies to: 23-23, 41-41
src/ItemCommand.h (2)
Line range hint
1-63: LGTM! Clean implementation with good documentation.The class is well-documented with clear purpose and usage instructions. The logging change maintains the same semantic meaning while adopting the new logging mechanism.
60-60: Verify LOG macro inclusionThe code uses the
LOGmacro but I don't see its inclusion. Please ensure the header file that defines theLOGmacro is properly included.Let's verify the LOG macro definition:
✅ Verification successful
Let me try a different approach to search for the LOG macro and its inclusion without specifying file extensions.
Let's check the utils.h file since it appears in the LOG macro usage results and is a common location for utility macros.
LOG macro is properly included through inheritance chain
The verification shows that:
- The
LOGmacro is defined insrc/utils/utils.hItemCommand.hincludesMenuItem.hMenuItem.hincludesutils/utils.hTherefore, the
LOGmacro is properly available inItemCommand.hthrough the include chain, and no additional includes are needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for LOG macro definition echo "Searching for LOG macro definition..." rg -l 'LOG\s*\(' --type hpp --type h # Search for potential includes of the logging header echo "Searching for logging-related includes..." rg -l '#include.*log' --type hpp --type hLength of output: 316
Script:
#!/bin/bash # Search for LOG macro definition with broader pattern echo "Searching for LOG macro definition..." rg -l 'LOG\s*\(' # Search for potential includes of the logging header echo "Searching for logging-related includes..." rg -l '#include.*log' # Look at the content of ItemCommand.h to see its includes echo "Checking ItemCommand.h includes..." rg -p '^#include' src/ItemCommand.hLength of output: 620
Script:
#!/bin/bash # Check utils.h content for LOG macro definition echo "Checking utils.h for LOG macro..." rg -p 'LOG' src/utils/utils.h # Check if utils.h is included in the chain echo "Checking if utils.h is included in MenuItem.h..." rg -p '^#include' src/MenuItem.hLength of output: 481
src/ItemToggle.h (1)
Line range hint
1-7: Verify LOG macro dependency.The file includes
utils/utils.hwhich should contain the LOG macro definition. Let's verify this to ensure proper compilation.✅ Verification successful
Based on the shell script results, I can now generate the final response since I have sufficient information about the LOG macro definition and its usage across the codebase.
LOG macro is properly included and consistently used
The verification shows that:
- The LOG macro is correctly defined in
utils/utils.hwith proper fallback- The macro implementation uses variadic arguments and Flash memory optimization
- The usage pattern in
ItemToggle.hmatches the consistent format used across the codebase:
- Component name followed by action
- Additional context values when relevant
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify LOG macro definition and its usage pattern across files # Check if LOG macro is defined in utils.h rg -A 3 "LOG\s*\(" "src/utils/utils.h" # Check consistency of LOG usage pattern across files rg -A 1 "LOG\s*\([^)]+\)" "src/"Length of output: 4837
src/MenuScreen.cpp (5)
71-71: Refer to previous review comment about adding context to back navigation logging.
78-78: LGTM! Logging implementation is correct and informative.The log message includes the viewShift parameter which provides valuable context for debugging horizontal scrolling.
85-85: LGTM! Logging implementation is correct and informative.The log message includes the viewShift parameter which provides valuable context for debugging horizontal scrolling.
97-97: LGTM! Logging implementation is correct and informative.The log message includes the cursor position which provides valuable context for debugging vertical navigation.
105-105: LGTM! Logging implementation is correct and informative.The log message includes the cursor position which provides valuable context for debugging vertical navigation.
src/renderer/MenuRenderer.h (1)
116-116: LGTM: Logging mechanism update is appropriate.The change from
printLogtoLOGaligns with the PR's objective of standardizing the logging mechanism across the codebase.src/ItemList.h (4)
102-102: LGTM: Appropriate logging placement after edit mode exit.The LOG placement captures the final state after exiting edit mode and executing the callback.
118-118: LGTM: Appropriate logging placement after edit mode entry.The LOG placement provides symmetry with the exit logging and captures the state when entering edit mode.
132-132: LGTM: Logging placement aligns with test requirements.Keeping the LOG call outside the if block is intentional to support test assertions for both change and no-change scenarios.
141-141: LGTM: Logging placement consistent with selectPrevious.The LOG placement follows the same pattern as selectPrevious(), supporting test assertions as intended.
src/ItemRangeBase.h (4)
66-66: LGTM: Logging change maintains functionality.The conversion to the new LOG macro preserves the necessary debug information while aligning with the new logging standard.
78-78: LGTM: Consistent with increment() logging pattern.The logging change maintains symmetry with the increment() method and follows the same logging standard.
125-125: LGTM: Edit mode transition logging updated correctly.The conversion to the LOG macro maintains the necessary state transition information while standardizing the logging mechanism.
Also applies to: 151-151
66-66: Verify thread safety of the new LOG macro.While the logging changes are consistent, it's important to ensure the new LOG macro is thread-safe, especially for concurrent menu operations.
Also applies to: 78-78, 125-125, 151-151
src/ItemInputCharset.h (3)
60-60: LGTM! Logging change looks good.The LOG macro is correctly implemented to track character selection during upward navigation.
67-67: LGTM! Logging change looks good.The LOG macro is correctly implemented to track character selection during downward navigation.
127-127: LGTM! Logging changes respect the established guidelines.The LOG macro implementation in both methods maintains the desired level of logging detail, following the established guideline to keep log messages concise.
Also applies to: 143-143
src/input/KeyboardAdapter.h (2)
139-139: LGTM - Respecting previous feedback on logging.Based on previous feedback, keeping these log statements commented out aligns with the author's preferences.
Also applies to: 143-143
201-201: LGTM - Clean logging implementation.The logging implementation is consistent and matches the established style. The state machine properly resets after processing C2 terminal symbols, maintaining clean state transitions.
Let's verify the state machine transitions:
Also applies to: 204-204, 210-210, 216-216, 219-219
✅ Verification successful
Let me gather more information about the state machine implementation to ensure completeness of the verification.
Let me check one more aspect to verify the complete state machine implementation, particularly focusing on the CSI handling logic.
State machine implementation verified - All transitions and resets are properly handled
The verification confirms that:
- The state machine properly handles CSI sequences with appropriate buffer management
- Reset calls are correctly placed after:
- Unsupported C1 commands
- C2 terminal symbols
- Unknown code sets
- Buffer management is bounded (CSI_BUFFER_SIZE = 5) with proper cursor tracking
- All special key logging (End, Home, Insert, PgUp, PgDn) is consistently implemented within the state machine
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify state machine implementation completeness # Look for any potential missing state transitions or inconsistent reset calls # Test 1: Check for all CodeSet enum values being handled ast-grep --pattern 'enum class CodeSet { $$$ }' # Test 2: Verify reset() calls in all necessary switch cases rg -A 2 'reset\(\);.*// Reset after' src/input/KeyboardAdapter.hLength of output: 483
Script:
#!/bin/bash # Let's analyze the state machine implementation more thoroughly # Check the CodeSet enum definition and its usage in switch statements ast-grep --pattern 'switch ($codeSet) { $$$ }' # Look for the csiBuffer and related state management rg -B 2 -A 5 'csiBuffer\[csiBufferCursor\]' src/input/KeyboardAdapter.h # Check for any other reset() calls we might have missed rg 'reset\(\)' src/input/KeyboardAdapter.hLength of output: 720
Script:
#!/bin/bash # Analyze CSI handling and state transitions # Look for CSI-related constants and their usage rg -B 2 -A 2 'CSI' src/input/KeyboardAdapter.h # Check the handleReceived method implementation ast-grep --pattern 'void handleReceived(uint8_t command) { $$$ }' # Look for buffer size constants and bounds checking rg 'csiBufferCursor' -B 2 -A 2 src/input/KeyboardAdapter.hLength of output: 1966
src/ItemInput.h (2)
103-103: LGTM! Logging implementation is well-placed.The logging is correctly positioned after the value change and within the conditional block, ensuring we only log actual changes.
103-103: Verify the LOG macro implementation.The logging changes consistently use the new LOG macro. Let's verify its implementation to ensure proper functionality.
Also applies to: 181-181, 194-194, 211-211, 232-232, 253-253, 290-290, 300-300
✅ Verification successful
Let me check the LOG macro implementation in more detail by examining its definition in utils.h.
I can now generate the final response based on the verification results.
LOG macro implementation is properly defined and consistently used
The LOG macro is well-implemented in
src/utils/utils.hwith:
- A preprocessor macro that forwards arguments to the
logfunction- Two inline function overloads for logging:
- Single argument version for command-only logs
- Template version for command-with-value logs
- Proper use of Flash memory storage with
F()macro- All usage patterns across the codebase follow the correct format
The implementation aligns well with the logging changes in the PR, providing both debug-enabled and no-op versions of the macro.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the LOG macro definition and its implementation echo "Searching for LOG macro definition..." rg -l "^#define\s+LOG\b" --type cpp echo "Checking LOG macro usage patterns..." rg "LOG\s*\(" --type cpp -A 1Length of output: 4794
Script:
#!/bin/bash # Get the complete LOG macro implementation from utils.h echo "LOG macro implementation:" rg -A 5 "^#define\s+LOG\b" src/utils/utils.h echo -e "\nChecking if there are any log function implementations:" rg "^.*void\s+log\s*\(" src/utils/utils.h -A 3Length of output: 782
Updated wait-serial messages to match expected format.
forntoh
force-pushed
the
enhancement/improved-logging
branch
from
ce85443 to
b4f958d
Compare
|
Memory usage change @ b4f958d
Click for full report table
Click for full report CSV |
Summary by CodeRabbit
New Features
printLogwithLOGacross various classes, enhancing consistency in logging practices.Bug Fixes
Documentation
Refactor
logfunction with template support, improving flexibility for logging different types.Chores
Checklist
General Requirements
breaking-changeif it introduces a breaking change.Refactor/Enhancement
enhancement.