feat(image): allocate each table at its own length, told to the plugins

[JulioSergioFS]

Stacked on #195 — targets that branch, so the diff here is only group B's own commit.

The fourteen image tables stop sharing one number. A project needing four output words and no analog input gets int_output four long and int_input at the minimum, instead of both at whatever the largest area needed — which is what BR01, FR21 and NFR04 asked for all along, and what image_sizes_largest() was quietly undoing.

How the sizes reach a plugin, without moving the struct

plugin_runtime_args_t carries a single buffer_size and CON06 guarantees pre-compiled plugins keep their field offsets, so the fourteen travel through a new optional symbol the loader resolves with dlsym — joining the five optional ones it already looks up — and PyObject_GetAttrString for Python, with PyErr_Clear() because a missing attribute leaves an exception set.

int set_image_sizes(const uint32_t *sizes, uint32_t count);

Presence is the declaration. Called before init(), because init() is where both native plugins copy the base pointers by value; delivering afterwards leaves a window per load in which a plugin holds new pointers and previous sizes.

If any plugin lacks it, the run stays square. Not a preference: a plugin bounding a byte index into bool_output and a word index into int_output with one buffer_size is correct exactly while the tables are equal. Decided per load, before allocating, and logged with the plugin that forced it.

buffer_size itself becomes the smallest of the fourteen. Bounding by the smallest refuses an index; bounding by the largest reads past every shorter table.

Three loops that used one length for fourteen tables

zero_slots (a memset past the end of the shorter ones — a heap overflow written by the function that exists to prevent this class of mistake), fill_null_pointers (null slots left in the longer tables, which is the state a plugin dereferences), and the journal's write bound.

Three enums, two orders

Found while doing the journal. journal_buffer_type_t and the s7comm plugin's type each group a width's memory beside its input and output; image_table_id_t puts every memory table at the end. JOURNAL_INT_MEMORY is 7 and IMAGE_TABLE_INT_MEMORY is 10, so a cast between them lands writes under another table's bounds — and journal_buffer.h says it "matches the OpenPLC image table types".

Both mappings are now written out explicitly and pinned by a pytest that also asserts the two orders really do still disagree, so making them identical becomes a deliberate act rather than a discovery.

In-tree consumers migrated with it

s7comm's eight clamps, EtherCAT's bounds check, the shared Python validator and the Modbus slave's eight segments each follow the table they actually address. All of them fall back to buffer_size when the sizes were never delivered — without that, an older runtime loading a newer plugin leaves every table at zero and the plugin refuses everything, silently.

The composed holding-register block is settled in simple_modbus's own docstring: the block already dispatches each address to one segment and therefore one table, so each segment clamps against its own and the block is the concatenation.

Deliberately not here

The deprecation attribute on buffer_size. The build carries -Werror, so it would fail every consumer still reading it rather than naming them. It goes in once the VPP packages are migrated, which is that repository's own task.

Verification

225 pytest, which is the suite CI runs. Every changed translation unit clean under -Wall -Wextra -Werror, including both journal build variants. s7comm and EtherCAT compiled against their real headers, with EtherCAT's two pre-existing -Waddress warnings unchanged from HEAD.

The container build compiles the core successfully and then fails initialising submodules — that is the worktree's .git not existing inside it, not the code.

🤖 Generated with Claude Code

[marconetsf]

Reviewed across three axes: the enum orders and the per-table decision, memory and the allocation loops, and the Python side with the ABI.

Severity on every point, here and inline: 🔴 Major (must fix before merge) · 🟡 Changes required (should be addressed) · 🟢 Nit (optional) · ❓ Question. Each carries a confidence out of ten.

One 🔴 Major, three 🟡 Changes required, five 🟢 Nits and one ❓ Question, all inline.

The enum mappings are correct — I checked both, entry by entry, against journal_buffer.h, s7comm_config.h and image_table_id.h:

