-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add buttons support in WB-MWAC v.1 template #858
base: master
Are you sure you want to change the base?
Conversation
WalkthroughThis pull request introduces version 2.153.3 of the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (6)
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
debian/changelog
(1 hunks)templates/config-wb-mwac.json.jinja
(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
templates/config-wb-mwac.json.jinja (5)
10-11
: LGTM! Constants are well-defined and consistently used.The new constants
IN_MODE_BUTTON
andMMATRIX_OUTPUTS_NUMBER
follow the existing naming convention and are used consistently throughout the template.
487-526
: Button press counters are properly implemented with real-time updates.The implementation includes:
- Separate counters for each press type
- Proper sporadic flag for real-time updates
- Disabled by default to avoid unnecessary traffic
Line range hint
624-734
: Translations are complete and well-documented.The implementation includes:
- Comprehensive translations for all new features
- Clear descriptions with recommended values
- Both English and Russian translations
144-205
: Button mode parameters are well-structured with clear constraints.The implementation includes:
- Proper debounce handling with mode-specific descriptions
- Reasonable defaults and ranges for timing parameters
- Clear conditions for parameter visibility
However, consider adding validation to ensure
in{n}_debounce_ms
is 5-10 times less thanin{n}_secp_waiting_time
as mentioned in the description.Run this script to verify the relationship between debounce and waiting time:
✅ Verification successful
Default timing parameters are correctly configured, but runtime validation is needed
The default values (debounce: 50ms, waiting time: 300ms) maintain the recommended 5-10x relationship. However, since these are configurable parameters, consider implementing runtime validation to ensure this relationship is maintained when users modify the values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that debounce time is 5-10 times less than waiting time # Expected: No instances where debounce >= waiting_time/5 for i in {1..6}; do echo "Checking input $i..." # Get debounce value rg -A 2 "in${i}_debounce_ms.*default.*" | grep "default" # Get waiting time value rg -A 2 "in${i}_secp_waiting_time.*default.*" | grep "default" doneLength of output: 1085
206-271
: Button press actions are comprehensive and well-organized.The implementation includes:
- Four types of button presses with appropriate actions
- Different actions for alarm vs regular outputs
- Proper address calculations using offsets
Consider adding validation to ensure the address ranges don't overlap with existing registers.
Run this script to check for address overlaps:
✅ Verification successful
Button press action addresses are properly separated and non-overlapping
The address calculations use well-defined ranges with sufficient gaps:
- Base addresses (544, 608, 672, 736) are separated by 64 addresses
- Each input's block of 8 addresses fits within these gaps
- No other template addresses overlap with these ranges
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential address overlaps in register mappings # Expected: No overlapping addresses # Extract all address definitions rg '"address":\s*\d+' -o | sort -n | uniq -dLength of output: 18618
Script:
#!/bin/bash # Description: Check for address overlaps specifically in button press action ranges # Expected: No overlapping addresses in these ranges # Extract addresses from config-wb-mwac.json.jinja and check if they fall in our ranges rg '"address":\s*(\d+)' -o templates/config-wb-mwac.json.jinja | grep -o '[0-9]\+' | sort -n | awk ' ($1 >= 544 && $1 <= 799) { range = "unknown" if ($1 >= 544 && $1 <= 607) range = "short press" else if ($1 >= 608 && $1 <= 671) range = "long press" else if ($1 >= 672 && $1 <= 735) range = "double press" else if ($1 >= 736 && $1 <= 799) range = "short then long press" print $1, range } ' | uniq -dLength of output: 448
Script:
#!/bin/bash # Description: Check if file exists and show all addresses in button press ranges # Expected: See all addresses in the ranges 544-799 # First check if file exists echo "Checking file existence:" ls -l templates/config-wb-mwac.json.jinja echo -e "\nAll addresses in button press ranges:" rg '"address":\s*(\d+)' -o templates/config-wb-mwac.json.jinja | grep -o '[0-9]\+' | sort -n | awk ' ($1 >= 544 && $1 <= 799) { range = "unknown" if ($1 >= 544 && $1 <= 607) range = "short press" else if ($1 >= 608 && $1 <= 671) range = "long press" else if ($1 >= 672 && $1 <= 735) range = "double press" else if ($1 >= 736 && $1 <= 799) range = "short then long press" print $1, range } 'Length of output: 718
Script:
#!/bin/bash # Description: Check address calculations in Jinja template # Expected: Find address calculation patterns for button actions echo "Searching for address calculations:" rg --multiline '"address":\s*\{\{.*?\}\}' templates/config-wb-mwac.json.jinja echo -e "\nChecking for MMATRIX_OUTPUTS_NUMBER definition:" rg "MMATRIX_OUTPUTS_NUMBER\s*=" templates/config-wb-mwac.json.jinjaLength of output: 1839
debian/changelog (1)
1-5
: LGTM! The changelog entry follows the Debian format.The changelog entry is well-formatted and includes all required fields:
- Package name and version
- Distribution and urgency
- Change description
- Author's name and email
- Timestamp
Добавил поддержку нажатий в шаблон WB-MWAC v.1, в прошивке поддержка уже есть.
Summary by CodeRabbit
New Features
Documentation
Version Update