Implementation of OPC Protocol #2092
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| /* | ||
| OPC Protocol support | ||
| */ | ||
|
|
||
| #include "wled.h" | ||
| #include "cbuf.h" | ||
|
|
||
| #ifdef WLED_ENABLE_OPC | ||
|
|
||
| #define OPC_SERVER_PORT 7890 | ||
| #define OPC_LISTEN_CHANNEL 1 | ||
| #define OPC_MAX_FRAME_STORAGE 30 | ||
| #define OPC_HEADER_SIZE 4 | ||
|
|
||
| void freeOPCClient(AsyncClient* client) { | ||
| client->close(true); | ||
| delete clientServed; | ||
| clientServed = nullptr; | ||
| } | ||
|
Comment on lines
+15
to
+19
Contributor
There was a problem hiding this comment. Critical bug: The function receives Proposed fix void freeOPCClient(AsyncClient* client) {
- client->close(true);
- delete clientServed;
+ if (client == nullptr) return;
+ client->close(true);
+ client->free();
+ delete client;
+ if (client == clientServed) clientServed = nullptr;
- clientServed = nullptr;
}🤖 Prompt for AI Agents |
||
|
|
||
| void handleOPCError(void* arg, AsyncClient* client, int8_t error) { | ||
| freeOPCClient(client); | ||
| } | ||
|
|
||
| void handleOPCDisconnect(void* arg, AsyncClient* client) { | ||
| freeOPCClient(client); | ||
| } | ||
|
|
||
| void handleOPCTimeOut(void* arg, AsyncClient* client, uint32_t time) { | ||
| freeOPCClient(client); | ||
| } | ||
|
|
||
| void handleOPCData(void* arg, AsyncClient* client, void *data, size_t len) { | ||
| if (len > opcCircularBufPtr->room()) return; | ||
| for (unsigned int i = 0; i < len; i++) { | ||
| opcCircularBufPtr->write(*((uint8_t*)data + i)); | ||
| } | ||
| } | ||
|
Comment on lines
+33
to
+38
Contributor
There was a problem hiding this comment. Missing null check on
Proposed fix void handleOPCData(void* arg, AsyncClient* client, void *data, size_t len) {
+ if (opcCircularBufPtr == nullptr) return;
if (len > opcCircularBufPtr->room()) return;
for (unsigned int i = 0; i < len; i++) {
opcCircularBufPtr->write(*((uint8_t*)data + i));
}
}🤖 Prompt for AI Agents |
||
|
|
||
| void handleOPCClient(void* arg, AsyncClient* client) { | ||
| //add client | ||
| if (clientServed != nullptr) freeOPCClient(client); | ||
| clientServed = client; | ||
|
|
||
| //clean and initialize buffer | ||
| opcDataLen = 0; | ||
| opcCircularBufPtr->flush(); | ||
| opcParserWaitingForData = false; | ||
|
|
||
| // register client events | ||
| client->onData(&handleOPCData, NULL); | ||
| client->onError(&handleOPCError, NULL); | ||
| client->onDisconnect(&handleOPCDisconnect, NULL); | ||
| client->onTimeout(&handleOPCTimeOut, NULL); | ||
| } | ||
|
Comment on lines
+40
to
+55
Contributor
There was a problem hiding this comment. Critical bug: frees wrong client when rejecting new connection. Line 42 calls Additionally, line 47 calls Proposed fix for client handling void handleOPCClient(void* arg, AsyncClient* client) {
- //add client
- if (clientServed != nullptr) freeOPCClient(client);
- clientServed = client;
+ // Reject new client if one is already being served
+ if (clientServed != nullptr) {
+ client->close(true);
+ delete client;
+ return;
+ }
+ clientServed = client;
- //clean and initialize buffer
+ // Clean and initialize buffer
opcDataLen = 0;
- opcCircularBufPtr->flush();
+ if (opcCircularBufPtr != nullptr) opcCircularBufPtr->flush();
opcParserWaitingForData = false;
-
- // register client events
- client->onData(&handleOPCData, NULL);
- client->onError(&handleOPCError, NULL);
- client->onDisconnect(&handleOPCDisconnect, NULL);
- client->onTimeout(&handleOPCTimeOut, NULL);
+
+ // Register client events
+ client->onData(&handleOPCData, NULL);
+ client->onError(&handleOPCError, NULL);
+ client->onDisconnect(&handleOPCDisconnect, NULL);
+ client->onTimeout(&handleOPCTimeOut, NULL);
}🤖 Prompt for AI Agents |
||
|
|
||
| void discardInvalidData(uint16_t len) { | ||
| if (len > opcCircularBufPtr->available()) { | ||
| opcCircularBufPtr->flush(); | ||
| } else { | ||
| for (uint16_t i = 0; i < len; i++) opcCircularBufPtr->read(); | ||
| } | ||
| } | ||
|
|
||
| void parseOPCLedValues(uint8_t chan, uint8_t cmd) { | ||
| const uint16_t opcDataLenDiv = opcDataLen / 3; | ||
| uint8_t r, g, b; | ||
| if ((chan == 0 || chan == OPC_LISTEN_CHANNEL) && cmd == 0) { | ||
| realtimeLock(realtimeTimeoutMs, REALTIME_MODE_OPC); | ||
|
|
||
| for (uint16_t i = 0; i < opcDataLenDiv && i < ledCount; i++) { | ||
| r = opcCircularBufPtr->read(); | ||
| g = opcCircularBufPtr->read(); | ||
| b = opcCircularBufPtr->read(); | ||
| setRealtimePixel(i, r, g, b, 0); | ||
|
|
||
| } | ||
| strip.show(); | ||
| if (opcDataLen > ledCount * 3) discardInvalidData(opcDataLen - ledCount * 3); | ||
| } else { | ||
| discardInvalidData(opcDataLen); | ||
| } | ||
| } | ||
|
Comment on lines
+65
to
+83
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify ledCount is defined as a global/accessible variable
rg -n '\bledCount\b' --type=cpp --type-add 'cpp:*.h' -g '!opc.cpp' | head -30Repository: wled/WLED Length of output: 595 🏁 Script executed: #!/bin/bash
# Search for ledCount declaration (extern, variable definition)
rg -n 'extern.*ledCount|^\s*uint.*ledCount|^uint.*ledCount' --type=cpp --type-add 'cpp:*.h'Repository: wled/WLED Length of output: 168 🏁 Script executed: #!/bin/bash
# Check includes in opc.cpp
head -30 wled00/opc.cpp | grep -E '^\s*#include'Repository: wled/WLED Length of output: 88 🏁 Script executed: #!/bin/bash
# Search for ledCount in FX.h
rg -n 'ledCount' wled00/FX.h | head -20Repository: wled/WLED Length of output: 35 🏁 Script executed: #!/bin/bash
# Search for ledCount in wled.h
rg -n 'ledCount' wled00/wled.hRepository: wled/WLED Length of output: 35 🏁 Script executed: #!/bin/bash
# Search for all ledCount declarations/definitions (excluding comments)
rg -n '^\s*(extern\s+)?(uint\d+_t|uint|int)\s+ledCount' --type=cpp --type-add 'cpp:*.h'Repository: wled/WLED Length of output: 35 🏁 Script executed: #!/bin/bash
# Search for ledCount usage pattern - what does the strip object have?
rg -n 'strip\.getLedCount|ledCount\s*=' wled00/ --type=cpp --type-add 'cpp:*.h' | head -20Repository: wled/WLED Length of output: 35 🏁 Script executed: #!/bin/bash
# Search for how LED count is accessed in the codebase
rg -n 'strip\.(ledCount|getLedCount|length|maxLeds)' wled00/ --type=cpp -A 1 | head -30Repository: wled/WLED Length of output: 35 🏁 Script executed: #!/bin/bash
# Check for any variable that might store LED count
rg -n '\blenstrip\b|\bstrip_len\b|\blastLed\b' wled00/ --type=cpp | head -20Repository: wled/WLED Length of output: 35 🏁 Script executed: #!/bin/bash
# Look at actual opc.cpp full context around line 65-83
sed -n '60,90p' wled00/opc.cppRepository: wled/WLED Length of output: 1058 🏁 Script executed: #!/bin/bash
# Search for ledCount as macro or in any context
rg -n 'ledCount' wled00/ --type=cpp --type-add 'cpp:*.h' -iRepository: wled/WLED Length of output: 885 🏁 Script executed: #!/bin/bash
# Check how other protocol handlers (like e131, mqtt, etc.) access LED count
rg -n 'for.*<.*ledCount|for.*<.*strip\.|setRealtimePixel' wled00/e131.cpp wled00/mqtt.cpp 2>/dev/null | head -20Repository: wled/WLED Length of output: 708 🏁 Script executed: #!/bin/bash
# Search for LED count access in FX_fcn.cpp (where setRealtimePixel is likely defined)
rg -n 'setRealtimePixel|ledCount|SEGLEN' wled00/FX_fcn.cpp | head -20Repository: wled/WLED Length of output: 35 🏁 Script executed: #!/bin/bash
# Search the entire opc.cpp for ledCount declaration
cat wled00/opc.cpp | head -70 | tail -40Repository: wled/WLED Length of output: 1202 🏁 Script executed: #!/bin/bash
# Look for what strip object provides for LED access
rg -n 'strip\.' wled00/opc.cpp -o | sort -uRepository: wled/WLED Length of output: 72 🏁 Script executed: #!/bin/bash
# Check what type of LED count the strip object has
rg -n 'class.*WS2812FX|uint.*_ledcount|_leds' wled00/FX.h | head -10Repository: wled/WLED Length of output: 86 🏁 Script executed: #!/bin/bash
# Get the full beginning of opc.cpp to see all globals
head -150 wled00/opc.cppRepository: wled/WLED Length of output: 4018 🏁 Script executed: #!/bin/bash
# Check WS2812FX class members around ledcount
rg -n '_leds|_ledcount|ledcount|getLedCount|length' wled00/FX.h -A 1 -B 1 | head -40Repository: wled/WLED Length of output: 1666 🏁 Script executed: #!/bin/bash
# Look for how many LEDs are stored - check strip initialization
rg -n 'WS2812FX.*strip|new WS2812FX' wled00/ --type=cpp -A 2 | head -20Repository: wled/WLED Length of output: 331 🏁 Script executed: #!/bin/bash
# Search for ledCount extern or global declaration in header files
rg -n 'extern.*ledCount|WLED_GLOBAL.*ledCount' wled00/ --type=cpp --type-add 'cpp:*.h'Repository: wled/WLED Length of output: 35 🏁 Script executed: #!/bin/bash
# Check if ledCount might be in a previous version or another branch
# Let's look for what globals are defined in wled.h that relate to LED count
rg -n 'WLED_GLOBAL.*uint.*LED|WLED_GLOBAL.*ledCount' wled00/wled.hRepository: wled/WLED Length of output: 308 🏁 Script executed: #!/bin/bash
# Verify getLengthTotal() works and is how other code accesses LED count
rg -n 'getLengthTotal\|_length' wled00/ --type=cpp | grep -v 'MIN_SHOW_DELAY\|groupLength' | head -20Repository: wled/WLED Length of output: 35 🏁 Script executed: #!/bin/bash
# Search for any declaration of ledCount as a variable (not in comments)
rg -n '(uint\d+_t|uint|int)\s+ledCount\s*[;=]' wled00/ --type=cpp --type-add 'cpp:*.h'Repository: wled/WLED Length of output: 35 🏁 Script executed: #!/bin/bash
# Check if ledCount might be a macro
rg -n '#define ledCount' wled00/ --type=cpp --type-add 'cpp:*.h'Repository: wled/WLED Length of output: 35 🏁 Script executed: #!/bin/bash
# Look at the entire wled.h for global declarations
grep -n 'ledCount\|_length' wled00/wled.h | head -20Repository: wled/WLED Length of output: 35 🏁 Script executed: #!/bin/bash
# Check if ledCount is perhaps defined conditionally or in another included file
rg -n 'ledCount' wled00/ --type=cpp -B 2 -A 2 | grep -E '^\s*(extern|static|const|uint)' | head -20Repository: wled/WLED Length of output: 35 🏁 Script executed: #!/bin/bash
# Do a comprehensive search - ledCount must be used somewhere or code won't compile
# Let's check if it's perhaps a very recent addition or if code is broken
git log --oneline -20 wled00/opc.cpp 2>/dev/null || echo "Git not available"Repository: wled/WLED Length of output: 93 🏁 Script executed: #!/bin/bash
# Check what the correct method should be - strip has getLengthTotal()
# Let's see if other realtime handlers use it
rg -n 'realtimeTimeoutMs|setRealtimePixel|getLengthTotal' wled00/mqtt.cpp wled00/ws.cpp 2>/dev/null | head -30Repository: wled/WLED Length of output: 112 🏁 Script executed: #!/bin/bash
# Final check: look at compilation context - search for any conditional defines
# that might create ledCount
rg -n 'ledCount' wled00/ -B 5 -A 1 | head -50Repository: wled/WLED Length of output: 2957 🏁 Script executed: #!/bin/bash
# Check if there's a forward declaration or extern in any header included by opc.cpp
rg -n 'extern.*ledCount' wled00/fcn_declare.h wled00/wled.h wled00/FX.h 2>/dev/null || echo "No extern ledCount found"Repository: wled/WLED Length of output: 77 🏁 Script executed: #!/bin/bash
# Final verification: check the actual current state of the OPC branch
# Look for any variable or macro that might shadow ledCount
rg -n '\bledCount\b' wled00/opc.cpp -B 10 | head -40Repository: wled/WLED Length of output: 1130 🏁 Script executed: #!/bin/bash
# Verify ws.cpp approach works - confirm getLengthTotal() is the right method
rg -n 'class WS2812FX' wled00/FX.h -A 150 | grep -E 'getLengthTotal|getLength' | head -5Repository: wled/WLED Length of output: 87 🏁 Script executed: #!/bin/bash
# One final check - search entire repository for any ledCount declaration or definition
find wled00 -name "*.h" -o -name "*.cpp" | xargs grep -l 'ledCount' | xargs grep -n '^[^/]*\(extern\|static\|uint\|int\).*ledCount'Repository: wled/WLED Length of output: 253 🏁 Script executed: #!/bin/bash
# Verify the exact error - see if ledCount could possibly be defined anywhere
rg -n 'ledCount\s*=' wled00/ --type=cpp --type-add 'cpp:*.h' -B 2Repository: wled/WLED Length of output: 35 🏁 Script executed: #!/bin/bash
# Check if there's a using statement or alias
rg -n 'using.*ledCount|#define.*ledCount' wled00/ --type=cpp --type-add 'cpp:*.h'Repository: wled/WLED Length of output: 479 Replace undeclared
Additionally, verify that 🤖 Prompt for AI Agents |
||
|
|
||
| void handleOPCPacket() { | ||
| // channel and command variables need staticity because of fragmented packet possibility | ||
| static uint8_t chan, cmd; | ||
|
|
||
| if (opcCircularBufPtr != nullptr && millis() - strip.getLastShow() > 15) { | ||
| if (opcCircularBufPtr->available() >= 4 && !realtimeOverride) { | ||
| //there's at least a header and no override | ||
| if(!opcParserWaitingForData) { | ||
| chan = (uint8_t)opcCircularBufPtr->read(); | ||
| cmd = (uint8_t)opcCircularBufPtr->read(); | ||
| opcDataLen = (uint8_t)opcCircularBufPtr->read() << 8 | (uint8_t)opcCircularBufPtr->read(); | ||
| if (opcDataLen > opcCircularBufPtr->available()) { | ||
| opcParserWaitingForData = true; | ||
| } else { | ||
| parseOPCLedValues(chan, cmd); | ||
| } | ||
| } else { | ||
| if (opcDataLen <= opcCircularBufPtr->available()) { | ||
| parseOPCLedValues(chan, cmd); | ||
| opcParserWaitingForData = false; | ||
| } | ||
| } | ||
| } else if (realtimeOverride && opcCircularBufPtr->available()) { | ||
| opcCircularBufPtr->flush(); | ||
| opcParserWaitingForData = false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void initOPCServer() { | ||
| if (opcServer != nullptr) { | ||
| opcServer->end(); | ||
| delete opcServer; | ||
| opcServer = nullptr; | ||
| } | ||
| if (opcCircularBufPtr != nullptr) { | ||
| opcCircularBufPtr->flush(); | ||
| delete opcCircularBufPtr; | ||
| opcCircularBufPtr = nullptr; | ||
| } | ||
| opcCircularBufPtr = new cbuf(OPC_MAX_FRAME_STORAGE * (ledCount + OPC_HEADER_SIZE)); | ||
| opcServer = new AsyncServer(OPC_SERVER_PORT); | ||
| opcServer->onClient(&handleOPCClient, opcServer); | ||
| opcServer->setNoDelay(true); | ||
| opcServer->begin(); | ||
|
|
||
| opcDataLen = 0; | ||
| opcParserWaitingForData = false; | ||
| } | ||
|
|
||
| #else | ||
| void initOPCServer(){} | ||
| void handleOPCClient() {} | ||
| void freeOPCClient(){} | ||
| void handleOPCData() {} | ||
| void handleOPCTimeOut (){} | ||
| void handleOPCError (){} | ||
| void handleOPCDisconnect (){} | ||
| #endif | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Function declarations lack
#ifdef WLED_ENABLE_OPCguard, and stub signatures mismatch.The OPC function declarations here have full signatures (e.g.,
void handleOPCClient(void* arg, AsyncClient* client)), but the stubs inopc.cpp(lines 137-142) have empty parameter lists (e.g.,void handleOPCClient() {}). This will cause linker errors whenWLED_ENABLE_OPCis not defined.Proposed fix: wrap declarations in ifdef and fix stub signatures
In
fcn_declare.h:In
opc.cpp, fix stubs:`#else` void initOPCServer(){} -void handleOPCClient() {} -void freeOPCClient(){} -void handleOPCData() {} -void handleOPCTimeOut (){} -void handleOPCError (){} -void handleOPCDisconnect (){} +void handleOPCPacket(){} `#endif`🤖 Prompt for AI Agents