• kJournalToImageTable: all fourteen present and correct. The headline case checks out (JOURNAL_INT_MEMORY 7 → IMAGE_TABLE_INT_MEMORY 10). Designated initializers mean a missing entry would silently default to table 0, and TestJournalMapping catches that, fails on a single swapped pair, and genuinely asserts the two orders still disagree. This one is well pinned.
• s7_image_table: all fourteen plus the BUFFER_TYPE_NONE case correct, and a switch with no default so a new enumerator breaks the build.

I also checked the two the brief did not ask about: ecat_table_for is correct for all ten %I/%Q combinations, and the pre-existing table_for is correct including the %MB-has-no-table case.

The ABI is untouched, and that is worth stating precisely. plugin_types.h has two hunks — a new include and a replaced comment. No field in plugin_runtime_args_t is added, removed, resized or reordered; buffer_size keeps its offset and its int type, only its meaning changes. So the ctypes mirror correctly needed no change and its offset assertions still hold. One packaging consequence to name: plugin_types.h now relatively includes ../plc_app/image_table_id.h, so a VPP package shipping that header alone will no longer compile. The header's own comment tells VPP authors to carry their own constants, and nothing in-tree ships the pair, so this is a note for whoever maintains the out-of-tree SDK rather than a finding.

Both new pytest files run in CI — top level of tests/pytest, outside all three --ignore paths, and test_modbus_exposure_fit.py's docstring says the placement is deliberate. Its per_table fixture teardown calls set_image_sizes([]) so it does not pollute the module-scoped fixture.

What has no coverage at all: plugin_image_sizes.c, image_sizes_flatten, image_tables_alloc(const image_sizes_t*), the two rewritten loops, journal_longest_table, journal_type_capacity, s7_image_table and ecat_table_for. Ceedling still does not run in CI, so none of it would run even if C tests existed. The journal fix is applied to both the lock-free and the dead mutex variant, and apply_write_raw / is_slot_forced sit above the #if, so nothing is half-applied there.

[marconetsf]

🔴 Major · confidence 9/10 · the feature is inert on a stock device

This loop returns false on the first plugin without the symbol, and plugins_default.conf ships five:

modbus_slave,  .../python/modbus_slave/simple_modbus.py
modbus_master, .../python/modbus_master/modbus_master_plugin.py
opcua,         .../python/opcua/plugin.py
s7comm,        ./build/plugins/libs7comm_plugin.so
ethercat,      ./build/plugins/libethercat_plugin.so

grep -rl set_image_sizes core/src/drivers/plugins/python/ matches exactly two files: the new shared/image_sizes.py and modbus_slave/simple_modbus.py. Neither modbus_master_plugin.py nor opcua/plugin.py imports it, so understands is false for them, image_sizes_flatten runs on every load, and the run is square on any device using the shipped config.

Per-table sizing therefore never activates as shipped. The two enum-order fixes, the three loop fixes and the s7comm/EtherCAT clamps all run in the degenerate case where they cannot differ from the old behaviour — which also means none of them is being exercised by anything.

The fix looks like one line each, and nothing else has to change: both plugins already bound through SafeBufferAccess → BufferValidator.validate_buffer_access, which this PR already migrated to table_capacity(buffer_type, self.args.buffer_size), and the names in buffer_types.py match IMAGE_TABLE_ORDER exactly.

from shared.image_sizes import set_image_sizes  # noqa: F401

If that is deliberately out of scope for group B, then the PR description should say the feature ships off by default — as written it reads as though it ships on.

Related, and cheap to fix while you are here (🟢 Nit, confidence 7/10): a degraded plugin also costs the run its per-table image. update_config/append_config leave native_plugin == NULL and set degraded = 1, so the plugin is skipped by every branch of plugin_driver_init, never receives runtime args and never touches the image — yet it makes understands false. On a box missing Npcap the EtherCAT plugin degrades and silently downgrades everyone else. Including disabled plugins is right, since plugin_driver_init inits them regardless of config.enabled; a degraded one is a different case. if (plugin->degraded) continue;, or at minimum log the distinction so an operator can tell "this plugin is old" from "this plugin failed to load".

