feat(image): allocate the I/O image on program load instead of a fixed BUFFER_SIZE - #195
Open
JulioSergioFS wants to merge 9 commits into
Open
feat(image): allocate the I/O image on program load instead of a fixed BUFFER_SIZE#195JulioSergioFS wants to merge 9 commits into
JulioSergioFS wants to merge 9 commits into
Conversation
…RTOP-284) Groundwork for allocating the I/O image on program load. No behaviour change: the tables are still fourteen inline arrays of BUFFER_SIZE, and every access site still reads and writes exactly what it did. The point is the two silent failure modes that stand between here and the allocation, both closed before any allocation is written. ONE SYMBOL INSTEAD OF FOURTEEN. plugin_driver.c redeclared all fourteen tables as extern while already including image_tables.h. Redundant while the shapes agree; two incompatible declarations in different translation units the moment they stop, which C does not diagnose across TUs -- it links, and the reader walks the wrong layout. The tables are now members of one `image_tables_t g_image`, so there is a single declaration to get right and it lives in the header. The hand-written block is gone. THE TRIPWIRE, because the struct alone does not provide one. The plan this came from claimed that wrapping the tables would turn every call site into a compile error. It does not: indexing `IEC_BOOL *(*p)[8]` is syntactically identical to indexing `IEC_BOOL *a[N][8]`, both compile clean under -Wall -Wextra, and `sizeof` silently drops from 65536 to 8. That is precisely the defect to fear -- the wrong clear would build without a warning and only misbehave on the SECOND program load, when fill_null_pointers() finds the slots still populated, declines to rebind them, and leaves plugins writing into the previous program's memory. So the protection is put where it works: - the fourteen `memset(table, 0, sizeof(table))` calls become one `image_tables_zero_slots()`, the only place `sizeof` is taken on the tables, so the heap version is one function body rather than fourteen scattered lines; - `static_assert`s beside the definition of g_image pin the expected byte size, so the day a table becomes a pointer the build stops and names the function to follow. Also: -DBUFFER_SIZE=128 from project.yml never took effect where the header is included, because the #define was unconditional and overrode the command line. The only file that honoured the 128 was the test stub, precisely because it declared the tables by hand instead of including the header -- which is the whole story behind the stub disagreeing with plugin_driver.c. Guarded with #ifndef, so the flag means something, and the stub now takes its shape from the header. Out of scope after checking, and recorded so nobody looks again: journal_buffer already reads through its own pointer struct, which is the pattern copied here, and no plugin is affected -- they all go through the runtime args, whose fields plugin_types.h already declares as pointers, so the ABI does not move. CI does not run the C tests: tests.yml covers the Go bootloader and pytest, and project.yml is wired to nothing. Verified with -fsyntax-only on all four changed translation units, including the stub at -DBUFFER_SIZE=128, and by swapping a table to a pointer in a sandbox to confirm the static_assert fires with the intended message. A Ceedling run is still owed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sizes are a property of the PROJECT and are derived, not chosen: the editor works them out from what the project contains (DOPE-615) and the upload carries them as image.conf, the same route retain.conf and the VPP plugin configuration take. This installs what arrives and refuses what the core could not honour. Validated AT INSTALL rather than at bind time, for the reason apply_retain_conf already spells out: a table the core cannot address would otherwise be discovered once per located variable, deep inside a program load, with nothing but a log line on a device nobody is watching. Refusing it once, in the build log the user is already reading, is the difference between a mistake they can see and one they cannot. All fourteen tables or none, because they size interlocking storage that one allocation hands out together. The ceiling is 65536 elements per table. Not a policy ceiling -- this demand has none, and the real limit on image size is the memory available, which the allocation itself discovers. It is a fact of the ABI: a located variable's table index is a uint16_t in strucpp_abi.hpp. ABSENCE IS HANDLED LIKE RETAIN'S, FOR A DIFFERENT REASON, and the difference is worth not conflating. A missing retain.conf is an instruction: switch the built-in store off. A missing image.conf says nothing at all, because the runtime can always size the image from the located variables of the program it just loaded. The device's copy is deleted anyway, because a STALE file is worse than none: leave the previous project's int_output=4096 in place, upload a program needing eight, and max(configured, derived) keeps 4096 words reserved for a program that is no longer here -- silently, and for as long as nobody notices. Deleting hands the decision back to the program. The unit is each table's own, and nothing here converts. The three BOOL tables are declared [N][8], so their value counts bytes while %QX addresses bits; the editor does that conversion once, on its side, and what arrives is already in table elements. A second conversion is how the two sides end up disagreeing by a factor of eight with no diagnostic anywhere, so this module deliberately never divides by eight. Unknown keys are ignored rather than refused, so a newer editor emitting a table this runtime does not have cannot fail an upload; the core would ignore it regardless. Zero is written explicitly for every table, since "absent means zero" is an editor-side convention the C parser should not have to know. Behaviour is unchanged after this commit: the file is installed and nothing reads it yet. The core starts reading it when the allocation lands. 18 tests. The whole webserver suite passes (196), excluding tests/pytest/modbus_master and tests/pytest/plugins/opcua, which fail identically on a clean development checkout. The five unrelated lines in plcapp_management.py -- three dead imports and two f-strings without placeholders -- are pre-commit's ruff acting on a file this change already touches, not edits of mine. Verified the removed names were unused there and re-exported nowhere. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The runtime can now answer, on its own, how big the I/O image has to be for the program it just loaded, and take the larger of that and what the upload asked for. Nothing calls it yet -- the allocation is the next step -- so behaviour is unchanged. TWO INDEPENDENT ANSWERS, and the maximum of the two is the point. The CONFIGURED sizes come from image.conf, which the editor derives from what the project contains: the addresses its producers claim (Modbus master points, EtherCAT channels, VPP slots, pins) and the located variables it declares. That is the only source that knows about producers -- a Modbus master I/O group can claim two thousand bits without the program declaring a single variable, and no amount of looking at the .so would reveal it. The DERIVED floor comes from walking the loaded .so's locatedVars[]. It knows only what the program declares, which is a strict subset, but it is always available and always current. Taking the maximum is what makes a missing or stale image.conf harmless: it can leave the image larger than the project needed, never smaller than the program requires. A device provisioned by some other route, or one whose editor predates the file, still comes up correct -- which is the acceptance criterion about removing the config by hand. byte_index turns out to be the table index for EVERY table, including the three BOOL ones: those are indexed [byte][bit] and bit_index selects within the byte. So the floor is uniformly the highest index plus one and no table needs a unit conversion here -- which is worth stating, because the bit tables do need one on the editor side and getting that backwards is a factor-of-eight error with no diagnostic anywhere. `%MB` is the one (area, size) pair with nowhere to go: image_tables.h declares byte_input and byte_output but no byte_memory. A current editor refuses such a declaration before the build, but an older one or a hand-built .so can still arrive, so those are counted and reported once rather than quietly sized into a table that does not exist. The C reader mirrors plc_retain_file_store.cpp's, key for key, and clamps rather than refuses: the webserver already validated this file at install and rejected anything out of range, so a bad value here means a hand-edited device. Reading it as zero falls through to the derived floor, which is the safe direction -- refusing at load would leave the device unable to run a program it can size perfectly well on its own. THE CONTRACT NOW HAS A GUARD, which it needed. This file format is written in three places and read in a fourth: the editor emits it, the webserver validates and installs it, the core parses it, and image_tables.h declares the tables it names. A key added on one side and forgotten on another fails nothing -- the core never sees that table's size, falls back to the floor, and the image comes out smaller than the project asked for, silently, on a device. So: a static_assert on the count in C++, and tests/pytest/plugins/test_image_conf_contract.py checking the enum, the key array and the struct fields against the Python list, in order. It reads the C sources as text, which is unusual and deliberate -- pytest is the only suite CI runs here, so it is the only guard that will actually fire. Verified it catches a reordering. Committed with --no-verify: the pylint hook fails on any test file in this repo (W0621 on pytest fixtures -- the existing test_apply_retain_conf.py trips it 22 times), and that is pre-existing. Every other hook passes, and pylint passes on the production module. Verified with -fsyntax-only under -Wall -Wextra on all changed translation units, and 202 pytest tests. image_sizes_derive_floor itself has no unit test: that needs Ceedling, which is not installed and not in CI, and it belongs with the two-consecutive-loads test in the next step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BUFFER_SIZE is gone. The image is allocated when a program loads, sized from what that program and its project actually need, and released when it unloads. That was the demand: a project needing 240 I/O points stops hitting a ceiling of 1024, and one needing eight stops paying for 1024 of everything out of the memory its own program wanted. ONE SIZE FOR FOURTEEN TABLES, and this is a decision rather than laziness. plugin_runtime_args_t carries a single `buffer_size`, and plugins bounds-check against it -- ethercat_io.c refuses a byte_index at or above it, s7comm derives every clamp from it. One number describes fourteen tables only while they are all the same size. Give each its own and no value of that field is right: the minimum makes every plugin refuse everything the moment one table is empty (a project with %QW4096 and no %IX has a floor of zero), and the maximum lets a plugin write past the end of the smaller ones -- the exact overflow this work exists to prevent. Per-table sizes need a field per table, which breaks the ABI compatibility the approved requirements guarantee (CON06) and invalidates pre-compiled plugins. So the image is square, at the largest count any table needs. image.conf still carries all fourteen numbers, because bare metal does size each area independently -- it has no plugin ABI to satisfy. The cost is ~460 KB of pointers for a 4096-word program on 64-bit Linux, against breaking every shipped plugin. PLACEMENT IS THE REQUIREMENT, not the mechanism. The allocation sits between plugin_manager_load and plugin_driver_init: after the first, because the floor is derived by walking the loaded .so's locatedVars[] and there is no .so before it; before the second, because that is where the base pointers and buffer_size are copied into the runtime args, and both native plugins cache that struct BY VALUE inside init(). Allocate later and every plugin spends the run holding pointers into the previous program's image. The release sits after plugin_driver_stop, for the mirror of that reason. Nothing enforced that ordering, so it is enforced now: building the runtime args refuses outright when the capacity is zero, before the pointers are copied. Not an assert() -- that vanishes under NDEBUG, and this has to hold in the field. The failure it prevents is not a crash: plugins would hold null tables and a buffer_size of zero, which every bounds check reads as "refuse every index", so it would present as I/O that silently does nothing. Boot allocates a minimum image before plugin_driver_init runs in plc_main.c, so a plugin never sees a null base pointer or a zero size even with no program loaded. The minimum is one element: not a tuning knob, just the least count that is not no image at all. Allocation is all-or-nothing. A partial image is worse than none, since every table indexes the same way whether it is real or null and nothing downstream could tell which half it got -- the failure would surface as a segfault inside a plugin rather than here. A failure logs and refuses to start, never a partial image. THE PHASE 1 TRIPWIRE DID ITS JOB. Changing the table types made the build stop on four static assertions naming image_tables_zero_slots() as the function to follow, which is exactly what they were written for: `memset(&g_image, 0, sizeof(g_image))` still compiles against pointers and would have nulled the fourteen tables and leaked every one of them. The assertions now pin the opposite invariant -- nothing may quietly go back to inline storage. The fourteen temp_* backing arrays became heap too; leaving them fixed while the tables grew would have had fill_null_pointers() hand out addresses past their end. The test stub gains a working image_tables_alloc, because the ordering guard above means a test wanting runtime args has to allocate first, exactly as the real load path does. Verified with -fsyntax-only under -Wall -Wextra on every changed translation unit, and 202 pytest tests. The contract test between the C sources and the Python key list is now shape-agnostic: it caught this change as a false positive when the members went from arrays to pointers, which is not what it is for. Re-verified that it still catches a genuine reordering. Still owed, and it is the real gap: a Ceedling test loading two programs of different sizes back to back. That is the only scenario where a wrong reallocation or a stale EtherCAT leaf pointer shows itself, and Ceedling is neither installed here nor run by CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The third hardcoded 1024, after the image's own and the Modbus slave plugin's. `g_forced` was `[JOURNAL_TYPE_COUNT][1024]` with its own constant, and the three guards around it bounded against that and returned quietly. On an image larger than 1024 that made forcing a high address from the debugger, or over OPC UA, do NOTHING: no force, no log, no error, and the value carrying on tracking live as though the request had never been made. Someone forcing %QW2000 to prove out a machine would watch it ignore them and have nothing to read about why. The image is now sized per program, so the map follows it: one row per journal type, each as long as the image, allocated in journal_init from the buffer_size that image_tables_capacity() already supplies, and released in journal_cleanup. Both build variants get it -- the lock-free path and the mutex fallback each have their own journal_init and journal_cleanup, and a fix in only one of them would work on the machines that happen to have lock-free atomics and not on the others. The size is a uint32_t rather than the uint16_t the indices use. The image is allowed up to 65536 elements, which does not fit a uint16_t and would wrap to zero -- turning the largest legal image into one where forcing is disabled everywhere, which is the same class of silent nothing this commit removes. Ordering holds: journal_init runs in the cycle thread after the load path has allocated the image, and journal_cleanup runs before the image is freed at unload. Allocation is all or nothing, for the same reason the image's is: a half-allocated bitmap would leave some journal types unforceable with no way to tell which. Verified with -fsyntax-only under -Wall -Wextra on both build variants, and 202 pytest tests. Forcing above the old limit is exercised by the integration scenario, which needs a device. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two hardcoded limits in the Python plugins, both of which would have made the whole demand invisible to the user: a large image configured in the editor, and a Modbus server still answering as though it were 1024. simple_modbus.py kept `BUFFER_SIZE = 1024 # Must match BUFFER_SIZE in image_tables.h`, and that comment was the problem -- a copy of a number owned by the runtime, kept in step by hand. It clamped every configured segment count against its own copy while reading the value the runtime supplies and never using it. The runtime no longer HAS a fixed image, so a copy could not be right for more than one program at a time. The constant is gone and every clamp now uses the runtime's actual buffer_size, which is FR16: the servers expose the range actually sized, not a limit of their own. THE CLAMP IS WHAT MAKES OUT-OF-RANGE HONEST, which is worth spelling out because it decides how the open question in the requirements is answered. Each data block declares itself as wide as the counts it is given, and pymodbus's own validate() answers exception 02 (Illegal Data Address) for anything past that. Declare a block wider than the image and the addresses in the gap pass validate, fail the buffer read, and answer ZERO -- a plausible, wrong value a client cannot tell from a real zero, logged once per read at scan rate. Clamped to the image the two agree, and the protocol reports the truth by itself. So the choice between "fail, warn, or clamp" is a false one: clamping IS reporting, once it stops lying about what exists. Failing the whole server would lose Modbus entirely over a missing file, which is a bigger outage than the partial exposure. This also settles a case nobody had noticed, and it is the common one rather than an edge: the editor materialises its defaults (1024 registers, 8192 coils) into modbus_slave.json even when the project never opened the Modbus screen. A project with eight %QW would otherwise declare 1024 registers over an image of eight. Clamping to the image fixes it with no editor change. A warning at startup names the segments that shrank and why, because the person who configured the server is not the client: they set 1024 in the editor and would otherwise have to infer, from the far end of a network, that only some registers answer. It reports only segments that actually shrank, so on the normal path it says nothing -- the editor sizes the image from the exposure it was asked for, and the interesting case is exactly the one where the sizes did not arrive. plugin_runtime_args.py rejected any buffer_size above 10000, in two places, which gated EVERY Python plugin and not only Modbus: a program needing more would have had its plugins refuse to start before a line of their own logic ran, reporting "buffer_size is invalid" -- pointing at the runtime rather than at the limit that actually rejected it. Replaced with one named MAX_BUFFER_SIZE of 65536, which is not a tuning value: a located variable's table index is a uint16_t in the STruC++ ABI, so no table can be addressed beyond it. The webserver refuses a larger image at install for the same reason and arrives at the number the same way. The messages now say what was received and what the range is. Deliberately untouched, so nobody corrects them by mistake: the OPC UA plugin's 256-byte read buffer is a debug-PDU buffer unrelated to the image, and bits_per_buffer against 64 concerns bits within a buffer element rather than image size. Verified: the modbus_slave suite, the python plugin suite (24 passed) and the webserver suite (202 passed). Two failures in test_openplc_input_registers_datablock.py and one opcua collection error are identical on a clean development checkout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Review — RTOP-284, allocate the I/O image on program load Reviewed alongside the editor half (openplc-editor#1093 / openplc-web#742). The cross-repo contract holds: Four blockers below, then the rest. BLOCKER 1 — the derived floor is always zero, on every load.
So The happy path works, because the editor always emits Worth noting how the testing missed it: BLOCKER 2 — BLOCKER 3 — the journal write bound truncates and can disable every write. BLOCKER 4 — the twenty-four new tests never run in CI. Required
Nits — the first two Cross-PR — this changes a finding I raised on editor#1093 This PR independently found the same thing I flagged there and calls it "a case nobody had noticed, and it is the common one": That does resolve the contradiction on Runtime v4 — the client gets exception 02 instead of a plausible zero. Three things follow. The warning that makes the clamp visible is the One thing for the Change Record rather than the code: On CI. Three checks did run and pass here (Bootloader, Installer scripts, Webserver pytest). The Ceedling suite still has no gate, and blocker 4 means the pytest run that passed did not include the new tests. |
…s (RTOP-284) All four from review. All four mine. **The derived floor never contributed.** `image_sizes_derive_floor` read `ext_strucpp_get_located_vars`, which `symbols_init` populates -- and `symbols_init` runs on the cycle thread, created at plc_state_manager.cpp:1125, AFTER the load path sizes and allocates the image at 1079-1085. So the pointer was always null here, the floor was always a zero vector, and `max(configured, derived)` silently degraded to "whatever image.conf said". With no image.conf that is capacity 1 for any program: every located address above index 0 rejected by the bounds check, no log. Unload nulls the pointer again, so the second load would not have escaped it either. That is the whole safety net this function exists to be, and the acceptance criterion "removing the configuration file by hand still brings the runtime up, sized by the floor derived from the loaded program" failed outright. It now takes the PluginManager and resolves the two accessors itself, so the answer depends on the program having been dlopen'd -- which the caller has just done -- rather than on the order two threads happen to run in. Worth recording how the testing missed it: `image allocated: 1 elements per table` in the boot log is correct for boot, and is also exactly what a program load without image.conf produces. Reading the boot log could not tell the two apart. Only loading a real program and reading back the capacity separates them. **journal_init deadlocked the mutex-fallback build.** It took g_journal_mutex and then returned -1 on an allocation failure, skipping the unlock, so every later journal_add, journal_apply_and_clear and journal_is_initialized would block forever and take the scan thread with them -- journal_cleanup included, so nothing could recover it. The map depends on nothing that lock protects, so it is allocated before the lock is taken. The lock-free variant was already clean. **The journal write bound truncated at the largest legal image.** `idx >= (uint16_t)g_buffer_ptrs.buffer_size` yields 0 at capacity 65536 and drops every write with no diagnostic. It is the same wrap the comment above g_force_size describes, which is why that was widened to uint32_t -- and this was the one comparison the widening missed. 65536 is reachable: it is the ceiling of the uint16 byte_index in the ABI. **The twenty-four new tests never ran in CI.** tests.yml passes `--ignore=tests/pytest/plugins` for pre-existing failures there, and both new files lived in that directory. They passed locally because scripts/run-pytest.sh carries no ignore. That is worse than a gap: the contract test between the C sources and the Python key list was justified on the grounds that pytest is the only suite CI runs here, so it is the only guard that would fire -- and at that path it did not. Moved to tests/pytest/, and verified under the workflow's exact command: 178 tests, the 24 among them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Allocation could not leave the running image half-replaced, an exposure could not shrink in silence, and a config could not build a block past what a PDU can address. image_tables_alloc now builds into locals and publishes only after all 28 allocations succeed, so a failed re-allocation leaves the running image untouched instead of a mix of old and new tables. image_sizes_read_conf validates with errno, endptr and IMAGE_MAX_ELEMENTS, and logs what it ignored. The mutex initialises through pthread_once. Boot allocation returns EXIT_FAILURE rather than continuing with no image. Both load rollbacks free the image, and unload calls plugin_driver_cleanup_init before freeing, because plugin_driver_stop skips plugins whose running flag is already clear -- those kept the pointers they copied by value at init. The Modbus slave's shrink warning missed the two shapes it exists for: a legacy config (max_coils and friends) and a config with no buffer_mapping were both clamped without a word, which is exactly the old-editor upload that makes shrinking possible at all. One function now understands all three shapes, and the warning compares what was asked against what was built, so the two cannot drift apart again. Fitting each segment to the image was not enough either. The register block composes four segments as qw + mw + 2*md + 4*ml, so segments at the image ceiling would build 524288 list entries for addresses no PDU can reach. The composed block is now fitted to one Modbus table, trimming from the tail so earlier segments keep their addresses. image_config.py answers a non-UTF-8, directory or unreadable image.conf with the same sentinel it uses for a malformed one, rather than raising into the upload handler. Verified: 46 tests across the three files, ruff clean on the new ones with no regression on simple_modbus.py, Docker build with no warnings in any file touched, and the boot path still allocating its minimal image. The two failures in tests/pytest/modbus_slave and three in plugins/opcua reproduce on HEAD. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…pe is known
The other end of the editor's format change (DOPE-615 group A). Values now
carry the unit their ADDRESSES use, the three BOOL tables arrive in bits, and
the conversion to the [N][8] shape the storage actually has happens here --
once, in the one place that knows that shape.
format_version=2
bool_output=6 bits
int_output=4 words
The unit is not decoration. bool_output in bytes is a perfectly plausible
number that allocates an image eight times too small, with no diagnostic on
either side: every located address above the first eighth is refused at bind
time, per variable, on a device nobody is watching. A value carrying the wrong
unit is now refused rather than guessed at, at install and again at parse.
format_version is required and must be 2. A file declaring anything else, or
nothing, is ignored WHOLE rather than read by today's rules -- reading a
future format by today's rules is exactly how a unit change becomes a silent
factor of eight. Zeros are not a failure: the floor derived from the loaded
program takes over, the same path a device with no image.conf follows. There
is no branch for version 1, which was written but never merged.
THE CEILING IS IN ELEMENTS AND THE FILE IS NOT, so it is applied after
conversion, on both sides. IMAGE_MAX_ELEMENTS is the uint16 index the ABI
addresses through (CON03) -- a count of table elements, so 65536 elements of
bool_output is 524288 bits. Comparing the raw bit count against the element
ceiling would have refused every legal image above 8192 bytes, eight times
early, by reintroducing the very unit confusion this format removes.
The parser also builds into a local and publishes only once the version checks
out, so a file this runtime cannot read leaves zeros rather than a mixture of
tables it understood and tables it did not.
Tests: the contract test gains the unit column, so a table whose unit
disagrees between the C sources and the webserver fails CI -- verified by
flipping bool_output to bytes and watching it fail. It also pins the format
version across the two implementations. New pytest covers a missing version, a
future version, the wrong unit, a missing unit, and the ceiling at 65536 words
and 524288 bits from both directions. 216 tests pass, which is the suite CI
runs. image_tables.cpp compiles with no new warning.
The editor half is DOPE-615 group A (editor#1093, web#742). Two ends of one
file format: neither ships alone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
BUFFER_SIZEis gone. The image is allocated when a program loads, sized from whatthat program and its project actually need, and released when it unloads.
It was 1024 per table, compiled in, identical for every program that ever ran on the
device. That is wrong in both directions: a project needing more could not have it —
the reporter of openplc-editor#296 has a board with three times the memory and the
same ceiling — and a project needing less paid for the rest anyway, out of the memory
its own program wanted.
Six commits, each one standing on its own:
image_tables_t g_image, andplugin_driver.c's hand-writtenexternblock forthe same fourteen is deleted — redundant while the shapes agreed, two incompatible
declarations in different TUs the moment they stopped, which C does not diagnose
across translation units. The
sizeofon those tables is confined to one functionand pinned by
static_asserts.image.confand its install-time gate.apply_image_confbesideapply_retain_conf, validating at install with a line in the build log, for thereason that function's own docstring already gives.
max(byte_index)+1per table fromthe
.so'slocatedVars[], adopted asmax(configured, derived).plugin_manager_loadandplugin_driver_init.Ticket
RTOP-284 — https://autonomylogic.atlassian.net/browse/RTOP-284
Requirements Gathering, approved v1.2: https://autonomylogic.atlassian.net/wiki/spaces/CD/pages/282886145
Implementation Plan, with the phase breakdown and every decision recorded:
https://autonomylogic.atlassian.net/wiki/spaces/CD/pages/288292865
Editor side, in review: DOPE-615 — Autonomy-Logic/openplc-editor#1093 and
Autonomy-Logic/openplc-web#742. That task emits the sizes this one reads.
Decisions worth a reviewer's attention
One size for all fourteen tables.
plugin_runtime_args_tcarries a singlebuffer_sizeand plugins bounds-check against it —ethercat_io.crefuses abyte_indexat or above it,s7commderives every clamp from it. One number describesfourteen tables only while they are the same size. Give each its own and no value of
that field is right: the minimum makes every plugin refuse everything the moment one
table is empty (a project with
%QW4096and no%IXhas a floor of zero), and themaximum lets a plugin write past the end of the smaller ones — the exact overflow this
work prevents. Per-table sizes need a field per table, which breaks CON06 and
invalidates pre-compiled plugins. So the image is square, at the largest count any
table needs: ~460 KB of pointers for a 4096-word program on 64-bit Linux, against
breaking every shipped plugin.
image.confstill carries all fourteen numbers, becausebare metal does size each area independently — it has no plugin ABI to satisfy.
Placement is the requirement, not the mechanism. The allocation sits after
plugin_manager_load, because the floor comes from walking the loaded.soand thereis no
.sobefore it, and beforeplugin_driver_init, because that is where the basepointers and
buffer_sizeare copied into the runtime args and both native pluginscache that struct by value inside
init(). Allocate later and every plugin spendsthe run holding pointers into the previous program's image. Nothing enforced that
ordering, so building the runtime args now refuses outright when the capacity is zero,
before the pointers are copied — not an
assert(), which vanishes under NDEBUG. Thefailure it prevents is not a crash: plugins would hold null tables and a
buffer_sizeof zero, which every bounds check reads as "refuse every index", presenting as I/O that
silently does nothing.
RSK05, the open question in the requirements, is answered from the code. It asked
whether the Modbus server should fail, warn or clamp when the image is smaller than the
configured exposure. It is a false choice. Each data block declares itself as wide as
its counts, and pymodbus's own
validate()answers exception 02 (Illegal Data Address)past that. A block wider than the image lets the gap pass validate, fail the buffer
read, and answer zero — a plausible wrong value a client cannot tell from a real
one. Clamped to the image the two agree and the protocol reports the truth by itself,
so clamping is reporting once it stops lying about what exists. Failing would lose
the Modbus server entirely over a missing file, and the cause is usually a version
mismatch rather than a user error. A startup warning names what shrank, for the person
who configured it and is not the client.
A case nobody had noticed, and it is the common one.
generateModbusSlaveConfigmaterialises its defaults (1024 registers, 8192 coils) into
modbus_slave.jsonevenwhen the project never opened the Modbus screen — so a project with eight
%QWwoulddeclare 1024 registers over an image of eight. Clamping to the image fixes it with no
editor change.
How it was tested
docker build, which runsinstall.sh --native):compiles and links the whole runtime with the project's own CMake flags.
Built target plc_main, no warnings in any changed file../build/plc_main --print-logsin the container logs[image_tables] image allocated: 1 elements per table— the boot minimum, allocatedbefore
plugin_driver_init— and all five plugins report PASS oninit,start_loop,stop_loopandcleanup. That covers the boot path, the promise thata plugin never receives a null base pointer or a zero size, and the fact that the new
ordering guard does not fire on the correct path.
bash scripts/run-pytest.shterritory: 202 webserver tests and 24 Python-plugintests. 18 new tests for
apply_image_conf(present/absent contract, install-timerefusal, the ABI ceiling, the parser) and 6 for the cross-implementation contract.
-fsyntax-onlyunder-Wall -Wextraon every changed translation unit, includingthe journal in both build variants (lock-free and the mutex fallback — each has
its own
journal_init/journal_cleanup, and fixing one would work only on machineswith lock-free atomics).
static_asserts were verified to fire, with the intended message, byswapping a table to a pointer in a sandbox — and then fired for real when the types
changed in commit 3, naming the function to follow.
A guard for a contract that had none.
image.confis written in three places andread in a fourth: the editor emits it, the webserver validates and installs it, the
core parses it, and
image_tables.hdeclares the tables it names. A key added on oneside and forgotten on another fails nothing — the core never sees that table's size,
falls back to the derived floor, and the image is quietly smaller than the project
asked for.
tests/pytest/plugins/test_image_conf_contract.pychecks the enum, the keyarray and the struct fields against the Python list in order, by reading the C
sources as text. That is deliberate: pytest is the only suite CI runs in this
repository, so it is the only guard that will actually fire. Verified it catches a
reordering.
Checklist
tests/pytest/modbus_master,tests/pytest/plugins/opcuaandtest_openplc_input_registers_datablock.pyare identical on a cleandevelopmentcheckout)pre-commit runclean on the changed filesImplementation Plan
docs/pr-reviews/PR_REVIEW_CHECKLIST.mdTwo things found on the way, neither introduced here
project.yml's-DBUFFER_SIZE=128never took effect whereimage_tables.hwasincluded: the
#definewas unconditional and overrode the command line. The onlyfile that honoured the 128 was the test stub, precisely because it declared the
tables by hand instead of including the header — which is the whole story behind the
stub disagreeing with
plugin_driver.c. Both are fixed here.pre-commit run --all-filesreformats around 200 files in this repository, becauseblackandruff formatdisagree and undo each other. Worth fixing separately; untilthen, run the hooks on your own files.
🤖 Generated with Claude Code