ENG-2276: fix for same Item.Start but different Item.Bit access #108
Conversation
WalkthroughThe pull request modifies the duplicate address detection logic in the S7 communication plugin. The changes enhance the specificity of address comparison by utilizing Changes
Assessment against linked issues
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 (2)
s7comm_plugin/s7comm_test.go (2)
71-96: Add test cases for additional edge cases.The current test suite covers basic scenarios well, but consider adding these edge cases:
- Addresses with same Start but different WordLen (e.g., "DB2.W0" vs "DB2.DW0")
- Addresses with same Start but mixed access types (e.g., "DB2.W0" vs "DB2.X0.0")
tests := []testCase{ + { + addresses: []string{"DB2.W0", "DB2.DW0"}, + expectedErrorMsg: nil, + }, + { + addresses: []string{"DB2.W0", "DB2.X0.0"}, + expectedErrorMsg: nil, + }, // ... existing test cases ... }
98-114: Improve test case descriptions for better readability.While the test logic is solid, the test description could be more specific about the scenario being tested.
-By("Testing with addresses"+fmt.Sprintf("%v", tc.addresses), func() { +By(fmt.Sprintf("Testing address combination: %v (expecting %v)", + tc.addresses, + tc.expectedErrorMsg != nil ? "duplicate error" : "no error" +), func() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
s7comm_plugin/s7comm.go(1 hunks)s7comm_plugin/s7comm_test.go(2 hunks)
🔇 Additional comments (2)
s7comm_plugin/s7comm.go (1)
182-184: LGTM! The duplicate check now correctly handles bit-wise access.The modified condition properly identifies duplicates by comparing all relevant fields (Area, DBNumber, Start, and Bit), which aligns with the PR objective of supporting different bit access for the same start address.
Let's verify that this change doesn't break existing functionality:
✅ Verification successful
Verified: The duplicate check modification is safe and doesn't impact existing functionality
The bit-wise access appears to be a new feature, and the modified duplicate check is the only place where
Item.Bitis used. This confirms that the change is backward compatible and won't break any existing functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing bit-wise address patterns in the codebase # to ensure we're not breaking existing usage rg -l 'Item\.Bit.*Item\.Start' --type goLength of output: 42
Script:
#!/bin/bash # Search for any usage of Bit field in Item struct rg "Item\.Bit" --type go -A 2 # Look for test files that might validate address handling fd ".*_test\.go" --type f --exec grep -l "Item" # Check the definition of Item struct ast-grep --pattern 'type Item struct { $$$ }'Length of output: 507
s7comm_plugin/s7comm_test.go (1)
65-68: LGTM! Well-structured test case definition.The test case structure is clean and appropriately captures both the input addresses and expected error messages for duplicate detection scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
s7comm_plugin/s7comm.go (1)
Line range hint
271-275: Fix buffer handling to prevent data accumulation.The buffer is never reset between messages, which could lead to memory growth and incorrect data as each message contains all previous messages' data.
Apply this diff to fix the buffer handling:
-buffer := make([]byte, 0) +for _, item := range b { + // Create a new buffer for each message + buffer := make([]byte, 0) -for _, item := range b { // Execute the converter function to get the converted data convertedData := item.ConverterFunc(item.Item.Data)
🧹 Nitpick comments (3)
s7comm_plugin/s7comm_test.go (1)
87-87: Fix typo in test case description.- Entry("same DBNumber but differen Item.Bit", + Entry("same DBNumber but different Item.Bit",🧰 Tools
🪛 GitHub Actions: opcua-plc
[warning] 23: Test skipped: environment variables are not set
s7comm_plugin/s7comm.go (2)
183-185: LGTM! The enhanced duplicate detection is more accurate.The use of
reflect.DeepEqualproperly handles all fields ofS7DataItem, including theBitfield, which fixes the issue with duplicate detection for items having the sameStartbut differentBitvalues.For future optimization, if performance becomes a concern, consider replacing
reflect.DeepEqualwith direct field comparison:-if reflect.DeepEqual(a.Item, b.Item) { +if a.Item.Area == b.Item.Area && + a.Item.WordLen == b.Item.WordLen && + a.Item.Bit == b.Item.Bit && + a.Item.DBNumber == b.Item.DBNumber && + a.Item.Start == b.Item.Start && + a.Item.Amount == b.Item.Amount {However, this optimization is not critical as:
- This code runs during initialization, not in the hot path
- The number of addresses is typically small
- The current implementation is more maintainable
Line range hint
264-269: Improve error message clarity in ReadBatch.The error message mentions "Reconnecting..." but the actual reconnection is handled by Benthos's auto-retry mechanism, which could be misleading.
Apply this diff to improve the error message:
- errMsg := fmt.Sprintf("Failed to read batch %d: %v. Reconnecting...", i+1, err) + errMsg := fmt.Sprintf("Failed to read batch %d: %v", i+1, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
s7comm_plugin/s7comm.go(2 hunks)s7comm_plugin/s7comm_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: opcua-plc
s7comm_plugin/s7comm_test.go
[warning] 23: Test skipped: environment variables are not set
🪛 GitHub Actions: tag-processor
s7comm_plugin/s7comm_test.go
[warning] 133-133: Test skipped: environment variables not set
🔇 Additional comments (2)
s7comm_plugin/s7comm_test.go (1)
63-81: Well-structured table-driven tests!The implementation follows good testing practices and properly addresses the previous review comment about using ginkgo table-driven tests.
🧰 Tools
🪛 GitHub Actions: opcua-plc
[warning] 23: Test skipped: environment variables are not set
s7comm_plugin/s7comm.go (1)
21-21: LGTM!The addition of the reflect package is necessary for the enhanced duplicate detection logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
s7comm_plugin/s7comm_test.go (1)
82-111: Consider adding more edge cases to strengthen test coverage.The current test entries provide good coverage of common scenarios. Consider adding these additional test cases to make the test suite more robust:
- Mixed area types (e.g., DB vs PE with same addresses)
- Edge cases with maximum/minimum values
- Invalid address formats
Example additions:
+ Entry("same address in different areas", + S7Addresses{ + addresses: []string{"DB2.W0", "PE2.W0"}, + expectedErrMsg: nil, + }), + Entry("invalid address format", + S7Addresses{ + addresses: []string{"DB2.W0", "DB2.INVALID"}, + expectedErrMsg: []string{"invalid address format"}, + }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
s7comm_plugin/s7comm_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-docker (amd64)
- GitHub Check: go-test-tag-processor
- GitHub Check: go-test-opcua-plc
- GitHub Check: go-test-s7-plc
- GitHub Check: go-test-sensorconnect
- GitHub Check: go-test-nodered-js
🔇 Additional comments (2)
s7comm_plugin/s7comm_test.go (2)
63-66: LGTM! Well-structured test data type.The
S7Addressesstruct is well-defined with clear and descriptive field names.
68-81: LGTM! Well-implemented table-driven tests.The implementation successfully addresses the previous review feedback by using Ginkgo's table-driven test pattern. The error handling is thorough and well-structured.
Description:
Item.Startbut a differentItem.BitItem.WordLenisn't set to bit-wise access the fieldBitshould contain the same valueTesting:
Summary by CodeRabbit
New Features
Tests