From 08a85d1c0667979ad3252a9b8c30e5bf429a2dfe Mon Sep 17 00:00:00 2001 From: Matthew Reed Date: Thu, 27 Aug 2026 20:11:22 +1200 Subject: [PATCH 1/3] fix(build): compile C++ libraries carried in a STruC++ upload GEN_CPP globbed the top level only, so a library under core/generated/libraries// was never compiled and its headers were off the include path. Discover sources recursively, add each library's src/ as an include root, and mkdir the object directory for nested sources. --- scripts/Makefile.strucpp | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/scripts/Makefile.strucpp b/scripts/Makefile.strucpp index cebd1ccb..753dcb20 100644 --- a/scripts/Makefile.strucpp +++ b/scripts/Makefile.strucpp @@ -5,11 +5,15 @@ # # - Per-file compilation rules let `make -j` saturate every core on # the target platform (Pi 4 = 4×, x86 servers = 8–32×). -# - `wildcard $(GENERATED_DIR)/*.cpp` discovers however many .cpp -# files STruC++ split into. The codegen emits one TU per POU plus -# a shared configuration.cpp, so the file set varies per project -# — keeping the list out of this file means no Makefile churn -# when POUs are added or removed. +# - Sources are discovered by searching $(GENERATED_DIR), so the +# file set varies per project with no Makefile churn when POUs +# are added or removed. The codegen emits one TU per POU plus a +# shared configuration.cpp. +# - A project can also carry C++ libraries under +# $(GENERATED_DIR)/libraries//, each an ordinary library +# folder with its sources under src/. Every such src/ goes on the +# include path and its sources are compiled, so a block resolves +# `#include ` the same way it does on Arduino. # - `ccache` is picked up automatically when present. Re-running a # build where only one POU's body changed reuses every other .o # from the cache, so incremental builds drop from minutes to a @@ -73,9 +77,16 @@ CC := $(if $(CCACHE),$(CCACHE) gcc,gcc) # preserve. Verified on an SLM-RP4: with the flag the .so leaves # /proc//maps on every stop and re-maps fresh on every start; without it, # it never leaves -- one build's image stays pinned for the life of the process. +# Libraries the editor materialised from enabled .stlib archives. Each is an +# ordinary library folder, so `src/` is its include root; a folder without one +# is taken as its own root. +RESOURCE_LIB_DIRS := $(wildcard $(GENERATED_DIR)/libraries/*) +RESOURCE_LIB_INC := $(foreach d,$(RESOURCE_LIB_DIRS),$(if $(wildcard $d/src),$d/src,$d)) + CXXFLAGS := -std=c++17 -O1 -pipe -fPIC -Wall -DSTRUCPP_THREADED -fno-gnu-unique -MMD -MP \ -Wno-unknown-pragmas -Wno-deprecated-declarations \ - -I $(GENERATED_DIR) -I $(RUNTIME_INC) -I $(PYTHON_INC) + -I $(GENERATED_DIR) -I $(RUNTIME_INC) -I $(PYTHON_INC) \ + $(addprefix -I ,$(RESOURCE_LIB_INC)) # Python POU stubs (`{external …}` blocks emitted by the editor) call # `getpid()`, `create_shm_name()`, `python_block_loader()`. Those @@ -85,10 +96,11 @@ CXXFLAGS := -std=c++17 -O1 -pipe -fPIC -Wall -DSTRUCPP_THREADED -fno-gnu-unique # any Python POU pay nothing; the header is declarations only. GENERATED_CXXFLAGS := $(CXXFLAGS) -include iec_python.h -# Discover every .cpp emitted into core/generated/. STruC++ splits -# across configuration.cpp + one pou_.cpp per POU, but this -# Makefile doesn't care — wildcard adapts. -GEN_CPP := $(wildcard $(GENERATED_DIR)/*.cpp) +# Discover every .cpp under core/generated/, at any depth: the top-level TUs +# STruC++ emitted, plus whatever each resource library carries below its own +# root. `sort` makes the order reproducible across filesystems and drops any +# duplicate. +GEN_CPP := $(sort $(shell find $(GENERATED_DIR) -name '*.cpp')) GEN_OBJ := $(patsubst $(GENERATED_DIR)/%.cpp,$(BUILD_DIR)/%.o,$(GEN_CPP)) SHIM_OBJ := $(BUILD_DIR)/runtime_v4_entry.o @@ -126,8 +138,12 @@ $(LIBPLC): $(GEN_OBJ) $(SHIM_OBJ) $(EXTRA_OBJS) | $(BUILD_DIR) # Generated TUs from the editor's upload. Force-include iec_python.h # so unqualified Python loader symbols in {external} blocks resolve. +# `$(@D)` because a nested source maps to a nested object: a library source at +# libraries/foo/src/transport/bar.cpp builds to +# $(BUILD_DIR)/libraries/foo/src/transport/bar.o. $(BUILD_DIR)/%.o: $(GENERATED_DIR)/%.cpp | $(BUILD_DIR) @echo "[INFO] Compiling $<..." + @mkdir -p $(@D) $(CXX) $(GENERATED_CXXFLAGS) -c $< -o $@ # Static runtime shim — lives in the runtime repo, not in the user From 7b9668465d939f802087ff4fc5cce6b208cb2a2f Mon Sep 17 00:00:00 2001 From: Matthew Reed Date: Thu, 24 Sep 2026 15:47:12 +1200 Subject: [PATCH 2/3] fix: keep retained values across program edits; build C sources in resource libraries - Retain store no longer discards stored values when the program MD5 changes. The layout hash check one layer up decides whether they still fit. - Makefile.strucpp now compiles .c files in vendored libraries with $(CC). It also reads `defines=` from each library's library.properties as -D flags. --- core/src/plc_app/plc_retain_file_store.cpp | 39 ++++++++-- scripts/Makefile.strucpp | 90 +++++++++++++++++++++- tests/host/test_plc_retain_file_store.cpp | 68 +++++++++++----- 3 files changed, 169 insertions(+), 28 deletions(-) diff --git a/core/src/plc_app/plc_retain_file_store.cpp b/core/src/plc_app/plc_retain_file_store.cpp index 4fbb4d03..d9467b69 100644 --- a/core/src/plc_app/plc_retain_file_store.cpp +++ b/core/src/plc_app/plc_retain_file_store.cpp @@ -319,11 +319,40 @@ int plc_retain_file_store_load(const char *program_md5, uint16_t md5_len, uint8_ if (memcmp(stored_id, program_md5, PROGRAM_ID_LEN) != 0) { - fclose(f); - discard_stored(); - log_info("Retain: stored values belong to a different program — storage cleared, " - "retained variables start at their initial values"); - return 0; + /* NOT discarded. Offered upward, and let the layout decide. + * + * PROGRAM_ID is the MD5 of program.st, so it changes on ANY edit — + * move a rung, rename a comment — and discarding here meant every + * retained value in a commissioned plant reset on every upload. That + * is the one guarantee the RETAIN qualifier exists to give: + * IEC 61131-3 §6.5.6.1 rule 1 defines a retained value as "the values + * the variables had when the resource or configuration was stopped", + * conditioned on the STARTING OPERATION being a warm restart and on + * nothing else. The words "download", "reload" and "online change" + * appear nowhere in Part 3. + * + * The check that belongs here already exists one layer up and was + * built for exactly this: ext_strucpp_retain_unpack() validates magic, + * format, crc, TRUNCATION and the LAYOUT HASH before a byte reaches a + * variable, and plc_retain.cpp names which of those failed. STruC++'s + * own debug-table-gen.ts says why it hashes the layout rather than the + * program: "a body edit leaves this unchanged and retained values + * survive, while adding, removing, retyping or reordering a retained + * variable changes it and the stored blob is refused. The project MD5 + * would have discarded retained state on every unrelated edit." + * + * So this gate was not reinforcing that design, it was defeating it — + * and ModBee's ESP32 store (NodeUioRetain.cpp) keeps the identity for + * saves and never compares it on read, which is why retain has been + * seen surviving program changes on real hardware and not here. + * + * WHAT IS GIVEN UP, stated plainly for the review: two DIFFERENT + * projects sharing one retain.bin path whose layout hashes happen to + * collide would now read each other's values. That is a 32-bit + * collision on a path nobody takes, weighed against a certainty on the + * path everybody takes. */ + log_info("Retain: stored values were written by a different program — " + "keeping them; the layout check decides whether they fit"); } const size_t n = fread(out, 1, cap, f); diff --git a/scripts/Makefile.strucpp b/scripts/Makefile.strucpp index 0526dfeb..f4d66d63 100644 --- a/scripts/Makefile.strucpp +++ b/scripts/Makefile.strucpp @@ -83,16 +83,48 @@ CXX_BIN := g++ # preserve. Verified on an SLM-RP4: with the flag the .so leaves # /proc//maps on every stop and re-maps fresh on every start; without it, # it never leaves -- one build's image stays pinned for the life of the process. +comma := , + # Libraries the editor materialised from enabled .stlib archives. Each is an # ordinary library folder, so `src/` is its include root; a folder without one # is taken as its own root. RESOURCE_LIB_DIRS := $(wildcard $(GENERATED_DIR)/libraries/*) RESOURCE_LIB_INC := $(foreach d,$(RESOURCE_LIB_DIRS),$(if $(wildcard $d/src),$d/src,$d)) +# A LIBRARY MAY NAME THE DEFINES ITS OWN SOURCES NEED. +# +# `defines=A,B,C` in a library's library.properties becomes -DA -DB -DC. The +# key is ours; arduino-cli ignores keys it does not know, so the same file +# stays valid as an Arduino library descriptor. +# +# WHY IT IS NEEDED, with the case that found it. A vendored C library is very +# often configured by a header it ships - wolfSSL is the example, and its own +# documentation says "the WOLFSSL_USER_SETTINGS macro must be defined +# project-wide", because its wolfcrypt/settings.h reads user_settings.h only +# when that macro is set. Without it every source compiles with DEFAULT +# features instead of the ones the library was vendored for. Nothing fails at +# compile time: the objects are produced, the .so links (a shared object may +# carry undefined symbols), and the failure arrives when the runtime dlopens +# the program - `undefined symbol: wolfSSL_CTX_get_cert_store`, naming a +# function whose absence has no obvious connection to a missing -D. +# +# This is the same gap as the .c discovery above, one layer along: the branch +# taught the build where a vendored library's HEADERS are and then how to +# compile its SOURCES, and a library that needs a define to configure those +# sources still could not say so. +# +# Only from libraries/, and only as -D: a library cannot inject an include +# path, a warning flag or an optimisation level into somebody else's build. +RESOURCE_LIB_DEFS := $(strip $(foreach d,$(RESOURCE_LIB_DIRS),\ + $(if $(wildcard $d/library.properties),\ + $(addprefix -D,$(subst $(comma), ,\ + $(patsubst defines=%,%,\ + $(filter defines=%,$(shell tr -d '\r' < $d/library.properties))))),))) + CXXFLAGS := -std=c++17 -O1 -pipe -fPIC -Wall -DSTRUCPP_THREADED -fno-gnu-unique -MMD -MP \ -Wno-unknown-pragmas -Wno-deprecated-declarations \ -I $(GENERATED_DIR) -I $(RUNTIME_INC) -I $(PYTHON_INC) \ - $(addprefix -I ,$(RESOURCE_LIB_INC)) + $(addprefix -I ,$(RESOURCE_LIB_INC)) $(RESOURCE_LIB_DEFS) # Python POU stubs (`{external …}` blocks emitted by the editor) call # `getpid()`, `create_shm_name()`, `python_block_loader()`. Those @@ -102,6 +134,23 @@ CXXFLAGS := -std=c++17 -O1 -pipe -fPIC -Wall -DSTRUCPP_THREADED -fno-gnu-unique # any Python POU pay nothing; the header is declarations only. GENERATED_CXXFLAGS := $(CXXFLAGS) -include iec_python.h +# And the same for a C source in a vendored library. +# +# Mirrored from CXXFLAGS rather than derived from it, deliberately. The C++ +# flags carry -std=c++17, -fno-gnu-unique, -DSTRUCPP_THREADED, the Python +# include root and a force-include of a C++ header, none of which mean anything +# to a C compiler - and subtracting them would break quietly the next time one +# was added. What has to stay in step is the INCLUDE ROOTS, and those are +# written out here so the next person editing either list can see both. +# +# No -std: the existing python_loader.c rule below does not set one either, and +# a vendored C library that needs a particular dialect says so in its own +# headers rather than having one imposed on it from here. +GENERATED_CFLAGS := -O1 -pipe -fPIC -Wall -MMD -MP \ + -Wno-unknown-pragmas -Wno-deprecated-declarations \ + -I $(GENERATED_DIR) -I $(RUNTIME_INC) \ + $(addprefix -I ,$(RESOURCE_LIB_INC)) $(RESOURCE_LIB_DEFS) + # --------------------------------------------------------------------------- # Retain capability probe. # @@ -144,12 +193,37 @@ SHIM_HAS_RETAIN := $(strip $(if $(wildcard $(RUNTIME_INC)/iec_retain.hpp),\ # the probe just measured, so they are already internally consistent. SHIM_CXXFLAGS := $(CXXFLAGS) $(if $(SHIM_HAS_RETAIN),-DSTRUCPP_SHIM_HAS_RETAIN,) -# Discover every .cpp under core/generated/, at any depth: the top-level TUs +# Discover every source under core/generated/, at any depth: the top-level TUs # STruC++ emitted, plus whatever each resource library carries below its own # root. `sort` makes the order reproducible across filesystems and drops any # duplicate. +# +# BOTH LANGUAGES, and the .c half was missing. This search found only *.cpp and +# the only pattern rule was for *.cpp, so every C source inside a vendored +# resource library compiled to nothing - and a C library is what a library +# vendors: TLS, compression, codecs and drivers are C almost without exception. +# AND IT DID NOT FAIL LOUDLY, which is the worst part. A shared object is +# allowed to carry undefined symbols, so the link SUCCEEDED and printed "Build +# complete"; the editor reported a clean compile and the failure surfaced later +# as `dlopen: undefined symbol` when the runtime tried to load the program. The +# file had not failed to compile — it had never been offered to a compiler, and +# nothing anywhere said so. +# +# This branch is where that gap came from. RESOURCE_LIB_DIRS and +# RESOURCE_LIB_INC above taught the build where a vendored library's HEADERS +# are; its sources were never added beside them. A vendored C library therefore +# got its headers found and its sources ignored - the same feature, half +# finished. GEN_CPP := $(sort $(shell find $(GENERATED_DIR) -name '*.cpp')) -GEN_OBJ := $(patsubst $(GENERATED_DIR)/%.cpp,$(BUILD_DIR)/%.o,$(GEN_CPP)) +GEN_C := $(sort $(shell find $(GENERATED_DIR) -name '*.c')) + +# One `sort` over the union as well as over each half: a library that shipped +# foo.c and foo.cpp side by side would otherwise name $(BUILD_DIR)/foo.o twice. +# Make still has to choose a rule for it, and chooses the .cpp one because it +# is written first - deterministic, and a file layout to avoid rather than one +# to support. +GEN_OBJ := $(sort $(patsubst $(GENERATED_DIR)/%.cpp,$(BUILD_DIR)/%.o,$(GEN_CPP)) \ + $(patsubst $(GENERATED_DIR)/%.c,$(BUILD_DIR)/%.o,$(GEN_C))) SHIM_OBJ := $(BUILD_DIR)/runtime_v4_entry.o PYTHON_OBJ := $(if $(wildcard $(PYTHON_LOADER)),$(BUILD_DIR)/python_loader.o,) @@ -194,6 +268,16 @@ $(BUILD_DIR)/%.o: $(GENERATED_DIR)/%.cpp | $(BUILD_DIR) @mkdir -p $(@D) $(CXX) $(GENERATED_CXXFLAGS) -c $< -o $@ +# The same, for a C source a resource library brought with it. Not the C++ +# compiler with a different flag set: a C library compiled as C++ fails on +# things that are legal C and not legal C++ - an implicit void* conversion, a +# variable called `new`, a designated initialiser out of order - and it fails +# deep inside somebody else's code where it reads as their bug. +$(BUILD_DIR)/%.o: $(GENERATED_DIR)/%.c | $(BUILD_DIR) + @echo "[INFO] Compiling $<..." + @mkdir -p $(@D) + $(CC) $(GENERATED_CFLAGS) -c $< -o $@ + # Static runtime shim — lives in the runtime repo, not in the user # upload. Doesn't get the iec_python.h force-include. # diff --git a/tests/host/test_plc_retain_file_store.cpp b/tests/host/test_plc_retain_file_store.cpp index 6d741807..635e5f50 100644 --- a/tests/host/test_plc_retain_file_store.cpp +++ b/tests/host/test_plc_retain_file_store.cpp @@ -13,14 +13,16 @@ * (Baremetal's flash driver DOES carry an explicit length, because its * region is fixed-size and trailing erased bytes read as 0xFF — the two * formats are deliberately not the same, and this test pins this one.); - * - discarding the payload when the identity does not match; + * - KEEPING the payload when the identity does not match, because the + * identity is the program's MD5 and so changes on any edit at all — the + * layout hash one layer up is what decides whether the bytes still fit; * - treating a file too short to carry the header as unattributable; * - holding the identity from load() so the next save() can label its bytes. * * Until this file existed, that path's only evidence was a by-hand run on an - * SLM-RP4 recorded in a PR body. The case it proves — a program's values are - * refused for a DIFFERENT program even when the retain layout is identical — is - * exactly the one a layout hash cannot catch, so it is worth being able to + * SLM-RP4 recorded in a PR body. The case it now proves — that an ordinary + * logic edit does NOT cost a commissioned plant its retained values — is the + * one that used to fail on every single upload, so it is worth being able to * re-run without hardware. * * WHY NOT CEEDLING, AND WHY NOT THE LIFECYCLE HARNESS @@ -215,14 +217,37 @@ static void case_same_program_restores() CHECK(memcmp(out, blob, sizeof(blob)) == 0, "the payload should come back unchanged"); } -/* THE CASE A LAYOUT HASH CANNOT CATCH. +/* A PROGRAM EDIT MUST NOT COST THE PLANT ITS RETAINED VALUES. * - * Both programs here retain the same shape — same length, same bytes would pack - * identically — and differ only in identity. Without this check the second - * program silently inherits the first one's state. */ -static void case_different_program_is_discarded() + * This case asserted the opposite until the fix this replaces it for, and the + * reversal is the whole point, so it is worth saying why rather than leaving a + * diff that looks like a check being weakened. + * + * The identity stored here is PROGRAM_ID — the MD5 of program.st — so it + * changes on ANY edit: move a rung, rename a comment, add a line anywhere. + * Discarding on a mismatch therefore meant every retained value in a + * commissioned plant reset on every upload, which is precisely the guarantee + * the RETAIN qualifier exists to give. IEC 61131-3 §6.5.6.1 rule 1 defines a + * retained value as "the values the variables had when the resource or + * configuration was stopped", conditioned on the starting operation being a + * warm restart and on nothing else; "download", "reload" and "online change" + * appear nowhere in Part 3. + * + * And the check that SHOULD refuse a genuinely incompatible blob already + * exists one layer up, in ext_strucpp_retain_unpack(): magic, format, crc, + * truncation and the LAYOUT HASH, with plc_retain.cpp naming which failed. + * STruC++ hashes the layout instead of the program deliberately — a body edit + * leaves it unchanged, while adding, removing, retyping or reordering a + * retained variable changes it and the blob is refused. This store sitting in + * front of that with a project MD5 defeated it rather than reinforcing it. + * + * WHAT IS GIVEN UP: two DIFFERENT projects sharing one retain.bin path whose + * layout hashes happen to collide would read each other's values — a 32-bit + * collision on a path nobody takes, against a certainty on the path everybody + * takes. */ +static void case_different_program_keeps_its_values() { - g_case = "a different program's values are discarded, not inherited"; + g_case = "an edited program still gets its retained values"; reset(); const uint8_t blob[] = {0xDE, 0xAD, 0xBE, 0xEF}; @@ -237,27 +262,30 @@ static void case_different_program_is_discarded() const int rc = plc_retain_file_store_load(MD5_B, PLC_RETAIN_PROGRAM_ID_LEN, out, sizeof(out), &got); - CHECK(rc == 0, "a stale store is not an error — it is an empty one"); - CHECK(got == 0, "NOTHING may be handed back to a different program"); - CHECK(!store_file_exists(), "the stale file should be removed, not left to be re-read"); + CHECK(rc == 0, "a store written by an earlier build is not an error"); + CHECK(got == sizeof(blob), "the bytes must be handed up, for the layout check to judge"); + CHECK(got == sizeof(blob) && memcmp(out, blob, sizeof(blob)) == 0, + "and handed up UNCHANGED — this is the plant's commissioned state"); + CHECK(store_file_exists(), "the file must not be removed on an identity mismatch"); CHECK(g_log.find("different program") != std::string::npos, - "the operator must be told storage was cleared, and why"); + "the operator should still be told the bytes predate this build"); + CHECK(g_log.find("storage cleared") == std::string::npos, + "and must NOT be told they were cleared, because they were not"); - /* And the new program's first commit must be labelled with ITS identity, - * from the id the load above held — otherwise the next start discards it - * too and retention never works for this program. */ + /* The identity still has to be taken from load(), so the next commit + * labels the bytes with the program that is actually running. */ const uint8_t fresh[] = {0x11, 0x22}; plc_retain_file_store_save(fresh, sizeof(fresh)); plc_retain_file_store_flush(); plc_retain_file_store_stop(); FILE *f = fopen(g_store_path.c_str(), "rb"); - CHECK(f != nullptr, "the new program should be able to store"); + CHECK(f != nullptr, "the running program should be able to store"); if (f) { char id[PLC_RETAIN_PROGRAM_ID_LEN]; CHECK(fread(id, 1, sizeof(id), f) == sizeof(id), "header readable"); CHECK(memcmp(id, MD5_B, sizeof(id)) == 0, - "the commit after a discard must carry the NEW program's id"); + "the next commit must carry the RUNNING program's id"); fclose(f); } } @@ -377,7 +405,7 @@ int main() printf("plc_retain_file_store — identity handling\n"); case_header_layout(); case_same_program_restores(); - case_different_program_is_discarded(); + case_different_program_keeps_its_values(); case_short_file_is_unattributable(); case_empty_store_is_not_an_error(); case_wrong_identity_length_is_refused(); From 3bdab7077a3e09d19c791b722fa43d3b468e2588 Mon Sep 17 00:00:00 2001 From: Matthew Reed Date: Wed, 7 Oct 2026 10:13:32 +1300 Subject: [PATCH 3/3] fix: retained values before the first scan, save on change, upload refusal reasons, 25 MB limit - Retain restore: plc_retain_read() queued every restored value on the debug-write journal, drained at the end of a cycle, so scan 1 ran on initial values and the restore then overwrote what it wrote; with more retained leaves than the journal holds (128) the rest were dropped. Restore now runs after journal_init() and applies each write immediately, before any task is released. - Retain save: the file store committed on a fixed timer, so a setpoint change waited up to a whole period. Now nothing is written without a change; a change after a quiet period is committed at once; changes within flush_seconds are held and committed, latest values only, when the period ends. Default flush_seconds 5 -> 10. - Upload refusals: analyze_zip sent its reasons (a file over the limit, an unsafe path, a disallowed type, the compression ratio, the total size) only to the server logger, so the editor and CLI saw "Compilation failed" with empty logs. Each reason now also goes to the build log the status endpoint returns. - Upload limit: a program with many function-block instances produces a debug-map.json over 10 MB (16.9 MB seen), so it was refused. The per-file limit is now 25 MB and the total 100 MB. MAX_CONTENT_LENGTH follows MAX_FILE_SIZE. Tests: host test for the save timing, pytest for the refusal reasons. --- core/src/plc_app/plc_retain.cpp | 13 +++++- core/src/plc_app/plc_retain.h | 6 ++- core/src/plc_app/plc_retain_file_store.cpp | 34 +++++++++++---- core/src/plc_app/plc_retain_file_store.h | 11 +++-- core/src/plc_app/plc_state_manager.cpp | 24 +++++------ tests/host/test_plc_retain_file_store.cpp | 42 +++++++++++++++++++ .../compile/test_analyze_zip_reasons.py | 42 +++++++++++++++++++ webserver/plcapp_management.py | 19 +++++---- 8 files changed, 158 insertions(+), 33 deletions(-) create mode 100644 tests/pytest/compile/test_analyze_zip_reasons.py diff --git a/core/src/plc_app/plc_retain.cpp b/core/src/plc_app/plc_retain.cpp index 281a6eab..173146e4 100644 --- a/core/src/plc_app/plc_retain.cpp +++ b/core/src/plc_app/plc_retain.cpp @@ -59,7 +59,18 @@ std::atomic g_active{false}; */ uint8_t retain_write_leaf(uint8_t arr, uint16_t elem, const uint8_t *bytes, uint16_t len) { - return runtime_external_write(arr, elem, (uint8_t)DBGW_OP_WRITE, bytes, len) == 0 ? 0x7E : 0x82; + const int rc = runtime_external_write(arr, elem, (uint8_t)DBGW_OP_WRITE, bytes, len); + + /* Restoring happens once, at program load, before any task is released — + * so apply the write now instead of leaving it for the dispatcher's + * cycle-end drain. Left queued, scan 1 would run on the initial values and + * the restore would then overwrite what scan 1 wrote; and a program with + * more retained leaves than the queue holds would lose the rest. */ + image_lock(); + debug_write_journal_drain(); + image_unlock(); + + return rc == 0 ? 0x7E : 0x82; } /** diff --git a/core/src/plc_app/plc_retain.h b/core/src/plc_app/plc_retain.h index 565f2b99..b6ef3c9a 100644 --- a/core/src/plc_app/plc_retain.h +++ b/core/src/plc_app/plc_retain.h @@ -115,8 +115,12 @@ void plc_retain_init(void); * a machine starting from its defaults is recoverable, one starting from * plausible-looking garbage is not. * + * The values are applied before this returns (not left for the dispatcher's + * cycle-end drain), so scan 1 sees them. + * * Safe and cheap when nothing is retained or no driver claimed the store, and - * idempotent: call once per program start, after plc_retain_init(). + * idempotent: call once per program start, after plc_retain_init() and + * journal_init(), before any task is released. */ void plc_retain_read(void); diff --git a/core/src/plc_app/plc_retain_file_store.cpp b/core/src/plc_app/plc_retain_file_store.cpp index d45b5e6d..3144bea1 100644 --- a/core/src/plc_app/plc_retain_file_store.cpp +++ b/core/src/plc_app/plc_retain_file_store.cpp @@ -11,6 +11,7 @@ #include "plc_retain.h" // PLC_RETAIN_PROGRAM_ID_LEN — one definition for both sides #include +#include #include #include #include @@ -55,7 +56,15 @@ bool g_dirty = false; std::string g_program_md5; std::string g_path; -int g_flush_seconds = 5; +int g_flush_seconds = 10; + +/* When the last commit happened, and whether one has happened since start. A + * change after a quiet period is committed at once; changes that follow within + * g_flush_seconds are held and committed, latest values only, when that period + * ends. So a setpoint is on disk straight away, and a value that keeps changing + * costs at most one write per period. Touched by the flusher thread only. */ +std::chrono::steady_clock::time_point g_last_commit; +bool g_committed_once = false; std::atomic g_enabled{false}; std::atomic g_running{false}; std::thread g_flusher; @@ -77,7 +86,7 @@ void read_config(const char *config_path) { g_enabled.store(false); g_path.clear(); - g_flush_seconds = 5; + g_flush_seconds = 10; FILE *f = fopen(config_path, "r"); if (!f) return; @@ -207,14 +216,17 @@ void discard_stored() void flush_loop() { + g_committed_once = false; while (g_running.load()) { - for (int i = 0; i < g_flush_seconds && g_running.load(); i++) - { - std::this_thread::sleep_for(std::chrono::seconds(1)); - } + /* A tenth of a second: how long a change after a quiet period waits, + * and how promptly a stop is noticed. */ + std::this_thread::sleep_for(std::chrono::milliseconds(100)); if (!g_running.load()) break; + const auto now = std::chrono::steady_clock::now(); + if (g_committed_once && now - g_last_commit < std::chrono::seconds(g_flush_seconds)) continue; + std::vector snapshot; std::string snapshot_id; { @@ -228,7 +240,12 @@ void flush_loop() snapshot_id = g_program_md5; g_dirty = false; } - if (!snapshot.empty()) commit(snapshot.data(), (uint16_t)snapshot.size(), snapshot_id); + if (!snapshot.empty()) + { + commit(snapshot.data(), (uint16_t)snapshot.size(), snapshot_id); + g_last_commit = std::chrono::steady_clock::now(); + g_committed_once = true; + } } } @@ -242,7 +259,8 @@ bool plc_retain_file_store_start(const char *config_path) g_running.store(true); g_flusher = std::thread(flush_loop); - log_info("Retain: built-in file store enabled — %s, flushing every %ds", + log_info("Retain: built-in file store enabled — %s, a change is saved at once, " + "then at most every %ds while values keep changing", g_path.c_str(), g_flush_seconds); return true; } diff --git a/core/src/plc_app/plc_retain_file_store.h b/core/src/plc_app/plc_retain_file_store.h index f79a3814..8daaac79 100644 --- a/core/src/plc_app/plc_retain_file_store.h +++ b/core/src/plc_app/plc_retain_file_store.h @@ -34,11 +34,14 @@ * * enabled=1 * path=/var/lib/openplc-runtime/retain.bin - * flush_seconds=5 + * flush_seconds=10 * - * `flush_seconds` bounds how much retained state a power cut costs, against how - * hard the storage is worked. It is a real trade and it belongs to whoever - * installs the machine, which is why it is configuration and not a constant. + * Nothing is written while nothing changes. A change after a quiet period is + * committed at once; changes that follow within `flush_seconds` are held and + * committed, latest values only, when that period ends. So a setpoint is kept + * straight away and `flush_seconds` bounds how hard a value that keeps changing + * works the storage — and how much of such a value a power cut can cost. It + * belongs to whoever installs the machine, which is why it is configuration. */ #ifndef PLC_RETAIN_FILE_STORE_H diff --git a/core/src/plc_app/plc_state_manager.cpp b/core/src/plc_app/plc_state_manager.cpp index 86e01e16..b9523692 100644 --- a/core/src/plc_app/plc_state_manager.cpp +++ b/core/src/plc_app/plc_state_manager.cpp @@ -443,18 +443,6 @@ void *plc_cycle_thread(void *arg) image_tables_fill_null_pointers(); pthread_mutex_unlock(itm); - /* Retained variables. init() decides once whether retain can run here — - * does the .so export the entry points, does the program retain anything, - * which driver will hold the bytes — and read() asks that driver for what it - * has for THIS program, which is also where a driver discards a previous - * program's values. Both must follow the located-variable binding above: a - * retained variable may also be located, and its image slot has to exist - * before anything writes through it. Both are no-ops when retain is not in - * play, and read() lands before the first task is released, so a new - * program never runs a scan on the old one's state. */ - plc_retain_init(); - plc_retain_read(); - journal_buffer_ptrs_t journal_ptrs = { .bool_input = bool_input, .bool_output = bool_output, @@ -482,6 +470,18 @@ void *plc_cycle_thread(void *arg) log_info("Journal buffer initialized"); } + /* Retained variables. init() decides once whether retain can run here — + * does the .so export the entry points, does the program retain anything, + * which driver will hold the bytes — and read() asks that driver for what it + * has for THIS program, which is also where a driver discards a previous + * program's values. Both must follow the located-variable binding and + * journal_init() above: a retained variable may also be located, its image + * slot has to exist, and its restore goes through the image journal. Both + * are no-ops when retain is not in play. read() applies every value before + * the first task is released, so scan 1 already sees the retained state. */ + plc_retain_init(); + plc_retain_read(); + if (plugin_driver) { plugin_driver_start(plugin_driver); diff --git a/tests/host/test_plc_retain_file_store.cpp b/tests/host/test_plc_retain_file_store.cpp index 0bc225f4..614a2614 100644 --- a/tests/host/test_plc_retain_file_store.cpp +++ b/tests/host/test_plc_retain_file_store.cpp @@ -51,6 +51,8 @@ #include #include #include +#include +#include #include #include @@ -393,6 +395,45 @@ static void case_unchanged_blob_is_not_rewritten() "an unchanged blob should not republish the file (write-and-rename changes the inode)"); } +/* The last `n` bytes of the store file: the blob, which the header precedes. */ +static bool stored_tail_is(const uint8_t *want, size_t n) +{ + FILE *f = fopen(g_store_path.c_str(), "rb"); + if (!f) return false; + uint8_t got[16] = {0}; + fseek(f, -(long)n, SEEK_END); + const size_t r = fread(got, 1, n, f); + fclose(f); + return r == n && memcmp(got, want, n) == 0; +} + +static void case_change_saved_at_once_then_held() +{ + g_case = "a change is saved at once; the next within the period waits for it to end"; + reset(); + write_conf(2); + plc_retain_file_store_start(g_conf_path.c_str()); + uint8_t out[512]; + uint16_t got = 0; + plc_retain_file_store_load(MD5_A, PLC_RETAIN_PROGRAM_ID_LEN, out, sizeof(out), &got); + + const uint8_t a[] = {1, 1, 1, 1}; + const uint8_t b[] = {2, 2, 2, 2}; + + plc_retain_file_store_save(a, sizeof(a)); + std::this_thread::sleep_for(std::chrono::milliseconds(400)); + CHECK(stored_tail_is(a, sizeof(a)), "the first change should be on disk within a moment, not a period"); + + plc_retain_file_store_save(b, sizeof(b)); + std::this_thread::sleep_for(std::chrono::milliseconds(400)); + CHECK(stored_tail_is(a, sizeof(a)), "a second change inside the period should be held"); + + std::this_thread::sleep_for(std::chrono::milliseconds(2000)); + CHECK(stored_tail_is(b, sizeof(b)), "the held change should be saved when the period ends"); + + plc_retain_file_store_stop(); +} + int main() { char tmpl[] = "/tmp/retain-store-test-XXXXXX"; @@ -413,6 +454,7 @@ int main() case_empty_store_is_not_an_error(); case_wrong_identity_length_is_refused(); case_unchanged_blob_is_not_rewritten(); + case_change_saved_at_once_then_held(); reset(); rmdir(g_dir.c_str()); diff --git a/tests/pytest/compile/test_analyze_zip_reasons.py b/tests/pytest/compile/test_analyze_zip_reasons.py new file mode 100644 index 00000000..43b5e3b4 --- /dev/null +++ b/tests/pytest/compile/test_analyze_zip_reasons.py @@ -0,0 +1,42 @@ +# SPDX-License-Identifier: MIT +# Copyright (c) 2026 Autonomy® + +"""A refused program bundle says why in the build log. + +analyze_zip sent its reasons only to the server logger, so the editor and +the CLI saw the build fail with empty logs and "Compilation failed". Each +refusal now also goes to the build log the status endpoint returns. +""" + +import zipfile + +from webserver import plcapp_management as pm + + +def _zip(tmp_path, name, data): + path = tmp_path / "program.zip" + with zipfile.ZipFile(path, "w", zipfile.ZIP_DEFLATED) as zf: + zf.writestr(name, data) + return str(path) + + +def test_file_over_the_limit_is_logged(tmp_path, monkeypatch): + monkeypatch.setattr(pm, "MAX_FILE_SIZE", 100) + pm.build_state.logs.clear() + ok, _ = pm.analyze_zip(_zip(tmp_path, "debug-map.json", "x" * 101)) + assert not ok + assert any("debug-map.json" in line and "limit" in line for line in pm.build_state.logs) + + +def test_unsafe_path_is_logged(tmp_path): + pm.build_state.logs.clear() + ok, _ = pm.analyze_zip(_zip(tmp_path, "../evil.c", "int x;")) + assert not ok + assert any("Unsafe path" in line for line in pm.build_state.logs) + + +def test_a_normal_bundle_logs_no_error(tmp_path): + pm.build_state.logs.clear() + ok, _ = pm.analyze_zip(_zip(tmp_path, "program.st", "PROGRAM P END_PROGRAM")) + assert ok + assert not any("[ERROR]" in line for line in pm.build_state.logs) diff --git a/webserver/plcapp_management.py b/webserver/plcapp_management.py index 1b58e351..55cbcb58 100644 --- a/webserver/plcapp_management.py +++ b/webserver/plcapp_management.py @@ -30,8 +30,8 @@ logger, _ = get_logger("runtime", use_buffer=True) -MAX_FILE_SIZE: Final[int] = 10 * 1024 * 1024 # 10 MB per file -MAX_TOTAL_SIZE: Final[int] = 50 * 1024 * 1024 # 50 MB total +MAX_FILE_SIZE: Final[int] = 25 * 1024 * 1024 # 25 MB per file (a large program's debug map passes 10 MB) +MAX_TOTAL_SIZE: Final[int] = 100 * 1024 * 1024 # 100 MB total (debug map + debug table source) DISALLOWED_EXT = (".exe", ".dll", ".sh", ".bat", ".js", ".vbs", ".scr") class BuildStatus(Enum): @@ -81,25 +81,29 @@ def analyze_zip(zip_path) -> tuple[bool, list]: # Check for path traversal or absolute paths if filename.startswith("/") or ".." in filename or ":" in filename: - # logger.warning("Dangerous path: %s", filename) + build_state.log(f"[ERROR] Unsafe path in program file: {filename}\n") safe = False # Check uncompressed size if uncompressed_size > MAX_FILE_SIZE: logger.warning("File too large: %s (%d bytes)", filename, uncompressed_size) + build_state.log( + f"[ERROR] {filename} is {uncompressed_size} bytes; " + f"the limit per file is {MAX_FILE_SIZE} bytes.\n") safe = False # Check compression ratio (ZIP bomb detection) if compressed_size > 0 and uncompressed_size / compressed_size > 1000: - # logger.warning("Suspicious compression ratio in %s", - # filename) + build_state.log( + f"[ERROR] {filename} compresses more than 1000:1; refused.\n") safe = False # Check disallowed extensions if ext in DISALLOWED_EXT: logger.warning("Disallowed extension: %s", filename) + build_state.log(f"[ERROR] File type not allowed: {filename}\n") safe = False total_size += uncompressed_size @@ -107,8 +111,9 @@ def analyze_zip(zip_path) -> tuple[bool, list]: # Check total size if total_size > MAX_TOTAL_SIZE: - # logger.warning("Total uncompressed size too large: %d bytes", - # total_size) + build_state.log( + f"[ERROR] The program files total {total_size} bytes; " + f"the limit is {MAX_TOTAL_SIZE} bytes.\n") safe = False if safe: