Re: [PR] Modbus Coil/DI array fix, support for Word Swap

[via GitHub]

chrisdutz commented on code in PR #2192:
URL: https://github.com/apache/plc4x/pull/2192#discussion_r2225107036


##########
plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/base/optimizer/ModbusOptimizer.java:
##########
@@ -436,6 +451,32 @@ private ReadBuffer getReadBuffer(byte[] data, 
ModbusByteOrder byteOrder) {
                 byte[] reordered = ModbusProtocolLogic.byteSwap(data);
                 return new ReadBufferByteBased(reordered, 
ByteOrder.LITTLE_ENDIAN);
             }
+            case BIG_ENDIAN_WORD_SWAP: {

Review Comment:
   I'm sort of not entirely happy with the names. I asked chatgpt for help 😉 
here's the response:
   
   You're dealing with eight different byte/word permutations for Modbus data 
interpretation, which is common when mapping PLC registers to types like int32, 
float, or double. The current enum names are a bit ambiguous and repetitive.
   
   🧠 Goal
   
   Come up with more descriptive, unambiguous names that clearly convey:
   
   Byte order within a word (e.g., big vs. little endian)
   
   Word order (e.g., whether 16-bit words are swapped)
   
   Whether both are swapped (combined swap)
   
   🔢 First, mapping by example
   
   Let's take the base array:
   [1, 2, 3, 4, 5, 6, 7, 8]
   Split into 16-bit words (2 bytes each):
   [1,2], [3,4], [5,6], [7,8]
   
   Then apply the transformations per enum:
   
   EnumResultByte OrderWord OrderByte Swap?Word 
Swap?BIG_ENDIAN[1,2,3,4,5,6,7,8]BigNormal❌❌LITTLE_ENDIAN[8,7,6,5,4,3,2,1]LittleReversed✅✅BIG_ENDIAN_BYTE_SWAP[2,1,4,3,6,5,8,7]LittleNormal✅❌LITTLE_ENDIAN_BYTE_SWAP[7,8,5,6,3,4,1,2]BigReversed❌✅BIG_ENDIAN_WORD_SWAP[3,4,1,2,7,8,5,6]BigSwapped❌✅LITTLE_ENDIAN_WORD_SWAP[6,5,8,7,2,1,4,3]LittleSwapped✅✅BIG_ENDIAN_WORD_SWAP_BYTE_SWAP[4,3,2,1,8,7,6,5]ReversedSwapped✅✅LITTLE_ENDIAN_WORD_SWAP_BYTE_SWAP[5,6,7,8,1,2,3,4]NormalSwapped❌✅
 
   
   ✅ Proposed Enum Names
   
   Old NameNew 
ProposalReasoningBIG_ENDIANBYTE_ORDER_BIG__WORD_ORDER_NORMALExplicitLITTLE_ENDIANBYTE_ORDER_LITTLE__WORD_ORDER_REVERSEDReflects
 full reversalBIG_ENDIAN_BYTE_SWAPBYTE_ORDER_LITTLE__WORD_ORDER_NORMALOnly byte 
swap = little 
endianLITTLE_ENDIAN_BYTE_SWAPBYTE_ORDER_BIG__WORD_ORDER_REVERSEDOnly word swap 
= word reversalBIG_ENDIAN_WORD_SWAPBYTE_ORDER_BIG__WORD_ORDER_SWAPPEDWords out 
of orderLITTLE_ENDIAN_WORD_SWAPBYTE_ORDER_LITTLE__WORD_ORDER_SWAPPEDByte+Word 
swapBIG_ENDIAN_WORD_SWAP_BYTE_SWAPBYTE_ORDER_REVERSED__WORD_ORDER_SWAPPEDBoth 
swapped, full 
reverseLITTLE_ENDIAN_WORD_SWAP_BYTE_SWAPBYTE_ORDER_NORMAL__WORD_ORDER_SWAPPEDJust
 word swap 
   
   💡 Optional Shorter Variant
   
   If you want brevity but still clarity:
   
   NORMAL, // [1,2,3,4] REVERSED, // [4,3,2,1] BYTE_SWAP, // [2,1,4,3] 
BYTE_SWAP_REVERSED, // [7,8,5,6,3,4,1,2] WORD_SWAP, // [3,4,1,2] 
BYTE_WORD_SWAP, // [6,5,8,7,2,1,4,3] FULL_REVERSED, // [4,3,2,1,8,7,6,5] 
WORD_SWAP_CONTIGUOUS // [5,6,7,8,1,2,3,4] 
   
   Would you prefer long descriptive or shorter but still clear names? I can 
adjust accordingly.
   
   



##########
plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/base/optimizer/ModbusOptimizer.java:
##########
@@ -436,6 +451,32 @@ private ReadBuffer getReadBuffer(byte[] data, 
ModbusByteOrder byteOrder) {
                 byte[] reordered = ModbusProtocolLogic.byteSwap(data);
                 return new ReadBufferByteBased(reordered, 
ByteOrder.LITTLE_ENDIAN);
             }
+            case BIG_ENDIAN_WORD_SWAP: {

Review Comment:
   Argh.. It fu**ed up the table 🙁



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@plc4x.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org