Feature/fw795 map6s add peaks #856
Conversation
WalkthroughThis pull request introduces version 2.154.1 of the Changes
Possibly related PRs
Suggested reviewers
Poem
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
templates/config-map6s-fw2.json (1)
284-294: Verify consistency in peak current channel configurations.The peak current channels (Ipeak 1-6) are consistently configured with:
- Correct register type (input)
- Consistent scale (0.016)
- Consistent format (s32)
- Consistent word order (little_endian)
- Logical address progression (0x181c, 0x181a, 0x1818, 0x281c, 0x281a, 0x2818)
However, consider adding documentation about:
- The meaning of the scale factor (0.016)
- The expected range of values
Also applies to: 399-409, 514-524, 629-639, 744-754, 859-869
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/TDeviceTemplatesTest.Validate.datis excluded by!**/*.dat
📒 Files selected for processing (2)
debian/changelog(1 hunks)templates/config-map6s-fw2.json(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (5)
debian/changelog (1)
1-5: LGTM! The changelog entry is well-formatted.The changelog entry follows the standard format and provides clear information about the changes:
- Version number follows semantic versioning
- Distribution and urgency are properly set
- Change description is concise and clear
- Maintainer information and timestamp are correctly formatted
templates/config-map6s-fw2.json (4)
260-269: Review the peaks measurement period parameter configuration.The parameter configuration looks good with appropriate constraints:
- Minimum value of 1 second prevents too frequent measurements
- Default value of 60 seconds is reasonable
- Clear firmware version requirement is specified
975-986: Review peak voltage channel configuration.The Upeak channel configuration includes an important error value (0x7FFFFFFF) that's not present in other peak channels.
Please clarify:
- Should the Ipeak channels also have an error_value field?
- Is 0x7FFFFFFF a special indicator value for this specific channel?
1091-1091: Verify translation completeness.The translations for firmware version requirements are properly added in both English and Russian.
Also applies to: 1131-1132
Line range hint
260-986: Verify firmware version compatibility.The changes require firmware v2.3.0 or newer, but some existing features require v2.10.0.
Run this script to check version compatibility:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
templates/config-map6s-fw2.json (2)
989-1000: Document the error value usage.The Upeak channel includes an error_value (0x7FFFFFFF) but lacks documentation explaining when this value is returned. Consider adding a description to clarify its usage.
286-296: Add firmware version requirement to peak measurement channels.While the peaks_measurement_period_s parameter specifies the firmware version requirement (v2.3.0+), this requirement is not documented for the peak measurement channels (Ipeak 1-6, Upeak). Consider adding the same description to these channels for clarity.
Also applies to: 403-413, 520-530, 637-647, 754-764, 871-881, 989-1000
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
debian/changelog(1 hunks)templates/config-map6s-fw2.json(24 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- debian/changelog
🔇 Additional comments (3)
templates/config-map6s-fw2.json (3)
260-269: LGTM! Well-structured parameter configuration.The peaks measurement period parameter is properly defined with appropriate constraints, firmware version requirement, and translations.
278-279: LGTM! Consistent type and units specification.The change from specific types to generic "value" type with explicit units improves consistency across all measurements.
Also applies to: 395-396, 512-513, 629-630, 746-747, 863-864, 980-981, 1034-1035, 1064-1065
286-296: Verify the scale factor for peak current measurements.The scale factor for peak current measurements (0.016) is significantly different from regular current measurements (2.44141e-7). Please verify that this is intentional and correct according to the firmware specifications.
Run the following script to search for any documentation or tests that validate these scale factors:
Also applies to: 403-413, 520-530, 637-647, 754-764, 871-881
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
templates/config-map6s-fw2.json (1)
Line range hint
1098-1139: Add translations for peak measurement channels.While firmware version requirement translations were added, the following translations are missing for the new channels:
- "Ipeak 1" through "Ipeak 6" in Russian
- "Upeak" in Russian
Apply this diff to add the missing translations:
"ru": { "WB-MAP6S-fw2_template_title": "WB-MAP6S fw2 (многоканальный счетчик электроэнергии)", + "Ipeak 1": "Пиковый ток 1", + "Ipeak 2": "Пиковый ток 2", + "Ipeak 3": "Пиковый ток 3", + "Ipeak 4": "Пиковый ток 4", + "Ipeak 5": "Пиковый ток 5", + "Ipeak 6": "Пиковый ток 6", + "Upeak": "Пиковое напряжение", "Channel 1": "Канал 1",
🧹 Nitpick comments (1)
templates/config-map6s-fw2.json (1)
981-993: Consider standardizing error value handling across channels.The Upeak channel includes an
error_valuefield (0x7FFFFFFF) while other channels don't have this field. Consider:
- Adding error values to other channels if they can report errors
- Documenting why only Upeak has this field if it's intentional
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/TDeviceTemplatesTest.Validate.datis excluded by!**/*.dat
📒 Files selected for processing (2)
debian/changelog(1 hunks)templates/config-map6s-fw2.json(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- debian/changelog
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
templates/config-map6s-fw2.json (2)
260-269: LGTM! Parameter configuration looks good.The
peaks_measurement_period_sparameter is well-structured with appropriate constraints and firmware version requirement.
284-295: Verify the scale factor for peak current measurements.The scale factor for Ipeak channels (0.016) is significantly different from Irms channels (2.44141e-7). While this might be intentional due to different measurement methods, please verify that this is correct according to the firmware specifications.
Also applies to: 400-411, 516-527, 632-643, 748-759, 864-875
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
templates/config-map6s-fw2.json (1)
974-975: Consider adding error_value to Urms channel for consistency.The voltage channels are properly configured, but for consistency with Upeak, consider adding error_value to Urms as well.
Apply this diff:
"type": "value", "units": "V", + "error_value": "0x7FFFFFFF", "semi-sporadic": true,Also applies to: 982-994
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
templates/config-map6s-fw2.json(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
templates/config-map6s-fw2.json (3)
260-269: LGTM: Well-structured parameter configuration.The
peaks_measurement_period_sparameter is properly configured with:
- Reasonable min (1) and default (60) values
- Clear firmware version requirement
- Proper grouping and order
284-295: LGTM: Consistent peak current channel configurations.The peak current channels (Ipeak 1-6) follow a consistent pattern with proper:
- Register addressing scheme
- Scale and format for current measurements
- Units specification
- Word order specification
Also applies to: 400-411, 516-527, 632-643, 748-759, 864-875
1099-1099: LGTM: Proper translations provided.The translations for the new parameter and firmware requirement message are properly added in both English (implicitly) and Russian.
Also applies to: 1139-1139
Добавил регистры пиковых значений.
Summary by CodeRabbit
New Features
Documentation
Changelog
wb-mqtt-serialpackage (2.154.1) with added peaks registers feature.