[marconetsf]

🟡 Changes required · confidence 8/10 · the force bound and the write bound now disagree

force_map_alloc(journal_longest_table()) sizes every row to the longest table, but journal_force_set still validates only against that row length (index >= g_force_size), while apply_write_raw and is_slot_forced validate against journal_type_capacity(type) — the table's own length.

Before this PR the two were the same number and could not disagree. Now they can. With bool_output at 1 element and int_output at 100, g_force_size is 100, so forcing JOURNAL_BOOL_OUTPUT index 5:

1. passes the check here, flips the bit and increments g_force_count — permanently disabling the g_force_count == 0 fast path in is_slot_forced for the whole run;
2. has its seed write refused by apply_write_raw's per-table bound;
3. and is refused again by is_slot_forced's per-table bound.

So the force is a complete no-op that logged nothing — which is exactly the failure the warning added in #195 was written to prevent. journal_force_clear has the same asymmetry.

Fix: bound both by the table as well as the row, so the warning fires where the write will actually be refused:

if ((uint8_t)type >= JOURNAL_TYPE_COUNT || index >= g_force_size ||
    (uint32_t)index >= journal_type_capacity((uint8_t)type))

Reachability depends on whether the debug-write drain can address a slot the program has no located variable for. If it cannot, this is defence in depth rather than a live bug — but the two bounds should still agree, because the whole point of the per-table work is that they no longer can be assumed equal.

[marconetsf]

🟡 Changes required · confidence 9/10 · a fourth copy of the table order, and the only one nothing pins

IMAGE_TABLE_ORDER is what turns the runtime's positional sizes array into the names every Python buffer accessor uses. I checked it entry for entry against image_table_id.h and it is correct today.

But test_image_conf_contract.py parametrizes over exactly three readers — the enum, the key array and the struct — and this file is not one of them. Reorder image_table_id_t and CI stays green while every Python plugin silently reads int_memory's length as lint_memory's. The module's own docstring says "going by name cannot pick up the wrong table", which is true only while this list matches the enum, and nothing enforces that.

Given that this PR's headline finding is three enums disagreeing on order, leaving the fourth unpinned is the gap most likely to bite next. Fix: add a fourth reader to the existing parametrize that imports shared.image_sizes (or regexes the list) and asserts IMAGE_TABLE_ORDER == list(image_config.IMAGE_TABLE_KEYS). One entry in a list that already runs in CI.

[marconetsf]

🟡 Changes required · confidence 9/10 · three of the four new mappings are unpinned

TestJournalMapping pins kJournalToImageTable properly — completeness, same-name correspondence, and that the two orders still disagree. But this PR introduces three more mappings over the same fourteen tables and none is checked by anything:

• s7_image_table here, the one the PR itself calls "a third order";
• ecat_table_for in ethercat_io.c;
• SEGMENT_TABLES in simple_modbus.py, where the tests exercise 3 of the 8 entries — qw_count→int_output, mw_count→int_memory and qx_bits→bool_output. md, ml, mx, ix and iw are untested.

I verified all three by hand and they are correct, so this is about what happens next rather than what is there now. Ceedling does not run in CI, so nothing will compile-check the two C ones either.

All three are pure name-to-name maps in plain switch or dict form, so the regex approach already used for kJournalToImageTable works unchanged: extract the BUFFER_TYPE_X → IMAGE_TABLE_X pairs, assert the names correspond and that all fourteen are present. That is the difference between "one wrong entry fails CI" and "one wrong entry writes under another table's bounds with no diagnostic", which is the risk this PR exists to close.

[marconetsf]

🟢 Nit · confidence 10/10 · the comment still describes the old contract, at the site that redefined it

The hunk only swapped the function name in the last sentence, leaving:

It describes all fourteen tables because they are all allocated at the same count; see image_sizes_flatten() for why the ABI leaves no room for anything else.

