diff --git a/core/src/drivers/plugin_driver.c b/core/src/drivers/plugin_driver.c index dba3668b..29e98ddd 100644 --- a/core/src/drivers/plugin_driver.c +++ b/core/src/drivers/plugin_driver.c @@ -30,21 +30,12 @@ #include #include -// External buffer declarations from image_tables.c -extern IEC_BOOL *bool_input[BUFFER_SIZE][8]; -extern IEC_BOOL *bool_output[BUFFER_SIZE][8]; -extern IEC_BYTE *byte_input[BUFFER_SIZE]; -extern IEC_BYTE *byte_output[BUFFER_SIZE]; -extern IEC_UINT *int_input[BUFFER_SIZE]; -extern IEC_UINT *int_output[BUFFER_SIZE]; -extern IEC_UDINT *dint_input[BUFFER_SIZE]; -extern IEC_UDINT *dint_output[BUFFER_SIZE]; -extern IEC_ULINT *lint_input[BUFFER_SIZE]; -extern IEC_ULINT *lint_output[BUFFER_SIZE]; -extern IEC_UINT *int_memory[BUFFER_SIZE]; -extern IEC_UDINT *dint_memory[BUFFER_SIZE]; -extern IEC_ULINT *lint_memory[BUFFER_SIZE]; -extern IEC_BOOL *bool_memory[BUFFER_SIZE][8]; +/* The image tables come from image_tables.h, included above. This file used to + * redeclare all fourteen of them by hand right here -- redundant while the + * shapes agreed, and two incompatible declarations in different translation + * units the moment they stopped, which C does not diagnose across TUs. Deleted + * for RTOP-284: there is one declaration now, `g_image`, and it lives in the + * header. */ static PyThreadState *main_tstate = NULL; static PyGILState_STATE gstate; static int has_python_plugin = 0; @@ -132,21 +123,20 @@ static uint16_t plugin_debug_read(uint8_t arr, uint16_t elem, uint8_t *dest) // bitmap. Return 0x7E (SUCCESS) once queued, 0x82 (OUT_OF_MEMORY) if the queue // is momentarily full, 0x81 (OUT_OF_BOUNDS) when no program is loaded. -static uint8_t plugin_debug_set(uint8_t arr, uint16_t elem, bool forcing, - const uint8_t *bytes, uint16_t len) +static uint8_t plugin_debug_set(uint8_t arr, uint16_t elem, bool forcing, const uint8_t *bytes, + uint16_t len) { - if (!ext_strucpp_debug_set) return 0x81; // no program loaded + if (!ext_strucpp_debug_set) + return 0x81; // no program loaded uint8_t op = forcing ? (uint8_t)DBGW_OP_FORCE : (uint8_t)DBGW_OP_UNFORCE; - int rc = runtime_external_write(arr, elem, op, - forcing ? bytes : NULL, - forcing ? len : 0); + int rc = runtime_external_write(arr, elem, op, forcing ? bytes : NULL, forcing ? len : 0); return (rc == 0) ? 0x7E : 0x82; } -static uint8_t plugin_debug_write(uint8_t arr, uint16_t elem, - const uint8_t *bytes, uint16_t len) +static uint8_t plugin_debug_write(uint8_t arr, uint16_t elem, const uint8_t *bytes, uint16_t len) { - if (!ext_strucpp_debug_write) return 0x81; // no program loaded + if (!ext_strucpp_debug_write) + return 0x81; // no program loaded int rc = runtime_external_write(arr, elem, (uint8_t)DBGW_OP_WRITE, bytes, len); return (rc == 0) ? 0x7E : 0x82; } @@ -225,7 +215,6 @@ static int plugin_get_plc_state(void) } } - // Python capsule destructor for runtime args // Breakpoint here to debug capsule issues static void plugin_runtime_args_capsule_destructor(PyObject *capsule) @@ -274,7 +263,8 @@ static PyObject *create_python_runtime_args_capsule(plugin_runtime_args_t *args) * its function pointers segfaults. */ static void teardown_plugin_instance(plugin_instance_t *plugin) { - if (!plugin) return; + if (!plugin) + return; if (plugin->running) { @@ -392,7 +382,7 @@ int plugin_driver_update_config(plugin_driver_t *driver, const char *config_file * GIL-holding after that), so for that loop we just need to make * sure we don't release the GIL we acquired here. */ PyGILState_STATE plugin_gstate = PyGILState_LOCKED; - int plugin_have_gil = Py_IsInitialized(); + int plugin_have_gil = Py_IsInitialized(); if (plugin_have_gil) { plugin_gstate = PyGILState_Ensure(); @@ -410,7 +400,7 @@ int plugin_driver_update_config(plugin_driver_t *driver, const char *config_file * and cause unnecessary GIL acquires throughout the driver. */ has_python_plugin = 0; - int degraded_count = 0; + int degraded_count = 0; driver->plugin_count = config_count; for (int w = 0; w < config_count; w++) @@ -601,6 +591,122 @@ int plugin_driver_load_config(plugin_driver_t *driver, const char *config_file) } // Send to plugin init function all args +/** + * The fourteen table lengths, for a plugin that asked to be told. + * + * Read once per plugin_driver_init() rather than cached: the image is + * reallocated on every program load, and a cached copy is exactly the stale + * state this symbol exists to prevent. + */ +static uint32_t image_sizes_snapshot(uint32_t *out, uint32_t cap) +{ + uint32_t n = 0; + for (int id = 0; id < IMAGE_TABLE_COUNT && n < cap; ++id) + { + out[n++] = image_table_capacity((image_table_id_t)id); + } + return n; +} + +/** + * Hand one plugin the fourteen lengths, before its init() runs. + * + * Returns 0 when the plugin has nothing to be told or accepted them, and + * non-zero when it refused -- which fails the plugin exactly as a failed + * init() does, because a plugin that cannot make sense of the image it is + * about to be handed should not be handed it. + */ +static int deliver_image_sizes(plugin_instance_t *plugin) +{ + uint32_t sizes[IMAGE_TABLE_COUNT]; + const uint32_t count = image_sizes_snapshot(sizes, IMAGE_TABLE_COUNT); + + if (plugin->config.type == PLUGIN_TYPE_NATIVE && plugin->native_plugin && + plugin->native_plugin->set_image_sizes) + { + const int rc = plugin->native_plugin->set_image_sizes(sizes, count); + if (rc != 0) + { + log_error("Plugin '%s' refused the image sizes (returned %d)", plugin->config.name, rc); + return rc; + } + } + + if (plugin->config.type == PLUGIN_TYPE_PYTHON && plugin->python_plugin && + plugin->python_plugin->pFuncSetImageSizes) + { + PyObject *list = PyList_New((Py_ssize_t)count); + if (!list) + { + PyErr_Clear(); + log_error("Plugin '%s': could not build the image size list", plugin->config.name); + return -1; + } + for (uint32_t i = 0; i < count; ++i) + { + /* PyList_SetItem steals the reference, so a failed PyLong_FromLong + * is the only leak to worry about and it cannot happen for a + * uint32_t that already exists. */ + PyList_SetItem(list, (Py_ssize_t)i, PyLong_FromUnsignedLong(sizes[i])); + } + PyObject *result = + PyObject_CallFunctionObjArgs(plugin->python_plugin->pFuncSetImageSizes, list, NULL); + Py_DECREF(list); + if (!result) + { + PyErr_Print(); + log_error("Plugin '%s' raised in set_image_sizes", plugin->config.name); + return -1; + } + Py_DECREF(result); + } + + return 0; +} + +bool plugin_driver_all_understand_per_table_sizes(plugin_driver_t *driver, + const char **first_without) +{ + if (first_without) + *first_without = NULL; + if (!driver) + return false; + + for (int i = 0; i < driver->plugin_count; i++) + { + plugin_instance_t *plugin = &driver->plugins[i]; + bool understands = false; + + /* A DEGRADED PLUGIN DOES NOT GET A VOTE. + * + * It failed to load, so plugin_driver_init skips it in every branch: + * it never receives runtime args and never touches the image. Letting + * it answer "no" would mean one box missing Npcap, where the EtherCAT + * plugin degrades, silently costs every OTHER plugin its per-table + * image -- a downgrade with no relation to anything that will actually + * read the tables. + * + * DISABLED plugins still vote, deliberately: plugin_driver_init + * initialises them regardless of the enabled flag, so a disabled + * plugin does hold the base pointers and does read the image. */ + if (plugin->degraded) + continue; + + if (plugin->config.type == PLUGIN_TYPE_NATIVE) + understands = plugin->native_plugin && plugin->native_plugin->set_image_sizes; + else if (plugin->config.type == PLUGIN_TYPE_PYTHON) + understands = plugin->python_plugin && plugin->python_plugin->pFuncSetImageSizes; + + if (!understands) + { + if (first_without) + *first_without = plugin->config.name; + return false; + } + } + return true; +} + int plugin_driver_init(plugin_driver_t *driver) { if (!driver) @@ -642,6 +748,18 @@ int plugin_driver_init(plugin_driver_t *driver) } return -1; } + /* BEFORE init(), because init() is where a plugin copies the + * base pointers by value and decides how big everything is. + * Delivering afterwards leaves a window, once per load, in which + * the plugin holds new pointers and previous sizes. */ + if (deliver_image_sizes(plugin) != 0) + { + Py_DECREF(args); + if (have_gil) + PyGILState_Release(local_gstate); + return -1; + } + // Call the Python init function with proper capsule PyObject *result = PyObject_CallFunctionObjArgs(plugin->python_plugin->pFuncInit, args, NULL); @@ -681,6 +799,15 @@ int plugin_driver_init(plugin_driver_t *driver) return -1; } + /* BEFORE init(), for the same reason as the Python path above. */ + if (deliver_image_sizes(plugin) != 0) + { + free_structured_args(args); + if (have_gil) + PyGILState_Release(local_gstate); + return -1; + } + // Call the native init function int result = plugin->native_plugin->init(args); if (result != 0) @@ -709,13 +836,27 @@ int plugin_driver_init(plugin_driver_t *driver) return 0; } +int plugin_driver_any_initialized(plugin_driver_t *driver) +{ + if (!driver) + return 0; + for (int i = 0; i < driver->plugin_count; i++) + { + if (driver->plugins[i].initialized) + return 1; + } + return 0; +} + int plugin_driver_cleanup_init(plugin_driver_t *driver) { - if (!driver) return 0; + if (!driver) + return 0; PyGILState_STATE local_gstate = PyGILState_LOCKED; - int have_gil = has_python_plugin && Py_IsInitialized(); - if (have_gil) local_gstate = PyGILState_Ensure(); + int have_gil = has_python_plugin && Py_IsInitialized(); + if (have_gil) + local_gstate = PyGILState_Ensure(); int cleaned = 0; /* Reverse order so dependent plugins (declared later, depend on @@ -723,7 +864,8 @@ int plugin_driver_cleanup_init(plugin_driver_t *driver) for (int i = driver->plugin_count - 1; i >= 0; --i) { plugin_instance_t *plugin = &driver->plugins[i]; - if (!plugin->initialized) continue; + if (!plugin->initialized) + continue; if (plugin->config.type == PLUGIN_TYPE_PYTHON && plugin->python_plugin) { @@ -738,7 +880,8 @@ int plugin_driver_cleanup_init(plugin_driver_t *driver) ++cleaned; } - if (have_gil) PyGILState_Release(local_gstate); + if (have_gil) + PyGILState_Release(local_gstate); return cleaned; } @@ -1052,21 +1195,43 @@ void *generate_structured_args_with_driver(plugin_type_t type, plugin_driver_t * log_debug("Allocated runtime args structure (size: %zu bytes)", sizeof(plugin_runtime_args_t)); + /* THE ORDERING INVARIANT, checked rather than assumed, and checked BEFORE + * the pointers are copied because copying null ones is the whole problem. + * + * The image has to be allocated by now: what is copied below is what both + * native plugins cache BY VALUE inside their init(), and they hold it for + * the rest of the run. Nothing in the code enforces the order -- it is a + * property of where plc_state_manager.cpp happens to call things, and it is + * exactly the invariant a later refactor moves without noticing. The + * symptom would not be 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. + * + * Deliberately not assert(), which vanishes under NDEBUG. This has to hold + * in the field, not only in a debug build. */ + if (image_tables_capacity() == 0) + { + log_error("[PLUGIN]: runtime args requested before the image was allocated — " + "plugins would cache null tables; refusing"); + free(args); + return NULL; + } + // Initialize all buffer pointers - args->bool_input = bool_input; - args->bool_output = bool_output; - args->byte_input = byte_input; - args->byte_output = byte_output; - args->int_input = int_input; - args->int_output = int_output; - args->dint_input = dint_input; - args->dint_output = dint_output; - args->lint_input = lint_input; - args->lint_output = lint_output; - args->int_memory = int_memory; - args->dint_memory = dint_memory; - args->lint_memory = lint_memory; - args->bool_memory = bool_memory; + args->bool_input = g_image.bool_input; + args->bool_output = g_image.bool_output; + args->byte_input = g_image.byte_input; + args->byte_output = g_image.byte_output; + args->int_input = g_image.int_input; + args->int_output = g_image.int_output; + args->dint_input = g_image.dint_input; + args->dint_output = g_image.dint_output; + args->lint_input = g_image.lint_input; + args->lint_output = g_image.lint_output; + args->int_memory = g_image.int_memory; + args->dint_memory = g_image.dint_memory; + args->lint_memory = g_image.lint_memory; + args->bool_memory = g_image.bool_memory; // Flush-on-lock image read API (image mutex + journal drain). Points // directly at the runtime's image_tables entries; writes use the journal. @@ -1092,7 +1257,20 @@ void *generate_structured_args_with_driver(plugin_type_t type, plugin_driver_t * sizeof(driver->plugins[plugin_index].config.plugin_related_config_path)); // Initialize buffer size info - args->buffer_size = BUFFER_SIZE; + /* THE SMALLEST OF THE FOURTEEN, not the length they all share. + * + * The tables no longer have one length, and this field cannot say so -- + * CON06 keeps the struct's offsets fixed. The minimum is the only safe + * single number for a consumer that has not been told they can differ: + * bounding by it refuses an index, where bounding by the largest reads + * past every shorter table. + * + * ethercat_io.c and s7comm no longer derive their clamps from this field; + * they export set_image_sizes and bound by the table each access actually + * addresses, falling back here only when the sizes were never delivered. + * plugin_types.h carries the same statement for plugin authors, and + * journal_buffer.h for the runtime's own copy. */ + args->buffer_size = (int)image_tables_capacity(); args->bits_per_buffer = 8; // Initialize logging functions @@ -1301,6 +1479,10 @@ int python_plugin_get_symbols(plugin_instance_t *plugin) // start_loop is optional Py_XDECREF(py_binds->pFuncStart); py_binds->pFuncStart = NULL; + /* A failed PyObject_GetAttrString leaves an AttributeError SET, and an + * optional lookup does not return, so it has to be cleared here or the + * next CPython call reports this absence as its own failure. */ + PyErr_Clear(); } py_binds->pFuncStop = PyObject_GetAttrString(py_binds->pModule, "stop_loop"); @@ -1309,6 +1491,23 @@ int python_plugin_get_symbols(plugin_instance_t *plugin) // stop_loop is optional Py_XDECREF(py_binds->pFuncStop); py_binds->pFuncStop = NULL; + /* A failed PyObject_GetAttrString leaves an AttributeError SET, and an + * optional lookup does not return, so it has to be cleared here or the + * next CPython call reports this absence as its own failure. */ + PyErr_Clear(); + } + + py_binds->pFuncSetImageSizes = PyObject_GetAttrString(py_binds->pModule, "set_image_sizes"); + if (!py_binds->pFuncSetImageSizes || !PyCallable_Check(py_binds->pFuncSetImageSizes)) + { + /* Optional. PyErr_Clear() is not decoration: a failed + * PyObject_GetAttrString leaves an AttributeError SET, and the next + * CPython call that checks would report this absence as its own + * failure. The four required lookups above never hit it because they + * return on failure. */ + Py_XDECREF(py_binds->pFuncSetImageSizes); + py_binds->pFuncSetImageSizes = NULL; + PyErr_Clear(); } py_binds->pFuncCleanup = PyObject_GetAttrString(py_binds->pModule, "cleanup"); @@ -1317,6 +1516,10 @@ int python_plugin_get_symbols(plugin_instance_t *plugin) // cleanup is optional Py_XDECREF(py_binds->pFuncCleanup); py_binds->pFuncCleanup = NULL; + /* A failed PyObject_GetAttrString leaves an AttributeError SET, and an + * optional lookup does not return, so it has to be cleared here or the + * next CPython call reports this absence as its own failure. */ + PyErr_Clear(); } // Store the python binds in the plugin instance @@ -1453,6 +1656,11 @@ int native_plugin_get_symbols(plugin_instance_t *plugin) native_bundle->retain_load = (plugin_retain_load_func_t)dlsym(handle, "retain_load"); native_bundle->retain_flush = (plugin_retain_flush_func_t)dlsym(handle, "retain_flush"); + /* Optional, like the five above: NULL simply means this plugin does not + * understand per-table image sizes, and the run stays square for it. */ + native_bundle->set_image_sizes = + (plugin_set_image_sizes_func_t)dlsym(handle, "set_image_sizes"); + // Store the native bundle and handle in the plugin instance plugin->native_plugin = native_bundle; @@ -1486,7 +1694,8 @@ void python_plugin_cycle(plugin_instance_t *plugin) static bool plugin_provides_retain_store(const plugin_instance_t *p) { - if (!p) return false; + if (!p) + return false; // A DISABLED plugin is not a store, even though its symbols resolved. // @@ -1497,7 +1706,8 @@ static bool plugin_provides_retain_store(const plugin_instance_t *p) // simply gone, with a log line at start saying retain is configured and // working. Found on hardware: an upload rewrote plugins.conf, disabled the // storage plugin, and retain went on claiming to work. - if (!p->config.enabled) return false; + if (!p->config.enabled) + return false; // BOTH halves required. A store that can save and not load is worse than // none: it would accept values every scan and silently never give them @@ -1508,13 +1718,15 @@ static bool plugin_provides_retain_store(const plugin_instance_t *p) plugin_instance_t *plugin_driver_find_retain_store(plugin_driver_t *driver) { - if (!driver) return NULL; + if (!driver) + return NULL; plugin_instance_t *chosen = NULL; for (int i = 0; i < driver->plugin_count; i++) { plugin_instance_t *p = &driver->plugins[i]; - if (p->degraded || !plugin_provides_retain_store(p)) continue; + if (p->degraded || !plugin_provides_retain_store(p)) + continue; if (!chosen) { @@ -1533,15 +1745,18 @@ plugin_instance_t *plugin_driver_find_retain_store(plugin_driver_t *driver) int plugin_driver_retain_save(plugin_instance_t *store, const uint8_t *blob, uint16_t len) { - if (!plugin_provides_retain_store(store)) return -1; + if (!plugin_provides_retain_store(store)) + return -1; return store->native_plugin->retain_save(blob, len); } int plugin_driver_retain_load(plugin_instance_t *store, const char *program_md5, uint16_t md5_len, uint8_t *out, uint16_t cap, uint16_t *out_len) { - if (out_len) *out_len = 0; - if (!plugin_provides_retain_store(store)) return -1; + if (out_len) + *out_len = 0; + if (!plugin_provides_retain_store(store)) + return -1; return store->native_plugin->retain_load(program_md5, md5_len, out, cap, out_len); } @@ -1550,7 +1765,8 @@ int plugin_driver_retain_flush(plugin_instance_t *store) // Optional third hook. A plugin without it is assumed to commit inside // save(), which is where durability belongs anyway — so "nothing to do" is // success, not a failure to report on every stop. - if (!store || !store->native_plugin || !store->native_plugin->retain_flush) return 0; + if (!store || !store->native_plugin || !store->native_plugin->retain_flush) + return 0; return store->native_plugin->retain_flush(); } @@ -1666,11 +1882,10 @@ int plugin_driver_execute_command(plugin_driver_t *driver, const char *plugin_na // Output is best-effort: malformed plugin output (doesn't start with // '{' and end with '}') is silently dropped, overflow truncates, and // the core STATS response is always preserved. -#define PLUGIN_STATS_SLOT_BUDGET 1024 -#define PLUGIN_STATS_TOTAL_BUDGET 8192 +#define PLUGIN_STATS_SLOT_BUDGET 1024 +#define PLUGIN_STATS_TOTAL_BUDGET 8192 -size_t plugin_driver_append_stats_json(plugin_driver_t *driver, char *buffer, - size_t buffer_size) +size_t plugin_driver_append_stats_json(plugin_driver_t *driver, char *buffer, size_t buffer_size) { if (!buffer || buffer_size == 0) return 0; @@ -1681,7 +1896,7 @@ size_t plugin_driver_append_stats_json(plugin_driver_t *driver, char *buffer, int had_newline = 0; if (len > 0 && buffer[len - 1] == '\n') { - had_newline = 1; + had_newline = 1; buffer[--len] = '\0'; } @@ -1691,7 +1906,7 @@ size_t plugin_driver_append_stats_json(plugin_driver_t *driver, char *buffer, { if (had_newline && len + 1 < buffer_size) { - buffer[len] = '\n'; + buffer[len] = '\n'; buffer[len + 1] = '\0'; len++; } @@ -1702,7 +1917,7 @@ size_t plugin_driver_append_stats_json(plugin_driver_t *driver, char *buffer, { if (had_newline && len + 1 < buffer_size) { - buffer[len] = '\n'; + buffer[len] = '\n'; buffer[len + 1] = '\0'; len++; } @@ -1732,8 +1947,8 @@ size_t plugin_driver_append_stats_json(plugin_driver_t *driver, char *buffer, if (slen < 2 || slot[0] != '{' || slot[slen - 1] != '}') continue; // malformed — drop silently - int n = snprintf(scratch + spos, sizeof(scratch) - spos, "%s\"%s\":%s", - emitted ? "," : "", p->config.name, slot); + int n = snprintf(scratch + spos, sizeof(scratch) - spos, "%s\"%s\":%s", emitted ? "," : "", + p->config.name, slot); if (n < 0 || (size_t)n >= sizeof(scratch) - spos) break; // scratch full; commit what we have @@ -1745,7 +1960,7 @@ size_t plugin_driver_append_stats_json(plugin_driver_t *driver, char *buffer, { if (had_newline && len + 1 < buffer_size) { - buffer[len] = '\n'; + buffer[len] = '\n'; buffer[len + 1] = '\0'; len++; } @@ -1755,14 +1970,14 @@ size_t plugin_driver_append_stats_json(plugin_driver_t *driver, char *buffer, // Splice: overwrite the closing '}' with ,"plugin_stats":{...}} and // re-append the newline if present. size_t insert_pos = len - 1; - int n = snprintf(buffer + insert_pos, buffer_size - insert_pos, - ",\"plugin_stats\":{%s}}%s", scratch, had_newline ? "\n" : ""); + int n = snprintf(buffer + insert_pos, buffer_size - insert_pos, ",\"plugin_stats\":{%s}}%s", + scratch, had_newline ? "\n" : ""); if (n < 0) { // snprintf failure — restore newline and bail. if (had_newline && len + 1 < buffer_size) { - buffer[len] = '\n'; + buffer[len] = '\n'; buffer[len + 1] = '\0'; len++; } @@ -1772,12 +1987,12 @@ size_t plugin_driver_append_stats_json(plugin_driver_t *driver, char *buffer, { // Would overflow the response buffer; roll back by restoring the '}' // and the newline. - buffer[insert_pos] = '}'; + buffer[insert_pos] = '}'; buffer[insert_pos + 1] = '\0'; - len = insert_pos + 1; + len = insert_pos + 1; if (had_newline && len + 1 < buffer_size) { - buffer[len] = '\n'; + buffer[len] = '\n'; buffer[len + 1] = '\0'; len++; } diff --git a/core/src/drivers/plugin_driver.h b/core/src/drivers/plugin_driver.h index dac0d063..4076a71f 100644 --- a/core/src/drivers/plugin_driver.h +++ b/core/src/drivers/plugin_driver.h @@ -82,6 +82,29 @@ typedef int (*plugin_retain_load_func_t)(const char *program_md5, uint16_t md5_l * assumed to commit inside save(), which is where durability belongs anyway. */ typedef int (*plugin_retain_flush_func_t)(void); +/* Optional, and its PRESENCE is the capability declaration (RTOP-284). + * + * The fourteen image tables no longer share one length, and + * `plugin_runtime_args_t` carries a single `buffer_size` that cannot say so. + * Rather than move that struct -- CON06 guarantees pre-compiled plugins keep + * their field offsets -- the sizes travel through a symbol the loader resolves + * with dlsym, exactly as it already does for execute_command, get_stats and + * the three retain_* hooks. + * + * Exporting it means "I understand per-table sizes". A plugin without it is + * not broken and is not refused; the runtime keeps the image SQUARE for that + * run instead, because a plugin bounding a byte index and a word index with + * the same number is only safe while the tables are equal. + * + * Called BEFORE init(), once per plugin_driver_init(). Self-contained on + * purpose -- no args -- which is what lets it run that early. A non-zero + * return fails the plugin exactly as a failed init() does. + * + * `sizes` is indexed by `image_table_id_t` and `count` is how many entries it + * carries, so a plugin built against an older enum reads the prefix it knows + * and ignores the rest. */ +typedef int (*plugin_set_image_sizes_func_t)(const uint32_t *sizes, uint32_t count); + typedef struct { void *handle; // Handle to the loaded shared library @@ -97,6 +120,9 @@ typedef struct plugin_retain_save_func_t retain_save; plugin_retain_load_func_t retain_load; plugin_retain_flush_func_t retain_flush; + /* Optional; NULL means this plugin does not understand per-table sizes and + * the image stays square for the run. */ + plugin_set_image_sizes_func_t set_image_sizes; } plugin_funct_bundle_t; // Plugin instance structure @@ -149,6 +175,22 @@ int plugin_driver_init(plugin_driver_t *driver); * state and duplicates threads/sockets. Safe to call when no plugins are * initialised. Returns the count of plugins it cleaned up. */ int plugin_driver_cleanup_init(plugin_driver_t *driver); + +/** + * Is any plugin still initialised? + * + * Asked before releasing the image, which is the half of the ordering + * requirement that had no check. The allocate-before-init half is guarded by a + * real runtime refusal in generate_structured_args_with_driver; this is its + * counterpart, so a refactor that frees the image while a plugin still holds + * the base pointers it copied by value at init() gets a diagnostic instead of + * a use-after-free. + * + * Not the same as "still running": cleanup_init skips a plugin whose + * `initialized` is 0, and a native plugin with no `cleanup` symbol keeps its + * by-value args copy either way. + */ +int plugin_driver_any_initialized(plugin_driver_t *driver); int plugin_driver_start(plugin_driver_t *driver); int plugin_driver_stop(plugin_driver_t *driver); void plugin_driver_destroy(plugin_driver_t *driver); @@ -187,6 +229,26 @@ int plugin_driver_retain_load(plugin_instance_t *store, const char *program_md5, uint8_t *out, uint16_t cap, uint16_t *out_len); int plugin_driver_retain_flush(plugin_instance_t *store); +/** + * Does every loaded plugin understand per-table image sizes? + * + * Asked once per program load, BEFORE the image is allocated, because the + * answer decides how it is allocated. If any plugin says no, the image stays + * SQUARE for that run -- every table the same length. + * + * That is not a preference. The shipped VPP plugins bound a byte index into + * bool_output and a word index into int_output with the same `buffer_size`: + * the largest table would be an out-of-bounds read on the smaller ones, and + * the smallest would silently drop configured I/O. Only equal lengths keep one + * bound honest. + * + * `first_without` receives the name of the first plugin that does not, so the + * decision can be logged with a reason. It is the only way an operator can + * tell the two modes apart. + */ +bool plugin_driver_all_understand_per_table_sizes(plugin_driver_t *driver, + const char **first_without); + // Route a command to a specific plugin by name (for async commands like scan) int plugin_driver_execute_command(plugin_driver_t *driver, const char *plugin_name, const char *command_json, char *response, size_t response_size); diff --git a/core/src/drivers/plugin_types.h b/core/src/drivers/plugin_types.h index 62ca61e6..d4c25f04 100644 --- a/core/src/drivers/plugin_types.h +++ b/core/src/drivers/plugin_types.h @@ -16,6 +16,16 @@ #define PLUGIN_TYPES_H #include "../lib/iec_types.h" +/* The image table identities, so a plugin receiving the sizes array can name + * the entries it indexes rather than counting positions (RTOP-284, B2). + * Publishing a type costs no ABI: no struct gains a field and no offset + * moves, which is what CON06 guarantees pre-compiled plugins. + * + * A plugin built against an OLDER runtime will not find this header, so + * anything that must work on both — a VPP package, which ships and is built + * independently of the runtime on the device — carries its own constants and + * keeps them in the order documented there. */ +#include "../plc_app/image_table_id.h" #include #include #include @@ -229,7 +239,36 @@ typedef struct /* Plugin configuration */ char plugin_specific_config_file_path[256]; - /* Buffer size information */ + /* THE SMALLEST TABLE, NOT THE ONLY ONE (RTOP-284). + * + * The fourteen image tables no longer share a length. This field cannot + * say that -- CON06 guarantees pre-compiled plugins keep their field + * offsets, so it does not move -- and it is now the MINIMUM of the + * fourteen rather than the length they all happened to have. + * + * The minimum is the only safe answer for a consumer that still reads one + * number: bounding by it refuses an index that would have run off the end + * of the shortest table, where bounding by the largest would have read + * past every table below it. Under-permissive, never over. + * + * A plugin that wants the truth exports `set_image_sizes` (plugin_driver.h) + * and receives all fourteen before its init() runs. When every loaded + * plugin does, the image is allocated per table; when any does not, it is + * kept square for that run and this field is again the length they all + * have. + * + * Not marked deprecated yet, and the reason is not what an earlier draft + * of this comment claimed. The runtime core does build with -Werror + * (core/src/CMakeLists.txt), but the plugins do not: they are configured + * by their own cmake invocation and the VPP packages by a plain Makefile, + * so the attribute would produce warnings there, not a build failure. + * + * It is deferred because the field is still the RIGHT thing to read: on a + * square run it is the length every table has, and it is the only bound a + * plugin that has not adopted set_image_sizes can use. Deprecating it now + * would warn at correct code, including in packages that ship + * independently and must keep working against older runtimes. The + * attribute goes in once the symbol is universal. */ int buffer_size; int bits_per_buffer; diff --git a/core/src/drivers/plugins/native/ethercat/CMakeLists.txt b/core/src/drivers/plugins/native/ethercat/CMakeLists.txt index 5126f2b2..54c5dc29 100644 --- a/core/src/drivers/plugins/native/ethercat/CMakeLists.txt +++ b/core/src/drivers/plugins/native/ethercat/CMakeLists.txt @@ -153,6 +153,10 @@ set(PLUGIN_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/ethercat_proc.c ${CMAKE_CURRENT_SOURCE_DIR}/ethercat_iface_state.c ${OPENPLC_ROOT}/core/src/drivers/plugins/native/plugin_logger.c + # Exports set_image_sizes, which is how this plugin declares it + # understands per-table image sizes (RTOP-284). Without it the + # runtime keeps the image square for every run this plugin is in. + ${OPENPLC_ROOT}/core/src/drivers/plugins/native/plugin_image_sizes.c ) # ============================================================================= diff --git a/core/src/drivers/plugins/native/ethercat/ethercat_io.c b/core/src/drivers/plugins/native/ethercat/ethercat_io.c index 97de392a..0531ae42 100644 --- a/core/src/drivers/plugins/native/ethercat/ethercat_io.c +++ b/core/src/drivers/plugins/native/ethercat/ethercat_io.c @@ -19,6 +19,8 @@ */ #include "ethercat_io.h" + +#include "../plugin_image_sizes.h" #include "ethercat_master.h" #include @@ -234,6 +236,31 @@ static int ecat_data_type_expected_iec_size(ecat_data_type_t dt) /** * @brief Return a human-readable name for an iec_size_t value */ +/* (direction, size) -> the image table that stores it. + * + * EtherCAT only ever emits %I and %Q, so there is no memory case to answer. + * The mapping is spelled out rather than arithmetic on the enums: the two + * orders are unrelated and a cast between them would land writes in another + * table's bounds. */ +static image_table_id_t ecat_table_for(iec_dir_t dir, iec_size_t size) +{ + const int in = (dir == IEC_DIR_INPUT); + switch (size) + { + case IEC_SIZE_BIT: + return in ? IMAGE_TABLE_BOOL_INPUT : IMAGE_TABLE_BOOL_OUTPUT; + case IEC_SIZE_BYTE: + return in ? IMAGE_TABLE_BYTE_INPUT : IMAGE_TABLE_BYTE_OUTPUT; + case IEC_SIZE_WORD: + return in ? IMAGE_TABLE_INT_INPUT : IMAGE_TABLE_INT_OUTPUT; + case IEC_SIZE_DWORD: + return in ? IMAGE_TABLE_DINT_INPUT : IMAGE_TABLE_DINT_OUTPUT; + case IEC_SIZE_LWORD: + return in ? IMAGE_TABLE_LINT_INPUT : IMAGE_TABLE_LINT_OUTPUT; + } + return IMAGE_TABLE_COUNT; +} + static const char *iec_size_name(iec_size_t sz) { switch (sz) { @@ -301,13 +328,31 @@ int ecat_io_build_channel_map(const ecat_config_t *config, continue; } - /* Bounds check against PLC buffer size */ - if (iec_loc.byte_index >= args->buffer_size) { + /* Bounds check against THIS LOCATION'S OWN TABLE. + * + * args->buffer_size is now the SMALLEST of the fourteen, so using + * it here would refuse every channel above the shortest table's + * length -- an EtherCAT slave silently losing most of its I/O on a + * project that sizes one area small. The table the location + * actually lands in is the only honest bound (RTOP-284). */ + const image_table_id_t table = ecat_table_for(iec_loc.direction, iec_loc.size); + /* Falls back to args->buffer_size when the sizes were never + * delivered. That is not belt and braces: without it a runtime + * that does not call set_image_sizes -- an older one loading this + * plugin -- leaves every table at zero here and EVERY channel is + * refused, taking the whole bus down silently. On a square run + * buffer_size IS the length every table has, so it is the right + * answer rather than a guess. */ + const uint32_t reach = plugin_image_sizes_known() + ? plugin_image_table_capacity(table) + : (uint32_t)(args->buffer_size > 0 ? args->buffer_size : 0); + if (iec_loc.byte_index < 0 || (uint32_t)iec_loc.byte_index >= reach) + { plugin_logger_warn(logger, - "Slave '%s' channel '%s': IEC location '%s' byte index %d " - "exceeds buffer size %d, skipping", - cfg_slave->name, ch->name, ch->iec_location, - iec_loc.byte_index, args->buffer_size); + "Slave '%s' channel '%s': IEC location '%s' byte index %d " + "is outside the %u element(s) that area has, skipping", + cfg_slave->name, ch->name, ch->iec_location, iec_loc.byte_index, + reach); errors++; continue; } diff --git a/core/src/drivers/plugins/native/ethercat/ethercat_plugin.c b/core/src/drivers/plugins/native/ethercat/ethercat_plugin.c index 7c08c922..782724a4 100644 --- a/core/src/drivers/plugins/native/ethercat/ethercat_plugin.c +++ b/core/src/drivers/plugins/native/ethercat/ethercat_plugin.c @@ -53,11 +53,13 @@ #include "plugin_logger.h" #include "plugin_types.h" #include "ethercat_plugin.h" + +#include "../plugin_image_sizes.h" +#include "cJSON.h" /* JSON parsing for execute_command */ #include "ethercat_config.h" -#include "ethercat_master.h" #include "ethercat_io.h" -#include "soem/soem.h" /* osal_get_monotonic_time, ec_timet */ -#include "cJSON.h" /* JSON parsing for execute_command */ +#include "ethercat_master.h" +#include "soem/soem.h" /* osal_get_monotonic_time, ec_timet */ /* Forward declaration: ecat_bus_thread is defined alongside the bus * loop further down in the file but referenced first by @@ -1198,7 +1200,17 @@ int init(void *args) * land in the runtime journal instead of stderr. */ ecat_config_set_logger(&g_logger); - plugin_logger_info(&g_logger, "Buffer size: %d", g_runtime_args.buffer_size); + /* What this plugin was told, not the deprecated single figure: on a + * per-table run buffer_size is the SMALLEST of the fourteen and says + * nothing about the areas this bus actually reaches. */ + if (!plugin_image_sizes_known()) + plugin_logger_info(&g_logger, "Image sizes: not delivered (square run)"); + else + plugin_logger_info(&g_logger, "Image sizes: bits in %u/%u, words in %u/%u", + plugin_image_table_capacity(IMAGE_TABLE_BOOL_INPUT), + plugin_image_table_capacity(IMAGE_TABLE_BOOL_OUTPUT), + plugin_image_table_capacity(IMAGE_TABLE_INT_INPUT), + plugin_image_table_capacity(IMAGE_TABLE_INT_OUTPUT)); /* Parse ALL master configurations from the JSON file */ const char *config_path = g_runtime_args.plugin_specific_config_file_path; diff --git a/core/src/drivers/plugins/native/plugin_image_sizes.c b/core/src/drivers/plugins/native/plugin_image_sizes.c new file mode 100644 index 00000000..1459dbda --- /dev/null +++ b/core/src/drivers/plugins/native/plugin_image_sizes.c @@ -0,0 +1,37 @@ +#include "plugin_image_sizes.h" + +#include + +/* Zeroed until the runtime calls set_image_sizes(), which it does once per + * plugin_driver_init() and therefore once per program load. Deliberately NOT + * remembered across loads: a cached copy from the previous program is exactly + * the stale state the per-load delivery exists to prevent. */ +static uint32_t g_sizes[IMAGE_TABLE_COUNT]; +static int g_known = 0; + +/** + * Exported for the runtime to find by dlsym. Its presence is the declaration. + * + * `count` is how many entries the runtime sent, which need not be + * IMAGE_TABLE_COUNT: a plugin built against an older enum reads the prefix it + * knows and ignores the rest, and one built against a newer enum leaves the + * tail at zero rather than reading past the array. + */ +int set_image_sizes(const uint32_t *sizes, uint32_t count) +{ + memset(g_sizes, 0, sizeof(g_sizes)); + if (!sizes) return -1; + + const uint32_t n = count < (uint32_t)IMAGE_TABLE_COUNT ? count : (uint32_t)IMAGE_TABLE_COUNT; + for (uint32_t i = 0; i < n; ++i) g_sizes[i] = sizes[i]; + g_known = 1; + return 0; +} + +uint32_t plugin_image_table_capacity(image_table_id_t id) +{ + if (id < 0 || id >= IMAGE_TABLE_COUNT) return 0; + return g_sizes[id]; +} + +int plugin_image_sizes_known(void) { return g_known; } diff --git a/core/src/drivers/plugins/native/plugin_image_sizes.h b/core/src/drivers/plugins/native/plugin_image_sizes.h new file mode 100644 index 00000000..7d4a64bd --- /dev/null +++ b/core/src/drivers/plugins/native/plugin_image_sizes.h @@ -0,0 +1,46 @@ +/** + * The fourteen image table lengths, for a native plugin (RTOP-284). + * + * Linking this file into a plugin gives it two things at once: the exported + * `set_image_sizes` symbol the runtime looks for -- whose PRESENCE is how a + * plugin declares it understands per-table sizes -- and the accessor to read + * back what it was told. + * + * One implementation rather than one per plugin, for the same reason the + * logger is shared: three copies of a fourteen-element cache is three places + * for the indexing to drift, and the whole point of this work is that the + * tables no longer share a length. + * + * A plugin that does NOT link this is not broken. The runtime keeps the image + * square for that run and says which plugin forced it. + */ + +#ifndef PLUGIN_IMAGE_SIZES_H +#define PLUGIN_IMAGE_SIZES_H + +#include "../../../plc_app/image_table_id.h" + +#include + +#ifdef __cplusplus +extern "C" { +#endif + +/** + * How long one table is, in its own elements. + * + * Returns 0 before the runtime has delivered the sizes and for an id this + * build does not know, which are the same answer for a caller: an area it + * cannot index into. Bounding against 0 refuses every access, which is the + * safe direction for a plugin asked to act before it has been told anything. + */ +uint32_t plugin_image_table_capacity(image_table_id_t id); + +/** Whether the runtime has delivered the sizes for this load yet. */ +int plugin_image_sizes_known(void); + +#ifdef __cplusplus +} +#endif + +#endif /* PLUGIN_IMAGE_SIZES_H */ diff --git a/core/src/drivers/plugins/native/s7comm/CMakeLists.txt b/core/src/drivers/plugins/native/s7comm/CMakeLists.txt index 706a705d..dc373d10 100644 --- a/core/src/drivers/plugins/native/s7comm/CMakeLists.txt +++ b/core/src/drivers/plugins/native/s7comm/CMakeLists.txt @@ -58,6 +58,10 @@ set(PLUGIN_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/s7comm_plugin.cpp ${CMAKE_CURRENT_SOURCE_DIR}/s7comm_config.c ${OPENPLC_ROOT}/core/src/drivers/plugins/native/plugin_logger.c + # Exports set_image_sizes, which is how this plugin declares it + # understands per-table image sizes (RTOP-284). Without it the + # runtime keeps the image square for every run this plugin is in. + ${OPENPLC_ROOT}/core/src/drivers/plugins/native/plugin_image_sizes.c ) # ============================================================================= diff --git a/core/src/drivers/plugins/native/s7comm/s7comm_plugin.cpp b/core/src/drivers/plugins/native/s7comm/s7comm_plugin.cpp index cd11024e..1753b507 100644 --- a/core/src/drivers/plugins/native/s7comm/s7comm_plugin.cpp +++ b/core/src/drivers/plugins/native/s7comm/s7comm_plugin.cpp @@ -32,6 +32,8 @@ extern "C" { #include "plugin_logger.h" #include "plugin_types.h" #include "s7comm_plugin.h" + +#include "../plugin_image_sizes.h" #include "s7comm_config.h" } @@ -335,7 +337,32 @@ extern "C" int init(void *args) plugin_logger_init(&g_logger, "S7COMM", args); plugin_logger_info(&g_logger, "Initializing S7Comm plugin..."); - plugin_logger_info(&g_logger, "Buffer size: %d", g_runtime_args.buffer_size); + /* What this plugin was actually told, not the deprecated single figure. + * On a per-table run `buffer_size` is the SMALLEST of the fourteen and + * says nothing useful about the areas this server serves. Only the + * non-empty tables, because fourteen figures of which most are one reads + * as noise. Nothing parses this line. */ + { + char sizes[192]; + int at = 0; + for (int id = 0; id < IMAGE_TABLE_COUNT && at < (int)sizeof(sizes) - 1; ++id) + { + const uint32_t n = plugin_image_table_capacity((image_table_id_t)id); + if (n <= 1) + continue; + const int wrote = + snprintf(sizes + at, sizeof(sizes) - (size_t)at, "%s%d:%u", at ? " " : "", id, n); + if (wrote < 0 || wrote >= (int)sizeof(sizes) - at) + break; + at += wrote; + } + if (!plugin_image_sizes_known()) + plugin_logger_info(&g_logger, "Image sizes: not delivered (square run)"); + else if (at == 0) + plugin_logger_info(&g_logger, "Image sizes: every table at the minimum"); + else + plugin_logger_info(&g_logger, "Image sizes by table id: %s", sizes); + } g_initialized = true; return 0; @@ -725,6 +752,72 @@ static int get_type_size(s7comm_buffer_type_t type) * ============================================================================= */ +/* s7comm_buffer_type_t -> the image table that stores it. + * + * A THIRD order for the same fourteen tables. This enum groups each width's + * memory beside its input and output, matching journal_buffer_type_t; + * image_table_id_t puts every memory table at the end -- and this enum starts + * at BUFFER_TYPE_NONE, so it is offset again. For one table, int_memory: + * BUFFER_TYPE_INT_MEMORY is 8, JOURNAL_INT_MEMORY is 7, IMAGE_TABLE_INT_MEMORY + * is 10. Three different numbers for one table is the whole argument, and a + * cast between any two reads and writes under another table's bounds. Written + * out rather than computed. */ +static image_table_id_t s7_image_table(s7comm_buffer_type_t type) +{ + switch (type) + { + case BUFFER_TYPE_BOOL_INPUT: + return IMAGE_TABLE_BOOL_INPUT; + case BUFFER_TYPE_BOOL_OUTPUT: + return IMAGE_TABLE_BOOL_OUTPUT; + case BUFFER_TYPE_BOOL_MEMORY: + return IMAGE_TABLE_BOOL_MEMORY; + case BUFFER_TYPE_BYTE_INPUT: + return IMAGE_TABLE_BYTE_INPUT; + case BUFFER_TYPE_BYTE_OUTPUT: + return IMAGE_TABLE_BYTE_OUTPUT; + case BUFFER_TYPE_INT_INPUT: + return IMAGE_TABLE_INT_INPUT; + case BUFFER_TYPE_INT_OUTPUT: + return IMAGE_TABLE_INT_OUTPUT; + case BUFFER_TYPE_INT_MEMORY: + return IMAGE_TABLE_INT_MEMORY; + case BUFFER_TYPE_DINT_INPUT: + return IMAGE_TABLE_DINT_INPUT; + case BUFFER_TYPE_DINT_OUTPUT: + return IMAGE_TABLE_DINT_OUTPUT; + case BUFFER_TYPE_DINT_MEMORY: + return IMAGE_TABLE_DINT_MEMORY; + case BUFFER_TYPE_LINT_INPUT: + return IMAGE_TABLE_LINT_INPUT; + case BUFFER_TYPE_LINT_OUTPUT: + return IMAGE_TABLE_LINT_OUTPUT; + case BUFFER_TYPE_LINT_MEMORY: + return IMAGE_TABLE_LINT_MEMORY; + /* An unmapped block. IMAGE_TABLE_COUNT is not a table, so + * plugin_image_table_capacity() answers 0 and every access is refused -- + * which is what an unconfigured block should do. */ + case BUFFER_TYPE_NONE: + break; + } + return IMAGE_TABLE_COUNT; +} + +/* How far the table behind `type` reaches, as a signed count so the clamps + * below can subtract a start offset without wrapping. */ +static int s7_table_reach(s7comm_buffer_type_t type) +{ + /* Falls back to the single figure when the sizes were never delivered. + * Without it, a runtime that does not call set_image_sizes -- an older one + * loading this plugin -- leaves every table at zero and every read and + * write is clamped to nothing, so the server answers zeros for everything + * with no error anywhere. On a square run buffer_size IS the length every + * table has. */ + if (!plugin_image_sizes_known()) + return g_runtime_args.buffer_size; + return (int)plugin_image_table_capacity(s7_image_table(type)); +} + /** * @brief Read OpenPLC bool buffer to destination (mutex must be held) */ @@ -750,7 +843,7 @@ static void read_openplc_bool_to_buffer(uint8_t *dest, int size, s7comm_buffer_t return; } - int max_bytes = g_runtime_args.buffer_size - start_buffer; + int max_bytes = s7_table_reach(type) - start_buffer; if (max_bytes > size) max_bytes = size; for (int byte_idx = 0; byte_idx < max_bytes; byte_idx++) { @@ -789,7 +882,7 @@ static void read_openplc_int_to_buffer(uint8_t *dest, int size, s7comm_buffer_ty uint16_t *s7_words = (uint16_t *)dest; int num_words = size / 2; - int max_words = g_runtime_args.buffer_size - start_buffer; + int max_words = s7_table_reach(type) - start_buffer; if (max_words > num_words) max_words = num_words; for (int i = 0; i < max_words; i++) { @@ -823,7 +916,7 @@ static void read_openplc_dint_to_buffer(uint8_t *dest, int size, s7comm_buffer_t uint32_t *s7_dwords = (uint32_t *)dest; int num_dwords = size / 4; - int max_dwords = g_runtime_args.buffer_size - start_buffer; + int max_dwords = s7_table_reach(type) - start_buffer; if (max_dwords > num_dwords) max_dwords = num_dwords; for (int i = 0; i < max_dwords; i++) { @@ -857,7 +950,7 @@ static void read_openplc_lint_to_buffer(uint8_t *dest, int size, s7comm_buffer_t uint64_t *s7_lwords = (uint64_t *)dest; int num_lwords = size / 8; - int max_lwords = g_runtime_args.buffer_size - start_buffer; + int max_lwords = s7_table_reach(type) - start_buffer; if (max_lwords > num_lwords) max_lwords = num_lwords; for (int i = 0; i < max_lwords; i++) { @@ -919,7 +1012,7 @@ static void write_bool_to_openplc_journal(uint8_t *src, int size, s7comm_buffer_ int journal_type = map_to_journal_type(type); if (journal_type < 0) return; - int max_bytes = g_runtime_args.buffer_size - start_buffer; + int max_bytes = s7_table_reach(type) - start_buffer; if (max_bytes > size) max_bytes = size; for (int byte_idx = 0; byte_idx < max_bytes; byte_idx++) { @@ -942,7 +1035,7 @@ static void write_int_to_openplc_journal(uint8_t *src, int size, s7comm_buffer_t uint16_t *s7_words = (uint16_t *)src; int num_words = size / 2; - int max_words = g_runtime_args.buffer_size - start_buffer; + int max_words = s7_table_reach(type) - start_buffer; if (max_words > num_words) max_words = num_words; for (int i = 0; i < max_words; i++) { @@ -961,7 +1054,7 @@ static void write_dint_to_openplc_journal(uint8_t *src, int size, s7comm_buffer_ uint32_t *s7_dwords = (uint32_t *)src; int num_dwords = size / 4; - int max_dwords = g_runtime_args.buffer_size - start_buffer; + int max_dwords = s7_table_reach(type) - start_buffer; if (max_dwords > num_dwords) max_dwords = num_dwords; for (int i = 0; i < max_dwords; i++) { @@ -980,7 +1073,7 @@ static void write_lint_to_openplc_journal(uint8_t *src, int size, s7comm_buffer_ uint64_t *s7_lwords = (uint64_t *)src; int num_lwords = size / 8; - int max_lwords = g_runtime_args.buffer_size - start_buffer; + int max_lwords = s7_table_reach(type) - start_buffer; if (max_lwords > num_lwords) max_lwords = num_lwords; for (int i = 0; i < max_lwords; i++) { diff --git a/core/src/drivers/plugins/python/examples/example_python_plugin.py b/core/src/drivers/plugins/python/examples/example_python_plugin.py index 5ed49e7c..cbb1133b 100644 --- a/core/src/drivers/plugins/python/examples/example_python_plugin.py +++ b/core/src/drivers/plugins/python/examples/example_python_plugin.py @@ -11,8 +11,9 @@ import threading import sys import os + # Add the parent directory to Python path to find shared module -sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..')) +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) # Import the correct type definitions from shared import ( @@ -20,7 +21,7 @@ safe_extract_runtime_args_from_capsule, SafeBufferAccess, SafeLoggingAccess, - PluginStructureValidator + PluginStructureValidator, ) # Global variable to track initialization @@ -31,6 +32,7 @@ _mainthread = None _stop = threading.Event() + def init(runtime_args_capsule): """ Plugin initialization function @@ -63,8 +65,11 @@ def init(runtime_args_capsule): if _safe_logging_access.is_valid: success, msg = _safe_logging_access.log_info("Python plugin initialization started") if success: - _safe_logging_access.log_debug("Plugin received buffer_size={}, bits_per_buffer={}".format( - runtime_args.buffer_size, runtime_args.bits_per_buffer)) + _safe_logging_access.log_debug( + "Plugin received buffer_size={}, bits_per_buffer={}".format( + runtime_args.buffer_size, runtime_args.bits_per_buffer + ) + ) else: print(f"(WARN) Logging failed: {msg}") else: @@ -75,19 +80,23 @@ def init(runtime_args_capsule): if buffer_size == -1: print(f"(FAIL) Failed to access buffer size: {size_error}") if _safe_logging_access.is_valid: - _safe_logging_access.log_error("Failed to access buffer size: %s", size_error) + _safe_logging_access.log_error(f"Failed to access buffer size: {size_error}") return False - print(f" Buffer size: {buffer_size}") - print(f" Bits per buffer: {runtime_args.bits_per_buffer}") - print(f" Structure details: {runtime_args}") + # Through the logger, not print(). This file is what vendors copy, and + # a print() from a plugin goes to whatever stdout the runtime happens + # to have rather than the central log the operator is reading. + _safe_logging_access.log_info(f"Buffer size (smallest table): {buffer_size}") + _safe_logging_access.log_info(f"Bits per buffer: {runtime_args.bits_per_buffer}") # Create safe buffer access wrapper _safe_buffer_access = SafeBufferAccess(runtime_args) if not _safe_buffer_access.is_valid: print(f"(FAIL) Failed to create safe buffer access: {_safe_buffer_access.error_msg}") if _safe_logging_access.is_valid: - _safe_logging_access.log_error("Failed to create safe buffer access: %s", _safe_buffer_access.error_msg) + _safe_logging_access.log_error( + f"Failed to create safe buffer access: {_safe_buffer_access.error_msg}" + ) return False # Store runtime args for later use @@ -96,7 +105,9 @@ def init(runtime_args_capsule): print("(PASS) Plugin initialized successfully") if _safe_logging_access.is_valid: - success, msg = _safe_logging_access.log_info("Python plugin initialization completed successfully") + success, msg = _safe_logging_access.log_info( + "Python plugin initialization completed successfully" + ) if not success: print(f"(WARN) Final logging failed: {msg}") @@ -105,27 +116,30 @@ def init(runtime_args_capsule): except Exception as e: print(f"(FAIL) Plugin initialization failed: {e}") import traceback + traceback.print_exc() return False + def start_loop(): """ Called when the plugin loop should start Optional function - not all plugins need this """ + def loop(): global _runtime_args, _stop print("Plugin start_loop called") while not _stop.is_set(): time.sleep(1) continue - global _mainthread _mainthread = threading.Thread(target=loop, daemon=True) _mainthread.start() return 0 + def stop_loop(): """ Called when the plugin loop should stop @@ -141,6 +155,7 @@ def stop_loop(): _mainthread = None print("(PASS) Main thread stopped") + def cleanup(): """ Plugin cleanup function @@ -158,11 +173,12 @@ def cleanup(): print("(PASS) Plugin cleaned up successfully") + if __name__ == "__main__": print("This is an example Python plugin for OpenPLC Runtime") print("Expected functions:") print(" - init(runtime_args_capsule) -> bool") - print(" - start_loop() -> None (optional)") + print(" - start_loop() -> None (optional)") print(" - stop_loop() -> None (optional)") print(" - run_cycle() -> None (optional)") print(" - cleanup() -> None (optional)") diff --git a/core/src/drivers/plugins/python/modbus_master/modbus_master_plugin.py b/core/src/drivers/plugins/python/modbus_master/modbus_master_plugin.py index e9d636be..da601808 100644 --- a/core/src/drivers/plugins/python/modbus_master/modbus_master_plugin.py +++ b/core/src/drivers/plugins/python/modbus_master/modbus_master_plugin.py @@ -20,6 +20,17 @@ ) # Import the configuration model +# Importing set_image_sizes is not a formality: the name has to exist in THIS +# module for the runtime to find it, and its presence is how this plugin +# declares it understands per-table image sizes (RTOP-284). The runtime keeps +# the image SQUARE for any run in which even one loaded plugin lacks it -- and +# this plugin ships in plugins_default.conf, so without this line per-table +# sizing never activates on a stock device. +# +# Nothing else is needed here: this plugin bounds through SafeBufferAccess -> +# BufferValidator, which already validates against the table each buffer lives +# in rather than against the single figure. +from shared.image_sizes import set_image_sizes # noqa: F401 from shared.plugin_config_decode.modbus_master_config_model import ( ERROR_HANDLING_SET_TO_ZERO, ModbusMasterConfig, @@ -97,6 +108,7 @@ class ModbusSlaveDevice(threading.Thread): Handles a single Modbus TCP device with its own connection. For RTU devices, use ModbusRtuBusHandler instead. """ + def __init__(self, device_config: Any, sba: SafeBufferAccess, plugin_logger: PluginLogger): super().__init__(daemon=True) self.device_config = device_config @@ -110,9 +122,11 @@ def __init__(self, device_config: Any, sba: SafeBufferAccess, plugin_logger: Plu host=device_config.host, port=device_config.port, timeout_ms=device_config.timeout_ms, - slave_id=device_config.slave_id + slave_id=device_config.slave_id, + ) + self.name = ( + f"ModbusSlave-{device_config.name}-TCP-{device_config.host}:{device_config.port}" ) - self.name = f"ModbusSlave-{device_config.name}-TCP-{device_config.host}:{device_config.port}" # Calculate GCD of all I/O point cycle times for this device self.gcd_cycle_time_ms = calculate_gcd_of_cycle_times(device_config.io_points) @@ -139,7 +153,9 @@ def run(self): # pylint: disable=too-many-locals # Connect with infinite retry if not self.connection_manager.connect_with_retry(self._stop_event): - self.logger.info(f"[{self.name}] Thread stopped before connection could be established.") + self.logger.info( + f"[{self.name}] Thread stopped before connection could be established." + ) return # Initialize cycle counter @@ -368,23 +384,25 @@ def run(self): # pylint: disable=too-many-locals if point.fc == 5: # Write Single Coil if len(values_to_write) > 0: response = self.connection_manager.client.write_coil( - address, values_to_write[0], device_id=self.connection_manager.slave_id + address, + values_to_write[0], + device_id=self.connection_manager.slave_id, ) else: self.logger.error( - f"[{self.name}] No data to write " - f"for FC 5, offset {address}" + f"[{self.name}] No data to write " f"for FC 5, offset {address}" ) continue elif point.fc == 6: # Write Single Register if len(values_to_write) > 0: response = self.connection_manager.client.write_register( - address, values_to_write[0], device_id=self.connection_manager.slave_id + address, + values_to_write[0], + device_id=self.connection_manager.slave_id, ) else: self.logger.error( - f"[{self.name}] No data to write " - f"for FC 6, offset {address}" + f"[{self.name}] No data to write " f"for FC 6, offset {address}" ) continue elif point.fc == 15: # Write Multiple Coils @@ -478,10 +496,10 @@ class ModbusBusHandler(threading.Thread): def __init__( self, - transport: str, # "tcp" or "rtu" - connection_config: dict, # tcp: {host, port, timeout_ms}; - # rtu: {serial_port, baud_rate, parity, stop_bits, data_bits, timeout_ms} - devices: List[Any], # List of ModbusDeviceConfig sharing this connection + transport: str, # "tcp" or "rtu" + connection_config: dict, # tcp: {host, port, timeout_ms}; + # rtu: {serial_port, baud_rate, parity, stop_bits, data_bits, timeout_ms} + devices: List[Any], # List of ModbusDeviceConfig sharing this connection sba: SafeBufferAccess, plugin_logger: PluginLogger, ): @@ -522,17 +540,23 @@ def __init__( self.all_io_points = [] for device in devices: for point in device.io_points: - self.all_io_points.append({ - "point": point, - "slave_id": device.slave_id, - "device_name": device.name, - }) + self.all_io_points.append( + { + "point": point, + "slave_id": device.slave_id, + "device_name": device.name, + } + ) # Calculate GCD of all IO point cycle times across all devices on this bus all_cycle_times = [p.cycle_time_ms for d in devices for p in d.io_points] - self.gcd_cycle_time_ms = calculate_gcd_of_cycle_times( - [type('obj', (object,), {'cycle_time_ms': ct})() for ct in all_cycle_times] - ) if all_cycle_times else 1000 + self.gcd_cycle_time_ms = ( + calculate_gcd_of_cycle_times( + [type("obj", (object,), {"cycle_time_ms": ct})() for ct in all_cycle_times] + ) + if all_cycle_times + else 1000 + ) device_names = ", ".join([d.name for d in devices]) self.logger.info( @@ -555,7 +579,9 @@ def run(self): # pylint: disable=too-many-locals,too-many-branches,too-many-sta # Connect with infinite retry if not self.connection_manager.connect_with_retry(self._stop_event): - self.logger.info(f"[{self.name}] Thread stopped before connection could be established.") + self.logger.info( + f"[{self.name}] Thread stopped before connection could be established." + ) return # Initialize cycle counter @@ -1047,7 +1073,9 @@ def start_loop(): try: if len(endpoint_devices) == 1: device_config = endpoint_devices[0] - device_thread = ModbusSlaveDevice(device_config, safe_buffer_accessor, logger) + device_thread = ModbusSlaveDevice( + device_config, safe_buffer_accessor, logger + ) device_thread.start() slave_threads.append(device_thread) logger.info( diff --git a/core/src/drivers/plugins/python/modbus_slave/simple_modbus.py b/core/src/drivers/plugins/python/modbus_slave/simple_modbus.py index 52c57aa4..be002f27 100644 --- a/core/src/drivers/plugins/python/modbus_slave/simple_modbus.py +++ b/core/src/drivers/plugins/python/modbus_slave/simple_modbus.py @@ -21,11 +21,40 @@ ModbusServerContext, ModbusSparseDataBlock, ) +from pymodbus.datastore.store import ExcCodes from pymodbus.server import ServerStop from pymodbus.server.server import ModbusTcpServer MAX_BITS = 8 -BUFFER_SIZE = 1024 # Must match BUFFER_SIZE in image_tables.h + +# A Modbus PDU carries a 16-bit address, so no data block can usefully be wider +# than this. The per-segment fit to the image is not enough by itself: the +# register block composes four segments end to end, so four segments each at +# the image ceiling would build a list far past anything a client can address. +MODBUS_MAX_ADDRESSES = 65536 + +# BUFFER_SIZE used to live here as `1024 # Must match BUFFER_SIZE in +# image_tables.h`, and that comment was the whole problem: a copy of a number +# owned by the runtime, kept in step by hand. It is gone, and the runtime's +# actual `buffer_size` is used instead (RTOP-284). +# +# The runtime no longer HAS a fixed image. It allocates one per program load, +# sized from what the project needs, so a copy here could not be right for more +# than one program at a time -- and being wrong is invisible: the exposed +# register block would be declared wider than the image, every address in the +# gap would fail the buffer read and answer zero. +# A SCADA reading %QW2000 would get a plausible, wrong value indistinguishable +# from a real zero. +# +# Clamping to the runtime's size instead makes the declared block match the +# image exactly, and getValues answers anything beyond it with exception 02 +# (Illegal Data Address). That is the client being told, by the protocol, in +# the standard way, rather than being handed a fabricated value. +# +# The refusal is OURS to issue. An earlier draft of this comment said pymodbus +# did it "out of its own validate()" -- it does not: 3.11 dropped validate from +# the datastore API and ModbusDeviceContext.getValues calls the block directly. +# Measured before the fix: every address up to 65000 answered zero. # Default segmentation configuration (matches v3 behavior) DEFAULT_HOLDING_REG_CONFIG = { @@ -58,6 +87,13 @@ safe_extract_runtime_args_from_capsule, ) +# Importing set_image_sizes is not a formality: the name has to exist in THIS +# module for the runtime to find it, and its presence is how this plugin +# declares it understands per-table image sizes (RTOP-284). Without it the +# runtime keeps the image square for every run this plugin is loaded in -- +# which, since it is loaded on most devices, would be every run. +from shared.image_sizes import set_image_sizes, table_capacity # noqa: E402,F401 + class OpenPLCDeviceContext(ModbusDeviceContext): """ @@ -171,9 +207,7 @@ def getValues(self, address, count=1): if not self.safe_buffer_access.is_valid: if logger: - logger.error( - f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}" - ) + logger.error(f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}") return [0] * count # Ensure thread-safe access @@ -215,9 +249,7 @@ def setValues(self, address, values): if not self.safe_buffer_access.is_valid: if logger: - logger.error( - f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}" - ) + logger.error(f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}") return # Journal writes are thread-safe, no mutex needed @@ -262,9 +294,7 @@ def getValues(self, address, count=1): if not self.safe_buffer_access.is_valid: if logger: - logger.error( - f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}" - ) + logger.error(f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}") return [0] * count # Ensure thread-safe access @@ -326,9 +356,7 @@ def getValues(self, address, count=1): if not self.safe_buffer_access.is_valid: if logger: - logger.error( - f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}" - ) + logger.error(f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}") return [0] * count # Ensure buffer mutex @@ -386,9 +414,7 @@ def getValues(self, address, count=1): if not self.safe_buffer_access.is_valid: if logger: - logger.error( - f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}" - ) + logger.error(f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}") return [0] * count # Ensure buffer mutex @@ -425,9 +451,7 @@ def setValues(self, address, values): if not self.safe_buffer_access.is_valid: if logger: - logger.error( - f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}" - ) + logger.error(f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}") return # Journal writes are thread-safe, no mutex needed @@ -501,9 +525,7 @@ def getValues(self, address, count=1): if not self.safe_buffer_access.is_valid: if logger: - logger.error( - f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}" - ) + logger.error(f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}") return [0] * count self.safe_buffer_access.acquire_mutex() @@ -535,7 +557,20 @@ def getValues(self, address, count=1): logger.error(f"Error reading coil %MX{mx_addr}: {error_msg}") values.append(0) else: - values.append(0) + # FORA DE TODO SEGMENTO: excecao 02, nunca zero. + # + # O clamp acima faz o bloco declarado bater com a imagem, + # e o plano era que o validate() do pymodbus recusasse o + # resto. Esse validate nao existe mais: em 3.11 a API do + # datastore perdeu o metodo e ModbusDeviceContext.getValues + # chama o bloco direto. Sem isto, todo endereco acima da + # faixa respondia 0 -- medido ate 65000 -- que e o valor + # plausivel e errado que o cliente nao distingue de um zero + # real, exatamente o que o clamp existe para evitar. + # + # getValues pode devolver um ExcCodes no lugar da lista; e + # o caminho que a propria assinatura do contexto declara. + return ExcCodes.ILLEGAL_ADDRESS return values finally: @@ -551,9 +586,7 @@ def setValues(self, address, values): if not self.safe_buffer_access.is_valid: if logger: - logger.error( - f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}" - ) + logger.error(f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}") return # Journal writes are thread-safe, no mutex needed @@ -722,9 +755,7 @@ def getValues(self, address, count=1): if not self.safe_buffer_access.is_valid: if logger: - logger.error( - f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}" - ) + logger.error(f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}") return [0] * count self.safe_buffer_access.acquire_mutex() @@ -781,7 +812,20 @@ def getValues(self, address, count=1): values.append(0) else: - values.append(0) + # FORA DE TODO SEGMENTO: excecao 02, nunca zero. + # + # O clamp acima faz o bloco declarado bater com a imagem, + # e o plano era que o validate() do pymodbus recusasse o + # resto. Esse validate nao existe mais: em 3.11 a API do + # datastore perdeu o metodo e ModbusDeviceContext.getValues + # chama o bloco direto. Sem isto, todo endereco acima da + # faixa respondia 0 -- medido ate 65000 -- que e o valor + # plausivel e errado que o cliente nao distingue de um zero + # real, exatamente o que o clamp existe para evitar. + # + # getValues pode devolver um ExcCodes no lugar da lista; e + # o caminho que a propria assinatura do contexto declara. + return ExcCodes.ILLEGAL_ADDRESS return values finally: @@ -799,9 +843,7 @@ def setValues(self, address, values): if not self.safe_buffer_access.is_valid: if logger: - logger.error( - f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}" - ) + logger.error(f"Safe buffer access not valid: {self.safe_buffer_access.error_msg}") return # Mutex needed for read-modify-write consistency on partial DINT/LINT updates @@ -881,92 +923,249 @@ def setValues(self, address, values): self.safe_buffer_access.release_mutex() -def parse_buffer_mapping_config(config_map): +SEGMENT_SECTIONS = { + "qw_count": "holding_registers", + "mw_count": "holding_registers", + "md_count": "holding_registers", + "ml_count": "holding_registers", + "qx_bits": "coils", + "mx_bits": "coils", + "ix_bits": "discrete_inputs", + "iw_count": "input_registers", +} + + +def _as_int(value, default): + """A count from an uploaded JSON file, or the default if it is not one.""" + if isinstance(value, bool) or not isinstance(value, int): + return default + return value if value >= 0 else default + + +def _section(buffer_mapping, name): + section = buffer_mapping.get(name) + return section if isinstance(section, dict) else {} + + +def _requested_counts(config_map): + """What the config asks each segment to expose, before any clamping. + + ONE PLACE UNDERSTANDS THE THREE SHAPES an uploaded config can take -- + segmented, legacy (max_coils and friends), and no buffer_mapping at all -- + so the clamp below and the warning above cannot disagree about what was + asked for. They did disagree: the warning read only the segmented shape, so + a legacy config was clamped without a word, and a config with no + buffer_mapping reported nothing while the defaults it falls back to were + clamped in silence. Those two are exactly the old-editor upload that the + clamp exists for -- the case where the image sizes never reached the + device is the only case where anything shrinks at all. + """ + buffer_mapping = config_map.get("buffer_mapping") + if not isinstance(buffer_mapping, dict): + buffer_mapping = {} + + if isinstance(buffer_mapping.get("holding_registers"), dict): + hr = _section(buffer_mapping, "holding_registers") + coils = _section(buffer_mapping, "coils") + di = _section(buffer_mapping, "discrete_inputs") + ir = _section(buffer_mapping, "input_registers") + return "segmented", { + "qw_count": _as_int(hr.get("qw_count"), DEFAULT_HOLDING_REG_CONFIG["qw_count"]), + "mw_count": _as_int(hr.get("mw_count"), DEFAULT_HOLDING_REG_CONFIG["mw_count"]), + "md_count": _as_int(hr.get("md_count"), DEFAULT_HOLDING_REG_CONFIG["md_count"]), + "ml_count": _as_int(hr.get("ml_count"), DEFAULT_HOLDING_REG_CONFIG["ml_count"]), + "qx_bits": _as_int(coils.get("qx_bits"), DEFAULT_COILS_CONFIG["qx_bits"]), + "mx_bits": _as_int(coils.get("mx_bits"), DEFAULT_COILS_CONFIG["mx_bits"]), + "ix_bits": _as_int(di.get("ix_bits"), DEFAULT_DISCRETE_INPUTS_CONFIG["ix_bits"]), + "iw_count": _as_int(ir.get("iw_count"), DEFAULT_INPUT_REGISTERS_CONFIG["iw_count"]), + } + + # Legacy shape, and with it the config that carries no buffer_mapping: the + # defaults are what ends up being clamped, so they are what was asked for. + return "legacy", { + "qw_count": _as_int(buffer_mapping.get("max_holding_registers"), 1024), + "mw_count": 0, # No memory support in legacy mode + "md_count": 0, + "ml_count": 0, + "qx_bits": _as_int(buffer_mapping.get("max_coils"), 8192), + "mx_bits": 0, # No memory support in legacy mode + "ix_bits": _as_int(buffer_mapping.get("max_discrete_inputs"), 8192), + "iw_count": _as_int(buffer_mapping.get("max_input_registers"), 1024), + } + + +def _trim_to_one_table(counts, layout): + """Shrink tail segments until the composed block fits one Modbus table. + + `layout` is the block's segments in address order, each with the number of + addresses one element occupies (%MD is two registers, %ML is four). The + blocks lay their segments out head to tail, so trimming from the tail + leaves every earlier segment at the address a client already knows. + """ + total = sum(counts[key] * width for key, width in layout) + for key, width in reversed(layout): + if total <= MODBUS_MAX_ADDRESSES: + return + over = total - MODBUS_MAX_ADDRESSES + drop = min(counts[key], -(-over // width)) + counts[key] -= drop + total -= drop * width + + +# THE COMPOSED BLOCK, DECIDED (RTOP-284, C1.4) +# +# The holding-register block lays four segments end to end -- qw | mw | 2*md | +# 4*ml -- so it spans four tables in one contiguous Modbus range. The question +# was what "clamp against its own table" means once those four differ in +# length, with three answers on the table: the lowest common extent, truncate +# at the first segment that runs out, or a non-contiguous layout. +# +# It is none of them, because the premise is wrong. The block already dispatches +# each ADDRESS to exactly one segment (OpenPLCSegmentedHoldingRegistersDataBlock +# keeps qw_end, mw_start/end, md_start/end, ml_start/end), and therefore to +# exactly one table. So each segment is clamped against its own table and the +# block is the concatenation of what is left. The other three answers all throw +# away storage the project asked for: the lowest common extent shrinks segments +# that have room, and truncating at the first exhausted one drops segments that +# have their own. +# +# THE CONSEQUENCE FOR THE CLIENT, which is the part worth stating: a segment's +# START ADDRESS moves when a table before it shrinks. That is not new -- the +# layout has always been a function of the counts, so it already moved whenever +# the user changed one -- and the address map screen in the editor is what +# tells them where each segment begins. What IS new is that the counts can now +# change because the project changed, without anyone editing the Modbus screen. +# +# Which image table each Modbus segment actually lives in. The clamp follows +# this rather than one figure, because the tables no longer share a length +# (RTOP-284): %QW comes out of int_output and %ML out of lint_memory, and a +# project may size those very differently. +SEGMENT_TABLES = { + "qw_count": "int_output", + "mw_count": "int_memory", + "md_count": "dint_memory", + "ml_count": "lint_memory", + "qx_bits": "bool_output", + "mx_bits": "bool_memory", + "ix_bits": "bool_input", + "iw_count": "int_input", +} + + +def _segment_limit(key, buffer_size): + """How much of its own table this segment may expose. + + Falls back to `buffer_size` when the runtime did not deliver per-table + sizes, because on a square run that IS the length every table has. + + Bit segments are in bits and their table is in elements of eight, which is + the same conversion the image.conf format makes explicit. + """ + reach = table_capacity(SEGMENT_TABLES[key], buffer_size) + return reach * MAX_BITS if key.endswith("_bits") else reach + + +def _fit_counts(asked, buffer_size): + """Fit the requested exposure to the image, then to the address space.""" + fitted = {key: min(asked[key], _segment_limit(key, buffer_size)) for key in SEGMENT_TABLES} + + # THE IMAGE CEILING IS NOT THE PROTOCOL'S CEILING. Fitting each segment to + # the image allows 65536 apiece, and the register block composes four of + # them as qw + mw + 2*md + 4*ml -- 524288 entries for addresses no PDU can + # reach, allocated as a Python list at startup. Fit the composed block too. + _trim_to_one_table(fitted, [("qw_count", 1), ("mw_count", 1), ("md_count", 2), ("ml_count", 4)]) + _trim_to_one_table(fitted, [("qx_bits", 1), ("mx_bits", 1)]) + _trim_to_one_table(fitted, [("ix_bits", 1)]) + _trim_to_one_table(fitted, [("iw_count", 1)]) + return fitted + + +def _log_clamped_segments(config_map, buffer_config): + """Say what the exposure could not hold, once, at startup. + + The clamp itself is already honest to a Modbus client -- anything past the + declared block gets exception 02 from pymodbus. But the person who + configured the server is not the client: they set 1024 registers in the + editor and would otherwise have to notice, from the other end of a network, + that only some of them answer. This is the line they can find on the device. + + Only reports segments that actually shrank, comparing what was asked for + against what was built, so it covers both reasons a segment can shrink: the + image is smaller than the exposure, or the composed block would not fit one + Modbus table. On the normal path nothing shrinks, because the editor sizes + the image from the exposure it was asked for. + """ + _, asked = _requested_counts(config_map) + + shrunk = [] + for key, section in SEGMENT_SECTIONS.items(): + built = buffer_config.get(section, {}).get(key, 0) + if asked[key] > built: + shrunk.append(f"{key} {asked[key]} -> {built}") + + if shrunk: + logger.warn( + "Modbus exposure reduced to fit the I/O image (" + + ", ".join(shrunk) + + "). Addresses beyond the reduced range answer Illegal Data Address. " + "This means the image sizes did not reach this device: check that the " + "program was uploaded by a current editor." + ) + + +def parse_buffer_mapping_config(config_map, buffer_size): """ Parse buffer_mapping configuration from JSON config. Supports both legacy format (max_coils, etc.) and new segmented format. + `buffer_size` is the runtime's ACTUAL image size for the program now + loaded, from ``runtime_args.safe_access_buffer_size()``. Every count is + clamped to it (RTOP-284) -- this used to clamp to a copy of the runtime's + old fixed 1024, kept in step by hand and unable to be right for more than + one program at a time. + + THE CLAMP IS WHAT MAKES OUT-OF-RANGE HONEST, not just tidy. Each data block + declares itself as wide as the counts returned here, and getValues answers + exception 02 (Illegal Data Address) for anything past that. Declare a block + wider than the image and the addresses in the gap fail the buffer read and + answer ZERO -- a plausible, wrong value that a client cannot tell from a + real zero, logged once per read at scan rate. Clamped to the image, the two + agree and the refusal is issued where the range is known. + + Note this also settles the case where the user configured nothing: the + editor materialises its defaults (1024 registers, 8192 coils) into + modbus_slave.json even when the project never opened the Modbus screen, so + the common project would otherwise declare 1024 registers over an image of + eight. + Returns a dict with parsed configuration for each data block type. """ - buffer_mapping = config_map.get("buffer_mapping", {}) - - # Check for new segmented format - if "holding_registers" in buffer_mapping and isinstance( - buffer_mapping["holding_registers"], dict - ): - # New segmented format - hr_config = buffer_mapping.get("holding_registers", {}) - coils_config = buffer_mapping.get("coils", {}) - di_config = buffer_mapping.get("discrete_inputs", {}) - ir_config = buffer_mapping.get("input_registers", {}) - - return { - "format": "segmented", - "holding_registers": { - "qw_count": min( - hr_config.get("qw_count", DEFAULT_HOLDING_REG_CONFIG["qw_count"]), BUFFER_SIZE - ), - "mw_count": min( - hr_config.get("mw_count", DEFAULT_HOLDING_REG_CONFIG["mw_count"]), BUFFER_SIZE - ), - "md_count": min( - hr_config.get("md_count", DEFAULT_HOLDING_REG_CONFIG["md_count"]), BUFFER_SIZE - ), - "ml_count": min( - hr_config.get("ml_count", DEFAULT_HOLDING_REG_CONFIG["ml_count"]), BUFFER_SIZE - ), - }, - "coils": { - "qx_bits": min( - coils_config.get("qx_bits", DEFAULT_COILS_CONFIG["qx_bits"]), - BUFFER_SIZE * MAX_BITS, - ), - "mx_bits": min( - coils_config.get("mx_bits", DEFAULT_COILS_CONFIG["mx_bits"]), - BUFFER_SIZE * MAX_BITS, - ), - }, - "discrete_inputs": { - "ix_bits": min( - di_config.get("ix_bits", DEFAULT_DISCRETE_INPUTS_CONFIG["ix_bits"]), - BUFFER_SIZE * MAX_BITS, - ), - }, - "input_registers": { - "iw_count": min( - ir_config.get("iw_count", DEFAULT_INPUT_REGISTERS_CONFIG["iw_count"]), - BUFFER_SIZE, - ), - }, - "word_order": config_map.get("word_order", "high_word_first"), - } - - # Legacy format (max_coils, max_discrete_inputs, etc.) - # Convert to segmented format with no memory location support - max_coils = buffer_mapping.get("max_coils", 8192) - max_discrete_inputs = buffer_mapping.get("max_discrete_inputs", 8192) - max_holding_registers = buffer_mapping.get("max_holding_registers", 1024) - max_input_registers = buffer_mapping.get("max_input_registers", 1024) + fmt, asked = _requested_counts(config_map) + fitted = _fit_counts(asked, buffer_size) return { - "format": "legacy", + "format": fmt, "holding_registers": { - "qw_count": min(max_holding_registers, BUFFER_SIZE), - "mw_count": 0, # No memory support in legacy mode - "md_count": 0, - "ml_count": 0, + "qw_count": fitted["qw_count"], + "mw_count": fitted["mw_count"], + "md_count": fitted["md_count"], + "ml_count": fitted["ml_count"], }, "coils": { - "qx_bits": min(max_coils, BUFFER_SIZE * MAX_BITS), - "mx_bits": 0, # No memory support in legacy mode + "qx_bits": fitted["qx_bits"], + "mx_bits": fitted["mx_bits"], }, "discrete_inputs": { - "ix_bits": min(max_discrete_inputs, BUFFER_SIZE * MAX_BITS), + "ix_bits": fitted["ix_bits"], }, "input_registers": { - "iw_count": min(max_input_registers, BUFFER_SIZE), + "iw_count": fitted["iw_count"], }, - "word_order": "high_word_first", + "word_order": ( + config_map.get("word_order", "high_word_first") + if fmt == "segmented" + else "high_word_first" + ), } @@ -1034,6 +1233,15 @@ def start_loop(): logger.error("Plugin not initialized") return False + # THE IMAGE SIZE COMES FIRST, because every count parsed below is clamped + # to it. The runtime allocates the image per program load (RTOP-284), so + # this is the size for the program running right now -- not a constant this + # plugin can keep a copy of. + buffer_size, size_error = runtime_args.safe_access_buffer_size() + if buffer_size == -1: + logger.error(f"Failed to access buffer size: {size_error}") + return False + # Load configuration and create data blocks try: # Try to load configuration from plugin_specific_config_file_path @@ -1056,7 +1264,7 @@ def start_loop(): logger.debug(f"Available config sections: {list(config_map.keys())}") # Parse buffer mapping configuration - buffer_config = parse_buffer_mapping_config(config_map) + buffer_config = parse_buffer_mapping_config(config_map, buffer_size) logger.info(f"Buffer mapping format: {buffer_config['format']}") else: logger.warn(f"Failed to load configuration file: {status} - using defaults") @@ -1068,14 +1276,14 @@ def start_loop(): # Use default configuration if not loaded from file if buffer_config is None: - buffer_config = parse_buffer_mapping_config({}) + config_map = {} + buffer_config = parse_buffer_mapping_config(config_map, buffer_size) logger.info("Using default buffer mapping configuration") - # Safely access buffer size using validation - buffer_size, size_error = runtime_args.safe_access_buffer_size() - if buffer_size == -1: - logger.error(f"Failed to access buffer size: {size_error}") - return False + # After both routes, because the defaults are clamped just like a + # config file is: a device with no modbus_slave.json at all still ends + # up exposing 1024 registers over whatever image the program needs. + _log_clamped_segments(config_map, buffer_config) # Create OpenPLC-connected data blocks based on configuration hr_config = buffer_config["holding_registers"] diff --git a/core/src/drivers/plugins/python/opcua/plugin.py b/core/src/drivers/plugins/python/opcua/plugin.py index 084c76b7..0c551fdf 100644 --- a/core/src/drivers/plugins/python/opcua/plugin.py +++ b/core/src/drivers/plugins/python/opcua/plugin.py @@ -32,6 +32,18 @@ SafeLoggingAccess, safe_extract_runtime_args_from_capsule, ) + +# Importing set_image_sizes is not a formality: the name has to exist in THIS +# module for the runtime to find it, and its presence is how this plugin +# declares it understands per-table image sizes (RTOP-284). The runtime keeps +# the image SQUARE for any run in which even one loaded plugin lacks it -- and +# this plugin ships in plugins_default.conf, so without this line per-table +# sizing never activates on a stock device. +# +# Nothing else is needed here: this plugin bounds through SafeBufferAccess -> +# BufferValidator, which already validates against the table each buffer lives +# in rather than against the single figure. +from shared.image_sizes import set_image_sizes # noqa: F401 from shared.plugin_config_decode.opcua_config_model import OpcuaConfig # Import local modules (use absolute imports for runtime compatibility) diff --git a/core/src/drivers/plugins/python/shared/buffer_validator.py b/core/src/drivers/plugins/python/shared/buffer_validator.py index 103107b9..92419b39 100644 --- a/core/src/drivers/plugins/python/shared/buffer_validator.py +++ b/core/src/drivers/plugins/python/shared/buffer_validator.py @@ -9,12 +9,14 @@ try: # Try relative imports first (when used as package) - from .component_interfaces import IBufferValidator from .buffer_types import get_buffer_types + from .component_interfaces import IBufferValidator + from .image_sizes import table_capacity except ImportError: # Fall back to absolute imports (when testing standalone) - from component_interfaces import IBufferValidator from buffer_types import get_buffer_types + from component_interfaces import IBufferValidator + from image_sizes import table_capacity class BufferValidator(IBufferValidator): @@ -56,8 +58,17 @@ def validate_buffer_index(self, buffer_idx: int, buffer_type: str) -> Tuple[bool if buffer_idx < 0: return False, f"Buffer index cannot be negative: {buffer_idx}" - if buffer_idx >= self.args.buffer_size: - return False, f"Buffer index out of range: {buffer_idx} >= {self.args.buffer_size}" + # Against THIS BUFFER'S OWN TABLE. args.buffer_size is now the + # smallest of the fourteen (RTOP-284), so validating against it + # would refuse every index above the shortest table's length in + # every longer one. When the sizes were not delivered -- a square + # run -- buffer_size IS the length they all have, so it is the + # right fallback rather than a guess. + reach = table_capacity(buffer_type, self.args.buffer_size) + if buffer_idx >= reach: + return False, ( + f"Buffer index out of range for {buffer_type}: {buffer_idx} >= {reach}" + ) return True, "Success" @@ -103,7 +114,7 @@ def validate_value_range(self, value: Any, buffer_type: str) -> Tuple[bool, str] min_val, max_val = buffer_type_obj.value_range # Handle boolean values - if buffer_type_obj.name == 'bool': + if buffer_type_obj.name == "bool": if isinstance(value, bool): return True, "Success" elif isinstance(value, (int, float)): @@ -132,8 +143,9 @@ def validate_value_range(self, value: Any, buffer_type: str) -> Tuple[bool, str] except (AttributeError, TypeError, ValueError) as e: return False, f"Value validation error: {e}" - def validate_operation_params(self, buffer_type: str, buffer_idx: int, - bit_idx: Optional[int] = None, value: Any = None) -> Tuple[bool, str]: + def validate_operation_params( + self, buffer_type: str, buffer_idx: int, bit_idx: Optional[int] = None, value: Any = None + ) -> Tuple[bool, str]: """ Comprehensive validation of all operation parameters. @@ -214,10 +226,10 @@ def get_validation_summary(self) -> dict: """ try: return { - 'buffer_size': self.args.buffer_size, - 'bits_per_buffer': self.args.bits_per_buffer, - 'supported_buffer_types': list(self.buffer_types.get_all_buffers().keys()), - 'supported_base_types': list(self.buffer_types.get_all_types().keys()) + "buffer_size": self.args.buffer_size, + "bits_per_buffer": self.args.bits_per_buffer, + "supported_buffer_types": list(self.buffer_types.get_all_buffers().keys()), + "supported_base_types": list(self.buffer_types.get_all_types().keys()), } except (AttributeError, TypeError) as e: - return {'error': str(e)} + return {"error": str(e)} diff --git a/core/src/drivers/plugins/python/shared/image_sizes.py b/core/src/drivers/plugins/python/shared/image_sizes.py new file mode 100644 index 00000000..4885ab21 --- /dev/null +++ b/core/src/drivers/plugins/python/shared/image_sizes.py @@ -0,0 +1,98 @@ +"""The fourteen image table lengths, for a Python plugin (RTOP-284). + +Importing ``set_image_sizes`` from here into a plugin module does two things at +once: it puts the name in that module's namespace, which is where the runtime +looks for it (``PyObject_GetAttrString(pModule, "set_image_sizes")``), and its +PRESENCE is how the plugin declares it understands per-table sizes. + + from shared.image_sizes import set_image_sizes, table_capacity + +A plugin that does not import it is not broken. The runtime keeps the image +square for that run and logs which plugin forced it -- which is the honest +outcome, because a plugin bounding a byte index and a word index with one +``buffer_size`` is correct exactly while the tables are equal. + +One implementation rather than one per plugin, for the same reason the native +side links one file: three copies of a fourteen-element cache is three places +for the indexing to drift. +""" + +# The order the runtime sends them in, which is the declaration order of +# image_tables_t. Names rather than indices on this side, because the Python +# buffer names already are these names -- see shared/buffer_types.py -- so +# nothing has to know the numbering. +# +# NOTE the trap this avoids: journal_buffer.h and the s7comm plugin each have +# their own fourteen in a DIFFERENT order. Going by name cannot pick up the +# wrong table; going by index can, and silently. +IMAGE_TABLE_ORDER: list[str] = [ + "bool_input", + "bool_output", + "byte_input", + "byte_output", + "int_input", + "int_output", + "dint_input", + "dint_output", + "lint_input", + "lint_output", + "int_memory", + "dint_memory", + "lint_memory", + "bool_memory", +] + +_sizes: dict[str, int] = {} + + +def set_image_sizes(sizes) -> int: + """Receive the table lengths, before ``init`` runs, once per program load. + + Deliberately not remembered across loads: the runtime calls this on every + load precisely so a plugin never acts on the previous program's shape. + + A list shorter than the fourteen leaves the rest unknown rather than + guessed, and a longer one is truncated, so a plugin and a runtime built + against different table lists still agree about the tables they share. + + Returns 0 on success, which is what the runtime requires; non-zero fails + the plugin exactly as a failed ``init`` does. + """ + # Built into a local and published only on success. _sizes is + # PROCESS-GLOBAL -- this module is imported once per interpreter and every + # Python plugin in that process shares it -- so a half-filled map left + # behind by a plugin being torn down would answer for the tables before + # the failure and fall back to buffer_size for the rest, in plugins that + # are still running. + parsed: dict[str, int] = {} + try: + values = list(sizes) + except TypeError: + _sizes.clear() + return -1 + + for name, count in zip(IMAGE_TABLE_ORDER, values): + try: + parsed[name] = int(count) + except (TypeError, ValueError): + _sizes.clear() + return -1 + + _sizes.clear() + _sizes.update(parsed) + return 0 + + +def sizes_known() -> bool: + """Whether the runtime has delivered the sizes for this load.""" + return bool(_sizes) + + +def table_capacity(name: str, default: int | None = None) -> int | None: + """How long one table is, by the name the buffer accessors already use. + + ``default`` is what to answer when the sizes have not been delivered -- + normally the caller's own ``buffer_size``, which on a square run is the + length every table has anyway. + """ + return _sizes.get(name, default) diff --git a/core/src/drivers/plugins/python/shared/plugin_runtime_args.py b/core/src/drivers/plugins/python/shared/plugin_runtime_args.py index 3514fcc8..f1957a4d 100644 --- a/core/src/drivers/plugins/python/shared/plugin_runtime_args.py +++ b/core/src/drivers/plugins/python/shared/plugin_runtime_args.py @@ -11,6 +11,22 @@ # Import IEC type definitions from .iec_types import IEC_BOOL, IEC_BYTE, IEC_UDINT, IEC_UINT, IEC_ULINT +# The largest image any plugin may be handed. +# +# NOT a tuning value and not a safety margin: a located variable's table index +# is a uint16_t in the STruC++ ABI, so no table can be addressed beyond this +# many elements. The runtime refuses a larger image at install +# (webserver/image_config.py) for the same reason, and this is the same number +# arrived at the same way. +# +# It replaces a bare literal that appeared twice here and gated EVERY Python +# plugin, not only Modbus: once the image stopped being a fixed 1024 +# (RTOP-284), any program needing more than that literal would have had its +# plugins refuse to start before a line of their own logic ran -- and the +# message said "buffer_size is invalid", which points at the runtime rather +# than at the limit that actually rejected it. +MAX_BUFFER_SIZE = 65536 + class PluginRuntimeArgs(ctypes.Structure): """ @@ -51,20 +67,35 @@ class PluginRuntimeArgs(ctypes.Structure): # debug_set toggles forcing; debug_write does a soft write that # respects existing forces (the next scan cycle can overwrite). ("debug_array_count", ctypes.CFUNCTYPE(ctypes.c_uint8)), - ("debug_elem_count", ctypes.CFUNCTYPE(ctypes.c_uint16, ctypes.c_uint8)), - ("debug_size", ctypes.CFUNCTYPE(ctypes.c_uint16, ctypes.c_uint8, ctypes.c_uint16)), - ("debug_read", ctypes.CFUNCTYPE(ctypes.c_uint16, - ctypes.c_uint8, ctypes.c_uint16, - ctypes.POINTER(ctypes.c_uint8))), - ("debug_set", ctypes.CFUNCTYPE(ctypes.c_uint8, - ctypes.c_uint8, ctypes.c_uint16, - ctypes.c_bool, - ctypes.POINTER(ctypes.c_uint8), - ctypes.c_uint16)), - ("debug_write", ctypes.CFUNCTYPE(ctypes.c_uint8, - ctypes.c_uint8, ctypes.c_uint16, - ctypes.POINTER(ctypes.c_uint8), - ctypes.c_uint16)), + ("debug_elem_count", ctypes.CFUNCTYPE(ctypes.c_uint16, ctypes.c_uint8)), + ("debug_size", ctypes.CFUNCTYPE(ctypes.c_uint16, ctypes.c_uint8, ctypes.c_uint16)), + ( + "debug_read", + ctypes.CFUNCTYPE( + ctypes.c_uint16, ctypes.c_uint8, ctypes.c_uint16, ctypes.POINTER(ctypes.c_uint8) + ), + ), + ( + "debug_set", + ctypes.CFUNCTYPE( + ctypes.c_uint8, + ctypes.c_uint8, + ctypes.c_uint16, + ctypes.c_bool, + ctypes.POINTER(ctypes.c_uint8), + ctypes.c_uint16, + ), + ), + ( + "debug_write", + ctypes.CFUNCTYPE( + ctypes.c_uint8, + ctypes.c_uint8, + ctypes.c_uint16, + ctypes.POINTER(ctypes.c_uint8), + ctypes.c_uint16, + ), + ), ("plugin_specific_config_file_path", ctypes.c_char * 256), # Buffer size information ("buffer_size", ctypes.c_int), @@ -76,11 +107,26 @@ class PluginRuntimeArgs(ctypes.Structure): ("log_error", ctypes.CFUNCTYPE(None, ctypes.c_char_p)), # Journal write function pointers for race-condition-free buffer writes # int (*func)(int type, int index, int bit/value, int value) - ("journal_write_bool", ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_int, ctypes.c_int, ctypes.c_int, ctypes.c_int)), - ("journal_write_byte", ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_int, ctypes.c_int, ctypes.c_int)), - ("journal_write_int", ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_int, ctypes.c_int, ctypes.c_int)), - ("journal_write_dint", ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_int, ctypes.c_int, ctypes.c_uint)), - ("journal_write_lint", ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_int, ctypes.c_int, ctypes.c_ulonglong)), + ( + "journal_write_bool", + ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_int, ctypes.c_int, ctypes.c_int, ctypes.c_int), + ), + ( + "journal_write_byte", + ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_int, ctypes.c_int, ctypes.c_int), + ), + ( + "journal_write_int", + ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_int, ctypes.c_int, ctypes.c_int), + ), + ( + "journal_write_dint", + ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_int, ctypes.c_int, ctypes.c_uint), + ), + ( + "journal_write_lint", + ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_int, ctypes.c_int, ctypes.c_ulonglong), + ), # Async request to stop the whole PLC: void (*)(const char *reason). # # This entry was missing while the C struct had the field, so every @@ -114,8 +160,11 @@ def validate_pointers(self): return False, "image_unlock function pointer is NULL" # Check buffer size is reasonable - if self.buffer_size <= 0 or self.buffer_size > 10000: - return False, f"buffer_size is invalid: {self.buffer_size}" + if self.buffer_size <= 0 or self.buffer_size > MAX_BUFFER_SIZE: + return ( + False, + f"buffer_size is {self.buffer_size}, outside 1..{MAX_BUFFER_SIZE}", + ) if self.bits_per_buffer <= 0 or self.bits_per_buffer > 64: return False, f"bits_per_buffer is invalid: {self.bits_per_buffer}" @@ -140,8 +189,8 @@ def safe_access_buffer_size(self): return -1, f"Validation failed: {msg}" size = self.buffer_size - if size <= 0 or size > 10000: - return -1, f"Invalid buffer size: {size}" + if size <= 0 or size > MAX_BUFFER_SIZE: + return -1, f"buffer_size is {size}, outside 1..{MAX_BUFFER_SIZE}" return size, "Success" diff --git a/core/src/drivers/python_plugin_bridge.h b/core/src/drivers/python_plugin_bridge.h index 7ccba5e4..b5caa601 100644 --- a/core/src/drivers/python_plugin_bridge.h +++ b/core/src/drivers/python_plugin_bridge.h @@ -27,6 +27,10 @@ typedef struct PyObject *pFuncStart; PyObject *pFuncStop; PyObject *pFuncCleanup; + /* Optional: set_image_sizes(sizes). NULL when the module does not define + * it, which declares that it does not understand per-table image sizes + * (RTOP-284). See plugin_driver.h for why presence is the declaration. */ + PyObject *pFuncSetImageSizes; PyObject *args_capsule; // Capsule containing plugin_runtime_args_t for lifetime management } python_binds_t; diff --git a/core/src/plc_app/debug_write_journal.cpp b/core/src/plc_app/debug_write_journal.cpp index 267c6467..14626f8d 100644 --- a/core/src/plc_app/debug_write_journal.cpp +++ b/core/src/plc_app/debug_write_journal.cpp @@ -142,16 +142,35 @@ void apply_located(const DbgwEntry *e, uint8_t area, uint8_t size, journal_write_located(jt, size, byte_index, bit_index, val); break; case DBGW_OP_FORCE: - /* Program view: pin the IECVar so get() returns the forced value. */ + /* IMAGE FIRST, AND ONLY THEN THE PROGRAM VIEW. + * + * These two pins have to agree or the debugger lies. The image slot + * can refuse -- the address is outside the table, or the forced-slot + * map was never allocated -- while `ext_strucpp_debug_set` always + * accepts, because the IECVar exists whatever the image is sized to. + * Pinning the IECVar first therefore showed the variable as FORCED in + * the editor and over OPC UA while the image slot took nothing, so the + * program kept driving it and the displayed value was fiction. A + * refusal the user is told is a success is worse than the refusal. + * + * There is no channel back to the requester from here: the write was + * enqueued by `runtime_external_write`, which already returned to the + * caller a cycle ago. Leaving the variable visibly unforced is the + * only honest signal this path still owns, and it is the one the + * person who asked for the force is looking at. */ + if (journal_force_set(jt, byte_index, bit_index, val) != 0) + break; if (ext_strucpp_debug_set) ext_strucpp_debug_set(e->arr, e->elem, true, e->bytes, e->len); - /* Image view: seed + pin the slot; copy_out and plugin writes drop. */ - journal_force_set(jt, byte_index, bit_index, val); break; case DBGW_OP_UNFORCE: + /* Unforce releases both regardless. A refused clear means the slot + * could not have been forced in the first place, and leaving the + * IECVar pinned because of it would strand the variable forced with + * no way to release it. */ + journal_force_clear(jt, byte_index, bit_index); if (ext_strucpp_debug_set) ext_strucpp_debug_set(e->arr, e->elem, false, nullptr, 0); - journal_force_clear(jt, byte_index, bit_index); break; default: break; diff --git a/core/src/plc_app/image_table_id.h b/core/src/plc_app/image_table_id.h new file mode 100644 index 00000000..10940cb0 --- /dev/null +++ b/core/src/plc_app/image_table_id.h @@ -0,0 +1,53 @@ +/** + * The identity of each I/O image table, and nothing else. + * + * Split out of image_tables.h so it can reach BOTH sides: the runtime, which + * owns the tables, and plugin_types.h, which plugins include. A plugin that + * exports `set_image_sizes` receives an array indexed by this enum, so it has + * to be able to name the entries -- and the alternative, a second copy of the + * enum in the plugin-facing header, is the drift this file exists to prevent. + * + * Publishing a TYPE costs no ABI: no struct gains a field and no offset moves, + * which is what CON06 guarantees pre-compiled plugins. + * + * ORDER IS THE CONTRACT. It is the declaration order of `image_tables_t`, the + * order `image.conf` is written in, and the order the sizes array arrives in. + * A pytest checks it against the editor's list and the webserver's + * (tests/pytest/test_image_conf_contract.py). Note that journal_buffer.h has + * its own fourteen in a DIFFERENT order -- see kJournalToImageTable. + */ + +#ifndef IMAGE_TABLE_ID_H +#define IMAGE_TABLE_ID_H + +#ifdef __cplusplus +extern "C" { +#endif + +/* One id per table, in the order image_tables_t declares them. Note the gap + * the list makes visible: byte_input and byte_output exist, byte_memory does + * not, so `%MB` has no storage on this runtime at all. */ +typedef enum +{ + IMAGE_TABLE_BOOL_INPUT = 0, + IMAGE_TABLE_BOOL_OUTPUT, + IMAGE_TABLE_BYTE_INPUT, + IMAGE_TABLE_BYTE_OUTPUT, + IMAGE_TABLE_INT_INPUT, + IMAGE_TABLE_INT_OUTPUT, + IMAGE_TABLE_DINT_INPUT, + IMAGE_TABLE_DINT_OUTPUT, + IMAGE_TABLE_LINT_INPUT, + IMAGE_TABLE_LINT_OUTPUT, + IMAGE_TABLE_INT_MEMORY, + IMAGE_TABLE_DINT_MEMORY, + IMAGE_TABLE_LINT_MEMORY, + IMAGE_TABLE_BOOL_MEMORY, + IMAGE_TABLE_COUNT +} image_table_id_t; + +#ifdef __cplusplus +} +#endif + +#endif /* IMAGE_TABLE_ID_H */ diff --git a/core/src/plc_app/image_tables.cpp b/core/src/plc_app/image_tables.cpp index a0113464..076b718f 100644 --- a/core/src/plc_app/image_tables.cpp +++ b/core/src/plc_app/image_tables.cpp @@ -5,9 +5,12 @@ // bind image-table buffer pointers. Plugins read/write through the // buffer pointers directly under the image-tables mutex. +#include #include +#include #include #include +#include #include @@ -32,25 +35,42 @@ extern "C" { // --------------------------------------------------------------------------- // Image-table storage // --------------------------------------------------------------------------- -IEC_BOOL *bool_input[BUFFER_SIZE][8]; -IEC_BOOL *bool_output[BUFFER_SIZE][8]; - -IEC_BYTE *byte_input[BUFFER_SIZE]; -IEC_BYTE *byte_output[BUFFER_SIZE]; - -IEC_UINT *int_input[BUFFER_SIZE]; -IEC_UINT *int_output[BUFFER_SIZE]; - -IEC_UDINT *dint_input[BUFFER_SIZE]; -IEC_UDINT *dint_output[BUFFER_SIZE]; - -IEC_ULINT *lint_input[BUFFER_SIZE]; -IEC_ULINT *lint_output[BUFFER_SIZE]; - -IEC_UINT *int_memory[BUFFER_SIZE]; -IEC_UDINT *dint_memory[BUFFER_SIZE]; -IEC_ULINT *lint_memory[BUFFER_SIZE]; -IEC_BOOL *bool_memory[BUFFER_SIZE][8]; +image_tables_t g_image; + +// How many elements each table currently holds. Zero means nothing is +// allocated and every table pointer is null, which is the state before the +// first program load and after the last unload. Every index into the image is +// bounded by this, so it lives beside the image rather than beside the +// allocator that sets it. +static uint32_t g_capacity = 0; + +/* The fourteen counts the image was actually allocated at. + * + * g_capacity survives as the SMALLEST of them, which is what a consumer that + * still reads one number must be given: bounding by the smallest table refuses + * an index that would have run off the end of it, where bounding by the + * largest would have read past four of them. See image_tables_capacity(). */ +static image_sizes_t g_sizes; + +// The tables are heap pointers now, and these assertions are what got us here +// safely. In their previous form they pinned the inline-array shape, so the +// moment the types changed the build stopped and named the function to follow. +// They now pin the opposite invariant: nothing may quietly go back to inline +// storage, and no table may drift to a shape whose element size differs from +// the one image_tables_alloc() allocates it at. +// +// The hazard they exist for has not gone away. Indexing a pointer-to-array is +// syntactically identical to indexing an array, and `sizeof` on the two differs +// by four orders of magnitude, so the compiler cannot tell a correct use site +// from a wrong one. `sizeof` on these tables appears in no other function. +static_assert(sizeof(g_image.bool_input) == sizeof(IEC_BOOL *(*)[8]), + "bool_input went back to inline storage: image_tables_alloc() and " + "image_tables_zero_slots() both assume a heap pointer."); +static_assert(sizeof(g_image.byte_input) == sizeof(IEC_BYTE **), + "byte_input went back to inline storage: see image_tables_alloc()."); +static_assert(sizeof(g_image) == 14 * sizeof(void *), + "the image struct gained, lost, or inlined a table -- " + "image_tables_alloc() allocates exactly fourteen."); // --------------------------------------------------------------------------- // strucpp shim: per-project located-variable descriptor accessors @@ -172,6 +192,28 @@ namespace { extern "C" pthread_mutex_t *image_tables_mutex(void) { + /* Initialised on first use rather than only in symbols_init. + * + * symbols_init runs on the cycle thread, so on the first program load the + * image-tables mutex was still a zero-filled pthread_mutex_t when the load + * path locked it around the allocation, and when the boot path did not + * lock it at all. Zero-filled happens to behave on glibc, but it is + * neither recursive nor priority-inheriting there -- the two properties + * this mutex is created for -- and it is undefined elsewhere. + * + * pthread_once, so the two callers cannot race to create it, and so it is + * created exactly once for the life of the process rather than per load. + * symbols_init's own guarded init is now redundant and harmless. */ + static pthread_once_t once = PTHREAD_ONCE_INIT; + pthread_once(&once, + [] + { + if (!g_locks_initialized) + { + init_recursive_pi_mutex(&g_image_tables_mutex); + g_locks_initialized = true; + } + }); return &g_image_tables_mutex; } @@ -343,6 +385,308 @@ static const void *located_pointer_at(const void *located_vars, uint32_t index) return lv[index].pointer; } +// --------------------------------------------------------------------------- +// How big the image has to be (RTOP-284) +// --------------------------------------------------------------------------- + +/* Local copy rather than shared with plc_retain_file_store.cpp, where the same + * three lines live in an anonymous namespace: hoisting a four-line string trim + * into a header shared between two config readers would couple them for no + * gain, and the parsers are deliberately independent -- each mirrors the file + * IT reads, key for key. */ +static std::string trimmed(const std::string &s) +{ + const size_t b = s.find_first_not_of(" \t\r\n"); + if (b == std::string::npos) return ""; + const size_t e = s.find_last_not_of(" \t\r\n"); + return s.substr(b, e - b + 1); +} + +static const char *const kImageTableKeys[IMAGE_TABLE_COUNT] = { + "bool_input", "bool_output", "byte_input", "byte_output", + "int_input", "int_output", "dint_input", "dint_output", + "lint_input", "lint_output", "int_memory", "dint_memory", + "lint_memory", "bool_memory", +}; + +/* The unit each table's ADDRESSES use, which is what image.conf carries. + * + * It is NOT always the unit the table is STORED in, and that gap is the whole + * reason the unit is written down. bool_output is declared IEC_BOOL *[N][8], + * so N counts bytes -- but %QX addresses bits, so the file says bits and the + * conversion happens here, once, where the storage shape is known. The editor + * emits the address's unit for every table and converts nothing. + * + * Parallel to kImageTableKeys, index for index. The contract test checks the + * pairing against the editor and the webserver, so a table whose unit + * disagrees across the four implementations fails CI. */ +static const char *const kImageTableUnits[IMAGE_TABLE_COUNT] = { + "bits", "bits", "bytes", "bytes", "words", "words", "dwords", + "dwords", "lwords", "lwords", "words", "dwords", "lwords", "bits", +}; + +static_assert(sizeof(kImageTableUnits) / sizeof(kImageTableUnits[0]) == IMAGE_TABLE_COUNT, + "kImageTableUnits and image_table_id_t disagree on how many tables there are."); + +/* The three BOOL tables, and only those, arrive in bits. */ +static bool table_is_in_bits(int i) +{ + return std::strcmp(kImageTableUnits[i], "bits") == 0; +} + +// A key missing here would make image_table_key() read past the array, and a +// spare one would go unnoticed. The count is the cheap half of keeping the enum +// and the strings in step; the ORDER is checked from the Python side, in +// tests/pytest/plugins/test_image_conf_contract.py, which is the only one of +// the three implementations of this file format that CI actually runs. +static_assert(sizeof(kImageTableKeys) / sizeof(kImageTableKeys[0]) == IMAGE_TABLE_COUNT, + "kImageTableKeys and image_table_id_t disagree on how many tables there are."); + +extern "C" const char *image_table_key(image_table_id_t id) +{ + return (id >= 0 && id < IMAGE_TABLE_COUNT) ? kImageTableKeys[id] : ""; +} + +/** + * (area, size) -> the table that stores it, or IMAGE_TABLE_COUNT for a + * combination this runtime has no storage for. + * + * There is exactly one such hole, and it is real rather than an oversight of + * this function: `%MB` (Memory + Byte). image_tables.h declares byte_input and + * byte_output but no byte_memory, so a program declaring `AT %MB4` names + * storage that does not exist. A current editor refuses that before the build + * (DOPE-615); an older one, or a hand-built .so, can still reach us, and the + * caller says so once rather than sizing a table that is not there. + */ +static image_table_id_t table_for(strucpp::LocatedArea area, strucpp::LocatedSize size) +{ + switch (area) + { + case strucpp::LocatedArea::Input: + switch (size) + { + case strucpp::LocatedSize::Bit: return IMAGE_TABLE_BOOL_INPUT; + case strucpp::LocatedSize::Byte: return IMAGE_TABLE_BYTE_INPUT; + case strucpp::LocatedSize::Word: return IMAGE_TABLE_INT_INPUT; + case strucpp::LocatedSize::DWord: return IMAGE_TABLE_DINT_INPUT; + case strucpp::LocatedSize::LWord: return IMAGE_TABLE_LINT_INPUT; + } + break; + case strucpp::LocatedArea::Output: + switch (size) + { + case strucpp::LocatedSize::Bit: return IMAGE_TABLE_BOOL_OUTPUT; + case strucpp::LocatedSize::Byte: return IMAGE_TABLE_BYTE_OUTPUT; + case strucpp::LocatedSize::Word: return IMAGE_TABLE_INT_OUTPUT; + case strucpp::LocatedSize::DWord: return IMAGE_TABLE_DINT_OUTPUT; + case strucpp::LocatedSize::LWord: return IMAGE_TABLE_LINT_OUTPUT; + } + break; + case strucpp::LocatedArea::Memory: + switch (size) + { + case strucpp::LocatedSize::Bit: return IMAGE_TABLE_BOOL_MEMORY; + case strucpp::LocatedSize::Word: return IMAGE_TABLE_INT_MEMORY; + case strucpp::LocatedSize::DWord: return IMAGE_TABLE_DINT_MEMORY; + case strucpp::LocatedSize::LWord: return IMAGE_TABLE_LINT_MEMORY; + case strucpp::LocatedSize::Byte: break; // %MB: no byte_memory table + } + break; + } + return IMAGE_TABLE_COUNT; +} + +extern "C" void image_sizes_read_conf(const char *config_path, image_sizes_t *out) +{ + if (!out) return; + std::memset(out, 0, sizeof(*out)); + + // A missing file is not an error. It means nobody delivered sizes for this + // program, and the caller falls back to the floor derived below -- which is + // also what makes an older editor, or a device provisioned by hand, work. + FILE *f = fopen(config_path, "r"); + if (!f) return; + + /* Built into a local and published only once the version checks out, so a + * file this runtime cannot read leaves ZEROS rather than a half-applied + * mixture of tables it understood and tables it did not. */ + image_sizes_t parsed; + std::memset(&parsed, 0, sizeof(parsed)); + long version = 0; + + char line[256]; + while (fgets(line, sizeof(line), f)) + { + std::string s = trimmed(line); + if (s.empty() || s[0] == '#') continue; + const size_t eq = s.find('='); + if (eq == std::string::npos) continue; + const std::string key = trimmed(s.substr(0, eq)); + const std::string val = trimmed(s.substr(eq + 1)); + + if (key == "format_version") + { + char *vend = nullptr; + version = strtol(val.c_str(), &vend, 10); + if (vend == val.c_str() || *vend != '\0') + version = 0; + continue; + } + + for (int i = 0; i < IMAGE_TABLE_COUNT; ++i) + { + if (key != kImageTableKeys[i]) continue; + + errno = 0; + char *endp = nullptr; + const long v = strtol(val.c_str(), &endp, 10); + + /* The unit is not decoration: it is what stops a bit count being + * allocated as an element count, which is a factor of eight with + * no diagnostic on either side. A value whose unit is not the one + * this table carries is refused rather than guessed at. */ + const std::string unit = (endp && endp != val.c_str()) ? trimmed(endp) : std::string(); + const bool unit_ok = unit == kImageTableUnits[i]; + + /* THE CEILING IS IN ELEMENTS, SO IT IS COMPARED AFTER CONVERTING. + * IMAGE_MAX_ELEMENTS is the uint16 index the ABI addresses through + * (CON03), a count of TABLE ELEMENTS. A BOOL table's file value is + * in bits, and 65536 elements is 524288 bits -- comparing the raw + * bit count against the element ceiling would refuse every legal + * image above 8192 bytes. */ + const long max_in_file_unit = + table_is_in_bits(i) ? (long)IMAGE_MAX_ELEMENTS * 8 : (long)IMAGE_MAX_ELEMENTS; + + /* Anything the runtime cannot honour reads as ZERO, which falls + * through to the floor derived from the program. That is the safe + * direction. Out of range, out of the uint16 the ABI addresses + * through, unparsed, trailing junk, or carrying the wrong unit: + * all of them mean the same thing here, which is "ignore me". + * + * The webserver refuses these at install, so reaching this branch + * means a hand-edited device. */ + const bool numeric_ok = + errno == 0 && endp != val.c_str() && v > 0 && v <= max_in_file_unit; + const bool usable = numeric_ok && unit_ok; + + if (!usable && !val.empty() && !(v == 0 && unit_ok)) + { + log_warn("[image_tables] image.conf: ignoring %s=%s, expected 1..%ld %s", + kImageTableKeys[i], val.c_str(), max_in_file_unit, kImageTableUnits[i]); + } + + /* Bits to elements, once, here. Round UP: the slots of a partial + * byte have to be addressable, and erring upward costs one byte + * where erring downward loses up to seven addresses. */ + uint32_t elements = 0; + if (usable) + { + elements = table_is_in_bits(i) ? (uint32_t)((v + 7) / 8) : (uint32_t)v; + } + parsed.elements[i] = elements; + break; + } + } + fclose(f); + + /* No version, or one this runtime does not know, refuses the WHOLE file. + * Guessing would mean reading a future format by today's rules, which is + * how a unit change becomes a silent factor of eight. Zeros here are not a + * failure: the floor derived from the loaded program takes over, which is + * the same path a device with no image.conf at all follows. + * + * There is no branch for version 1. It was written but never merged, so no + * device has ever read this file in that form. */ + if (version != IMAGE_CONF_FORMAT_VERSION) + { + log_warn("[image_tables] image.conf: format_version %ld is not %d; ignoring the file " + "and sizing from the loaded program instead", + version, IMAGE_CONF_FORMAT_VERSION); + return; + } + + *out = parsed; +} + +extern "C" void image_sizes_derive_floor(PluginManager *pm, image_sizes_t *out) +{ + if (!out) return; + std::memset(out, 0, sizeof(*out)); + + /* RESOLVED HERE, NOT READ FROM THE GLOBALS, and that is the whole point of + * taking `pm`. + * + * The obvious version of this function read ext_strucpp_get_located_vars. + * Those globals are populated by symbols_init, which runs on the cycle + * thread (plc_state_manager.cpp) — created AFTER the load path sizes and + * allocates the image. So they were 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 meant capacity 1 for + * every program, every located address above index 0 rejected by the + * bounds check, and no log to show for it — precisely the safety net this + * function exists to be. Unload nulls them again, so the second load would + * not have escaped it either. + * + * Resolving from the PluginManager makes the answer depend on the program + * being dlopen'd, which the caller has just done, rather than on the order + * two threads happen to run in. */ + GetLocatedVarsFn get_vars = nullptr; + GetLocatedCountFn get_count = nullptr; + if (pm) + { + *(void **)&get_vars = plugin_manager_get_symbol(pm, "strucpp_get_located_vars"); + *(void **)&get_count = plugin_manager_get_symbol(pm, "strucpp_get_located_var_count"); + } + if (!get_vars) get_vars = ext_strucpp_get_located_vars; + if (!get_count) get_count = ext_strucpp_get_located_var_count; + + if (!get_vars || !get_count) + { + // No program loaded, or one whose accessors are absent. Zeros, so the + // caller sizes from the configuration alone -- and at boot, when there + // is no program at all, from nothing. + return; + } + + const strucpp::LocatedVar *lv = get_vars(); + const uint32_t n = get_count(); + if (!lv) return; + + uint32_t unstorable = 0; + + for (uint32_t i = 0; i < n; ++i) + { + const image_table_id_t id = table_for(lv[i].area, lv[i].size); + if (id == IMAGE_TABLE_COUNT) + { + ++unstorable; + continue; + } + // byte_index IS the table index for every table, including the 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 different unit here. + const uint32_t needed = (uint32_t)lv[i].byte_index + 1u; + if (needed > out->elements[id]) out->elements[id] = needed; + } + + if (unstorable) + { + log_warn("[image_tables] %u located variable(s) address %%MB, which this " + "runtime has no table for - they will not be serviced", + unstorable); + } +} + +extern "C" void image_sizes_take_max(image_sizes_t *dst, const image_sizes_t *other) +{ + if (!dst || !other) return; + for (int i = 0; i < IMAGE_TABLE_COUNT; ++i) + { + if (other->elements[i] > dst->elements[i]) dst->elements[i] = other->elements[i]; + } +} + void image_tables_bind_located_vars(void) { if (!ext_strucpp_get_located_vars || !ext_strucpp_get_located_var_count) @@ -509,36 +853,41 @@ uint64_t threaded_image_read(const strucpp::LocatedVar &v) { uint16_t bi = v.byte_index; uint8_t b = v.bit_index; - if (bi >= BUFFER_SIZE) return 0; + /* The bound is THIS VAR'S TABLE, not one figure for fourteen. With the + * tables at different lengths, g_capacity (the smallest) would refuse + * valid indices in every longer table, and the largest would have let an + * index run off the end of every shorter one. */ + if (bi >= image_table_capacity(table_for(v.area, v.size))) + return 0; switch (v.area) { case strucpp::LocatedArea::Input: switch (v.size) { - case strucpp::LocatedSize::Bit: return (b < 8 && bool_input[bi][b]) ? (*bool_input[bi][b] ? 1u : 0u) : 0u; - case strucpp::LocatedSize::Byte: return byte_input[bi] ? *byte_input[bi] : 0u; - case strucpp::LocatedSize::Word: return int_input[bi] ? *int_input[bi] : 0u; - case strucpp::LocatedSize::DWord: return dint_input[bi] ? *dint_input[bi] : 0u; - case strucpp::LocatedSize::LWord: return lint_input[bi] ? *lint_input[bi] : 0u; + case strucpp::LocatedSize::Bit: return (b < 8 && g_image.bool_input[bi][b]) ? (*g_image.bool_input[bi][b] ? 1u : 0u) : 0u; + case strucpp::LocatedSize::Byte: return g_image.byte_input[bi] ? *g_image.byte_input[bi] : 0u; + case strucpp::LocatedSize::Word: return g_image.int_input[bi] ? *g_image.int_input[bi] : 0u; + case strucpp::LocatedSize::DWord: return g_image.dint_input[bi] ? *g_image.dint_input[bi] : 0u; + case strucpp::LocatedSize::LWord: return g_image.lint_input[bi] ? *g_image.lint_input[bi] : 0u; } break; case strucpp::LocatedArea::Output: switch (v.size) { - case strucpp::LocatedSize::Bit: return (b < 8 && bool_output[bi][b]) ? (*bool_output[bi][b] ? 1u : 0u) : 0u; - case strucpp::LocatedSize::Byte: return byte_output[bi] ? *byte_output[bi] : 0u; - case strucpp::LocatedSize::Word: return int_output[bi] ? *int_output[bi] : 0u; - case strucpp::LocatedSize::DWord: return dint_output[bi] ? *dint_output[bi] : 0u; - case strucpp::LocatedSize::LWord: return lint_output[bi] ? *lint_output[bi] : 0u; + case strucpp::LocatedSize::Bit: return (b < 8 && g_image.bool_output[bi][b]) ? (*g_image.bool_output[bi][b] ? 1u : 0u) : 0u; + case strucpp::LocatedSize::Byte: return g_image.byte_output[bi] ? *g_image.byte_output[bi] : 0u; + case strucpp::LocatedSize::Word: return g_image.int_output[bi] ? *g_image.int_output[bi] : 0u; + case strucpp::LocatedSize::DWord: return g_image.dint_output[bi] ? *g_image.dint_output[bi] : 0u; + case strucpp::LocatedSize::LWord: return g_image.lint_output[bi] ? *g_image.lint_output[bi] : 0u; } break; case strucpp::LocatedArea::Memory: switch (v.size) { - case strucpp::LocatedSize::Bit: return (b < 8 && bool_memory[bi][b]) ? (*bool_memory[bi][b] ? 1u : 0u) : 0u; - case strucpp::LocatedSize::Word: return int_memory[bi] ? *int_memory[bi] : 0u; - case strucpp::LocatedSize::DWord: return dint_memory[bi] ? *dint_memory[bi] : 0u; - case strucpp::LocatedSize::LWord: return lint_memory[bi] ? *lint_memory[bi] : 0u; + case strucpp::LocatedSize::Bit: return (b < 8 && g_image.bool_memory[bi][b]) ? (*g_image.bool_memory[bi][b] ? 1u : 0u) : 0u; + case strucpp::LocatedSize::Word: return g_image.int_memory[bi] ? *g_image.int_memory[bi] : 0u; + case strucpp::LocatedSize::DWord: return g_image.dint_memory[bi] ? *g_image.dint_memory[bi] : 0u; + case strucpp::LocatedSize::LWord: return g_image.lint_memory[bi] ? *g_image.lint_memory[bi] : 0u; default: break; } break; @@ -650,45 +999,401 @@ extern "C" void image_tables_copy_config_globals_out(void) // --------------------------------------------------------------------------- // Backing storage for slots not covered by located variables. // --------------------------------------------------------------------------- -static IEC_BOOL temp_bool_input[BUFFER_SIZE][8]; -static IEC_BOOL temp_bool_output[BUFFER_SIZE][8]; -static IEC_BYTE temp_byte_input[BUFFER_SIZE]; -static IEC_BYTE temp_byte_output[BUFFER_SIZE]; -static IEC_UINT temp_int_input[BUFFER_SIZE]; -static IEC_UINT temp_int_output[BUFFER_SIZE]; -static IEC_UDINT temp_dint_input[BUFFER_SIZE]; -static IEC_UDINT temp_dint_output[BUFFER_SIZE]; -static IEC_ULINT temp_lint_input[BUFFER_SIZE]; -static IEC_ULINT temp_lint_output[BUFFER_SIZE]; -static IEC_UINT temp_int_memory[BUFFER_SIZE]; -static IEC_UDINT temp_dint_memory[BUFFER_SIZE]; -static IEC_ULINT temp_lint_memory[BUFFER_SIZE]; -static IEC_BOOL temp_bool_memory[BUFFER_SIZE][8]; +// Backing storage for image slots no located variable claims. Heap, and the +// same length as the tables that point into it -- these were fourteen more +// [BUFFER_SIZE] statics, and leaving them fixed while the tables grew would put +// fill_null_pointers() to work handing out addresses past their end. +static IEC_BOOL (*temp_bool_input)[8] = nullptr; +static IEC_BOOL (*temp_bool_output)[8] = nullptr; +static IEC_BOOL (*temp_bool_memory)[8] = nullptr; +static IEC_BYTE *temp_byte_input = nullptr; +static IEC_BYTE *temp_byte_output = nullptr; +static IEC_UINT *temp_int_input = nullptr; +static IEC_UINT *temp_int_output = nullptr; +static IEC_UDINT *temp_dint_input = nullptr; +static IEC_UDINT *temp_dint_output = nullptr; +static IEC_ULINT *temp_lint_input = nullptr; +static IEC_ULINT *temp_lint_output = nullptr; +static IEC_UINT *temp_int_memory = nullptr; +static IEC_UDINT *temp_dint_memory = nullptr; +static IEC_ULINT *temp_lint_memory = nullptr; + +/* The smallest image that is not no image at all. + * + * Not a tuning knob and not a guess: it is the least count that leaves every + * base pointer non-null and buffer_size non-zero, which is what plugins are + * promised even at boot, before any program exists. A plugin bounds-checking + * against it accepts index 0 and nothing else, which is the correct answer for + * an image with nothing in it. */ +static const uint32_t IMAGE_MIN_ELEMENTS = 1; + +extern "C" uint32_t image_tables_capacity(void) { return g_capacity; } + +extern "C" uint32_t image_table_capacity(image_table_id_t id) +{ + if (id < 0 || id >= IMAGE_TABLE_COUNT) + return 0; + return g_sizes.elements[id]; +} -void image_tables_fill_null_pointers(void) +extern "C" void image_sizes_flatten(image_sizes_t *sizes) { - int filled = 0; - for (int i = 0; i < BUFFER_SIZE; ++i) + if (!sizes) + return; + uint32_t largest = 0; + for (int i = 0; i < IMAGE_TABLE_COUNT; ++i) + { + if (sizes->elements[i] > largest) largest = sizes->elements[i]; + } + for (int i = 0; i < IMAGE_TABLE_COUNT; ++i) + sizes->elements[i] = largest; +} + +extern "C" void image_tables_free(void) +{ + free(g_image.bool_input); + free(g_image.bool_output); + free(g_image.bool_memory); + free(g_image.byte_input); + free(g_image.byte_output); + free(g_image.int_input); + free(g_image.int_output); + free(g_image.dint_input); + free(g_image.dint_output); + free(g_image.lint_input); + free(g_image.lint_output); + free(g_image.int_memory); + free(g_image.dint_memory); + free(g_image.lint_memory); + + free(temp_bool_input); + free(temp_bool_output); + free(temp_bool_memory); + free(temp_byte_input); + free(temp_byte_output); + free(temp_int_input); + free(temp_int_output); + free(temp_dint_input); + free(temp_dint_output); + free(temp_lint_input); + free(temp_lint_output); + free(temp_int_memory); + free(temp_dint_memory); + free(temp_lint_memory); + + // Null every pointer, not just free it. A dangling table would index + // exactly as a live one does, and the next fill_null_pointers() would read + // freed memory to decide whether a slot needs backing. + std::memset(&g_image, 0, sizeof(g_image)); + temp_bool_input = nullptr; + temp_bool_output = nullptr; + temp_bool_memory = nullptr; + temp_byte_input = nullptr; + temp_byte_output = nullptr; + temp_int_input = nullptr; + temp_int_output = nullptr; + temp_dint_input = nullptr; + temp_dint_output = nullptr; + temp_lint_input = nullptr; + temp_lint_output = nullptr; + temp_int_memory = nullptr; + temp_dint_memory = nullptr; + temp_lint_memory = nullptr; + + g_capacity = 0; + std::memset(&g_sizes, 0, sizeof(g_sizes)); +} + +extern "C" bool image_tables_alloc(const image_sizes_t *sizes) +{ + /* A FLOOR OF ONE PER TABLE, not zero, and it is worth being explicit about + * why: an area the program never touches could allocate nothing at all and + * save eight bytes, but then its base pointer is NULL and every plugin + * that does not check the count first dereferences it. FR15 says no plugin + * ever receives an invalid image, including before a program is loaded. + * One element per table costs a pointer and removes that whole class of + * bug; "an area with no producers consumes nothing" (NFR04) is still true + * of the storage that matters, which is the temp buffers and the slots. */ + image_sizes_t want; + if (sizes) + want = *sizes; + else + std::memset(&want, 0, sizeof(want)); + for (int i = 0; i < IMAGE_TABLE_COUNT; ++i) + { + if (want.elements[i] < IMAGE_MIN_ELEMENTS) + want.elements[i] = IMAGE_MIN_ELEMENTS; + } + +#define N(id) (want.elements[id]) + + /* BUILT INTO LOCALS AND PUBLISHED ONLY ON SUCCESS. + * + * This used to call image_tables_free() first and allocate into g_image + * directly, which made the all-or-nothing promise in the header only half + * true: it covered the new image, not the one it had just destroyed. A + * re-allocation that failed left capacity 0 and fourteen null tables while + * every plugin still held the base pointers it cached by value at init(), + * so the failure surfaced inside a plugin rather than here. + * + * Now nothing observable changes until all twenty-eight allocations have + * succeeded. A failure frees the locals and leaves the running image + * exactly as it was, which is what lets the caller log and stop with the + * device still in a describable state. */ + image_tables_t next; + std::memset(&next, 0, sizeof(next)); + + IEC_BOOL(*t_bool_input)[8] = nullptr; + IEC_BOOL(*t_bool_output)[8] = nullptr; + IEC_BOOL(*t_bool_memory)[8] = nullptr; + IEC_BYTE *t_byte_input = nullptr; + IEC_BYTE *t_byte_output = nullptr; + IEC_UINT *t_int_input = nullptr; + IEC_UINT *t_int_output = nullptr; + IEC_UDINT *t_dint_input = nullptr; + IEC_UDINT *t_dint_output = nullptr; + IEC_ULINT *t_lint_input = nullptr; + IEC_ULINT *t_lint_output = nullptr; + IEC_UINT *t_int_memory = nullptr; + IEC_UDINT *t_dint_memory = nullptr; + IEC_ULINT *t_lint_memory = nullptr; + + next.bool_input = (IEC_BOOL * (*)[8]) calloc(N(IMAGE_TABLE_BOOL_INPUT), sizeof(IEC_BOOL *[8])); + next.bool_output = + (IEC_BOOL * (*)[8]) calloc(N(IMAGE_TABLE_BOOL_OUTPUT), sizeof(IEC_BOOL *[8])); + next.bool_memory = + (IEC_BOOL * (*)[8]) calloc(N(IMAGE_TABLE_BOOL_MEMORY), sizeof(IEC_BOOL *[8])); + next.byte_input = (IEC_BYTE **)calloc(N(IMAGE_TABLE_BYTE_INPUT), sizeof(IEC_BYTE *)); + next.byte_output = (IEC_BYTE **)calloc(N(IMAGE_TABLE_BYTE_OUTPUT), sizeof(IEC_BYTE *)); + next.int_input = (IEC_UINT **)calloc(N(IMAGE_TABLE_INT_INPUT), sizeof(IEC_UINT *)); + next.int_output = (IEC_UINT **)calloc(N(IMAGE_TABLE_INT_OUTPUT), sizeof(IEC_UINT *)); + next.dint_input = (IEC_UDINT **)calloc(N(IMAGE_TABLE_DINT_INPUT), sizeof(IEC_UDINT *)); + next.dint_output = (IEC_UDINT **)calloc(N(IMAGE_TABLE_DINT_OUTPUT), sizeof(IEC_UDINT *)); + next.lint_input = (IEC_ULINT **)calloc(N(IMAGE_TABLE_LINT_INPUT), sizeof(IEC_ULINT *)); + next.lint_output = (IEC_ULINT **)calloc(N(IMAGE_TABLE_LINT_OUTPUT), sizeof(IEC_ULINT *)); + next.int_memory = (IEC_UINT **)calloc(N(IMAGE_TABLE_INT_MEMORY), sizeof(IEC_UINT *)); + next.dint_memory = (IEC_UDINT **)calloc(N(IMAGE_TABLE_DINT_MEMORY), sizeof(IEC_UDINT *)); + next.lint_memory = (IEC_ULINT **)calloc(N(IMAGE_TABLE_LINT_MEMORY), sizeof(IEC_ULINT *)); + + t_bool_input = (IEC_BOOL(*)[8])calloc(N(IMAGE_TABLE_BOOL_INPUT), sizeof(IEC_BOOL[8])); + t_bool_output = (IEC_BOOL(*)[8])calloc(N(IMAGE_TABLE_BOOL_OUTPUT), sizeof(IEC_BOOL[8])); + t_bool_memory = (IEC_BOOL(*)[8])calloc(N(IMAGE_TABLE_BOOL_MEMORY), sizeof(IEC_BOOL[8])); + t_byte_input = (IEC_BYTE *)calloc(N(IMAGE_TABLE_BYTE_INPUT), sizeof(IEC_BYTE)); + t_byte_output = (IEC_BYTE *)calloc(N(IMAGE_TABLE_BYTE_OUTPUT), sizeof(IEC_BYTE)); + t_int_input = (IEC_UINT *)calloc(N(IMAGE_TABLE_INT_INPUT), sizeof(IEC_UINT)); + t_int_output = (IEC_UINT *)calloc(N(IMAGE_TABLE_INT_OUTPUT), sizeof(IEC_UINT)); + t_dint_input = (IEC_UDINT *)calloc(N(IMAGE_TABLE_DINT_INPUT), sizeof(IEC_UDINT)); + t_dint_output = (IEC_UDINT *)calloc(N(IMAGE_TABLE_DINT_OUTPUT), sizeof(IEC_UDINT)); + t_lint_input = (IEC_ULINT *)calloc(N(IMAGE_TABLE_LINT_INPUT), sizeof(IEC_ULINT)); + t_lint_output = (IEC_ULINT *)calloc(N(IMAGE_TABLE_LINT_OUTPUT), sizeof(IEC_ULINT)); + t_int_memory = (IEC_UINT *)calloc(N(IMAGE_TABLE_INT_MEMORY), sizeof(IEC_UINT)); + t_dint_memory = (IEC_UDINT *)calloc(N(IMAGE_TABLE_DINT_MEMORY), sizeof(IEC_UDINT)); + t_lint_memory = (IEC_ULINT *)calloc(N(IMAGE_TABLE_LINT_MEMORY), sizeof(IEC_ULINT)); +/* Undefined right after the last use, not inside a runtime branch: the + * preprocessor does not care which branch it sits in, so putting it in the + * failure path only worked because every use happened to be above it. */ +#undef N + + const bool complete = next.bool_input && next.bool_output && next.bool_memory && + next.byte_input && next.byte_output && next.int_input && + next.int_output && next.dint_input && next.dint_output && + next.lint_input && next.lint_output && next.int_memory && + next.dint_memory && next.lint_memory && t_bool_input && t_bool_output && + t_bool_memory && t_byte_input && t_byte_output && t_int_input && + t_int_output && t_dint_input && t_dint_output && t_lint_input && + t_lint_output && t_int_memory && t_dint_memory && t_lint_memory; + + if (!complete) { - for (int b = 0; b < 8; ++b) + free(next.bool_input); + free(next.bool_output); + free(next.bool_memory); + free(next.byte_input); + free(next.byte_output); + free(next.int_input); + free(next.int_output); + free(next.dint_input); + free(next.dint_output); + free(next.lint_input); + free(next.lint_output); + free(next.int_memory); + free(next.dint_memory); + free(next.lint_memory); + free(t_bool_input); + free(t_bool_output); + free(t_bool_memory); + free(t_byte_input); + free(t_byte_output); + free(t_int_input); + free(t_int_output); + free(t_dint_input); + free(t_dint_output); + free(t_lint_input); + free(t_lint_output); + free(t_int_memory); + free(t_dint_memory); + free(t_lint_memory); + log_error("[image_tables] could not allocate the image; the previous one is untouched"); + return false; + } + + // Everything succeeded: retire the old image and publish the new one. + image_tables_free(); + + g_image = next; + temp_bool_input = t_bool_input; + temp_bool_output = t_bool_output; + temp_bool_memory = t_bool_memory; + temp_byte_input = t_byte_input; + temp_byte_output = t_byte_output; + temp_int_input = t_int_input; + temp_int_output = t_int_output; + temp_dint_input = t_dint_input; + temp_dint_output = t_dint_output; + temp_lint_input = t_lint_input; + temp_lint_output = t_lint_output; + temp_int_memory = t_int_memory; + temp_dint_memory = t_dint_memory; + temp_lint_memory = t_lint_memory; + g_sizes = want; + + /* The SMALLEST table, not the largest, for anything still reading one + * number. Bounding by the smallest refuses an index that would have run + * off the end of it; bounding by the largest reads past every table below + * it. Under-permissive is the only safe direction for a consumer that has + * not been told the tables differ. */ + g_capacity = want.elements[0]; + for (int i = 1; i < IMAGE_TABLE_COUNT; ++i) + { + if (want.elements[i] < g_capacity) + g_capacity = want.elements[i]; + } + + /* Names only the tables the program actually uses. Fourteen figures of + * which eleven are usually one reads as noise, and the point of the line + * is to let someone watching a load see that the image followed their + * project. */ + { + char summary[256]; + int at = 0; + for (int i = 0; i < IMAGE_TABLE_COUNT && at < (int)sizeof(summary) - 1; ++i) { - if (!bool_input[i][b]) { temp_bool_input[i][b] = 0; bool_input[i][b] = &temp_bool_input[i][b]; ++filled; } - if (!bool_output[i][b]) { temp_bool_output[i][b] = 0; bool_output[i][b] = &temp_bool_output[i][b]; ++filled; } - if (!bool_memory[i][b]) { temp_bool_memory[i][b] = 0; bool_memory[i][b] = &temp_bool_memory[i][b]; ++filled; } + if (want.elements[i] <= IMAGE_MIN_ELEMENTS) + continue; + const int wrote = snprintf(summary + at, sizeof(summary) - (size_t)at, "%s%s=%u", + at ? ", " : "", kImageTableKeys[i], want.elements[i]); + if (wrote < 0 || wrote >= (int)sizeof(summary) - at) + break; + at += wrote; } - if (!byte_input[i]) { temp_byte_input[i] = 0; byte_input[i] = &temp_byte_input[i]; ++filled; } - if (!byte_output[i]) { temp_byte_output[i] = 0; byte_output[i] = &temp_byte_output[i]; ++filled; } - if (!int_input[i]) { temp_int_input[i] = 0; int_input[i] = &temp_int_input[i]; ++filled; } - if (!int_output[i]) { temp_int_output[i] = 0; int_output[i] = &temp_int_output[i]; ++filled; } - if (!dint_input[i]) { temp_dint_input[i] = 0; dint_input[i] = &temp_dint_input[i]; ++filled; } - if (!dint_output[i]) { temp_dint_output[i] = 0; dint_output[i] = &temp_dint_output[i]; ++filled; } - if (!lint_input[i]) { temp_lint_input[i] = 0; lint_input[i] = &temp_lint_input[i]; ++filled; } - if (!lint_output[i]) { temp_lint_output[i] = 0; lint_output[i] = &temp_lint_output[i]; ++filled; } - if (!int_memory[i]) { temp_int_memory[i] = 0; int_memory[i] = &temp_int_memory[i]; ++filled; } - if (!dint_memory[i]) { temp_dint_memory[i] = 0; dint_memory[i] = &temp_dint_memory[i]; ++filled; } - if (!lint_memory[i]) { temp_lint_memory[i] = 0; lint_memory[i] = &temp_lint_memory[i]; ++filled; } + if (at == 0) + snprintf(summary, sizeof(summary), "every table at the minimum"); + log_info("[image_tables] image allocated per table (%s)", summary); } - log_info("[image_tables] filled %d NULL slots with backing buffers", filled); +#undef N + return true; +} + +void image_tables_fill_null_pointers(void) +{ + /* EACH TABLE WALKED TO ITS OWN LENGTH. + * + * One loop bound for fourteen tables was correct only while they were all + * equal. With per-table sizing the smallest bound would leave the longer + * tables holding null slots -- which is the state a plugin dereferences -- + * and the largest would index past the end of every shorter one, writing + * through a pointer read from beyond the allocation. */ + int filled = 0; + +#define FILL_BITS(field, temp, id) \ + for (uint32_t i = 0; i < g_sizes.elements[id]; ++i) \ + for (int b = 0; b < 8; ++b) \ + if (!g_image.field[i][b]) \ + { \ + temp[i][b] = 0; \ + g_image.field[i][b] = &temp[i][b]; \ + ++filled; \ + } + +#define FILL(field, temp, id) \ + for (uint32_t i = 0; i < g_sizes.elements[id]; ++i) \ + if (!g_image.field[i]) \ + { \ + temp[i] = 0; \ + g_image.field[i] = &temp[i]; \ + ++filled; \ + } + + FILL_BITS(bool_input, temp_bool_input, IMAGE_TABLE_BOOL_INPUT) + FILL_BITS(bool_output, temp_bool_output, IMAGE_TABLE_BOOL_OUTPUT) + FILL_BITS(bool_memory, temp_bool_memory, IMAGE_TABLE_BOOL_MEMORY) + FILL(byte_input, temp_byte_input, IMAGE_TABLE_BYTE_INPUT) + FILL(byte_output, temp_byte_output, IMAGE_TABLE_BYTE_OUTPUT) + FILL(int_input, temp_int_input, IMAGE_TABLE_INT_INPUT) + FILL(int_output, temp_int_output, IMAGE_TABLE_INT_OUTPUT) + FILL(dint_input, temp_dint_input, IMAGE_TABLE_DINT_INPUT) + FILL(dint_output, temp_dint_output, IMAGE_TABLE_DINT_OUTPUT) + FILL(lint_input, temp_lint_input, IMAGE_TABLE_LINT_INPUT) + FILL(lint_output, temp_lint_output, IMAGE_TABLE_LINT_OUTPUT) + FILL(int_memory, temp_int_memory, IMAGE_TABLE_INT_MEMORY) + FILL(dint_memory, temp_dint_memory, IMAGE_TABLE_DINT_MEMORY) + FILL(lint_memory, temp_lint_memory, IMAGE_TABLE_LINT_MEMORY) + +#undef FILL_BITS +#undef FILL + + if (filled > 0) + log_info("[image_tables] filled %d null slots with temporaries", filled); +} + +/** + * Null every slot of every table. THE ONLY PLACE `sizeof` IS TAKEN ON THEM. + * + * This used to be fourteen `memset(table, 0, sizeof(table))` calls at the top + * of image_tables_clear_null_pointers(). Fourteen call sites is fourteen + * places to miss when the tables become pointers plus counts (RTOP-284), and + * missing one is silent: `sizeof` drops from 65536 to 8, it compiles without a + * warning, and the damage only shows on the SECOND program load, when + * fill_null_pointers() finds the slots still populated and declines to rebind + * them -- so plugins keep writing into the previous program's memory. + * + * One function, so the heap version is one function body, and the + * static_asserts beside the definition of g_image say when to write it. + */ +static void image_tables_zero_slots(void) +{ + // Was `memset(&g_image, 0, sizeof(g_image))` while the tables were inline + // arrays. That line still compiles now and is now WRONG: it would null the + // fourteen pointers and leak every table. This is the one function the + // static_asserts above point at, and this is the change they were asking + // for -- the length comes from g_capacity, never from sizeof. + /* EACH TABLE BY ITS OWN LENGTH. One figure for fourteen was safe only + * while they were all equal: the smallest would leave the longer tables + * half stale, and the largest would memset past the end of the shorter + * ones -- a heap overflow written by the very function that exists to stop + * this class of mistake. */ + if (image_tables_capacity() == 0 && g_sizes.elements[0] == 0) + return; + +#define Z(field, id, type) \ + if (g_image.field) \ + std::memset(g_image.field, 0, (size_t)g_sizes.elements[id] * sizeof(type)) + + Z(bool_input, IMAGE_TABLE_BOOL_INPUT, IEC_BOOL *[8]); + Z(bool_output, IMAGE_TABLE_BOOL_OUTPUT, IEC_BOOL *[8]); + Z(bool_memory, IMAGE_TABLE_BOOL_MEMORY, IEC_BOOL *[8]); + Z(byte_input, IMAGE_TABLE_BYTE_INPUT, IEC_BYTE *); + Z(byte_output, IMAGE_TABLE_BYTE_OUTPUT, IEC_BYTE *); + Z(int_input, IMAGE_TABLE_INT_INPUT, IEC_UINT *); + Z(int_output, IMAGE_TABLE_INT_OUTPUT, IEC_UINT *); + Z(dint_input, IMAGE_TABLE_DINT_INPUT, IEC_UDINT *); + Z(dint_output, IMAGE_TABLE_DINT_OUTPUT, IEC_UDINT *); + Z(lint_input, IMAGE_TABLE_LINT_INPUT, IEC_ULINT *); + Z(lint_output, IMAGE_TABLE_LINT_OUTPUT, IEC_ULINT *); + Z(int_memory, IMAGE_TABLE_INT_MEMORY, IEC_UINT *); + Z(dint_memory, IMAGE_TABLE_DINT_MEMORY, IEC_UDINT *); + Z(lint_memory, IMAGE_TABLE_LINT_MEMORY, IEC_ULINT *); +#undef Z } void image_tables_clear_null_pointers(void) @@ -702,20 +1407,7 @@ void image_tables_clear_null_pointers(void) g_located_globals_idx = nullptr; g_located_globals_n = 0; - std::memset(bool_input, 0, sizeof(bool_input)); - std::memset(bool_output, 0, sizeof(bool_output)); - std::memset(byte_input, 0, sizeof(byte_input)); - std::memset(byte_output, 0, sizeof(byte_output)); - std::memset(int_input, 0, sizeof(int_input)); - std::memset(int_output, 0, sizeof(int_output)); - std::memset(dint_input, 0, sizeof(dint_input)); - std::memset(dint_output, 0, sizeof(dint_output)); - std::memset(lint_input, 0, sizeof(lint_input)); - std::memset(lint_output, 0, sizeof(lint_output)); - std::memset(int_memory, 0, sizeof(int_memory)); - std::memset(dint_memory, 0, sizeof(dint_memory)); - std::memset(lint_memory, 0, sizeof(lint_memory)); - std::memset(bool_memory, 0, sizeof(bool_memory)); + image_tables_zero_slots(); ext_strucpp_advance_time = nullptr; ext_strucpp_set_current_time = nullptr; diff --git a/core/src/plc_app/image_tables.h b/core/src/plc_app/image_tables.h index 34d73e52..3a5d9425 100644 --- a/core/src/plc_app/image_tables.h +++ b/core/src/plc_app/image_tables.h @@ -1,6 +1,7 @@ #ifndef IMAGE_TABLES_H #define IMAGE_TABLES_H +#include "image_table_id.h" #include #include #include @@ -13,7 +14,19 @@ extern "C" { #endif -#define BUFFER_SIZE 1024 +/* BUFFER_SIZE is gone, and its absence is the point of RTOP-284. + * + * It was 1024 per table, compiled in, identical for every program that ever + * ran on the device: a project needing more could not have it, and a project + * needing less paid for the rest anyway, out of the memory its own program + * wanted. The image is now allocated per program load -- see + * image_tables_alloc() and image_tables_capacity() below, which is where a + * size comes from now. + * + * Nothing should reintroduce it. If some code needs to know how big the image + * is, the answer is image_tables_capacity(), and the answer changes between + * program loads. `-DBUFFER_SIZE=` in project.yml is inert and can go + * whenever that file is next touched. */ #define libplc_build_dir "./build" /* ------------------------------------------------------------------------- @@ -23,27 +36,186 @@ extern "C" * which walks strucpp::locatedVars[] and points each slot at the * matching IECVar's underlying primitive storage. Plugins read/write * these directly under the image-tables mutex. + * + * ONE SYMBOL, NOT FOURTEEN, and that is the point of the struct. + * + * These used to be fourteen separate globals, which meant fourteen chances + * for another translation unit to declare one by hand and get it subtly + * wrong. `plugin_driver.c` did exactly that: it redeclared all fourteen as + * extern while already including this header. Redundant while the shapes + * agree; two incompatible declarations in different TUs the moment they + * stop, which C does not diagnose across TUs -- it links, and the reader + * walks the wrong layout. With one struct there is one declaration to get + * right, and it lives here. + * + * What the struct deliberately does NOT do is make the eventual switch to + * heap allocation (RTOP-284) a compile error. Indexing `IEC_BOOL *(*p)[8]` + * is syntactically identical to indexing `IEC_BOOL *a[N][8]`, so every + * access site compiles unchanged either way -- verified, with -Wall + * -Wextra. The tripwires that do work are in image_tables.cpp: the size + * assertions next to the definition, and the fact that `sizeof` on these + * tables now appears in exactly one function. * --------------------------------------------------------------------- */ - extern IEC_BOOL *bool_input[BUFFER_SIZE][8]; - extern IEC_BOOL *bool_output[BUFFER_SIZE][8]; + typedef struct + { + /* Heap-allocated by image_tables_alloc(), each one + * image_tables_capacity() elements long. These are the very types + * plugin_types.h already declares for the same tables, which is what + * lets the runtime args keep pointing straight at them. + * + * Indexing reads exactly as it did when these were [BUFFER_SIZE] + * arrays. That is not a convenience -- it is the hazard: the compiler + * cannot tell the two shapes apart at a use site, and `sizeof` silently + * went from 65536 to 8 when they changed. The size assertions and the + * single zeroing function in image_tables.cpp exist for exactly that, + * and they are what caught this transition. */ + IEC_BOOL *(*bool_input)[8]; + IEC_BOOL *(*bool_output)[8]; + + IEC_BYTE **byte_input; + IEC_BYTE **byte_output; + + IEC_UINT **int_input; + IEC_UINT **int_output; - extern IEC_BYTE *byte_input[BUFFER_SIZE]; - extern IEC_BYTE *byte_output[BUFFER_SIZE]; + IEC_UDINT **dint_input; + IEC_UDINT **dint_output; - extern IEC_UINT *int_input[BUFFER_SIZE]; - extern IEC_UINT *int_output[BUFFER_SIZE]; + IEC_ULINT **lint_input; + IEC_ULINT **lint_output; - extern IEC_UDINT *dint_input[BUFFER_SIZE]; - extern IEC_UDINT *dint_output[BUFFER_SIZE]; + IEC_UINT **int_memory; + IEC_UDINT **dint_memory; + IEC_ULINT **lint_memory; + IEC_BOOL *(*bool_memory)[8]; + } image_tables_t; - extern IEC_ULINT *lint_input[BUFFER_SIZE]; - extern IEC_ULINT *lint_output[BUFFER_SIZE]; + extern image_tables_t g_image; - extern IEC_UINT *int_memory[BUFFER_SIZE]; - extern IEC_UDINT *dint_memory[BUFFER_SIZE]; - extern IEC_ULINT *lint_memory[BUFFER_SIZE]; - extern IEC_BOOL *bool_memory[BUFFER_SIZE][8]; + /* ------------------------------------------------------------------------- + * How big the image has to be (RTOP-284) + * + * Two independent answers, and the runtime takes the larger: + * + * CONFIGURED -- `image.conf`, installed from the program upload. The + * editor derives it 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, because a Modbus master I/O group can claim + * two thousand bits without the program declaring a single variable. + * + * DERIVED -- walked out of the loaded .so's locatedVars[]. This one knows + * only what the PROGRAM declares, which is a subset, but it is always + * available and always current. + * + * The maximum of the two is what makes a missing or stale `image.conf` + * harmless: it can leave the image larger than needed, never smaller than + * the program requires. A device that was provisioned by some other route, + * or whose editor predates the file, still comes up correct. + * --------------------------------------------------------------------- */ + + /* The table identities live in their own header so plugin_types.h can + * reach them without pulling the runtime internals in. */ + + /* Elements per table, in that table's own unit -- which for the three BOOL + * tables is BYTES, because they are declared [N][8], and for every other + * table is the number of addresses. Zero is a real answer: a program with + * no `%QX` has no reason to carry a bool_output image. */ + typedef struct + { + uint32_t elements[IMAGE_TABLE_COUNT]; + } image_sizes_t; + + /** The key `image.conf` uses for a table, which is the table's own name. */ + const char *image_table_key(image_table_id_t id); + + /** Read the installed `image.conf`. Every table zero when the file is + * absent, which means "nothing configured, size from the program". */ + void image_sizes_read_conf(const char *config_path, image_sizes_t *out); + + /** Walk the loaded .so's locatedVars[] for the floor the PROGRAM requires. + * Zeroes `out` first, so an unloaded or symbol-less program yields zeros + * rather than stale numbers. + * + * Takes the PluginManager and resolves the accessors from it rather than + * reading the file-scope ones: those are populated by `symbols_init`, + * which runs on the cycle thread and therefore AFTER the load path has + * already sized and allocated the image. Reading them here made the floor + * a zero vector on every load. */ + void image_sizes_derive_floor(PluginManager *pm, image_sizes_t *out); + + /** Per table, the larger of the two. */ + void image_sizes_take_max(image_sizes_t *dst, const image_sizes_t *other); + + /** + * Make every table the same length: the largest any of them needs. + * + * The square fallback, for a run where some plugin does not understand + * per-table sizes. It takes and returns an `image_sizes_t` rather than + * collapsing to one number on purpose (RTOP-284): a single figure for + * fourteen tables is the assumption this work exists to remove, and a + * helper that produces one is an invitation to reintroduce it. + * + * The LARGEST, not the smallest, because square has to cover every + * area the program actually uses. It costs memory the project does not + * need, which is the price of a plugin that cannot be told the truth. + */ + void image_sizes_flatten(image_sizes_t *sizes); + + /** The most any one table may hold: the ceiling of the uint16 `byte_index` + * in the STruC++ ABI, so no located variable can address beyond it. The + * webserver refuses a larger `image.conf` at install for the same reason + * and reaches the number the same way. */ +#define IMAGE_MAX_ELEMENTS 65536u + + /** The `image.conf` wire format this runtime reads. + * + * Version 2 carries a unit word on every value and leaves the three BOOL + * tables in bits, which is the unit their ADDRESSES use; this file + * converts to the [N][8] shape the storage has. A file declaring any other + * version, or none, is ignored whole rather than read by today's rules -- + * guessing is how a unit change becomes a silent factor of eight. + * + * There is no version 1 to be compatible with. It was written but never + * merged, so no device has ever read this file in that form. */ +#define IMAGE_CONF_FORMAT_VERSION 2 + + /** + * Allocate the image at `elements` per table, replacing whatever is there. + * + * CALLER MUST HOLD THE IMAGE-TABLES MUTEX, as with bind / fill / clear + * below. Stated because the two call sites used to disagree: the load path + * locked and the boot path did not, and nothing said which was right. + * + * All or nothing, and that now covers the image already running: the new + * tables are built into locals and published only once every allocation + * has succeeded, so a failure leaves the previous image exactly as it was. + * Returns false, having changed nothing observable; the caller logs and + * stops. A partial image would be worse than none, because every table + * indexes the same way whether it is real or null. + */ + bool image_tables_alloc(const image_sizes_t *sizes); + + /** Release the image. Safe to call when nothing is allocated. + * CALLER MUST HOLD THE IMAGE-TABLES MUTEX. */ + void image_tables_free(void); + + /** How many elements each table currently holds; 0 before any allocation. */ + uint32_t image_tables_capacity(void); + + /** How long one table actually is, in its own elements. + * + * The tables no longer share a length, so this is the only honest answer + * to "how far does this area reach". `image_tables_capacity()` remains for + * consumers that read a single number and returns the SMALLEST of the + * fourteen, which refuses an index rather than letting one run off the end + * of a shorter table. + * + * Zero for an id outside the enum, which is the safe reading: a caller + * that asks about a table this runtime does not have gets an area it + * cannot index into. */ + uint32_t image_table_capacity(image_table_id_t id); /* ------------------------------------------------------------------------- * Resolved .so symbols (populated by symbols_init). @@ -64,24 +236,20 @@ extern "C" /* Hierarchical debug PDU shims (defined inside the .so by * debug_dispatch.hpp under STRUCPP_V4_DEBUG_EXPORTS_DEFINE). */ - extern uint8_t (*ext_strucpp_debug_array_count)(void); - extern uint16_t (*ext_strucpp_debug_elem_count) (uint8_t arr); - extern uint16_t (*ext_strucpp_debug_size) (uint8_t arr, uint16_t elem); - extern uint8_t (*ext_strucpp_debug_set) (uint8_t arr, uint16_t elem, - bool forcing, - const uint8_t *bytes, - uint16_t len); - extern uint16_t (*ext_strucpp_debug_read) (uint8_t arr, uint16_t elem, - uint8_t *dest); + extern uint8_t (*ext_strucpp_debug_array_count)(void); + extern uint16_t (*ext_strucpp_debug_elem_count)(uint8_t arr); + extern uint16_t (*ext_strucpp_debug_size)(uint8_t arr, uint16_t elem); + extern uint8_t (*ext_strucpp_debug_set)(uint8_t arr, uint16_t elem, bool forcing, + const uint8_t *bytes, uint16_t len); + extern uint16_t (*ext_strucpp_debug_read)(uint8_t arr, uint16_t elem, uint8_t *dest); /* Soft write — updates the variable's underlying value via * IECVar::set(). If the variable is currently forced, the write is * silently ignored (force remains authoritative). Distinct from * ext_strucpp_debug_set(forcing=true) which pins the value * indefinitely. Used by plugins (OPC-UA, BACnet) that want regular * write semantics rather than debugger-style forcing. */ - extern uint8_t (*ext_strucpp_debug_write) (uint8_t arr, uint16_t elem, - const uint8_t *bytes, - uint16_t len); + extern uint8_t (*ext_strucpp_debug_write)(uint8_t arr, uint16_t elem, const uint8_t *bytes, + uint16_t len); /* ---- Retain marshalling (NODE-94) -------------------------------------- * @@ -98,12 +266,12 @@ extern "C" * * Optional: a program built by an older STruC++ resolves these to NULL and * the retain path simply never runs. */ - extern size_t (*ext_strucpp_retain_blob_size) (void); + extern size_t (*ext_strucpp_retain_blob_size)(void); extern uint32_t (*ext_strucpp_retain_layout_hash)(void); - extern size_t (*ext_strucpp_retain_pack) (uint8_t *out, size_t cap); - extern uint8_t (*ext_strucpp_retain_unpack) (const uint8_t *blob, size_t len, - uint8_t (*write_leaf)(uint8_t, uint16_t, - const uint8_t *, uint16_t)); + extern size_t (*ext_strucpp_retain_pack)(uint8_t *out, size_t cap); + extern uint8_t (*ext_strucpp_retain_unpack)(const uint8_t *blob, size_t len, + uint8_t (*write_leaf)(uint8_t, uint16_t, + const uint8_t *, uint16_t)); /* Located-variable classifier. Reports whether a debug (arr, elem) leaf is * a LOCATED variable and, if so, its image location (area / size / @@ -112,10 +280,8 @@ extern "C" * through the image journal + forced-slot bitmap (copy_in would clobber a * direct IECVar poke). OPTIONAL: an older .so without it leaves the pointer * NULL, and the drain treats every leaf as a global (IECVar) write. */ - extern int (*ext_strucpp_debug_locate) (uint8_t arr, uint16_t elem, - uint8_t *area, uint8_t *size, - uint16_t *byte_index, - uint8_t *bit_index); + extern int (*ext_strucpp_debug_locate)(uint8_t arr, uint16_t elem, uint8_t *area, uint8_t *size, + uint16_t *byte_index, uint8_t *bit_index); /* ------------------------------------------------------------------------- * Symbol resolution. diff --git a/core/src/plc_app/journal_buffer.c b/core/src/plc_app/journal_buffer.c index 7c72c55c..b376f9c9 100644 --- a/core/src/plc_app/journal_buffer.c +++ b/core/src/plc_app/journal_buffer.c @@ -1,3 +1,30 @@ +/* Out-of-range forces, COUNTED rather than logged. + * + * Saying it out loud is right -- a silent drop is the defect this change set + * out to remove -- but not from here. Both force paths run under image_lock, + * on plc_cycle_thread, which has called set_realtime_priority(). log_warn + * takes log_mutex, a plain mutex with no priority inheritance (unlike the + * image mutex, which is built through init_recursive_pi_mutex precisely + * because it needs it) and then does a blocking socket write. So a SCHED_FIFO + * dispatcher could block behind a low-priority logging thread WHILE HOLDING + * the image lock, stalling every plugin thread waiting on it. + * + * It is unrate-limited too: one line per offending entry, up to + * DBGW_MAX_ENTRIES per drain, and an OPC-UA client repeatedly writing one bad + * address reproduces it every tick. + * + * So the count is incremented here and reported from off the real-time path + * by journal_take_force_drops(), which the dispatcher can read between + * cycles. The information survives; the stall does not. */ +static unsigned g_force_oob_drops = 0; + +unsigned journal_take_force_drops(void) +{ + const unsigned n = g_force_oob_drops; + g_force_oob_drops = 0; + return n; +} + /** * @file journal_buffer.c * @brief Journal Buffer Implementation for Race-Condition-Free Plugin Writes @@ -23,12 +50,14 @@ */ #include "journal_buffer.h" +#include "image_tables.h" #include "utils/log.h" #include "utils/utils.h" +#include +#include #include +#include #include -#include -#include /* The lock-free path needs 32-bit (control word, atomic_uint) and 8-bit * (per-slot publish flag, atomic_uchar) atomics to be ALWAYS lock-free. @@ -71,25 +100,183 @@ static int journal_add(uint8_t type, uint16_t index, uint8_t bit, uint64_t value * * Mutated only from the dispatcher's debug-write drain and read only from * apply_entry() — both under image_lock — so no atomics are required. - * JBUF_FORCE_SIZE mirrors the image BUFFER_SIZE; a runtime guard keeps this - * safe even if the two ever diverge. + * + * SIZED FROM THE IMAGE, not from a constant of its own (RTOP-284). This was a + * fixed 1024 per journal type -- a third hardcoded 1024, alongside the image's + * and the Modbus slave plugin's -- and the three guards below bounded against + * it 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. The image can be any size now, so this follows it: one row + * per journal type, each as long as the image. * --------------------------------------------------------------------------- */ -#define JBUF_FORCE_SIZE 1024 -static uint8_t g_forced[JOURNAL_TYPE_COUNT][JBUF_FORCE_SIZE]; -static int g_force_count = 0; +/* JOURNAL TYPES AND IMAGE TABLE IDS ARE NOT THE SAME ORDER, despite both + * having fourteen members and journal_buffer.h saying this enum "matches the + * OpenPLC image table types". It matches the CONCEPTS, not the indices: + * journal puts each width's memory table beside its input and output + * (..._INPUT, ..._OUTPUT, ..._MEMORY) while image_tables.h groups all the + * memory tables at the end. JOURNAL_INT_MEMORY is 7; IMAGE_TABLE_INT_MEMORY + * is 10. + * + * So a cast between them is silent corruption: writes land in another table's + * bounds and the wrong area is refused or admitted. The mapping is written + * out, once, here. */ +static const image_table_id_t kJournalToImageTable[JOURNAL_TYPE_COUNT] = { + [JOURNAL_BOOL_INPUT] = IMAGE_TABLE_BOOL_INPUT, + [JOURNAL_BOOL_OUTPUT] = IMAGE_TABLE_BOOL_OUTPUT, + [JOURNAL_BOOL_MEMORY] = IMAGE_TABLE_BOOL_MEMORY, + [JOURNAL_BYTE_INPUT] = IMAGE_TABLE_BYTE_INPUT, + [JOURNAL_BYTE_OUTPUT] = IMAGE_TABLE_BYTE_OUTPUT, + [JOURNAL_INT_INPUT] = IMAGE_TABLE_INT_INPUT, + [JOURNAL_INT_OUTPUT] = IMAGE_TABLE_INT_OUTPUT, + [JOURNAL_INT_MEMORY] = IMAGE_TABLE_INT_MEMORY, + [JOURNAL_DINT_INPUT] = IMAGE_TABLE_DINT_INPUT, + [JOURNAL_DINT_OUTPUT] = IMAGE_TABLE_DINT_OUTPUT, + [JOURNAL_DINT_MEMORY] = IMAGE_TABLE_DINT_MEMORY, + [JOURNAL_LINT_INPUT] = IMAGE_TABLE_LINT_INPUT, + [JOURNAL_LINT_OUTPUT] = IMAGE_TABLE_LINT_OUTPUT, + [JOURNAL_LINT_MEMORY] = IMAGE_TABLE_LINT_MEMORY, +}; + +image_table_id_t journal_type_to_image_table(uint8_t type) +{ + if (type >= JOURNAL_TYPE_COUNT) + return IMAGE_TABLE_COUNT; + return kJournalToImageTable[type]; +} + +/** The longest table, which is how long a forced-slot row has to be: rows are + * one length for all fourteen types, so the longest is the only one that can + * record a forced slot anywhere any table reaches. Under-allocating here is + * what silently stopped a high address being forced at all. */ +static uint32_t journal_longest_table(void) +{ + uint32_t longest = 0; + for (int t = 0; t < JOURNAL_TYPE_COUNT; ++t) + { + const uint32_t n = g_buffer_ptrs.table_sizes[t]; + if (n > longest) + longest = n; + } + return longest; +} + +/** + * How far this journal type's table actually reaches. + * + * FROM THE SNAPSHOT, not from the live image sizes, and the two are not the + * same question. `g_buffer_ptrs.table_sizes[]` was filled at journal_init from + * the SAME moment as the table pointers beside it; `image_table_capacity()` + * reads `g_sizes`, which `image_tables_alloc` rewrites under the image-tables + * mutex that this path does not hold. Every caller here uses the result to + * index one of those pointers, so taking the length from a later moment than + * the allocation it bounds is how a dropped write becomes an out-of-bounds + * index instead. + * + * The two cannot diverge today -- the image is allocated before the cycle + * thread that calls journal_init exists, and a re-load stops that thread + * first -- so this is not a bug being fixed. It is the implicit coupling made + * explicit, which is what the rest of this task has been doing, and it also + * turns a non-inlinable cross-TU call per journal entry on the drain path back + * into the struct field read it used to be. + */ +static uint32_t journal_type_capacity(uint8_t type) +{ + if (type >= JOURNAL_TYPE_COUNT) + return 0; + return g_buffer_ptrs.table_sizes[type]; +} + +static uint8_t *g_forced[JOURNAL_TYPE_COUNT]; +/* uint32_t, not uint16_t: 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 "forcing is disabled everywhere". The indices compared against it + * are uint16_t and promote cleanly. */ +static uint32_t g_force_size = 0; /* rows are this long; 0 = not allocated */ +static int g_force_count = 0; + +/* Allocate the forced-slot bitmap to match the image. All or nothing: a + * partially allocated bitmap would leave some journal types unforceable with + * no way to tell which, which is the silent failure this change removes. */ +static void force_map_free(void); + +static int force_map_alloc(uint32_t elements) +{ + /* Release first. Assigning over g_forced[t] unconditionally leaked all + * fourteen rows on any journal_init not preceded by a journal_cleanup, + * which is a shape the state machine does not currently produce but does + * not forbid either. */ + force_map_free(); + + for (int t = 0; t < JOURNAL_TYPE_COUNT; t++) + { + g_forced[t] = (uint8_t *)calloc(elements ? elements : 1, sizeof(uint8_t)); + if (g_forced[t] == NULL) + { + for (int u = 0; u < JOURNAL_TYPE_COUNT; u++) + { + free(g_forced[u]); + g_forced[u] = NULL; + } + g_force_size = 0; + return -1; + } + } + g_force_size = elements; + g_force_count = 0; + return 0; +} + +static void force_map_free(void) +{ + /* THE GUARDS GO DOWN FIRST, and the order is the whole point. + * + * is_slot_forced() checks g_force_count and g_force_size and only then + * indexes g_forced[type][idx]. Freeing the rows before clearing those two + * leaves a window in which a reader passes both checks and dereferences a + * row that is already NULL. + * + * The window is reachable rather than theoretical: image_lock is handed to + * every plugin as args->image_lock and calls journal_apply_and_clear(), so + * is_slot_forced runs on plugin-owned threads -- EtherCAT's bus thread and + * the s7comm server callback among them -- and this function takes no + * lock. Clearing first means a reader that sees either guard down never + * indexes a row at all. + * + * g_force_count first of all, because it is the fast-path check and the + * only one a reader with nothing forced ever reaches. */ + g_force_count = 0; + g_force_size = 0; + + for (int t = 0; t < JOURNAL_TYPE_COUNT; t++) + { + free(g_forced[t]); + g_forced[t] = NULL; + } +} static inline int type_is_bool(uint8_t t) { - return t == JOURNAL_BOOL_INPUT || t == JOURNAL_BOOL_OUTPUT || - t == JOURNAL_BOOL_MEMORY; + return t == JOURNAL_BOOL_INPUT || t == JOURNAL_BOOL_OUTPUT || t == JOURNAL_BOOL_MEMORY; } static inline int is_slot_forced(uint8_t type, uint16_t idx, uint8_t bit) { - if (g_force_count == 0) return 0; /* fast path: nothing forced */ - if (type >= JOURNAL_TYPE_COUNT || idx >= JBUF_FORCE_SIZE) return 0; - if (type_is_bool(type)) { - if (bit >= 8) return 0; + if (g_force_count == 0) + return 0; /* fast path: nothing forced */ + /* Two bounds, and both matter: the row has to exist (g_force_size is how + * long every row was allocated) and the slot has to be one this table + * actually has. With the tables at different lengths the second is the + * real one -- a row is as long as the LARGEST table so every type has + * somewhere to record, and the per-table check is what stops a forced slot + * being honoured in an area that does not reach that far. */ + if (type >= JOURNAL_TYPE_COUNT || idx >= g_force_size || + (uint32_t)idx >= journal_type_capacity(type)) + return 0; + if (type_is_bool(type)) + { + if (bit >= 8) + return 0; return (g_forced[type][idx] >> bit) & 1; } return g_forced[type][idx] != 0; @@ -101,8 +288,18 @@ static void apply_write_raw(const journal_entry_t *entry) { uint16_t idx = entry->index; - /* Bounds check */ - if (idx >= (uint16_t)g_buffer_ptrs.buffer_size) { + /* Bounds check against THIS ENTRY'S OWN TABLE. + * + * It used to compare against g_buffer_ptrs.buffer_size, one figure for + * fourteen tables. That number is now the SMALLEST of them, so a write to + * any longer table above the smallest table's length would be dropped -- + * silently, which is the failure mode this whole area keeps producing. + * + * Still compared as uint32_t rather than through a (uint16_t) cast: the + * image may reach 65536, which that cast turns into 0 and drops every + * write at exactly the largest legal image. */ + if ((uint32_t)idx >= journal_type_capacity(entry->buffer_type)) + { return; } @@ -116,86 +313,143 @@ static void apply_write_raw(const journal_entry_t *entry) * corrupting unrelated storage (observed: VAR_GLOBALs in the .so). Reject * any bool entry whose bit_index is out of range so a torn/stale entry can * never escalate into an out-of-bounds pointer write. */ - if ((entry->buffer_type == JOURNAL_BOOL_INPUT || - entry->buffer_type == JOURNAL_BOOL_OUTPUT || + if ((entry->buffer_type == JOURNAL_BOOL_INPUT || entry->buffer_type == JOURNAL_BOOL_OUTPUT || entry->buffer_type == JOURNAL_BOOL_MEMORY) && - entry->bit_index >= 8) { + entry->bit_index >= 8) + { return; } - switch ((journal_buffer_type_t)entry->buffer_type) { - case JOURNAL_BOOL_INPUT: { - IEC_BOOL *ptr = g_buffer_ptrs.bool_input[idx][entry->bit_index]; - if (ptr != NULL) { *ptr = (IEC_BOOL)(entry->value & 1); } - break; + switch ((journal_buffer_type_t)entry->buffer_type) + { + case JOURNAL_BOOL_INPUT: + { + IEC_BOOL *ptr = g_buffer_ptrs.bool_input[idx][entry->bit_index]; + if (ptr != NULL) + { + *ptr = (IEC_BOOL)(entry->value & 1); } - case JOURNAL_BOOL_OUTPUT: { - IEC_BOOL *ptr = g_buffer_ptrs.bool_output[idx][entry->bit_index]; - if (ptr != NULL) { *ptr = (IEC_BOOL)(entry->value & 1); } - break; + break; + } + case JOURNAL_BOOL_OUTPUT: + { + IEC_BOOL *ptr = g_buffer_ptrs.bool_output[idx][entry->bit_index]; + if (ptr != NULL) + { + *ptr = (IEC_BOOL)(entry->value & 1); } - case JOURNAL_BOOL_MEMORY: { - IEC_BOOL *ptr = g_buffer_ptrs.bool_memory[idx][entry->bit_index]; - if (ptr != NULL) { *ptr = (IEC_BOOL)(entry->value & 1); } - break; + break; + } + case JOURNAL_BOOL_MEMORY: + { + IEC_BOOL *ptr = g_buffer_ptrs.bool_memory[idx][entry->bit_index]; + if (ptr != NULL) + { + *ptr = (IEC_BOOL)(entry->value & 1); } - case JOURNAL_BYTE_INPUT: { - IEC_BYTE *ptr = g_buffer_ptrs.byte_input[idx]; - if (ptr != NULL) { *ptr = (IEC_BYTE)(entry->value & 0xFF); } - break; + break; + } + case JOURNAL_BYTE_INPUT: + { + IEC_BYTE *ptr = g_buffer_ptrs.byte_input[idx]; + if (ptr != NULL) + { + *ptr = (IEC_BYTE)(entry->value & 0xFF); } - case JOURNAL_BYTE_OUTPUT: { - IEC_BYTE *ptr = g_buffer_ptrs.byte_output[idx]; - if (ptr != NULL) { *ptr = (IEC_BYTE)(entry->value & 0xFF); } - break; + break; + } + case JOURNAL_BYTE_OUTPUT: + { + IEC_BYTE *ptr = g_buffer_ptrs.byte_output[idx]; + if (ptr != NULL) + { + *ptr = (IEC_BYTE)(entry->value & 0xFF); } - case JOURNAL_INT_INPUT: { - IEC_UINT *ptr = g_buffer_ptrs.int_input[idx]; - if (ptr != NULL) { *ptr = (IEC_UINT)(entry->value & 0xFFFF); } - break; + break; + } + case JOURNAL_INT_INPUT: + { + IEC_UINT *ptr = g_buffer_ptrs.int_input[idx]; + if (ptr != NULL) + { + *ptr = (IEC_UINT)(entry->value & 0xFFFF); } - case JOURNAL_INT_OUTPUT: { - IEC_UINT *ptr = g_buffer_ptrs.int_output[idx]; - if (ptr != NULL) { *ptr = (IEC_UINT)(entry->value & 0xFFFF); } - break; + break; + } + case JOURNAL_INT_OUTPUT: + { + IEC_UINT *ptr = g_buffer_ptrs.int_output[idx]; + if (ptr != NULL) + { + *ptr = (IEC_UINT)(entry->value & 0xFFFF); } - case JOURNAL_INT_MEMORY: { - IEC_UINT *ptr = g_buffer_ptrs.int_memory[idx]; - if (ptr != NULL) { *ptr = (IEC_UINT)(entry->value & 0xFFFF); } - break; + break; + } + case JOURNAL_INT_MEMORY: + { + IEC_UINT *ptr = g_buffer_ptrs.int_memory[idx]; + if (ptr != NULL) + { + *ptr = (IEC_UINT)(entry->value & 0xFFFF); } - case JOURNAL_DINT_INPUT: { - IEC_UDINT *ptr = g_buffer_ptrs.dint_input[idx]; - if (ptr != NULL) { *ptr = (IEC_UDINT)(entry->value & 0xFFFFFFFF); } - break; + break; + } + case JOURNAL_DINT_INPUT: + { + IEC_UDINT *ptr = g_buffer_ptrs.dint_input[idx]; + if (ptr != NULL) + { + *ptr = (IEC_UDINT)(entry->value & 0xFFFFFFFF); } - case JOURNAL_DINT_OUTPUT: { - IEC_UDINT *ptr = g_buffer_ptrs.dint_output[idx]; - if (ptr != NULL) { *ptr = (IEC_UDINT)(entry->value & 0xFFFFFFFF); } - break; + break; + } + case JOURNAL_DINT_OUTPUT: + { + IEC_UDINT *ptr = g_buffer_ptrs.dint_output[idx]; + if (ptr != NULL) + { + *ptr = (IEC_UDINT)(entry->value & 0xFFFFFFFF); } - case JOURNAL_DINT_MEMORY: { - IEC_UDINT *ptr = g_buffer_ptrs.dint_memory[idx]; - if (ptr != NULL) { *ptr = (IEC_UDINT)(entry->value & 0xFFFFFFFF); } - break; + break; + } + case JOURNAL_DINT_MEMORY: + { + IEC_UDINT *ptr = g_buffer_ptrs.dint_memory[idx]; + if (ptr != NULL) + { + *ptr = (IEC_UDINT)(entry->value & 0xFFFFFFFF); } - case JOURNAL_LINT_INPUT: { - IEC_ULINT *ptr = g_buffer_ptrs.lint_input[idx]; - if (ptr != NULL) { *ptr = (IEC_ULINT)entry->value; } - break; + break; + } + case JOURNAL_LINT_INPUT: + { + IEC_ULINT *ptr = g_buffer_ptrs.lint_input[idx]; + if (ptr != NULL) + { + *ptr = (IEC_ULINT)entry->value; } - case JOURNAL_LINT_OUTPUT: { - IEC_ULINT *ptr = g_buffer_ptrs.lint_output[idx]; - if (ptr != NULL) { *ptr = (IEC_ULINT)entry->value; } - break; + break; + } + case JOURNAL_LINT_OUTPUT: + { + IEC_ULINT *ptr = g_buffer_ptrs.lint_output[idx]; + if (ptr != NULL) + { + *ptr = (IEC_ULINT)entry->value; } - case JOURNAL_LINT_MEMORY: { - IEC_ULINT *ptr = g_buffer_ptrs.lint_memory[idx]; - if (ptr != NULL) { *ptr = (IEC_ULINT)entry->value; } - break; + break; + } + case JOURNAL_LINT_MEMORY: + { + IEC_ULINT *ptr = g_buffer_ptrs.lint_memory[idx]; + if (ptr != NULL) + { + *ptr = (IEC_ULINT)entry->value; } - default: - break; + break; + } + default: + break; } } @@ -205,7 +459,8 @@ static void apply_write_raw(const journal_entry_t *entry) * so a forced located output stays pinned no matter who writes it.) */ static void apply_entry(const journal_entry_t *entry) { - if (is_slot_forced(entry->buffer_type, entry->index, entry->bit_index)) { + if (is_slot_forced(entry->buffer_type, entry->index, entry->bit_index)) + { return; } apply_write_raw(entry); @@ -215,18 +470,36 @@ static void apply_entry(const journal_entry_t *entry) * (bypassing the drop), then every later journal write to it is dropped until * journal_force_clear. Called only from the dispatcher's debug-write drain, * under image_lock — the same serialization domain as apply_entry. */ -void journal_force_set(journal_buffer_type_t type, uint16_t index, - uint8_t bit, uint64_t value) +int journal_force_set(journal_buffer_type_t type, uint16_t index, uint8_t bit, uint64_t value) { - if ((uint8_t)type >= JOURNAL_TYPE_COUNT || index >= JBUF_FORCE_SIZE) { - return; + /* BOTH BOUNDS: the row AND the table. + * + * g_force_size is how long every row was allocated -- the LONGEST table, + * so each type has somewhere to record. It is not how far this type's + * table reaches. While every table had the same length the two were one + * number and could not disagree; they can now. + * + * With bool_output at 1 element and int_output at 100, g_force_size is + * 100, so forcing bool_output index 5 passed this check, flipped the bit + * and incremented g_force_count -- permanently disabling the fast path in + * is_slot_forced -- while apply_write_raw and is_slot_forced both refused + * it on the per-table bound. A force that did nothing at all, and said + * nothing, which is the failure this guard exists to report. */ + if ((uint8_t)type >= JOURNAL_TYPE_COUNT || index >= g_force_size || + (uint32_t)index >= journal_type_capacity((uint8_t)type)) + { + /* Counted, not logged: see g_force_oob_drops. When the map was never + * allocated g_force_size is 0 and EVERY force lands here. */ + g_force_oob_drops++; + return -1; } - if (type_is_bool((uint8_t)type) && bit >= 8) { - return; + if (type_is_bool((uint8_t)type) && bit >= 8) + { + return -1; } - uint8_t mask = type_is_bool((uint8_t)type) ? (uint8_t)(1u << bit) - : (uint8_t)0x01; - if (!(g_forced[type][index] & mask)) { + uint8_t mask = type_is_bool((uint8_t)type) ? (uint8_t)(1u << bit) : (uint8_t)0x01; + if (!(g_forced[type][index] & mask)) + { g_forced[type][index] |= mask; g_force_count++; } @@ -237,26 +510,48 @@ void journal_force_set(journal_buffer_type_t type, uint16_t index, e.index = index; e.value = value; apply_write_raw(&e); /* seed — must land, so it bypasses the drop check */ + return 0; } /* Release a forced image slot. The next journal write (program copy_out or a * plugin) is no longer dropped, so the slot tracks the live value again. */ -void journal_force_clear(journal_buffer_type_t type, uint16_t index, uint8_t bit) +int journal_force_clear(journal_buffer_type_t type, uint16_t index, uint8_t bit) { - if ((uint8_t)type >= JOURNAL_TYPE_COUNT || index >= JBUF_FORCE_SIZE) { - return; + /* BOTH BOUNDS: the row AND the table. + * + * g_force_size is how long every row was allocated -- the LONGEST table, + * so each type has somewhere to record. It is not how far this type's + * table reaches. While every table had the same length the two were one + * number and could not disagree; they can now. + * + * With bool_output at 1 element and int_output at 100, g_force_size is + * 100, so forcing bool_output index 5 passed this check, flipped the bit + * and incremented g_force_count -- permanently disabling the fast path in + * is_slot_forced -- while apply_write_raw and is_slot_forced both refused + * it on the per-table bound. A force that did nothing at all, and said + * nothing, which is the failure this guard exists to report. */ + if ((uint8_t)type >= JOURNAL_TYPE_COUNT || index >= g_force_size || + (uint32_t)index >= journal_type_capacity((uint8_t)type)) + { + /* Counted, not logged: see g_force_oob_drops. When the map was never + * allocated g_force_size is 0 and EVERY force lands here. */ + g_force_oob_drops++; + return -1; } - if (type_is_bool((uint8_t)type) && bit >= 8) { - return; + if (type_is_bool((uint8_t)type) && bit >= 8) + { + return -1; } - uint8_t mask = type_is_bool((uint8_t)type) ? (uint8_t)(1u << bit) - : (uint8_t)0x01; - if (g_forced[type][index] & mask) { + uint8_t mask = type_is_bool((uint8_t)type) ? (uint8_t)(1u << bit) : (uint8_t)0x01; + if (g_forced[type][index] & mask) + { g_forced[type][index] &= (uint8_t)~mask; - if (g_force_count > 0) { + if (g_force_count > 0) + { g_force_count--; } } + return 0; } #if JOURNAL_LOCKFREE @@ -282,39 +577,55 @@ void journal_force_clear(journal_buffer_type_t type, uint16_t index, uint8_t bit * consumer from a single thread, so read-active-then-exchange is race-free. */ -#define JOURNAL_NBANKS 2 -#define JOURNAL_BANK_SHIFT 31u -#define JOURNAL_COUNT_MASK 0x7FFFFFFFu +#define JOURNAL_NBANKS 2 +#define JOURNAL_BANK_SHIFT 31u +#define JOURNAL_COUNT_MASK 0x7FFFFFFFu /* Bounded wait for an in-flight producer's publish at flip time. Each spin is * one acquire load; this caps the consumer's wait so a dead/stalled producer * can never hang the scan. ~one yield every 64 spins helps on single-core. */ #define JOURNAL_PUBLISH_SPIN_MAX 200000u -typedef struct { +typedef struct +{ journal_entry_t entries[JOURNAL_MAX_ENTRIES]; - atomic_uchar published[JOURNAL_MAX_ENTRIES]; /* 0 = empty, 1 = ready */ + atomic_uchar published[JOURNAL_MAX_ENTRIES]; /* 0 = empty, 1 = ready */ } journal_bank_t; static journal_bank_t g_banks[JOURNAL_NBANKS]; -static atomic_uint g_control; /* [bank:1][count:31] */ -static atomic_bool g_initialized = false; +static atomic_uint g_control; /* [bank:1][count:31] */ +static atomic_bool g_initialized = false; int journal_init(const journal_buffer_ptrs_t *buffer_ptrs) { - if (buffer_ptrs == NULL) { + if (buffer_ptrs == NULL) + { log_error("Journal: buffer_ptrs is NULL"); return -1; } - if (buffer_ptrs->image_mutex == NULL) { + if (buffer_ptrs->image_mutex == NULL) + { log_error("Journal: image_mutex is NULL"); return -1; } memcpy(&g_buffer_ptrs, buffer_ptrs, sizeof(journal_buffer_ptrs_t)); - for (int b = 0; b < JOURNAL_NBANKS; b++) { + /* The forced-slot bitmap follows the image, so forcing works across the + * whole of it rather than the first 1024 slots. Rows are as long as the + * LONGEST table, because they are one length for all fourteen types and + * anything shorter cannot record a forced slot in the tables above it. */ + if (force_map_alloc(journal_longest_table()) != 0) + { + log_error("Journal: could not allocate the forced-slot map for %u slots", + journal_longest_table()); + return -1; + } + + for (int b = 0; b < JOURNAL_NBANKS; b++) + { memset(g_banks[b].entries, 0, sizeof(g_banks[b].entries)); - for (size_t i = 0; i < JOURNAL_MAX_ENTRIES; i++) { + for (size_t i = 0; i < JOURNAL_MAX_ENTRIES; i++) + { atomic_init(&g_banks[b].published[i], 0); } } @@ -330,6 +641,7 @@ void journal_cleanup(void) { atomic_store_explicit(&g_initialized, false, memory_order_release); atomic_store_explicit(&g_control, 0u, memory_order_relaxed); + force_map_free(); memset(&g_buffer_ptrs, 0, sizeof(g_buffer_ptrs)); } @@ -340,7 +652,8 @@ bool journal_is_initialized(void) static int journal_add(uint8_t type, uint16_t index, uint8_t bit, uint64_t value) { - if (!atomic_load_explicit(&g_initialized, memory_order_acquire)) { + if (!atomic_load_explicit(&g_initialized, memory_order_acquire)) + { return -1; } @@ -350,18 +663,19 @@ static int journal_add(uint8_t type, uint16_t index, uint8_t bit, uint64_t value uint32_t bank = ctrl >> JOURNAL_BANK_SHIFT; uint32_t slot = ctrl & JOURNAL_COUNT_MASK; - if (slot >= JOURNAL_MAX_ENTRIES) { + if (slot >= JOURNAL_MAX_ENTRIES) + { /* Overflow: the active bank is full for this cycle. Drop. The consumer * detects and reports the drop count from the raw control count. */ return -1; } journal_entry_t *e = &g_banks[bank].entries[slot]; - e->sequence = slot; - e->buffer_type = type; - e->bit_index = bit; - e->index = index; - e->value = value; + e->sequence = slot; + e->buffer_type = type; + e->bit_index = bit; + e->index = index; + e->value = value; /* Publish: release pairs with the consumer's acquire so the full entry is * visible before the flag is observed set. */ @@ -371,7 +685,8 @@ static int journal_add(uint8_t type, uint16_t index, uint8_t bit, uint64_t value void journal_apply_and_clear(void) { - if (!atomic_load_explicit(&g_initialized, memory_order_acquire)) { + if (!atomic_load_explicit(&g_initialized, memory_order_acquire)) + { return; } @@ -381,7 +696,8 @@ void journal_apply_and_clear(void) * later) -- the same ordering guarantee a flush-on-lock read offers. This * keeps a read-heavy plugin (locking every cycle to read %Q via image_lock) * from flipping the journal needlessly and racing producers mid-publish. */ - if ((atomic_load_explicit(&g_control, memory_order_relaxed) & JOURNAL_COUNT_MASK) == 0) { + if ((atomic_load_explicit(&g_control, memory_order_relaxed) & JOURNAL_COUNT_MASK) == 0) + { return; } @@ -391,14 +707,14 @@ void journal_apply_and_clear(void) /* Flip + reset count in one RMW. Linearizes producers into either the * retired bank (counted in `old`) or the fresh bank (count from 0). */ - uint32_t old = atomic_exchange_explicit(&g_control, - newbank << JOURNAL_BANK_SHIFT, - memory_order_acq_rel); - uint32_t retired = old >> JOURNAL_BANK_SHIFT; /* == active */ + uint32_t old = + atomic_exchange_explicit(&g_control, newbank << JOURNAL_BANK_SHIFT, memory_order_acq_rel); + uint32_t retired = old >> JOURNAL_BANK_SHIFT; /* == active */ uint32_t raw = old & JOURNAL_COUNT_MASK; uint32_t count = raw; - if (count > JOURNAL_MAX_ENTRIES) { + if (count > JOURNAL_MAX_ENTRIES) + { log_warn("[JOURNAL] overflow: %u write(s) dropped this cycle " "(capacity=%d) -- increase JOURNAL_MAX_ENTRIES or reduce the " "plugin write rate", @@ -407,19 +723,26 @@ void journal_apply_and_clear(void) } journal_bank_t *bank = &g_banks[retired]; - for (uint32_t i = 0; i < count; i++) { + for (uint32_t i = 0; i < count; i++) + { uint32_t spins = 0; - while (atomic_load_explicit(&bank->published[i], memory_order_acquire) == 0) { - if (++spins >= JOURNAL_PUBLISH_SPIN_MAX) { + while (atomic_load_explicit(&bank->published[i], memory_order_acquire) == 0) + { + if (++spins >= JOURNAL_PUBLISH_SPIN_MAX) + { break; } - if ((spins & 0x3Fu) == 0) { + if ((spins & 0x3Fu) == 0) + { sched_yield(); } } - if (atomic_load_explicit(&bank->published[i], memory_order_acquire) != 0) { + if (atomic_load_explicit(&bank->published[i], memory_order_acquire) != 0) + { apply_entry(&bank->entries[i]); - } else { + } + else + { /* Producer claimed the slot before the flip but never published * (died / pathologically delayed). Skip to keep the scan bounded. */ log_warn("[JOURNAL] slot %u unpublished at flip; skipped", i); @@ -448,31 +771,47 @@ uint32_t journal_get_sequence(void) */ static journal_entry_t g_entries[JOURNAL_MAX_ENTRIES]; -static size_t g_count = 0; -static uint32_t g_next_sequence = 0; +static size_t g_count = 0; +static uint32_t g_next_sequence = 0; static pthread_mutex_t g_journal_mutex; -static bool g_initialized = false; +static bool g_initialized = false; static void emergency_flush_locked(void); int journal_init(const journal_buffer_ptrs_t *buffer_ptrs) { - if (buffer_ptrs == NULL) { + if (buffer_ptrs == NULL) + { log_error("Journal: buffer_ptrs is NULL"); return -1; } - if (buffer_ptrs->image_mutex == NULL) { + if (buffer_ptrs->image_mutex == NULL) + { log_error("Journal: image_mutex is NULL"); return -1; } - if (init_rt_mutex(&g_journal_mutex) != 0) { + if (init_rt_mutex(&g_journal_mutex) != 0) + { fprintf(stderr, "[JOURNAL] Error: failed to initialize mutex\n"); return -1; } + /* Allocated BEFORE the lock is taken, deliberately. Inside it, the early + * return on failure would skip the unlock at the end of this function and + * leave g_journal_mutex held forever -- every later journal_add, + * journal_apply_and_clear and journal_is_initialized would block, taking + * the scan thread with them, and journal_cleanup could not recover it. The + * map depends on nothing this lock protects. */ + if (force_map_alloc(journal_longest_table()) != 0) + { + log_error("Journal: could not allocate the forced-slot map for %u slots", + journal_longest_table()); + return -1; + } + pthread_mutex_lock(&g_journal_mutex); memcpy(&g_buffer_ptrs, buffer_ptrs, sizeof(journal_buffer_ptrs_t)); - g_count = 0; + g_count = 0; g_next_sequence = 0; memset(g_entries, 0, sizeof(g_entries)); g_initialized = true; @@ -485,9 +824,10 @@ int journal_init(const journal_buffer_ptrs_t *buffer_ptrs) void journal_cleanup(void) { pthread_mutex_lock(&g_journal_mutex); - g_initialized = false; - g_count = 0; + g_initialized = false; + g_count = 0; g_next_sequence = 0; + force_map_free(); memset(&g_buffer_ptrs, 0, sizeof(g_buffer_ptrs)); pthread_mutex_unlock(&g_journal_mutex); pthread_mutex_destroy(&g_journal_mutex); @@ -504,20 +844,22 @@ bool journal_is_initialized(void) static int journal_add(uint8_t type, uint16_t index, uint8_t bit, uint64_t value) { - if (!g_initialized) { + if (!g_initialized) + { return -1; } pthread_mutex_lock(&g_journal_mutex); - if (g_count >= JOURNAL_MAX_ENTRIES) { + if (g_count >= JOURNAL_MAX_ENTRIES) + { emergency_flush_locked(); } journal_entry_t *e = &g_entries[g_count]; - e->sequence = g_next_sequence++; - e->buffer_type = type; - e->bit_index = bit; - e->index = index; - e->value = value; + e->sequence = g_next_sequence++; + e->buffer_type = type; + e->bit_index = bit; + e->index = index; + e->value = value; g_count++; pthread_mutex_unlock(&g_journal_mutex); @@ -526,14 +868,16 @@ static int journal_add(uint8_t type, uint16_t index, uint8_t bit, uint64_t value void journal_apply_and_clear(void) { - if (!g_initialized) { + if (!g_initialized) + { return; } pthread_mutex_lock(&g_journal_mutex); - for (size_t i = 0; i < g_count; i++) { + for (size_t i = 0; i < g_count; i++) + { apply_entry(&g_entries[i]); } - g_count = 0; + g_count = 0; g_next_sequence = 0; pthread_mutex_unlock(&g_journal_mutex); } @@ -546,10 +890,11 @@ static void emergency_flush_locked(void) pthread_mutex_unlock(&g_journal_mutex); pthread_mutex_lock(g_buffer_ptrs.image_mutex); pthread_mutex_lock(&g_journal_mutex); - for (size_t i = 0; i < g_count; i++) { + for (size_t i = 0; i < g_count; i++) + { apply_entry(&g_entries[i]); } - g_count = 0; + g_count = 0; g_next_sequence = 0; pthread_mutex_unlock(g_buffer_ptrs.image_mutex); } @@ -580,57 +925,50 @@ uint32_t journal_get_sequence(void) * ============================================================================= */ -int journal_write_bool(journal_buffer_type_t type, uint16_t index, - uint8_t bit, bool value) +int journal_write_bool(journal_buffer_type_t type, uint16_t index, uint8_t bit, bool value) { - if (type != JOURNAL_BOOL_INPUT && - type != JOURNAL_BOOL_OUTPUT && - type != JOURNAL_BOOL_MEMORY) { + if (type != JOURNAL_BOOL_INPUT && type != JOURNAL_BOOL_OUTPUT && type != JOURNAL_BOOL_MEMORY) + { return -1; } - if (bit > 7) { + if (bit > 7) + { return -1; } return journal_add((uint8_t)type, index, bit, value ? 1u : 0u); } -int journal_write_byte(journal_buffer_type_t type, uint16_t index, - uint8_t value) +int journal_write_byte(journal_buffer_type_t type, uint16_t index, uint8_t value) { - if (type != JOURNAL_BYTE_INPUT && type != JOURNAL_BYTE_OUTPUT) { + if (type != JOURNAL_BYTE_INPUT && type != JOURNAL_BYTE_OUTPUT) + { return -1; } return journal_add((uint8_t)type, index, 0xFF, value); } -int journal_write_int(journal_buffer_type_t type, uint16_t index, - uint16_t value) +int journal_write_int(journal_buffer_type_t type, uint16_t index, uint16_t value) { - if (type != JOURNAL_INT_INPUT && - type != JOURNAL_INT_OUTPUT && - type != JOURNAL_INT_MEMORY) { + if (type != JOURNAL_INT_INPUT && type != JOURNAL_INT_OUTPUT && type != JOURNAL_INT_MEMORY) + { return -1; } return journal_add((uint8_t)type, index, 0xFF, value); } -int journal_write_dint(journal_buffer_type_t type, uint16_t index, - uint32_t value) +int journal_write_dint(journal_buffer_type_t type, uint16_t index, uint32_t value) { - if (type != JOURNAL_DINT_INPUT && - type != JOURNAL_DINT_OUTPUT && - type != JOURNAL_DINT_MEMORY) { + if (type != JOURNAL_DINT_INPUT && type != JOURNAL_DINT_OUTPUT && type != JOURNAL_DINT_MEMORY) + { return -1; } return journal_add((uint8_t)type, index, 0xFF, value); } -int journal_write_lint(journal_buffer_type_t type, uint16_t index, - uint64_t value) +int journal_write_lint(journal_buffer_type_t type, uint16_t index, uint64_t value) { - if (type != JOURNAL_LINT_INPUT && - type != JOURNAL_LINT_OUTPUT && - type != JOURNAL_LINT_MEMORY) { + if (type != JOURNAL_LINT_INPUT && type != JOURNAL_LINT_OUTPUT && type != JOURNAL_LINT_MEMORY) + { return -1; } return journal_add((uint8_t)type, index, 0xFF, value); diff --git a/core/src/plc_app/journal_buffer.h b/core/src/plc_app/journal_buffer.h index 8ce684b4..e240b6aa 100644 --- a/core/src/plc_app/journal_buffer.h +++ b/core/src/plc_app/journal_buffer.h @@ -25,11 +25,12 @@ #ifndef JOURNAL_BUFFER_H #define JOURNAL_BUFFER_H +#include "../lib/iec_types.h" +#include "image_table_id.h" +#include #include -#include #include -#include -#include "../lib/iec_types.h" +#include #ifdef __cplusplus extern "C" { @@ -115,13 +116,66 @@ typedef struct { IEC_ULINT **lint_output; IEC_ULINT **lint_memory; - /* Buffer size (number of elements in each array) */ + /* How long each array above is, in its own elements, indexed by + * journal_buffer_type_t. + * + * Taken at journal_init, from the SAME moment as the pointers beside it. + * The bound and the pointers have to come from one point in time: reading + * the live image sizes while holding pointers captured earlier would, if + * the two ever diverged, apply a new length to an old allocation. They do + * not diverge today -- the image is allocated before the cycle thread that + * calls journal_init exists, and a re-load stops that thread first -- but + * the coupling was implicit, and implicit is what this whole task keeps + * finding. */ + uint32_t table_sizes[JOURNAL_TYPE_COUNT]; + + /* THE SMALLEST ARRAY, NOT THE LENGTH OF ALL OF THEM (RTOP-284). + * + * The arrays above no longer share a length. This field was a second copy + * of the one-number assumption, internal to the runtime, and it is kept + * only for the few places that still want a conservative single figure: + * it is the minimum, so using it as a bound refuses an index rather than + * letting one run off the end of a shorter array. + * + * Anything bounding a WRITE asks image_table_capacity() for the table that + * write is going to, which is what apply_write_raw does. */ int buffer_size; /* Image table mutex (for emergency flush and apply operations) */ pthread_mutex_t *image_mutex; } journal_buffer_ptrs_t; +/** + * @brief How many forces were dropped for being outside the image, and reset. + * + * Forces that name an address the image does not have are counted rather than + * logged, because both force paths run under `image_lock()` on the real-time + * thread and `log_warn` takes a mutex with no priority inheritance before a + * blocking socket write. + * + * Call this from OFF the real-time path -- between cycles, or when answering a + * status request -- and report what it returns. Reading clears the counter, so + * each drop is reported once. + * + * @return Drops since the last call. + */ +/** + * @brief Which image table a journal buffer type stores. + * + * The two enums name the same fourteen tables in DIFFERENT orders -- the + * journal puts each width's memory beside its input and output, image_tables.h + * groups the memory tables at the end -- so a cast between them lands under + * another table's bounds. Exposed so the caller filling `table_sizes` uses the + * same mapping the journal itself does rather than a second copy of it. + * + * @param type A `journal_buffer_type_t`. + * @return The matching `image_table_id_t`, or `IMAGE_TABLE_COUNT` if the type + * is out of range. + */ +image_table_id_t journal_type_to_image_table(uint8_t type); + +unsigned journal_take_force_drops(void); + /** * @brief Initialize the journal buffer system * @@ -215,12 +269,21 @@ int journal_write_lint(journal_buffer_type_t type, uint16_t index, * @param index Buffer array index * @param bit Bit index (0-7) for BOOL types; ignored otherwise * @param value Forced value (sized for the largest type) + * @return 0 when the slot is now forced, -1 when the request was refused + * because the address is outside the image (or the bit index outside + * a BOOL byte). + * + * RETURNS RATHER THAN ONLY COUNTING, because the caller can act on it and the + * counter cannot. `apply_located` pins the program's IECVar too, and a force + * that this function refuses but the IECVar accepts shows in the debugger as + * forced while the image slot is untouched — the one state that is worse than + * a refusal, since it is a refusal the user is told is a success. * * @note MUST be called only from the dispatcher's debug-write drain, under the * image lock — the same serialization domain as journal_apply_and_clear. */ -void journal_force_set(journal_buffer_type_t type, uint16_t index, - uint8_t bit, uint64_t value); +int journal_force_set(journal_buffer_type_t type, uint16_t index, + uint8_t bit, uint64_t value); /** * @brief Release a forced image slot (writes flow through again) @@ -228,11 +291,13 @@ void journal_force_set(journal_buffer_type_t type, uint16_t index, * @param type Buffer type * @param index Buffer array index * @param bit Bit index (0-7) for BOOL types; ignored otherwise + * @return 0 when the slot is no longer forced, -1 when the request was + * refused because the address is outside the image. * * @note Same calling constraint as journal_force_set. */ -void journal_force_clear(journal_buffer_type_t type, uint16_t index, - uint8_t bit); +int journal_force_clear(journal_buffer_type_t type, uint16_t index, + uint8_t bit); /** * @brief Apply all pending journal entries to image tables and clear the journal diff --git a/core/src/plc_app/plc_main.c b/core/src/plc_app/plc_main.c index b8ed246b..c4c9a9d3 100644 --- a/core/src/plc_app/plc_main.c +++ b/core/src/plc_app/plc_main.c @@ -150,6 +150,31 @@ int main(int argc, char *argv[]) log_info("[PLUGIN]: Plugin driver system created"); if (plugin_driver_load_config(plugin_driver, "./plugins.conf") == 0) { + /* An image before the plugins see one, even though no program is + * loaded yet. plugin_driver_init() copies the base pointers and + * buffer_size into every plugin's args, and a plugin is entitled to + * a valid image from the moment it initialises -- never a NULL base + * pointer and never a zero size. The minimum is what + * image_tables_alloc() clamps to, per table: the smallest count + * that is not no image at all. NULL asks for exactly that, which + * says "no program has told me anything yet" rather than passing a + * zeroed struct that reads like a real answer. A program load + * reallocates it properly. */ + pthread_mutex_t *itm = image_tables_mutex(); + pthread_mutex_lock(itm); + const bool image_ok = image_tables_alloc(NULL); + pthread_mutex_unlock(itm); + + if (!image_ok) + { + /* Log AND STOP, which is the criterion. Falling through was + * worse than it looked: with capacity 0 the ordering guard in + * plugin_driver refuses the runtime args for every plugin, so + * the runtime came up with nothing initialised and one line to + * say why -- a runtime that looks alive and drives nothing. */ + log_error("[PLUGIN]: could not allocate the boot image — refusing to start"); + return EXIT_FAILURE; + } plugin_driver_init(plugin_driver); log_info("[PLUGIN]: All plugins initialized (not started)"); } diff --git a/core/src/plc_app/plc_state_manager.cpp b/core/src/plc_app/plc_state_manager.cpp index 48a70196..ded9afe3 100644 --- a/core/src/plc_app/plc_state_manager.cpp +++ b/core/src/plc_app/plc_state_manager.cpp @@ -453,31 +453,64 @@ void *plc_cycle_thread(void *arg) plc_retain_read(); journal_buffer_ptrs_t journal_ptrs = { - .bool_input = bool_input, - .bool_output = bool_output, - .bool_memory = bool_memory, - .byte_input = byte_input, - .byte_output = byte_output, - .int_input = int_input, - .int_output = int_output, - .int_memory = int_memory, - .dint_input = dint_input, - .dint_output = dint_output, - .dint_memory = dint_memory, - .lint_input = lint_input, - .lint_output = lint_output, - .lint_memory = lint_memory, - .buffer_size = BUFFER_SIZE, - .image_mutex = itm, + .bool_input = g_image.bool_input, + .bool_output = g_image.bool_output, + .bool_memory = g_image.bool_memory, + .byte_input = g_image.byte_input, + .byte_output = g_image.byte_output, + .int_input = g_image.int_input, + .int_output = g_image.int_output, + .int_memory = g_image.int_memory, + .dint_input = g_image.dint_input, + .dint_output = g_image.dint_output, + .dint_memory = g_image.dint_memory, + .lint_input = g_image.lint_input, + .lint_output = g_image.lint_output, + .lint_memory = g_image.lint_memory, + /* Follows the image: journal_buffer.c bounds every forced write + * against this, so a stale constant here would silently drop writes to + * the part of the image beyond it. */ + .table_sizes = {}, + .buffer_size = (int)image_tables_capacity(), + .image_mutex = itm, }; - if (journal_init(&journal_ptrs) != 0) + + /* The fourteen lengths, captured HERE, at the same moment as the pointers + * above. journal_buffer.c bounds each write by the table it addresses, and + * reading that from the live image while holding pointers taken earlier + * would apply a new length to an old allocation if the two ever diverged. + * + * The journal's own order, not the image's -- they are the same fourteen + * tables in different orders, which is the trap kJournalToImageTable + * exists for. */ + for (int t = 0; t < JOURNAL_TYPE_COUNT; ++t) { - log_error("Failed to initialize journal buffer"); + journal_ptrs.table_sizes[t] = image_table_capacity(journal_type_to_image_table(t)); } - else + if (journal_init(&journal_ptrs) != 0) { - log_info("Journal buffer initialized"); + /* FATAL, not a log line, and this is newly true. + * + * journal_init could only fail on a NULL argument before -- a + * programming error caught at the first boot. It now allocates the + * forced-slot map, so it fails under memory pressure at run time, and + * that path leaves g_initialized false. + * + * With g_initialized false every journal_add returns -1 and + * journal_apply_and_clear returns immediately. That is not only + * "plugins cannot write": the program's own copy-out goes through the + * same journal, so carrying on would take real-time priority, spawn + * the task threads and publish RUNNING with every located output + * permanently dead and one line to say so. Same criterion as the image + * allocation below -- log and stop, never a half-working runtime. */ + log_error("Failed to initialize journal buffer — refusing to start"); + pthread_mutex_lock(&state_mutex); + plc_state = PLC_STATE_ERROR; + pthread_mutex_unlock(&state_mutex); + log_info("PLC State: ERROR"); + return NULL; } + log_info("Journal buffer initialized"); if (plugin_driver) { @@ -985,6 +1018,20 @@ void *plc_cycle_thread(void *arg) * these bytes are actually committed now; no-op with no store. */ plc_retain_save(); image_unlock(); + + /* Forces the drain had to drop, reported from OUTSIDE the + * image lock and off the logging-under-lock path that + * journal_buffer.c deliberately avoids. Rate-limited by + * construction: reading the counter clears it, so a client + * hammering one bad address gets one line per cycle rather + * than one per entry. */ + { + const unsigned dropped = journal_take_force_drops(); + if (dropped > 0) + log_warn("[JOURNAL] %u force(s) named an address outside the " + "image and were ignored this cycle", + dropped); + } if (plugin_driver) plugin_driver_cycle_end(plugin_driver); cycle_end_pending = false; pthread_mutex_lock(&done_mutex); @@ -1028,6 +1075,18 @@ extern "C" int load_plc_program(PluginManager *pm) * value; nothing writes it during a transition. */ log_info("Loading PLC application"); + /* OUTSIDE `if (plugin_driver)`, deliberately. The image is the + * PROGRAM'S storage -- it is where located variables live -- so it + * must not be conditional on there being a plugin driver. + * + * plc_main.c skips its whole plugin block when plugin_driver_create() + * returns NULL, with no error and no exit. A later START then reached + * here, skipped this, and left capacity at zero for the life of the + * process: fill_null_pointers looped zero times, journal_init got + * fourteen null bases, every journal write was rejected and every + * located read returned zero. The PLC scanned and drove nothing, + * silently -- which is the failure plc_main.c refuses to ship a few + * lines above its own boot allocation. */ if (plugin_driver) { if (plugin_driver_update_config(plugin_driver, "./plugins.conf") != 0) @@ -1055,6 +1114,97 @@ extern "C" int load_plc_program(PluginManager *pm) plugin_manager_destroy(pm); return -1; } + } + + /* PLUGIN CONFIG IS LOADED BEFORE THE IMAGE IS SIZED, and the order is + * load-bearing rather than tidy. + * + * The per-table decision below asks each plugin whether it exports + * set_image_sizes, and for a Python plugin that answer IS the resolved + * symbol -- which `plugin_driver_update_config` is what resolves. On + * the first load after boot the symbols happened to be resolved + * already, from the boot-time load, so the decision saw them. On every + * RELOAD the preceding stop had released them, the decision read NULL, + * and the image silently fell back to square for the rest of the + * process. Per-table sizing therefore worked once per boot and never + * again -- and a stop/start is the ordinary way to change program. + * + * Loading the config first costs nothing: it reads plugins.conf and + * resolves symbols, and touches no image state. + * + * The image block stays OUTSIDE `if (plugin_driver)`. The image is the + * program's storage, not the plugins', and gating it on a plugin + * concern is what left g_capacity at zero when plugin_driver was NULL. + * A NULL driver makes the capability check answer false, which sizes + * the image square -- the conservative side, and correct. */ + + /* SIZE AND ALLOCATE THE IMAGE, and do it HERE. + * + * After plugin_manager_load, because the floor is derived by + * walking the loaded .so's locatedVars[] and there is no .so to + * walk before it. Before plugin_driver_init, because that is where + * plugin_driver.c copies the base pointers and buffer_size into the + * runtime args, and both native plugins copy that struct BY VALUE + * inside their init(). Allocate after, and every plugin spends the + * run holding pointers into the image of the program before this + * one. + * + * Two sources, larger wins: image.conf, which the editor derived + * from what the project contains, and the floor this runtime + * derives from the program itself. That is what makes a missing or + * stale image.conf unable to undersize -- see image_tables.h. */ + { + image_sizes_t configured; + image_sizes_t floor; + image_sizes_read_conf("./image.conf", &configured); + image_sizes_derive_floor(pm, &floor); + image_sizes_take_max(&configured, &floor); + + /* PER TABLE, OR SQUARE, DECIDED PER RUN. + * + * Per-table is what the project asked for and what the image + * exists to deliver. It is only safe when every loaded plugin + * understands it: 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. + * One that does not export set_image_sizes has not been told + * they can differ, so for that run they do not. + * + * Logged with the plugin that forced it, because the two modes + * are otherwise indistinguishable from outside. */ + const char *forced_by = NULL; + if (!plugin_driver_all_understand_per_table_sizes(plugin_driver, &forced_by)) + { + image_sizes_flatten(&configured); + log_info("[PLUGIN]: image kept square: plugin '%s' does not declare " + "set_image_sizes", + forced_by ? forced_by : "(unknown)"); + } + + pthread_mutex_t *itm = image_tables_mutex(); + pthread_mutex_lock(itm); + const bool ok = image_tables_alloc(&configured); + pthread_mutex_unlock(itm); + + if (!ok) + { + /* Log and stop, never a partial image. The alternative is + * starting with tables that do not cover the program's own + * addresses, which reads and writes nothing and reports + * nothing. */ + log_error("[PLUGIN]: image allocation failed — refusing to start"); + pthread_mutex_lock(&state_mutex); + plc_state = PLC_STATE_ERROR; + pthread_mutex_unlock(&state_mutex); + log_info("PLC State: ERROR"); + if (pm == plc_program) plc_program = NULL; + plugin_manager_destroy(pm); + return -1; + } + } + + if (plugin_driver) + { if (plugin_driver_init(plugin_driver) != 0) { /* Roll back any plugins that did initialise before the @@ -1064,6 +1214,32 @@ extern "C" int load_plc_program(PluginManager *pm) * masters or OPC-UA sockets. */ log_error("[PLUGIN]: Plugin init failed — rolling back"); plugin_driver_cleanup_init(plugin_driver); + /* The image this load allocated goes back too. cleanup_init has + * just undone every plugin's init(), so nothing holds the base + * pointers any more, and leaving it would keep a whole image + * reserved for a program that never started. */ + { + pthread_mutex_t *rollback_itm = image_tables_mutex(); + pthread_mutex_lock(rollback_itm); + /* THE OTHER HALF OF THE ORDERING, now checked rather than assumed. + * + * Allocate-before-init is refused at run time in + * generate_structured_args_with_driver. Release-after-stop had only a + * comment, so a refactor moving a cleanup_init call below this free + * got no diagnostic at all -- and both native plugins spend the run + * holding the base pointers they copied by value at init(). */ + if (plugin_driver_any_initialized(plugin_driver)) + { + log_error( + "[PLUGIN]: refusing to free the image while a plugin is still " + "initialised — it still holds the base pointers it copied at init()"); + } + else + { + image_tables_free(); + } + pthread_mutex_unlock(rollback_itm); + } pthread_mutex_lock(&state_mutex); plc_state = PLC_STATE_ERROR; pthread_mutex_unlock(&state_mutex); @@ -1083,6 +1259,30 @@ extern "C" int load_plc_program(PluginManager *pm) * before bailing — otherwise the next start retries init() * on a half-initialised driver. */ if (plugin_driver) plugin_driver_cleanup_init(plugin_driver); + /* Same reasoning as the init-failure rollback above: the plugins + * have been de-initialised, so the image they were pointing at is + * free to go and must, or this load leaks it. */ + { + pthread_mutex_t *rollback_itm = image_tables_mutex(); + pthread_mutex_lock(rollback_itm); + /* THE OTHER HALF OF THE ORDERING, now checked rather than assumed. + * + * Allocate-before-init is refused at run time in + * generate_structured_args_with_driver. Release-after-stop had only a + * comment, so a refactor moving a cleanup_init call below this free + * got no diagnostic at all -- and both native plugins spend the run + * holding the base pointers they copied by value at init(). */ + if (plugin_driver_any_initialized(plugin_driver)) + { + log_error("[PLUGIN]: refusing to free the image while a plugin is still " + "initialised — it still holds the base pointers it copied at init()"); + } + else + { + image_tables_free(); + } + pthread_mutex_unlock(rollback_itm); + } pthread_mutex_lock(&state_mutex); plc_state = PLC_STATE_ERROR; pthread_mutex_unlock(&state_mutex); @@ -1150,15 +1350,65 @@ extern "C" int unload_plc_program(PluginManager *pm) * one that buffers. */ plc_retain_flush(); + plugin_driver_stop(plugin_driver); + + /* AFTER plugin_driver_stop, not before, and the reason is a lifetime + * rather than tidiness. + * + * journal_cleanup() frees the forced-slot rows, which used to be + * static storage and now are not. image_lock is handed to every plugin + * as args->image_lock and calls journal_apply_and_clear(), so + * is_slot_forced() runs on plugin-owned threads -- EtherCAT's bus + * thread and the s7comm server callback among them. Freeing those rows + * while those threads are live is a use-after-free reachable from an + * ordinary operation: forcing a variable from the debugger or over + * OPC UA is all it takes for there to be anything to read. + * + * Stopping the plugins first is the only thing that guarantees no + * plugin thread is inside image_lock(). force_map_free() clears its + * guards before freeing as a second line of defence, but ordering is + * what actually closes this. + * + * plc_retain_flush() above still has to run BEFORE the stop, because a + * plugin-backed store has to be alive to answer -- so the two sit on + * opposite sides of it deliberately. */ journal_cleanup(); debug_write_journal_reset(); log_info("Journal buffer cleaned up"); - plugin_driver_stop(plugin_driver); + /* STOP IS NOT ENOUGH TO MAKE THE IMAGE FREEABLE, which is easy to miss. + * + * plugin_driver_stop skips any plugin whose `running` is 0, and a + * plugin can be initialised and never started -- ethercat is + * deliberately init'd even when disabled. Such a plugin still holds + * the by-value copy of the base pointers it took in init(), so + * stopping the running ones and freeing would leave it pointing at + * released memory. cleanup_init undoes init() for every plugin, + * started or not, which is what actually ends the last reference. */ + plugin_driver_cleanup_init(plugin_driver); pthread_mutex_t *itm = image_tables_mutex(); pthread_mutex_lock(itm); image_tables_clear_null_pointers(); + /* Released only after every plugin has been stopped AND de-initialised + * above. Both native plugins cache these pointers by value at init(), + * so freeing any earlier hands them memory that belongs to nobody. */ + /* THE OTHER HALF OF THE ORDERING, now checked rather than assumed. + * + * Allocate-before-init is refused at run time in + * generate_structured_args_with_driver. Release-after-stop had only a + * comment, so a refactor moving a cleanup_init call below this free + * got no diagnostic at all -- and both native plugins spend the run + * holding the base pointers they copied by value at init(). */ + if (plugin_driver_any_initialized(plugin_driver)) + { + log_error("[PLUGIN]: refusing to free the image while a plugin is still " + "initialised — it still holds the base pointers it copied at init()"); + } + else + { + image_tables_free(); + } pthread_mutex_unlock(itm); void (*python_cleanup)(void); diff --git a/tests/pytest/test_apply_image_conf.py b/tests/pytest/test_apply_image_conf.py new file mode 100644 index 00000000..e4c31ffd --- /dev/null +++ b/tests/pytest/test_apply_image_conf.py @@ -0,0 +1,354 @@ +"""Behavioural tests for installing the I/O image sizes from an upload. + +The sizes belong to the project and are derived rather than chosen, so the +interesting behaviour is not "can a file be copied" — it is the present/absent +contract that keeps the project authoritative, and one case where this file +deliberately differs from retain.conf: + + * an upload carrying image.conf sizes the device; + * an upload WITHOUT one removes the device's copy, so the runtime sizes the + image from the program it just loaded. Not because absence is an + instruction — it is not, unlike retain's — but because a STALE file keeps + the previous project's memory reserved for a program that is gone; + * a stanza the core could not honour is refused loudly, and does not leave the + previous project's sizes quietly in force. + +The last two are the cases that would otherwise be invisible: nothing on a +running device shows you that its image is sized for the program before last. +""" + +import os + +import pytest + +from webserver import image_config, plcapp_management + + +@pytest.fixture(autouse=True) +def isolated_conf(tmp_path, monkeypatch): + """Point the install destination at a temp file, never a real runtime root.""" + dest = tmp_path / "runtime" / "image.conf" + dest.parent.mkdir(parents=True, exist_ok=True) + monkeypatch.setattr(plcapp_management, "IMAGE_CONF_PATH", dest) + return dest + + +@pytest.fixture() +def upload(tmp_path): + """An extracted upload directory, as safe_extract would leave it.""" + d = tmp_path / "generated" + d.mkdir() + return d + + +def write_conf(directory, version=image_config.IMAGE_CONF_FORMAT_VERSION, **sizes): + """An upload's image.conf, in the format the current editor emits. + + Every value carries the unit of the ADDRESS its table stores, so the number + and its unit cannot disagree. `version=None` omits the declaration, which a + reader must refuse rather than guess at. + """ + lines = [] if version is None else [f"format_version={version}"] + lines += [f"{k}={v} {image_config.IMAGE_TABLE_UNITS[k]}" for k, v in sizes.items()] + (directory / "image.conf").write_text("\n".join(lines) + "\n", encoding="utf-8") + + +def write_raw_conf(directory, body): + """An image.conf written byte for byte, for the malformed cases.""" + (directory / "image.conf").write_text(body, encoding="utf-8") + + +def installed(dest): + """Just the counts of the installed file.""" + _version, sizes, _units = image_config.read_image_conf_file(dest) + return sizes + + +class TestPresentAbsentContract: + def test_an_upload_carrying_sizes_installs_them(self, upload, isolated_conf): + write_conf(upload, int_output=4096, bool_output=64, int_memory=20) + plcapp_management.apply_image_conf(str(upload)) + + assert isolated_conf.exists() + sizes = installed(isolated_conf) + assert sizes["int_output"] == 4096 + assert sizes["bool_output"] == 64 + assert sizes["int_memory"] == 20 + + def test_every_table_is_written_including_the_zeros(self, upload, isolated_conf): + # "Absent means zero" is an editor-side convention; the C parser should + # not have to know it. + write_conf(upload, int_output=8) + plcapp_management.apply_image_conf(str(upload)) + + body = isolated_conf.read_text(encoding="utf-8") + for key in image_config.IMAGE_TABLE_KEYS: + assert f"{key}=" in body + assert "bool_input=0" in body + + def test_an_upload_without_the_file_removes_a_stale_copy(self, upload, isolated_conf): + # THE CASE THAT MATTERS. A previous project sized the device for 4096 + # output words; this upload says nothing. Leaving the old file in place + # would keep 4096 reserved for a program that is no longer here. + write_conf(upload, int_output=4096) + plcapp_management.apply_image_conf(str(upload)) + assert isolated_conf.exists() + + os.remove(upload / "image.conf") + plcapp_management.apply_image_conf(str(upload)) + + assert not isolated_conf.exists() + + def test_an_upload_without_the_file_and_no_copy_is_a_no_op(self, upload, isolated_conf): + plcapp_management.apply_image_conf(str(upload)) + assert not isolated_conf.exists() + + +class TestRefusal: + def test_a_table_beyond_the_abi_limit_is_refused(self, upload, isolated_conf): + write_conf(upload, int_output=image_config.MAX_TABLE_ELEMENTS + 1) + plcapp_management.apply_image_conf(str(upload)) + assert not isolated_conf.exists() + + def test_the_abi_limit_itself_is_accepted(self, upload, isolated_conf): + # It is a fact of the ABI, not a policy ceiling, so the boundary value + # is legal and only what exceeds it is not. + write_conf(upload, int_output=image_config.MAX_TABLE_ELEMENTS) + plcapp_management.apply_image_conf(str(upload)) + assert installed(isolated_conf)["int_output"] == image_config.MAX_TABLE_ELEMENTS + + def test_a_negative_size_is_refused(self, upload, isolated_conf): + write_conf(upload, int_memory=-1) + plcapp_management.apply_image_conf(str(upload)) + assert not isolated_conf.exists() + + def test_a_non_utf8_file_is_refused_rather_than_raising(self, upload, isolated_conf): + # The file comes from an upload, so its bytes are whatever was sent. + # UnicodeDecodeError used to escape to app.py, which answers + # "Unexpected error: ..." to the client. + (upload / "image.conf").write_bytes(b"int_output=\xff\xfe\n") + plcapp_management.apply_image_conf(str(upload)) + assert not isolated_conf.exists() + + def test_a_directory_named_image_conf_is_refused_rather_than_raising( + self, upload, isolated_conf + ): + (upload / "image.conf").mkdir() + plcapp_management.apply_image_conf(str(upload)) + assert not isolated_conf.exists() + + def test_a_garbled_value_is_refused_rather_than_raising(self, upload, isolated_conf): + (upload / "image.conf").write_text("int_output=lots\n", encoding="utf-8") + plcapp_management.apply_image_conf(str(upload)) + assert not isolated_conf.exists() + + def test_a_refusal_does_not_leave_the_previous_sizes_in_force(self, upload, isolated_conf): + write_conf(upload, int_output=4096) + plcapp_management.apply_image_conf(str(upload)) + assert isolated_conf.exists() + + write_conf(upload, int_output=image_config.MAX_TABLE_ELEMENTS + 1) + plcapp_management.apply_image_conf(str(upload)) + + # Not "still 4096": the device must not stay sized by a project the user + # is no longer running. + assert not isolated_conf.exists() + + def test_all_fourteen_or_none(self, upload, isolated_conf): + # The tables size interlocking storage that one allocation hands out + # together, so a single bad value refuses the whole stanza rather than + # installing the thirteen that were fine. + write_conf(upload, int_output=8, bool_output=64, lint_memory=-3) + plcapp_management.apply_image_conf(str(upload)) + assert not isolated_conf.exists() + + +class TestParser: + def test_a_missing_file_reads_as_every_table_zero(self, tmp_path): + _v, sizes, _u = image_config.read_image_conf_file(tmp_path / "nope.conf") + assert sizes == {key: 0 for key in image_config.IMAGE_TABLE_KEYS} + + def test_comments_and_blank_lines_are_ignored(self, tmp_path): + p = tmp_path / "image.conf" + p.write_text("# a comment\n\nint_output=12\n \n", encoding="utf-8") + assert image_config.read_image_conf_file(p)[1]["int_output"] == 12 + + def test_an_unknown_table_is_ignored_rather_than_refused(self, tmp_path, upload, isolated_conf): + # A newer editor emitting a table this runtime does not have must not + # fail the upload; the core would ignore it anyway. + write_raw_conf(upload, "format_version=2\nint_output=8 words\nbyte_memory=64 bytes\n") + plcapp_management.apply_image_conf(str(upload)) + assert installed(isolated_conf)["int_output"] == 8 + assert "byte_memory" not in isolated_conf.read_text(encoding="utf-8") + + def test_the_installed_file_is_byte_stable_for_the_same_sizes(self, upload, isolated_conf): + write_conf(upload, int_output=8, bool_output=64) + plcapp_management.apply_image_conf(str(upload)) + first = isolated_conf.read_text(encoding="utf-8") + + # Same sizes, different order in the uploaded file. + write_conf(upload, bool_output=64, int_output=8) + plcapp_management.apply_image_conf(str(upload)) + + assert isolated_conf.read_text(encoding="utf-8") == first + + def test_no_key_for_a_table_the_runtime_does_not_have(self): + # byte_input and byte_output exist; byte_memory does not, so %MB has no + # storage on this runtime at all. + assert "byte_input" in image_config.IMAGE_TABLE_KEYS + assert "byte_memory" not in image_config.IMAGE_TABLE_KEYS + assert len(image_config.IMAGE_TABLE_KEYS) == 14 + + +class TestFormatVersion: + """The version is checked before anything else, and refuses the whole file. + + Every check below it reads the values by this version's rules, so a file + from a format we do not know is not a file with fourteen suspicious numbers + in it -- it is a file we cannot claim to have understood. + """ + + def test_a_file_with_no_version_is_refused(self, upload, isolated_conf): + write_conf(upload, version=None, int_output=8) + plcapp_management.apply_image_conf(str(upload)) + assert not isolated_conf.exists() + + def test_a_future_version_is_refused_rather_than_guessed_at(self, upload, isolated_conf): + write_conf(upload, version=3, int_output=8) + plcapp_management.apply_image_conf(str(upload)) + assert not isolated_conf.exists() + + def test_a_refused_version_does_not_leave_the_previous_sizes_in_force( + self, upload, isolated_conf + ): + # The device would otherwise keep running sized by a project that is no + # longer on it, which is the whole reason a stale file is worse than none. + isolated_conf.write_text("format_version=2\nint_output=4096 words\n", encoding="utf-8") + write_conf(upload, version=None, int_output=8) + plcapp_management.apply_image_conf(str(upload)) + assert not isolated_conf.exists() + + def test_the_installed_file_declares_the_version(self, upload, isolated_conf): + write_conf(upload, int_output=8) + plcapp_management.apply_image_conf(str(upload)) + assert "format_version=2" in isolated_conf.read_text(encoding="utf-8") + + +class TestUnits: + def test_nothing_here_converts_bits_to_bytes(self, upload, isolated_conf): + # The BOOL tables travel in BITS, because %QX addresses bits, and the + # core converts to the [N][8] shape its storage has. Converting here + # too is how the two sides end up disagreeing by a factor of eight with + # no diagnostic anywhere, so 64 bits in must be 64 bits out. + write_conf(upload, bool_output=64) + plcapp_management.apply_image_conf(str(upload)) + assert installed(isolated_conf)["bool_output"] == 64 + + def test_every_table_is_written_with_its_own_unit(self, upload, isolated_conf): + write_conf(upload, int_output=8) + plcapp_management.apply_image_conf(str(upload)) + body = isolated_conf.read_text(encoding="utf-8") + for key, unit in image_config.IMAGE_TABLE_UNITS.items(): + assert f"{key}=" in body + assert body.split(f"{key}=")[1].split("\n")[0].endswith(unit) + + def test_a_value_in_the_wrong_unit_is_refused(self, upload, isolated_conf): + # The case the unit word exists for. Bytes is a perfectly plausible + # unit for bool_output -- its STORAGE is in bytes -- and reading 64 as + # bytes rather than bits allocates eight times what was asked for. + write_raw_conf(upload, "format_version=2\nbool_output=64 bytes\n") + plcapp_management.apply_image_conf(str(upload)) + assert not isolated_conf.exists() + + def test_a_value_with_no_unit_is_refused(self, upload, isolated_conf): + # Which is exactly what a version-1 file looks like. + write_raw_conf(upload, "format_version=2\nint_output=8\n") + plcapp_management.apply_image_conf(str(upload)) + assert not isolated_conf.exists() + + +class TestRefusalMessages: + """The message is the point of validating here rather than in the core. + + This module's own docstring says so: refusing it here, with a line in the + build log the user is already watching, is the only place a person sees it. + A message that names the wrong mistake sends them looking for something + that is not there. + """ + + def test_a_non_integer_count_says_so(self): + # Used to read "cannot be negative (got -1)", because the parse failure + # was stored as -1 and fell into the negative branch. + with pytest.raises(image_config.ImageConfigError) as excinfo: + image_config.validate_table_count("int_output", None, "words") + assert "whole number" in str(excinfo.value) + assert "negative" not in str(excinfo.value) + + def test_an_actually_negative_count_still_says_negative(self): + with pytest.raises(image_config.ImageConfigError) as excinfo: + image_config.validate_table_count("int_output", -3, "words") + assert "negative" in str(excinfo.value) + assert "-3" in str(excinfo.value) + + def test_a_garbled_value_in_a_real_file_reaches_the_right_message(self, upload, isolated_conf): + # End to end, because the sentinel is what connects the two. + write_raw_conf(upload, "format_version=2\nint_output=4.5 words\n") + _v, sizes, units = image_config.read_image_conf_file(upload / "image.conf") + with pytest.raises(image_config.ImageConfigError) as excinfo: + image_config.validate_image_conf(2, sizes, units) + assert "whole number" in str(excinfo.value) + + +class TestCeiling: + """The ABI ceiling is in ELEMENTS, and the file is not. + + Comparing a bit count against an element ceiling would refuse every legal + image above 8192 bytes -- eight times early, and by exactly the unit + confusion the format was changed to remove. + """ + + def test_a_word_table_is_capped_at_the_abi_limit(self, upload, isolated_conf): + write_conf(upload, int_output=image_config.MAX_TABLE_ELEMENTS) + plcapp_management.apply_image_conf(str(upload)) + assert isolated_conf.exists() + + def test_a_word_table_one_past_the_limit_is_refused(self, upload, isolated_conf): + write_conf(upload, int_output=image_config.MAX_TABLE_ELEMENTS + 1) + plcapp_management.apply_image_conf(str(upload)) + assert not isolated_conf.exists() + + def test_a_bit_table_may_reach_eight_bits_per_element(self, upload, isolated_conf): + # 65536 elements of bool_output is 524288 bits, and every one of them + # is addressable. This is the case a ceiling in the wrong unit breaks. + write_conf(upload, bool_output=image_config.MAX_TABLE_ELEMENTS * 8) + plcapp_management.apply_image_conf(str(upload)) + assert isolated_conf.exists() + assert installed(isolated_conf)["bool_output"] == image_config.MAX_TABLE_ELEMENTS * 8 + + def test_a_bit_table_one_bit_past_the_limit_is_refused(self, upload, isolated_conf): + write_conf(upload, bool_output=image_config.MAX_TABLE_ELEMENTS * 8 + 1) + plcapp_management.apply_image_conf(str(upload)) + assert not isolated_conf.exists() + + def test_the_ceiling_is_reported_in_the_unit_that_was_asked_for(self): + with pytest.raises(image_config.ImageConfigError) as excinfo: + image_config.validate_table_count( + "bool_output", image_config.MAX_TABLE_ELEMENTS * 8 + 1, "bits" + ) + # A message quoting an element ceiling against a bit count is the same + # ambiguity in prose. + assert "bits" in str(excinfo.value) + + +class TestLogging: + def test_the_log_line_names_only_the_tables_actually_sized(self): + sizes = {key: 0 for key in image_config.IMAGE_TABLE_KEYS} + sizes["int_output"] = 4096 + line = image_config.describe_image_conf(sizes) + # The unit travels with the number here too: a log line saying "4096" + # for a bit table would be the same ambiguity the format removed. + assert line == "int_output=4096 words" + + def test_an_empty_image_says_so_rather_than_listing_nothing(self): + sizes = {key: 0 for key in image_config.IMAGE_TABLE_KEYS} + assert image_config.describe_image_conf(sizes) == "every table zero" diff --git a/tests/pytest/test_image_conf_contract.py b/tests/pytest/test_image_conf_contract.py new file mode 100644 index 00000000..a3239013 --- /dev/null +++ b/tests/pytest/test_image_conf_contract.py @@ -0,0 +1,323 @@ +"""`image.conf` has three implementations. This is the only one CI can check. + +The file format is written in three places and read in a fourth: + + * the OpenPLC editor emits it (``generate-image-conf.ts``, DOPE-615); + * ``webserver/image_config.py`` validates and installs it; + * ``core/src/plc_app/image_tables.cpp`` parses it in the PLC application; + * ``core/src/plc_app/image_tables.h`` declares the tables it names. + +A key added on one side and forgotten on another does not fail anything. The +core simply never sees that table's size, falls back to the floor derived from +the program, and the image comes out smaller than the project asked for -- +silently, on a device, with no diagnostic anywhere. The C++ side has a +``static_assert`` for the count; the ORDER, and the agreement between C and +Python, have nowhere else to be checked. + +So it is checked here, by reading the C sources as text. That is unusual and +deliberate: pytest is the only suite this repository runs in CI (the Ceedling +project in ``project.yml`` is wired to nothing), so a Python test is the only +guard that will actually run. It parses rather than imports because there is no +binding between the two languages to import through. + +The editor lives in another repository and cannot be reached from here. Its +half of the contract is pinned by its own tests, and by the fact that all three +lists are in the same order for the same reason: they follow the declaration +order of ``image_tables.h``. +""" + +import re +from pathlib import Path + +import pytest + +from webserver import image_config + +# parents[2] because this file sits at tests/pytest/, not tests/pytest/plugins/. +# It was moved out of plugins/ because .github/workflows/tests.yml passes +# --ignore=tests/pytest/plugins for pre-existing failures there, so a guard +# living in that directory would never fire in CI -- which is the one thing +# this test was written to be. +REPO_ROOT = Path(__file__).resolve().parents[2] +IMAGE_TABLES_H = REPO_ROOT / "core" / "src" / "plc_app" / "image_tables.h" +# The enum moved out of image_tables.h so plugin_types.h could reach it without +# pulling the runtime internals in (RTOP-284, B2). Publishing a type costs no +# ABI, and a plugin receiving an array indexed by it has to be able to name the +# entries -- the alternative being a second copy of the enum, which is the +# drift this whole file exists to catch. +IMAGE_TABLE_ID_H = REPO_ROOT / "core" / "src" / "plc_app" / "image_table_id.h" +IMAGE_TABLES_CPP = REPO_ROOT / "core" / "src" / "plc_app" / "image_tables.cpp" +JOURNAL_H = REPO_ROOT / "core" / "src" / "plc_app" / "journal_buffer.h" +JOURNAL_C = REPO_ROOT / "core" / "src" / "plc_app" / "journal_buffer.c" +S7COMM_C = REPO_ROOT / "core/src/drivers/plugins/native/s7comm/s7comm_plugin.cpp" +ETHERCAT_C = REPO_ROOT / "core/src/drivers/plugins/native/ethercat/ethercat_io.c" +MODBUS_PY = REPO_ROOT / "core/src/drivers/plugins/python/modbus_slave/simple_modbus.py" + + +def _enum_ids() -> list[str]: + """`image_table_id_t` members, in declaration order, lowercased.""" + body = re.search( + r"typedef enum\s*\{(.*?)\}\s*image_table_id_t", IMAGE_TABLE_ID_H.read_text(), re.DOTALL + ) + assert body, "image_table_id_t not found — has image_table_id.h been restructured?" + return [m.lower() for m in re.findall(r"IMAGE_TABLE_([A-Z_]+)", body.group(1)) if m != "COUNT"] + + +def _c_keys() -> list[str]: + """The strings `kImageTableKeys` maps those ids to, in order.""" + body = re.search( + r"kImageTableKeys\[IMAGE_TABLE_COUNT\] = \{(.*?)\};", IMAGE_TABLES_CPP.read_text(), re.DOTALL + ) + assert body, "kImageTableKeys not found — has the parser been restructured?" + return re.findall(r'"([a-z_]+)"', body.group(1)) + + +def _struct_fields() -> list[str]: + """The tables `image_tables_t` actually declares, in order. + + Matches the member NAME rather than any particular declarator, because the + tables have already changed shape once: they were `IEC_BYTE *x[N]` and + `IEC_BOOL *x[N][8]` inline arrays, and are now `IEC_BYTE **x` and + `IEC_BOOL *(*x)[8]` heap pointers. This test exists to catch a table being + added, removed or reordered, not to have an opinion on how it is spelled. + """ + body = re.search( + r"typedef struct\s*\{(.*?)\}\s*image_tables_t", IMAGE_TABLES_H.read_text(), re.DOTALL + ) + assert body, "image_tables_t not found — has the header been restructured?" + lines = [line for line in body.group(1).splitlines() if line.strip().startswith(("IEC_",))] + return [re.search(r"\*(\w+)\)?(?:\[\d+\])?;", line).group(1) for line in lines] + + +def _c_units() -> list[str]: + """The units `kImageTableUnits` pairs with those keys, in order.""" + body = re.search( + r"kImageTableUnits\[IMAGE_TABLE_COUNT\] = \{(.*?)\};", + IMAGE_TABLES_CPP.read_text(), + re.DOTALL, + ) + assert body, "kImageTableUnits not found — has the parser been restructured?" + return re.findall(r'"([a-z]+)"', body.group(1)) + + +def _python_plugin_order() -> list[str]: + """`IMAGE_TABLE_ORDER` in the shared Python module plugins import. + + The FOURTH copy of the order, and the one nothing pinned. It is what turns + the runtime's positional `sizes` array into the names every Python buffer + accessor uses, so a reorder of `image_table_id_t` would leave CI green + while every Python plugin silently read one table's length as another's. + Read as text rather than imported, like the C readers, because importing + the plugin package needs its virtualenv. + """ + src = (REPO_ROOT / "core/src/drivers/plugins/python/shared/image_sizes.py").read_text() + body = re.search(r"IMAGE_TABLE_ORDER: list\[str\] = \[(.*?)\]", src, re.DOTALL) + assert body, "IMAGE_TABLE_ORDER not found — has the module been restructured?" + return re.findall(r'"([a-z_]+)"', body.group(1)) + + +@pytest.mark.parametrize( + "name,reader", + [ + ("enum", _enum_ids), + ("key array", _c_keys), + ("struct", _struct_fields), + ("python plugin order", _python_plugin_order), + ], +) +def test_the_c_side_lists_agree_with_python_exactly(name, reader): + # Order matters as much as membership: the key array is indexed BY the enum, + # so a reordering of either one silently maps a table to another table's + # name. Nothing would fail; the sizes would just land in the wrong places. + assert reader() == list( + image_config.IMAGE_TABLE_KEYS + ), f"the C {name} and webserver/image_config.IMAGE_TABLE_KEYS have drifted" + + +def test_there_are_fourteen_tables(): + # Pinned as a number rather than derived, so adding a table to one side and + # not the others fails here rather than passing by agreeing with itself. + assert len(image_config.IMAGE_TABLE_KEYS) == 14 + + +def test_memory_has_no_byte_table(): + # Not an oversight to be tidied up: image_tables.h declares byte_input and + # byte_output but no byte_memory, so `%MB` has no storage on this runtime. + # The editor refuses such a declaration before the build, and the core's + # floor derivation counts it as unstorable and says so once. Anyone + # "fixing" this list would break that agreement. + keys = image_config.IMAGE_TABLE_KEYS + assert "byte_input" in keys and "byte_output" in keys + assert "byte_memory" not in keys + + +def test_the_c_and_python_units_agree_table_for_table(): + # THE UNIT IS THE HALF THAT COSTS A FACTOR OF EIGHT. A table whose unit + # disagrees across the implementations is not a parse error anywhere: the + # editor writes bits, a reader takes them for elements, and the image comes + # out eight times too small with every located address above the first + # eighth silently refused at bind time. + assert _c_units() == [image_config.IMAGE_TABLE_UNITS[k] for k in image_config.IMAGE_TABLE_KEYS] + + +def test_only_the_bool_tables_are_in_bits(): + # The three that are declared IEC_BOOL *[N][8]: their storage is in bytes + # while their addresses are in bits, which is the whole reason the file + # carries a unit at all. Every other table stores what it addresses. + in_bits = [k for k, u in image_config.IMAGE_TABLE_UNITS.items() if u == "bits"] + assert in_bits == ["bool_input", "bool_output", "bool_memory"] + + +def test_the_units_are_ones_both_sides_know(): + assert set(image_config.IMAGE_TABLE_UNITS.values()) == { + "bits", + "bytes", + "words", + "dwords", + "lwords", + } + + +def test_the_format_version_agrees_across_the_implementations(): + # The core refuses a file whose version it does not know, so a bump on one + # side and not the other stops every upload rather than mis-reading one. + header = (REPO_ROOT / "core" / "src" / "plc_app" / "image_tables.h").read_text() + match = re.search(r"#define IMAGE_CONF_FORMAT_VERSION (\d+)", header) + assert match, "IMAGE_CONF_FORMAT_VERSION not found in image_tables.h" + assert int(match.group(1)) == image_config.IMAGE_CONF_FORMAT_VERSION + + +def test_the_abi_limit_matches_the_index_width(): + # A located variable's table index is a uint16_t in strucpp_abi.hpp, which + # is where this number comes from. It is a fact of the ABI, not a policy + # ceiling, so it moves only if that field does. + assert image_config.MAX_TABLE_ELEMENTS == 1 << 16 + + +class TestJournalMapping: + """The journal's type enum and the image table enum are NOT the same order. + + Both have fourteen members and journal_buffer.h says this enum "matches the + OpenPLC image table types" -- it matches the concepts, not the indices. + The journal groups each width's memory table beside its input and output; + image_tables.h puts every memory table at the end. JOURNAL_INT_MEMORY is 7 + and IMAGE_TABLE_INT_MEMORY is 10. + + A cast between them therefore corrupts silently: a write lands under + another table's bounds, and an area is refused or admitted wrongly. The + runtime maps them explicitly (kJournalToImageTable); this checks that the + map is complete and that it is still needed. + """ + + @staticmethod + def _journal_ids() -> list[str]: + body = re.search( + r"typedef enum\s*\{(.*?)\}\s*journal_buffer_type_t", JOURNAL_H.read_text(), re.DOTALL + ) + assert body, "journal_buffer_type_t not found" + return [ + m.lower() for m in re.findall(r"JOURNAL_([A-Z_]+)", body.group(1)) if m != "TYPE_COUNT" + ] + + @staticmethod + def _mapping() -> dict[str, str]: + body = re.search( + r"kJournalToImageTable\[JOURNAL_TYPE_COUNT\] = \{(.*?)\};", + JOURNAL_C.read_text(), + re.DOTALL, + ) + assert body, "kJournalToImageTable not found — has the journal been restructured?" + return { + journal.lower(): image.lower() + for journal, image in re.findall( + r"\[JOURNAL_([A-Z_]+)\]\s*=\s*IMAGE_TABLE_([A-Z_]+)", body.group(1) + ) + } + + def test_every_journal_type_maps_to_a_table(self): + assert sorted(self._mapping()) == sorted(self._journal_ids()) + + def test_each_one_maps_to_the_table_of_the_same_name(self): + # The map is about ORDER, not renaming: JOURNAL_INT_MEMORY must reach + # IMAGE_TABLE_INT_MEMORY, whatever index either one sits at. + for journal, image in self._mapping().items(): + assert journal == image, f"JOURNAL_{journal.upper()} maps to the wrong table" + + def test_the_two_enums_really_do_disagree_on_order(self): + # If they were ever made identical the map could go -- but silently + # assuming they are identical is the bug. This fails if someone + # reorders one to match, which is the moment to revisit the map on + # purpose rather than discover it by corruption. + assert self._journal_ids() != list(image_config.IMAGE_TABLE_KEYS) + + def test_the_journal_covers_every_table_the_image_has(self): + assert sorted(self._journal_ids()) == sorted(image_config.IMAGE_TABLE_KEYS) + + +class TestTheOtherTableMappings: + """Three more name-to-name maps over the same fourteen tables. + + `kJournalToImageTable` is pinned by TestJournalMapping. These three are the + same shape and were pinned by nothing, which matters because Ceedling does + not run in CI -- nothing compile-checks the two C ones either. A single + wrong entry writes or reads under another table's bounds with no + diagnostic, which is the failure the whole enum-order finding was about. + """ + + @staticmethod + def _pairs(path, pattern) -> dict[str, str]: + return {a.lower(): b.lower() for a, b in re.findall(pattern, path.read_text())} + + def test_s7comm_maps_every_buffer_type_to_the_table_of_the_same_name(self): + pairs = self._pairs( + S7COMM_C, r"case BUFFER_TYPE_([A-Z_]+):\s*return IMAGE_TABLE_([A-Z_]+);" + ) + assert pairs, "s7_image_table not found — has the plugin been restructured?" + assert sorted(pairs) == sorted(image_config.IMAGE_TABLE_KEYS) + for buffer_type, table in pairs.items(): + assert ( + buffer_type == table + ), f"BUFFER_TYPE_{buffer_type.upper()} maps to the wrong table" + + def test_ethercat_maps_each_direction_and_width_to_the_right_pair(self): + src = ETHERCAT_C.read_text() + body = re.search(r"ecat_table_for\(.*?\n\}", src, re.DOTALL) + assert body, "ecat_table_for not found — has the plugin been restructured?" + + # EtherCAT only ever emits %I and %Q, so there is no memory case. + expected = { + "BIT": ("BOOL_INPUT", "BOOL_OUTPUT"), + "BYTE": ("BYTE_INPUT", "BYTE_OUTPUT"), + "WORD": ("INT_INPUT", "INT_OUTPUT"), + "DWORD": ("DINT_INPUT", "DINT_OUTPUT"), + "LWORD": ("LINT_INPUT", "LINT_OUTPUT"), + } + for size, (in_table, out_table) in expected.items(): + # The case label and its return sit on separate lines after + # clang-format, so the match has to span them. + arm = re.search( + rf"case IEC_SIZE_{size}:\s*return in \? IMAGE_TABLE_(\w+) : IMAGE_TABLE_(\w+);", + body.group(0), + ) + assert arm, f"IEC_SIZE_{size} is not mapped" + assert arm.group(1) == in_table, f"IEC_SIZE_{size} input side is wrong" + assert arm.group(2) == out_table, f"IEC_SIZE_{size} output side is wrong" + + def test_the_modbus_segments_name_the_tables_they_live_in(self): + body = re.search(r"SEGMENT_TABLES = \{(.*?)\}", MODBUS_PY.read_text(), re.DOTALL) + assert body, "SEGMENT_TABLES not found" + segments = dict(re.findall(r'"(\w+)":\s*"(\w+)"', body.group(1))) + + # All eight, not the three the behavioural tests happen to exercise. + assert segments == { + "qw_count": "int_output", + "mw_count": "int_memory", + "md_count": "dint_memory", + "ml_count": "lint_memory", + "qx_bits": "bool_output", + "mx_bits": "bool_memory", + "ix_bits": "bool_input", + "iw_count": "int_input", + } + for table in segments.values(): + assert table in image_config.IMAGE_TABLE_KEYS diff --git a/tests/pytest/test_modbus_exposure_fit.py b/tests/pytest/test_modbus_exposure_fit.py new file mode 100644 index 00000000..7ec5a0c5 --- /dev/null +++ b/tests/pytest/test_modbus_exposure_fit.py @@ -0,0 +1,311 @@ +"""How the Modbus slave fits its declared exposure to the image (RTOP-284). + +These sit in ``tests/pytest`` rather than ``tests/pytest/modbus_slave`` on +purpose: the workflow ignores that directory, so a test placed there would +never run. They exercise pure functions and need only the pymodbus that the +workflow already installs for collection. + +What they are protecting is a shrink that is invisible by construction. When +the exposure does not fit, the server answers Illegal Data Address for the +addresses it dropped -- correct, but indistinguishable over a network from a +server that was configured that way on purpose. The startup warning is the +only place the person who configured it can find out, and it has to fire for +every config shape, including the old-editor ones that are the reason the +shrink can happen at all. +""" + +import importlib.util +import pathlib + +import pytest + +PLUGIN = ( + pathlib.Path(__file__).resolve().parents[2] + / "core/src/drivers/plugins/python/modbus_slave/simple_modbus.py" +) + + +@pytest.fixture(scope="module") +def sm(): + spec = importlib.util.spec_from_file_location("simple_modbus_under_test", PLUGIN) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +@pytest.fixture +def warnings(sm, monkeypatch): + collected = [] + + class CollectingLogger: + def warn(self, message): + collected.append(message) + + def info(self, *args, **kwargs): + pass + + def error(self, *args, **kwargs): + pass + + def debug(self, *args, **kwargs): + pass + + monkeypatch.setattr(sm, "logger", CollectingLogger()) + return collected + + +def fit_and_report(sm, config, buffer_size): + parsed = sm.parse_buffer_mapping_config(config, buffer_size) + sm._log_clamped_segments(config, parsed) + return parsed + + +def segmented(**counts): + return { + "buffer_mapping": { + "holding_registers": { + "qw_count": counts.get("qw", 0), + "mw_count": counts.get("mw", 0), + "md_count": counts.get("md", 0), + "ml_count": counts.get("ml", 0), + }, + "coils": {"qx_bits": counts.get("qx", 0), "mx_bits": counts.get("mx", 0)}, + "discrete_inputs": {"ix_bits": counts.get("ix", 0)}, + "input_registers": {"iw_count": counts.get("iw", 0)}, + } + } + + +# --- the exposure is fitted to the image --------------------------------- + + +def test_segment_larger_than_the_image_is_cut_to_it(sm): + parsed = sm.parse_buffer_mapping_config(segmented(qw=1024), 8) + assert parsed["holding_registers"]["qw_count"] == 8 + + +def test_bit_segments_get_eight_slots_per_image_element(sm): + parsed = sm.parse_buffer_mapping_config(segmented(qx=8192), 8) + assert parsed["coils"]["qx_bits"] == 64 + + +def test_an_exposure_the_image_holds_is_left_alone(sm, warnings): + parsed = fit_and_report(sm, segmented(qw=10, qx=16, ix=16, iw=10), 1024) + assert parsed["holding_registers"]["qw_count"] == 10 + assert parsed["coils"]["qx_bits"] == 16 + assert warnings == [] + + +# --- the warning covers every config shape, not just the segmented one ---- + + +def test_the_legacy_shape_is_reported_when_it_shrinks(sm, warnings): + # max_coils and friends: what an editor that predates image.conf uploads, + # and the shape that used to shrink without a word. + legacy = {"buffer_mapping": {"max_coils": 8192, "max_holding_registers": 1024}} + parsed = fit_and_report(sm, legacy, 8) + + assert parsed["holding_registers"]["qw_count"] == 8 + assert parsed["coils"]["qx_bits"] == 64 + assert len(warnings) == 1 + assert "qw_count 1024 -> 8" in warnings[0] + assert "qx_bits 8192 -> 64" in warnings[0] + + +def test_a_config_without_buffer_mapping_is_reported_when_it_shrinks(sm, warnings): + # The defaults it falls back to are clamped exactly like a declared count, + # so they are what was asked for. + fit_and_report(sm, {"network_configuration": {"host": "0.0.0.0", "port": 502}}, 8) + + assert len(warnings) == 1 + assert "qw_count 1024 -> 8" in warnings[0] + assert "ix_bits 8192 -> 64" in warnings[0] + + +def test_an_omitted_segment_of_a_segmented_config_is_reported(sm, warnings): + partial = {"buffer_mapping": {"holding_registers": {"qw_count": 4}}} + fit_and_report(sm, partial, 8) + + # qw_count fits; the segments the config never mentioned take the + # generator's defaults and those are what shrink. + assert len(warnings) == 1 + assert "qw_count" not in warnings[0] + assert "mw_count 1024 -> 8" in warnings[0] + + +# --- the composed block fits one Modbus table ----------------------------- + + +def test_the_register_block_never_exceeds_one_table(sm): + ceiling = sm.MODBUS_MAX_ADDRESSES + parsed = sm.parse_buffer_mapping_config( + segmented(qw=ceiling, mw=ceiling, md=ceiling, ml=ceiling), ceiling + ) + hr = parsed["holding_registers"] + + # The block lays the segments out as %QW | %MW | %MD | %ML, at two + # registers per %MD and four per %ML. + composed = hr["qw_count"] + hr["mw_count"] + hr["md_count"] * 2 + hr["ml_count"] * 4 + assert composed == ceiling + + +def test_the_coil_block_never_exceeds_one_table(sm): + ceiling = sm.MODBUS_MAX_ADDRESSES + parsed = sm.parse_buffer_mapping_config( + segmented(qx=ceiling * sm.MAX_BITS, mx=ceiling * sm.MAX_BITS), ceiling + ) + coils = parsed["coils"] + + assert coils["qx_bits"] + coils["mx_bits"] == ceiling + + +def test_trimming_takes_from_the_tail_so_earlier_segments_keep_their_addresses(sm): + ceiling = sm.MODBUS_MAX_ADDRESSES + parsed = sm.parse_buffer_mapping_config( + segmented(qw=ceiling, mw=ceiling, md=ceiling, ml=ceiling), ceiling + ) + hr = parsed["holding_registers"] + + # %QW starts at address 0 and is the segment a client is most likely to + # already be reading, so it is the last one to lose anything. + assert hr["qw_count"] == ceiling + assert hr["ml_count"] == 0 + + +def test_a_block_that_already_fits_is_not_trimmed(sm): + parsed = sm.parse_buffer_mapping_config(segmented(qw=100, mw=100, md=100, ml=100), 1024) + hr = parsed["holding_registers"] + + assert (hr["qw_count"], hr["mw_count"], hr["md_count"], hr["ml_count"]) == (100, 100, 100, 100) + + +def test_the_pdu_trim_is_reported_too(sm, warnings): + ceiling = sm.MODBUS_MAX_ADDRESSES + fit_and_report(sm, segmented(qw=ceiling, mw=ceiling, md=ceiling, ml=ceiling), ceiling) + + # The image held every segment; the address space did not. Same warning, + # because from the configurer's side it is the same surprise. + assert len(warnings) == 1 + assert "ml_count" in warnings[0] + + +# --- a malformed config must not take the server down --------------------- + + +@pytest.mark.parametrize("junk", ["muitos", None, -5, True, {"nested": 1}]) +def test_a_count_that_is_not_a_count_falls_back_to_the_default(sm, junk): + config = {"buffer_mapping": {"holding_registers": {"qw_count": junk}}} + parsed = sm.parse_buffer_mapping_config(config, 4096) + + assert parsed["holding_registers"]["qw_count"] == sm.DEFAULT_HOLDING_REG_CONFIG["qw_count"] + + +def test_a_buffer_mapping_that_is_not_an_object_is_treated_as_absent(sm): + parsed = sm.parse_buffer_mapping_config({"buffer_mapping": []}, 4096) + + assert parsed["format"] == "legacy" + assert parsed["holding_registers"]["qw_count"] == 1024 + + +def test_a_section_that_is_not_an_object_falls_back_to_defaults(sm): + config = {"buffer_mapping": {"holding_registers": {"qw_count": 4}, "coils": "nope"}} + parsed = sm.parse_buffer_mapping_config(config, 4096) + + assert parsed["holding_registers"]["qw_count"] == 4 + assert parsed["coils"]["qx_bits"] == sm.DEFAULT_COILS_CONFIG["qx_bits"] + + +# --- the shape the config declares is preserved --------------------------- + + +def test_the_segmented_shape_keeps_its_word_order(sm): + config = segmented(qw=4) + config["word_order"] = "low_word_first" + assert sm.parse_buffer_mapping_config(config, 1024)["word_order"] == "low_word_first" + + +def test_the_legacy_shape_has_no_memory_segments(sm): + legacy = {"buffer_mapping": {"max_holding_registers": 16, "max_coils": 16}} + parsed = sm.parse_buffer_mapping_config(legacy, 1024) + + assert parsed["format"] == "legacy" + assert parsed["holding_registers"]["mw_count"] == 0 + assert parsed["coils"]["mx_bits"] == 0 + assert parsed["word_order"] == "high_word_first" + + +# --- per-table clamping (RTOP-284, C1.3/C1.4) ---------------------------- + + +@pytest.fixture +def per_table(sm): + """Deliver per-table sizes the way the runtime does, then clear them.""" + from shared import image_sizes + + def deliver(**by_name): + sizes = [by_name.get(name, 0) for name in image_sizes.IMAGE_TABLE_ORDER] + assert image_sizes.set_image_sizes(sizes) == 0 + + yield deliver + image_sizes.set_image_sizes([]) + + +def test_each_segment_is_clamped_against_its_own_table(sm, per_table): + # %QW comes out of int_output and %MW out of int_memory. One figure for + # both would have to be the smaller, losing the larger table's range. + per_table(int_output=100, int_memory=4) + parsed = sm.parse_buffer_mapping_config(segmented(qw=100, mw=100), 8) + assert parsed["holding_registers"]["qw_count"] == 100 + assert parsed["holding_registers"]["mw_count"] == 4 + + +def test_a_bit_segment_reads_its_table_in_bits(sm, per_table): + # bool_output is in elements of eight; %QX addresses the bits. + per_table(bool_output=2) + parsed = sm.parse_buffer_mapping_config(segmented(qx=8192), 8) + assert parsed["coils"]["qx_bits"] == 16 + + +def test_an_empty_table_exposes_nothing(sm, per_table): + per_table(int_output=0, int_memory=50) + parsed = sm.parse_buffer_mapping_config(segmented(qw=10, mw=10), 8) + assert parsed["holding_registers"]["qw_count"] == 0 + assert parsed["holding_registers"]["mw_count"] == 10 + + +def test_a_square_run_falls_back_to_the_single_figure(sm): + # No sizes delivered: buffer_size IS the length every table has, so it is + # the right answer rather than a guess. + from shared import image_sizes + + image_sizes.set_image_sizes([]) + parsed = sm.parse_buffer_mapping_config(segmented(qw=100, mw=100), 8) + assert parsed["holding_registers"]["qw_count"] == 8 + assert parsed["holding_registers"]["mw_count"] == 8 + + +def test_the_module_exports_the_symbol_the_runtime_looks_for(sm): + # PyObject_GetAttrString(pModule, "set_image_sizes") has to find it HERE, + # in this module's namespace -- importing it is what declares the + # capability, and without it every run this plugin is in stays square. + assert callable(getattr(sm, "set_image_sizes", None)) + + +def test_a_failed_delivery_leaves_no_half_filled_map(sm): + """A mid-list failure must not answer for the tables before it. + + `_sizes` is process-global: the module is imported once per interpreter and + every Python plugin in that process shares it. A plugin being torn down on + a bad delivery could otherwise leave a partial map behind for plugins that + keep running -- answering for the tables it parsed and falling back to + buffer_size for the rest. + """ + from shared import image_sizes + + assert image_sizes.set_image_sizes([10] * 14) == 0 + assert image_sizes.sizes_known() + + # Fails on the third entry, after two were parsed. + assert image_sizes.set_image_sizes([1, 2, "nao-e-numero", 4]) == -1 + assert not image_sizes.sizes_known() + assert image_sizes.table_capacity("bool_input", 99) == 99 diff --git a/tests/pytest/test_plugins_declare_image_sizes.py b/tests/pytest/test_plugins_declare_image_sizes.py new file mode 100644 index 00000000..a1c41ac0 --- /dev/null +++ b/tests/pytest/test_plugins_declare_image_sizes.py @@ -0,0 +1,81 @@ +"""Every plugin the runtime ships has to declare per-table image sizes. + +The runtime keeps the image SQUARE for any run in which even one loaded plugin +lacks `set_image_sizes` -- correctly, because a plugin bounding a byte index +and a word index with one `buffer_size` is only right while the tables are +equal. The consequence is that ONE plugin without it disables the feature for +the whole device. + +That is exactly what shipped: `simple_modbus.py` had it and +`modbus_master_plugin.py` and `opcua/plugin.py` did not, so per-table sizing +never activated on a stock `plugins_default.conf` and every fix beneath it ran +in the degenerate case where it could not differ from the old behaviour. + +Nothing failed. The image was simply square, which is also what a correct +square run looks like. This test is the only thing that tells the two apart. +""" + +import re +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +PLUGINS_CONF = REPO_ROOT / "plugins_default.conf" +PYTHON_PLUGINS = REPO_ROOT / "core/src/drivers/plugins/python" + + +def shipped_entries() -> list[tuple[str, str]]: + """`(name, path)` for every plugin the default config loads.""" + out = [] + for raw in PLUGINS_CONF.read_text().splitlines(): + line = raw.strip() + if not line or line.startswith("#"): + continue + fields = line.split(",") + if len(fields) >= 2: + out.append((fields[0].strip(), fields[1].strip())) + return out + + +def test_the_default_config_is_readable(): + # A guard on the guard: a rename or a move that makes the parse return + # nothing would otherwise turn this whole suite into a silent pass. + assert len(shipped_entries()) >= 5 + + +@pytest.mark.parametrize( + "name,path", + [(n, p) for n, p in shipped_entries() if p.endswith(".py")], +) +def test_every_shipped_python_plugin_declares_set_image_sizes(name, path): + source = (REPO_ROOT / path.lstrip("./")).read_text() + # The NAME has to be in this module's namespace, which is where + # PyObject_GetAttrString looks. Importing it from shared.image_sizes is how + # that is done; defining it directly would also work. + assert re.search( + r"^\s*(from .*import .*\bset_image_sizes\b|def set_image_sizes\b)", source, re.MULTILINE + ), ( + f"plugin '{name}' ({path}) does not declare set_image_sizes, so every run " + f"it is loaded in keeps the image square" + ) + + +def test_every_shipped_native_plugin_links_the_helper(): + # The native side declares it by linking plugin_image_sizes.c, which + # defines and exports the symbol. Checked through the build files, because + # the .so is not in the tree. + native = [p for _, p in shipped_entries() if p.endswith(".so")] + assert native, "no native plugin in the default config — has it been restructured?" + + for cmake in (REPO_ROOT / "core/src/drivers/plugins/native").glob("*/CMakeLists.txt"): + assert "plugin_image_sizes.c" in cmake.read_text(), ( + f"{cmake.parent.name} does not link plugin_image_sizes.c, so it exports no " + f"set_image_sizes and every run it is loaded in keeps the image square" + ) + + +def test_the_shared_module_is_what_they_import(): + # One implementation rather than one per plugin: three copies of a + # fourteen-element cache is three places for the indexing to drift. + assert (PYTHON_PLUGINS / "shared/image_sizes.py").exists() diff --git a/tests/support/debug_handler_mocks.c b/tests/support/debug_handler_mocks.c index 23d5b042..8dc867c9 100644 --- a/tests/support/debug_handler_mocks.c +++ b/tests/support/debug_handler_mocks.c @@ -45,9 +45,31 @@ uint16_t (*ext_strucpp_debug_read) (uint8_t, uint16_t, uint8_t *) = NULL; uint8_t (*ext_strucpp_debug_write) (uint8_t, uint16_t, const uint8_t *, uint16_t) = NULL; -/* ext_strucpp_program_md5 lives in utils.c; just bring the declaration - * in via the public header. */ -extern char *ext_strucpp_program_md5; +/* ext_strucpp_program_md5, scan_counter and runtime_external_write are DEFINED + * here, not just declared. + * + * The first two live in utils.c and the third in debug_write_journal.cpp, and + * no test target compiles any of those files -- so every link of a target that + * pulls in debug_handler.c failed on undefined references. The whole Ceedling + * suite has been unbuildable on development for that reason; the suite does + * not run in CI, so nothing said so. + * + * Defining them in the mock is the right side to fix: this file exists to + * stand in for the runtime's globals, and no target links both it and utils.c. + * The write stub accepts and drops -- nothing here asserts on the + * external-write queue. */ +char *ext_strucpp_program_md5 = NULL; +unsigned long scan_counter = 0; + +int runtime_external_write(uint8_t arr, uint16_t elem, uint8_t op, const uint8_t *bytes, uint16_t len) +{ + (void)arr; + (void)elem; + (void)op; + (void)bytes; + (void)len; + return 0; +} /* ----------------------------------------------------------------------- * State backing the fakes. Reset between tests. diff --git a/tests/support/ethercat_stubs.c b/tests/support/ethercat_stubs.c index dc2f8d0f..528259e1 100644 --- a/tests/support/ethercat_stubs.c +++ b/tests/support/ethercat_stubs.c @@ -11,6 +11,7 @@ #include "ethercat_config.h" #include "ethercat_master.h" +#include "plugin_image_sizes.h" #include "plugin_logger.h" #include @@ -69,3 +70,36 @@ __attribute__((weak)) int ecat_master_get_slave_count(ecat_master_instance_t *in (void)inst; return 0; } + +/* ---- ecat_data_type_to_string: real one lives in ethercat_data_types.c ---- + * + * ethercat_io.c logs the type name on three paths and no test target compiles + * that file, so every link of ethercat_io.c failed on it. Pre-existing: the + * same undefined reference happens on development. Only reached by log lines, + * so a fixed string keeps the tests honest without pulling the table in. */ +__attribute__((weak)) const char *ecat_data_type_to_string(ecat_data_type_t dt) +{ + (void)dt; + return ""; +} + +/* ---- plugin_image_sizes: the shared helper the runtime hands the plugins ---- + * + * `ecat_io_build_channel_map` asks these two how far a table reaches + * (RTOP-284). The real definitions are in plugin_image_sizes.c, which no test + * target compiles -- the EtherCAT tests link against stubs rather than the + * plugin's own sources, which is what this whole file is for. + * + * NOT KNOWN, so the caller takes its `buffer_size` fallback: that is the shape + * a run has when no runtime ever delivered the per-table sizes, which is the + * conservative side and the one these tests were written against. */ +__attribute__((weak)) int plugin_image_sizes_known(void) +{ + return 0; +} + +__attribute__((weak)) uint32_t plugin_image_table_capacity(image_table_id_t id) +{ + (void)id; + return 0; +} diff --git a/tests/support/plugin_driver_stubs.c b/tests/support/plugin_driver_stubs.c index fbc902d6..9d7fa0e1 100644 --- a/tests/support/plugin_driver_stubs.c +++ b/tests/support/plugin_driver_stubs.c @@ -1,11 +1,13 @@ +#include "image_tables.h" +#include "journal_buffer.h" #include "plugin_config.h" #include "plugin_driver.h" -#include "journal_buffer.h" #include #include #include #include +#include // Stub: base_tick_ns (utils.c) -- the runtime stores the PLC scan tick // interval here (GCD of declared task intervals). Plugin drivers @@ -13,21 +15,87 @@ // stub value is enough for the unit tests. uint64_t base_tick_ns = 0; -// Stub implementations for external buffer variables (image_tables.c) -IEC_BOOL *bool_input[BUFFER_SIZE][8]; -IEC_BOOL *bool_output[BUFFER_SIZE][8]; -IEC_BYTE *byte_input[BUFFER_SIZE]; -IEC_BYTE *byte_output[BUFFER_SIZE]; -IEC_UINT *int_input[BUFFER_SIZE]; -IEC_UINT *int_output[BUFFER_SIZE]; -IEC_UDINT *dint_input[BUFFER_SIZE]; -IEC_UDINT *dint_output[BUFFER_SIZE]; -IEC_ULINT *lint_input[BUFFER_SIZE]; -IEC_ULINT *lint_output[BUFFER_SIZE]; -IEC_UINT *int_memory[BUFFER_SIZE]; -IEC_UDINT *dint_memory[BUFFER_SIZE]; -IEC_ULINT *lint_memory[BUFFER_SIZE]; -IEC_BOOL *bool_memory[BUFFER_SIZE][8]; +// Stub storage for the image tables (defined in image_tables.cpp in the real +// build). One symbol rather than fourteen, and it takes its shape from +// image_tables.h -- which is the point: the stub used to spell the fourteen +// arrays out by hand at BUFFER_SIZE=128 (project.yml) while plugin_driver.c +// saw them at 1024, a disagreement the linker was happy to accept. +// +// The tables are heap pointers in the real build, allocated per program load. +// Here they point at fixed arrays of STUB_IMAGE_ELEMENTS, which is all the +// plugin_driver tests need: they check that the runtime args are populated, +// not that the image is the right size. +image_tables_t g_image; + +#define STUB_IMAGE_ELEMENTS 128 + +static IEC_BOOL *stub_bool_input[STUB_IMAGE_ELEMENTS][8]; +static IEC_BOOL *stub_bool_output[STUB_IMAGE_ELEMENTS][8]; +static IEC_BOOL *stub_bool_memory[STUB_IMAGE_ELEMENTS][8]; +static IEC_BYTE *stub_byte_input[STUB_IMAGE_ELEMENTS]; +static IEC_BYTE *stub_byte_output[STUB_IMAGE_ELEMENTS]; +static IEC_UINT *stub_int_input[STUB_IMAGE_ELEMENTS]; +static IEC_UINT *stub_int_output[STUB_IMAGE_ELEMENTS]; +static IEC_UDINT *stub_dint_input[STUB_IMAGE_ELEMENTS]; +static IEC_UDINT *stub_dint_output[STUB_IMAGE_ELEMENTS]; +static IEC_ULINT *stub_lint_input[STUB_IMAGE_ELEMENTS]; +static IEC_ULINT *stub_lint_output[STUB_IMAGE_ELEMENTS]; +static IEC_UINT *stub_int_memory[STUB_IMAGE_ELEMENTS]; +static IEC_UDINT *stub_dint_memory[STUB_IMAGE_ELEMENTS]; +static IEC_ULINT *stub_lint_memory[STUB_IMAGE_ELEMENTS]; + +// Stub: image_tables_alloc (image_tables.cpp). Points the tables at the fixed +// storage above and ignores the requested sizes -- there is no allocator here +// to exercise. plugin_driver.c refuses to build runtime args while the +// capacity is zero, which is the ordering invariant it now enforces, so a test +// that wants args has to call this first exactly as the real load path does. +// +// TAKES THE SIZES STRUCT, not a single count (RTOP-284). The signature moved +// when the fourteen tables stopped sharing a length, and this stub kept the +// old one -- so the C unit tests did not compile at all on this branch. +// Nothing caught it because the Ceedling suite does not run in CI, and it +// would not even configure until the SOEM submodule and its generated +// ec_options.h were in place. Running it by hand is what surfaced this. +bool image_tables_alloc(const image_sizes_t *sizes) +{ + (void)sizes; + g_image.bool_input = stub_bool_input; + g_image.bool_output = stub_bool_output; + g_image.bool_memory = stub_bool_memory; + g_image.byte_input = stub_byte_input; + g_image.byte_output = stub_byte_output; + g_image.int_input = stub_int_input; + g_image.int_output = stub_int_output; + g_image.dint_input = stub_dint_input; + g_image.dint_output = stub_dint_output; + g_image.lint_input = stub_lint_input; + g_image.lint_output = stub_lint_output; + g_image.int_memory = stub_int_memory; + g_image.dint_memory = stub_dint_memory; + g_image.lint_memory = stub_lint_memory; + return true; +} + +void image_tables_free(void) +{ + memset(&g_image, 0, sizeof(g_image)); +} + +uint32_t image_tables_capacity(void) +{ + return g_image.byte_input ? STUB_IMAGE_ELEMENTS : 0u; +} + +// Stub: image_table_capacity (image_tables.cpp). Every stub table is the same +// fixed length, so the per-table answer is the same as the single-number one. +// That is a property of the stub and not of the runtime, where the whole point +// is that the fourteen differ -- a test about per-table lengths belongs against +// the real allocator, not here. +uint32_t image_table_capacity(image_table_id_t id) +{ + (void)id; + return g_image.byte_input ? STUB_IMAGE_ELEMENTS : 0u; +} // Stub: plugin_manager_destroy (plcapp_manager.c) void plugin_manager_destroy(PluginManager *manager) @@ -43,8 +111,7 @@ __attribute__((weak)) int init_rt_mutex(pthread_mutex_t *mutex) } // Stub: journal_write_* (journal_buffer.c) -int journal_write_bool(journal_buffer_type_t type, uint16_t index, - uint8_t bit, bool value) +int journal_write_bool(journal_buffer_type_t type, uint16_t index, uint8_t bit, bool value) { (void)type; (void)index; @@ -53,8 +120,7 @@ int journal_write_bool(journal_buffer_type_t type, uint16_t index, return 0; } -int journal_write_byte(journal_buffer_type_t type, uint16_t index, - uint8_t value) +int journal_write_byte(journal_buffer_type_t type, uint16_t index, uint8_t value) { (void)type; (void)index; @@ -62,8 +128,7 @@ int journal_write_byte(journal_buffer_type_t type, uint16_t index, return 0; } -int journal_write_int(journal_buffer_type_t type, uint16_t index, - uint16_t value) +int journal_write_int(journal_buffer_type_t type, uint16_t index, uint16_t value) { (void)type; (void)index; @@ -71,8 +136,7 @@ int journal_write_int(journal_buffer_type_t type, uint16_t index, return 0; } -int journal_write_dint(journal_buffer_type_t type, uint16_t index, - uint32_t value) +int journal_write_dint(journal_buffer_type_t type, uint16_t index, uint32_t value) { (void)type; (void)index; @@ -80,8 +144,7 @@ int journal_write_dint(journal_buffer_type_t type, uint16_t index, return 0; } -int journal_write_lint(journal_buffer_type_t type, uint16_t index, - uint64_t value) +int journal_write_lint(journal_buffer_type_t type, uint16_t index, uint64_t value) { (void)type; (void)index; @@ -101,7 +164,7 @@ int journal_write_lint(journal_buffer_type_t type, uint16_t index, // real lock lives in plc_state_manager.cpp; tests don't pull that .cpp in, // so we provide no-op stubs. Tests that exercise the lifecycle (rather // than just the per-tracker math) will need to link the real symbols. -void plc_tasks_reader_lock(void) {} +void plc_tasks_reader_lock(void) {} void plc_tasks_reader_unlock(void) {} // Stub: log_* (log.c) @@ -123,4 +186,4 @@ void log_warn(const char *fmt, ...) void log_error(const char *fmt, ...) { (void)fmt; -} \ No newline at end of file +} diff --git a/webserver/app.py b/webserver/app.py index 13782175..7417580e 100644 --- a/webserver/app.py +++ b/webserver/app.py @@ -33,6 +33,7 @@ MAX_FILE_SIZE, BuildStatus, analyze_zip, + apply_image_conf, apply_retain_conf, apply_vpp_plugin_conf, build_state, @@ -398,6 +399,16 @@ def handle_upload_file(data: dict) -> dict: # route still reaches the right answer. apply_retain_conf(extract_dir) + # I/O image sizes, same route and the same present/absent handling -- + # but for a different reason, which is worth not conflating. A missing + # retain.conf above is an instruction ("switch the store off"); a + # missing image.conf says nothing, because the runtime can always size + # the image from the located variables of the program it just loaded. + # The device's copy is removed anyway, because a STALE one is worse than + # none: the previous project's sizes would otherwise keep memory + # reserved for a program that is no longer here. + apply_image_conf(extract_dir) + # Update built-in plugin configurations based on extracted config files update_plugin_configurations(extract_dir) diff --git a/webserver/image_config.py b/webserver/image_config.py new file mode 100644 index 00000000..d2fff269 --- /dev/null +++ b/webserver/image_config.py @@ -0,0 +1,303 @@ +"""I/O image sizes for the program now installed. + +WHO OWNS THESE SIZES +-------------------- +The PROJECT does, and nobody chooses them. They are DERIVED by the editor from +what the project actually contains -- the addresses its producers claim and the +located variables it declares -- and emitted as ``image.conf`` into the program +upload, the same way ``retain.conf`` and the VPP plugin configuration travel. +The webserver's only job is to install what arrives (``apply_image_conf`` in +``plcapp_management``) and to refuse a stanza the core could not honour. There +is no endpoint to change them on a running device: a device sized out of band +would disagree with the program running on it, and the program is the thing a +user can see. + +Before this existed the image was ``BUFFER_SIZE 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, and a project needing +less paid for the rest anyway, out of the memory its own program wanted +(RTOP-284, editor side DOPE-615). + +Flat ``key=value`` rather than JSON, for the reason recorded for +``retain.conf``: the core parses it in C++ during startup, before any plugin +exists, and a dependency-free parser for fourteen integers is a better trade +there than pulling a JSON library into the PLC application. + +WHAT AN ABSENT FILE MEANS, AND WHY IT IS NOT THE SAME AS RETAIN'S +----------------------------------------------------------------- +For ``retain.conf``, absence is an instruction: switch the built-in store off. +Here absence carries no instruction at all -- it means "nothing to tell you, +size the image from the program you just loaded", which the runtime can always +do by walking the located variables in the ``.so``. + +That distinction matters for what ``apply_image_conf`` does, and the answer is +NOT "leave the device's copy alone". A stale copy is worse than no copy: a +project needing 4096 output words leaves ``int_output=4096`` behind, the next +program needs eight, and ``max(configured, derived)`` keeps 4096 words reserved +for a program that is no longer on the device -- silently, and for as long as +nobody notices. So an upload without the file REMOVES the device's copy, and +the runtime falls back to what the program itself requires. + +THE UNIT IS EACH TABLE'S OWN, WHICH IS NOT ALWAYS THE ADDRESS'S +-------------------------------------------------------------- +Every value is a count of that table's elements, matching +``core/src/plc_app/image_tables.h``. For eleven tables that is the number of +addresses (``int_memory[N]`` holds N ``%MW``s). For the three BOOL tables it is +not: they are declared ``IEC_BOOL *table[N][8]``, so N counts BYTES while +``%QX`` addresses bits. The editor does that conversion once, on its side, and +what arrives here is already in table elements. Nothing in this module divides +or multiplies by eight, deliberately -- a second conversion is how the two +sides end up disagreeing by a factor of eight with no diagnostic anywhere. +""" + +from __future__ import annotations + +import os +from pathlib import Path + +from webserver.logger import get_logger + +# The runtime's working directory (systemd `WorkingDirectory=$OPENPLC_DIR`), so +# image.conf lands beside retain.conf where the core looks for it. +logger, _ = get_logger("runtime", use_buffer=True) + +RUNTIME_ROOT = Path(os.path.abspath(os.path.dirname(__file__))).parent +IMAGE_CONF_PATH = RUNTIME_ROOT / "image.conf" + +# The fourteen tables of core/src/plc_app/image_tables.h, in the order it +# declares them. Fixed order so the installed file is byte-stable for the same +# program, which the editor also guarantees on its side. +# +# Note the gap this list makes visible: there is byte_input and byte_output but +# no byte_memory, so `%MB` has no storage on this runtime at all. +IMAGE_TABLE_UNITS = { + "bool_input": "bits", + "bool_output": "bits", + "byte_input": "bytes", + "byte_output": "bytes", + "int_input": "words", + "int_output": "words", + "dint_input": "dwords", + "dint_output": "dwords", + "lint_input": "lwords", + "lint_output": "lwords", + "int_memory": "words", + "dint_memory": "dwords", + "lint_memory": "lwords", + "bool_memory": "bits", +} +IMAGE_TABLE_KEYS = tuple(IMAGE_TABLE_UNITS) + +# The wire format this runtime reads. A file declaring any other version, or +# none, is refused whole rather than read by today's rules: guessing is how a +# unit change becomes a silent factor of eight. There is no version 1 to be +# compatible with -- it was written but never merged, so no device has ever +# read this file in that form. +IMAGE_CONF_FORMAT_VERSION = 2 + +# A located variable's table index is carried as a uint16_t in the STruC++ ABI +# (`LocatedVar.byte_index`, core/src/lib/strucpp_abi.hpp), so no table can be +# addressed beyond this many elements. +# +# NOT a policy ceiling -- the demand has none, and the limit on image size is +# the memory actually available, which the allocation itself discovers. This is +# a fact of the ABI: roughly 65 times the old fixed value, not expected to bind, +# and refused here rather than silently truncated at bind time. +MAX_TABLE_ELEMENTS = 65536 + + +def max_in_file_unit(key: str) -> int: + """The ceiling for one table, expressed in the unit its file value uses. + + THE CEILING IS IN ELEMENTS AND THE FILE IS NOT, which is exactly the kind + of mismatch the unit word exists to prevent -- so it must not be + reintroduced here. A BOOL table's value is in bits and its storage is + ``IEC_BOOL *[N][8]``, so 65536 elements is 524288 bits. Comparing the raw + bit count against the element ceiling would refuse every legal image above + 8192 bytes, eight times early. + """ + return MAX_TABLE_ELEMENTS * 8 if IMAGE_TABLE_UNITS[key] == "bits" else MAX_TABLE_ELEMENTS + + +class ImageConfigError(ValueError): + """Raised for a size the runtime would not be able to honour.""" + + +def read_image_conf_file( + path: str | os.PathLike, +) -> tuple[int, dict[str, int | None], dict[str, str]]: + """Parse an ``image.conf``, with every unset table read as zero. + + Takes a path rather than assuming the runtime root, because the file worth + checking is the one that just arrived in the upload -- validating the + installed copy would be validating it after the point where a refusal could + still help. + + ZERO IS A REAL ANSWER, not a missing one. A program with no ``%QX`` has no + reason to carry a ``bool_output`` image, and the memory it does not reserve + goes back to the program. So an absent key and ``key=0`` mean the same + thing, and a missing file yields all zeros -- which the caller reads as + "size this from the program instead". + + Unknown keys are ignored rather than refused: a newer editor emitting a + table this runtime does not have must not fail the upload, and the core + would ignore it anyway. + """ + sizes = {key: 0 for key in IMAGE_TABLE_KEYS} + units = {key: IMAGE_TABLE_UNITS[key] for key in IMAGE_TABLE_KEYS} + version = 0 + try: + with open(path, "r", encoding="utf-8") as handle: + for raw in handle: + line = raw.strip() + if not line or line.startswith("#") or "=" not in line: + continue + key, _, value = line.partition("=") + key, value = key.strip(), value.strip() + if key == "format_version": + try: + version = int(value) + except ValueError: + version = -1 + continue + if key not in sizes: + continue + # " ": the unit is checked in validation, where a + # wrong one produces the same clear refusal as an out-of-range + # count. A missing unit reads as an empty string and fails + # there rather than being guessed at here. + count, _, unit = value.partition(" ") + units[key] = unit.strip() + try: + sizes[key] = int(count) + except ValueError: + # None, not -1, and the difference is the message. A -1 fell + # into the "cannot be negative" branch below, so `4.5 words` + # was refused with "int_output cannot be negative (got -1)" + # and the reader went looking for a minus sign that is not + # there. int(None) raises TypeError, which reaches the + # handler that says what actually happened -- and refusing + # it here, where someone is watching the build log, is this + # module's whole reason for existing. + sizes[key] = None + except FileNotFoundError: + pass + except (UnicodeDecodeError, IsADirectoryError, PermissionError, OSError) as exc: + # The file arrives from an upload, so its bytes are attacker-shaped in + # the ordinary sense: a non-UTF-8 image.conf raises UnicodeDecodeError + # and a directory entry of that name raises IsADirectoryError. Both used + # to escape to app.py, which answers `Unexpected error: {e}` to the + # client. Unreadable means "no sizes delivered", which the caller + # already knows how to handle -- it refuses the stanza and falls back to + # the floor derived from the program. + logger.warning("Image: could not read %s (%s); treating as no sizes", path, exc) + return -1, {key: None for key in IMAGE_TABLE_KEYS}, units + return version, sizes, units + + +def validate_table_count(key: str, value: object, unit: object) -> int: + """Check one table's count, in the unit its own addresses use. + + Refused at INSTALL, with a line in the build log the user is already + watching, for the same reason the retain settings are: a size the core + cannot honour would otherwise be discovered at bind time, per located + variable, with nothing but a log line on a device nobody is looking at. + + The unit is checked too, and that is not pedantry. A ``bool_output`` value + written in bytes rather than bits is a perfectly plausible number that + allocates an image eight times too small, with no diagnostic on either + side. Refusing it here is the only place a person sees it. + """ + expected = IMAGE_TABLE_UNITS[key] + if unit != expected: + got = unit if unit else "no unit" + raise ImageConfigError(f"{key} must be given in {expected} (got {got}).") + try: + count = int(value) + except (TypeError, ValueError) as exc: + raise ImageConfigError(f"{key} must be a whole number of {expected}.") from exc + if count < 0: + raise ImageConfigError(f"{key} cannot be negative (got {count}).") + ceiling = max_in_file_unit(key) + if count > ceiling: + raise ImageConfigError( + f"{key} asks for {count} {expected}; the located-variable ABI " + f"addresses at most {MAX_TABLE_ELEMENTS} elements, which is " + f"{ceiling} {expected}." + ) + return count + + +def validate_image_conf( + version: int, sizes: dict[str, int | None], units: dict[str, str] +) -> dict[str, int]: + """Validate the version and every table, returning the normalised counts. + + The version is checked FIRST and refuses the whole file, because every + check below reads the values by this version's rules. A file from a format + we do not know is not a file with fourteen suspicious numbers in it -- it + is a file we cannot claim to have understood. + + Then all fourteen or none: the tables size interlocking storage that one + allocation hands out together, and a half-applied image is worse than a + refused one. + """ + if version != IMAGE_CONF_FORMAT_VERSION: + raise ImageConfigError( + f"image.conf declares format_version {version}; this runtime reads " + f"format_version {IMAGE_CONF_FORMAT_VERSION}. Re-upload the project " + f"from a current editor." + ) + return { + key: validate_table_count(key, sizes.get(key, 0), units.get(key)) + for key in IMAGE_TABLE_KEYS + } + + +def write_image_conf_file(path: str | os.PathLike, sizes: dict[str, int]) -> None: + """Write an ``image.conf`` the core can read. + + Writes the VALIDATED stanza rather than byte-copying the upload, so what + the core reads back is exactly what was checked here -- and so unknown keys + and absent ones both land as the explicit zeros the core expects, instead of + leaving it to infer them. + + Write-and-rename, like the retain store does it: a torn ``image.conf`` read + at the next program load would size the image from half a file. + """ + lines = [ + "# I/O image sizes for this program.", + "# Installed from the program upload; the project is the source, and the", + "# editor derives these from what the project actually contains.", + "# Read by the PLC application at program load.", + "# Edits here are overwritten on the next upload.", + "#", + "# One key per table in core/src/plc_app/image_tables.h. Every value", + "# carries the unit of the ADDRESS it stores: the three BOOL tables are", + "# in bits, because %QX addresses bits, and the core converts to the", + "# [N][8] shape its storage actually has. Zero means the program", + "# addresses nothing there and the runtime allocates nothing for it.", + f"format_version={IMAGE_CONF_FORMAT_VERSION}", + ] + lines += [f"{key}={sizes[key]} {IMAGE_TABLE_UNITS[key]}" for key in IMAGE_TABLE_KEYS] + + target = Path(path) + tmp = target.with_suffix(".conf.tmp") + with open(tmp, "w", encoding="utf-8") as handle: + handle.write("\n".join(lines) + "\n") + handle.flush() + os.fsync(handle.fileno()) + os.replace(tmp, target) + + +def describe_image_conf(sizes: dict[str, int]) -> str: + """One log line naming the tables that are actually sized. + + Only the non-zero ones: fourteen keys of which eleven are usually zero + reads as noise, and the point of the line is to let someone watching the + build see that the image followed their project. + """ + used = [ + f"{key}={sizes[key]} {IMAGE_TABLE_UNITS[key]}" for key in IMAGE_TABLE_KEYS if sizes[key] > 0 + ] + return ", ".join(used) if used else "every table zero" diff --git a/webserver/plcapp_management.py b/webserver/plcapp_management.py index eac28d88..fabfe433 100644 --- a/webserver/plcapp_management.py +++ b/webserver/plcapp_management.py @@ -11,8 +11,16 @@ from webserver import project_snapshot from webserver.config import VPP_DATA_DIR -from webserver.logger import LogParser, get_logger -from webserver.plugin_config_model import PluginConfig, PluginsConfiguration, PluginType +from webserver.image_config import ( + IMAGE_CONF_PATH, + ImageConfigError, + describe_image_conf, + read_image_conf_file, + validate_image_conf, + write_image_conf_file, +) +from webserver.logger import get_logger +from webserver.plugin_config_model import PluginsConfiguration from webserver.retain_config import ( RETAIN_CONF_PATH, RetainConfigError, @@ -27,10 +35,11 @@ logger, _ = get_logger("runtime", use_buffer=True) -MAX_FILE_SIZE: Final[int] = 10 * 1024 * 1024 # 10 MB per file +MAX_FILE_SIZE: Final[int] = 10 * 1024 * 1024 # 10 MB per file MAX_TOTAL_SIZE: Final[int] = 50 * 1024 * 1024 # 50 MB total DISALLOWED_EXT = (".exe", ".dll", ".sh", ".bat", ".js", ".vbs", ".scr") + class BuildStatus(Enum): IDLE = auto() UNZIPPING = auto() @@ -38,6 +47,7 @@ class BuildStatus(Enum): SUCCESS = auto() FAILED = auto() + @dataclass class BuildProcess: status: BuildStatus = BuildStatus.IDLE @@ -83,20 +93,18 @@ def analyze_zip(zip_path) -> tuple[bool, list]: # Check uncompressed size if uncompressed_size > MAX_FILE_SIZE: - logger.warning("File too large: %s (%d bytes)", - filename, uncompressed_size) + logger.warning("File too large: %s (%d bytes)", filename, uncompressed_size) 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) + # filename) safe = False # Check disallowed extensions if ext in DISALLOWED_EXT: - logger.warning("Disallowed extension: %s", - filename) + logger.warning("Disallowed extension: %s", filename) safe = False total_size += uncompressed_size @@ -104,7 +112,7 @@ 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", + # logger.warning("Total uncompressed size too large: %d bytes", # total_size) safe = False @@ -138,10 +146,14 @@ def safe_extract(zip_path, dest_dir, valid_files): filename = info.filename # Normalize path separators for cross-platform compatibility (Windows \ to Unix /) - filename = filename.replace('\\', '/') + filename = filename.replace("\\", "/") # Skip macOS junk and directories - if filename.startswith("__MACOSX/") or filename.endswith(".DS_Store") or filename.endswith("/"): + if ( + filename.startswith("__MACOSX/") + or filename.endswith(".DS_Store") + or filename.endswith("/") + ): continue # Optionally strip single root folder @@ -177,27 +189,35 @@ def safe_extract(zip_path, dest_dir, valid_files): def update_plugin_configurations(generated_dir: str = "core/generated"): """ Update plugin configurations based on available config files. - + Scans generated/conf/ for config files, copies them to plugin directories, and updates plugins.conf to enable/disable plugins accordingly. """ plugins_conf_path = "plugins.conf" conf_dir = os.path.join(generated_dir, "conf") - build_state.log(f"[DEBUG] update_plugin_configurations called with generated_dir='{generated_dir}'\n") + build_state.log( + f"[DEBUG] update_plugin_configurations called with generated_dir='{generated_dir}'\n" + ) build_state.log(f"[DEBUG] Looking for config files in: {conf_dir}\n") # Load current plugin configuration using the dataclass plugins_config = PluginsConfiguration.from_file(plugins_conf_path) - build_state.log(f"[DEBUG] Loaded {len(plugins_config.plugins)} plugins from {plugins_conf_path}\n") - + build_state.log( + f"[DEBUG] Loaded {len(plugins_config.plugins)} plugins from {plugins_conf_path}\n" + ) + # Log initial state for plugin in plugins_config.plugins: - build_state.log(f"[DEBUG] Initial state - {plugin.name}: enabled={plugin.enabled}, config_path='{plugin.config_path}'\n") + build_state.log( + f"[DEBUG] Initial state - {plugin.name}: enabled={plugin.enabled}, config_path='{plugin.config_path}'\n" + ) # Check if conf directory exists if not os.path.exists(conf_dir): - build_state.log(f"[INFO] No conf directory found in {generated_dir}, disabling all plugins\n") + build_state.log( + f"[INFO] No conf directory found in {generated_dir}, disabling all plugins\n" + ) # When there's no conf directory, disable all currently enabled plugins plugins_updated = 0 update_messages = [] @@ -206,23 +226,27 @@ def update_plugin_configurations(generated_dir: str = "core/generated"): plugin.enabled = False plugins_updated += 1 update_messages.append(f"Disabled plugin '{plugin.name}' (no conf directory found)") - + # Log the updates - build_state.log(f"[INFO] Found 0 config files (no conf directory): []\n") - + build_state.log("[INFO] Found 0 config files (no conf directory): []\n") + for message in update_messages: build_state.log(f"[INFO] {message}\n") else: # Process config files normally when conf directory exists # Use the utility method to update plugins based on available config files # Copy config files to plugin directories instead of referencing them directly - plugins_updated, update_messages = plugins_config.update_plugins_from_config_dir(conf_dir, copy_to_plugin_dirs=True) - + plugins_updated, update_messages = plugins_config.update_plugins_from_config_dir( + conf_dir, copy_to_plugin_dirs=True + ) + # Log the updates config_files = glob.glob(os.path.join(conf_dir, "*.json")) available_configs = {os.path.splitext(os.path.basename(f))[0]: f for f in config_files} - build_state.log(f"[INFO] Found {len(available_configs)} config files in {conf_dir}: {list(available_configs.keys())}\n") - + build_state.log( + f"[INFO] Found {len(available_configs)} config files in {conf_dir}: {list(available_configs.keys())}\n" + ) + for message in update_messages: if "Copied config file" in message: build_state.log(f"[INFO] {message}\n") @@ -236,17 +260,23 @@ def update_plugin_configurations(generated_dir: str = "core/generated"): # Save the updated configuration if plugins_config.to_file(plugins_conf_path): - build_state.log(f"[INFO] Plugin configuration update complete. {plugins_updated} plugins updated.\n") - + build_state.log( + f"[INFO] Plugin configuration update complete. {plugins_updated} plugins updated.\n" + ) + # Log final state for plugin in plugins_config.plugins: - build_state.log(f"[DEBUG] Final state - {plugin.name}: enabled={plugin.enabled}, config_path='{plugin.config_path}'\n") - + build_state.log( + f"[DEBUG] Final state - {plugin.name}: enabled={plugin.enabled}, config_path='{plugin.config_path}'\n" + ) + # Log configuration summary summary = plugins_config.get_config_summary() - build_state.log(f"[INFO] Plugin summary: {summary['enabled']}/{summary['total']} enabled " - f"({summary['python']} Python, {summary['native']} Native)\n") - + build_state.log( + f"[INFO] Plugin summary: {summary['enabled']}/{summary['total']} enabled " + f"({summary['python']} Python, {summary['native']} Native)\n" + ) + # Validate configurations and log any issues issues = plugins_config.validate_plugins() if issues: @@ -281,7 +311,9 @@ def _wait_for_plc_idle(runtime_manager: RuntimeManager, timeout_s: float) -> boo return False -def validate_vpp_plugins_conf(conf_path: str, runtime_root: str, vpp_build_dir: str) -> tuple[bool, str]: +def validate_vpp_plugins_conf( + conf_path: str, runtime_root: str, vpp_build_dir: str +) -> tuple[bool, str]: """Containment check for an upload-supplied ``vpp_plugins.conf``. The ``path`` field of this file is what the C plugin loader passes straight @@ -340,7 +372,10 @@ def against_root(candidate: str) -> str: "(VPP plugins may only load objects built by this upload)" ) if p.config_path and not is_inside_root(against_root(p.config_path), runtime_root): - return False, f"plugin '{p.name}' config_path '{p.config_path}' escapes the runtime root" + return ( + False, + f"plugin '{p.name}' config_path '{p.config_path}' escapes the runtime root", + ) return True, "" @@ -390,7 +425,7 @@ def apply_vpp_plugin_conf(generated_dir: str = "core/generated") -> None: # Copy vpp_plugins.conf to runtime root shutil.copy2(uploaded_conf, VPP_CONF_DEST) - build_state.log(f"[INFO] VPP: installed vpp_plugins.conf from upload\n") + build_state.log("[INFO] VPP: installed vpp_plugins.conf from upload\n") # Copy each VPP plugin's config file into the persistent dir and rewrite # its config_path to point there (see the loop below). config_path is the @@ -405,7 +440,9 @@ def apply_vpp_plugin_conf(generated_dir: str = "core/generated") -> None: continue src_config = os.path.join(conf_dir, f"{p.name}.json") if not os.path.exists(src_config): - build_state.log(f"[WARNING] VPP: conf/{p.name}.json not found in upload, skipping\n") + build_state.log( + f"[WARNING] VPP: conf/{p.name}.json not found in upload, skipping\n" + ) continue # Relocate the config (and its license sibling) OUT of build/vpp and @@ -426,7 +463,9 @@ def apply_vpp_plugin_conf(generated_dir: str = "core/generated") -> None: continue dest_config = os.path.join(str(VPP_DATA_DIR), f"{p.name}.json") if not is_inside_root(dest_config, str(VPP_DATA_DIR)): - build_state.log(f"[WARNING] VPP: config dest '{dest_config}' escapes the persistent dir, skipping\n") + build_state.log( + f"[WARNING] VPP: config dest '{dest_config}' escapes the persistent dir, skipping\n" + ) continue # The old build/vpp sibling of THIS plugin, so a device licensed # before this change can be migrated below. Derived from the FIXED @@ -470,7 +509,9 @@ def apply_vpp_plugin_conf(generated_dir: str = "core/generated") -> None: # today when 0x4A reads EMPTY. try: shutil.copy2(old_license, dest_license) - build_state.log(f"[INFO] VPP: migrated {p.name}.license {old_license} -> {dest_license}\n") + build_state.log( + f"[INFO] VPP: migrated {p.name}.license {old_license} -> {dest_license}\n" + ) except OSError as exc: build_state.log(f"[WARNING] VPP: could not migrate {p.name}.license: {exc}\n") @@ -479,7 +520,9 @@ def apply_vpp_plugin_conf(generated_dir: str = "core/generated") -> None: # not the build/vpp one the editor emitted. if rewrote_paths: vpp_conf_plugins.to_file(VPP_CONF_DEST) - build_state.log("[INFO] VPP: rewrote vpp_plugins.conf config_path to the persistent dir\n") + build_state.log( + "[INFO] VPP: rewrote vpp_plugins.conf config_path to the persistent dir\n" + ) else: # No VPP in this upload — remove any stale vpp_plugins.conf so # the plugin loader does not attempt to load old VPP drivers. @@ -582,6 +625,77 @@ def apply_retain_conf(generated_dir: str = "core/generated") -> None: ) +def apply_image_conf(generated_dir: str = "core/generated") -> None: + """Apply or remove the I/O image sizes for this upload. + + The sizes are owned by the PROJECT and derived rather than chosen: the + editor works them out from what the project contains and the upload carries + them here as ``image.conf``, exactly as it carries ``retain.conf``. This + function installs what arrives and refuses what the core could not honour. + + * **Upload includes image.conf** -> validate it and write the normalised + stanza to the runtime root, where the PLC application reads it at the next + program load. + + * **Upload does not include image.conf** -> delete any existing copy. + + THAT ABSENT CASE IS NOT THE SAME AS RETAIN'S, though the action matches. + A missing ``retain.conf`` is an instruction -- switch the built-in store + off. A missing ``image.conf`` says nothing at all: the runtime can always + derive the sizes from the located variables of the program it just loaded. + The reason to delete anyway is that a STALE file is worse than none. Leave + the previous project's ``int_output=4096`` in place, upload a program that + needs eight, and ``max(configured, derived)`` keeps 4096 words reserved for + a program that is no longer on the device -- silently, and for as long as + nobody notices. Deleting hands the decision back to the program. + + Validation happens HERE rather than at bind time, for the same reason it + does for retain: a table the core cannot address would otherwise be + discovered once per located variable, deep in 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. + + Note what this function does NOT do: it does not size anything itself, and + it does not consult the program. The floor derived from the ``.so`` is the + core's business at load time, which is what keeps a device that was + provisioned by some other route reaching the same answer. + """ + IMAGE_CONF_NAME = "image.conf" + uploaded_conf = os.path.join(generated_dir, IMAGE_CONF_NAME) + dest = str(IMAGE_CONF_PATH) + + if not os.path.exists(uploaded_conf): + if os.path.exists(dest): + os.remove(dest) + build_state.log( + "[INFO] Image: removed stale image.conf (this upload carries no " + "sizes; the runtime will size the image from the program)\n" + ) + return + + # Parse with the same reader the core's sizes go through, so what is + # validated here is exactly what the core will read back. + version, sizes, units = read_image_conf_file(uploaded_conf) + + try: + sizes = validate_image_conf(version, sizes, units) + except ImageConfigError as e: + build_state.log(f"[ERROR] Image: refusing image.conf from upload: {e}\n") + # Leave no half-applied state, and in particular do not leave the + # PREVIOUS project's sizes in force: the user would be looking at a + # device sized by a project they are no longer running. + if os.path.exists(dest): + os.remove(dest) + build_state.log("[INFO] Image: removed previous image.conf\n") + return + + write_image_conf_file(dest, sizes) + build_state.log( + f"[INFO] Image: installed image.conf from upload ({describe_image_conf(sizes)})\n" + ) + + def run_compile(runtime_manager: RuntimeManager, cwd: str = "core/generated", clean: bool = False): """Run compile script synchronously (wait for completion) and update status/logs. @@ -602,7 +716,7 @@ def stream_output(pipe, prefix): # the operator sees the drainer error, and the finally{} pipe # close lets the child see EOF instead of blocking. try: - for line in iter(pipe.readline, ''): + for line in iter(pipe.readline, ""): msg = f"{prefix}{line}" build_state.log(msg) except Exception as e: @@ -639,7 +753,7 @@ def wait_step(proc: subprocess.Popen, step_name: str) -> bool: # next poll. try: build_state.status = BuildStatus.COMPILING - build_state.log(f"[INFO] Starting compilation\n") + build_state.log("[INFO] Starting compilation\n") # --- Optional clean step --- if clean: @@ -684,12 +798,14 @@ def wait_step(proc: subprocess.Popen, step_name: str) -> bool: stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, - errors='replace', - bufsize=1 + errors="replace", + bufsize=1, ) threading.Thread(target=stream_output, args=(compile_proc.stdout, ""), daemon=True).start() - threading.Thread(target=stream_output, args=(compile_proc.stderr, "[ERROR] "), daemon=True).start() + threading.Thread( + target=stream_output, args=(compile_proc.stderr, "[ERROR] "), daemon=True + ).start() # Block until compile finishes. compile_ok = wait_step(compile_proc, "Build") @@ -716,12 +832,14 @@ def wait_step(proc: subprocess.Popen, step_name: str) -> bool: stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, - errors='replace', - bufsize=1 + errors="replace", + bufsize=1, ) threading.Thread(target=stream_output, args=(cleanup_proc.stdout, ""), daemon=True).start() - threading.Thread(target=stream_output, args=(cleanup_proc.stderr, "[ERROR] "), daemon=True).start() + threading.Thread( + target=stream_output, args=(cleanup_proc.stderr, "[ERROR] "), daemon=True + ).start() cleanup_ok = wait_step(cleanup_proc, "Cleanup")