Every clause is now false: the tables are not all allocated at the same count, ethercat_io.c and s7comm no longer derive their clamps from this field, and image_tables_capacity() is the minimum.

plugin_types.h and journal_buffer.h both got the correct new wording. This is the third copy and the one a reader of plugin_driver.c actually hits — reuse the minimum-is-the-only-safe-single-number text from the other two.

[marconetsf]

🟢 Nit · confidence 10/10 · off-by-one in the comment

BUFFER_TYPE_INT_MEMORY is 7 and IMAGE_TABLE_INT_MEMORY is 10

s7comm_config.h starts the enum with BUFFER_TYPE_NONE = 0, so BUFFER_TYPE_INT_MEMORY is 8. The 7 is JOURNAL_INT_MEMORY, which is presumably where the number came from.

The mapping code itself is correct — I verified all fourteen entries plus the NONE case — and the point about a cast being unsafe is if anything stronger once the off-by-one is fixed. "8 here, 7 in the journal, 10 in the image" is the more useful sentence, since three different numbers for one table is the whole argument.

[marconetsf]

🟢 Nit · confidence 8/10 · the PyErr_Clear() is right, the comment justifying it is not

The block itself is correct and correctly positioned. But the comment says "the four required lookups above never hit it because they return on failure", and only one lookup above is required: pFuncStart and pFuncStop are optional and both Py_XDECREF; = NULL; without clearing, and pFuncCleanup below does the same.

So for a plugin that defines set_image_sizes but not cleanup — which is the modbus_slave shape — an AttributeError is still left set on exit from this function. Pre-existing, and the new code happens to mop up after start_loop/stop_loop, but the comment asserts an invariant the file does not hold.

Either add PyErr_Clear() to the other three optional blocks, which is two lines and makes the comment true, or reword the comment.

[marconetsf]

🟢 Nit · confidence 7/10 · a mid-list failure leaves a half-delivered map behind

On a bad entry this returns -1 with _sizes holding everything parsed so far, so sizes_known() is True and table_capacity() answers for the tables before the failure while falling back to buffer_size for the rest.

The runtime does fail the plugin on the non-zero return, but this module is imported once per interpreter and every Python plugin in the process shares _sizes — so a plugin being torn down can leave a partial map behind for plugins that keep running. A _sizes.clear() before each return -1 makes the failure atomic.

Worth naming the general property too: the capability is declared per module, but the cache it feeds is process-global, so a declaring and a non-declaring Python plugin in the same interpreter read the same numbers. Harmless today because a non-declaring plugin forces the run square — but it is not what "presence is the declaration" implies, and it stops being harmless if the square rule is ever relaxed.

[marconetsf]

🟢 Nit · confidence 9/10 · #undef N inside a runtime branch

The preprocessor does not care which branch this sits in: N is undefined from this line onward, and the second #undef N before return true is a no-op on an already-undefined macro. It compiles correctly today only because every N(...) use happens to be above the failure block, which makes it fragile to a later edit that adds one below.

Move the single #undef N to just after the last calloc and drop the other.

[marconetsf]

❓ Question · confidence 5/10 · the bound and the pointers now come from two different points in time

apply_write_raw used to compare against g_buffer_ptrs.buffer_size, a value memcpy'd into the journal at journal_init. It now calls journal_type_capacity(...) → image_table_capacity(), which reads g_sizes — a global that image_tables_alloc rewrites under the image-tables mutex while this reads it without that mutex.

From the load sequence the scan thread does not exist while alloc runs, so I could not construct a concrete race and I am not calling it a bug. Flagging it because the bound and the table pointers in g_buffer_ptrs are now sourced from two different moments — the pointers from journal_init, the length live — and if that ever diverges the symptom is an out-of-bounds index rather than a dropped write.

If the invariant is "no allocation while the journal is initialised", one line saying so next to this check would make it checkable by the next reader. Separately and much smaller: this is now a non-inlinable cross-TU call per journal entry on the drain path, where it used to be a struct field read.