diff --git a/CMakeLists.txt b/CMakeLists.txt index 263757e..6785346 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -131,6 +131,19 @@ target_link_libraries( nlohmann_json::nlohmann_json cmake_git_version_tracking) +if(WIN32) + # For crash_handler.cpp's minidump writer (MiniDumpWriteDump). + target_link_libraries(${PROJECT_NAME} PRIVATE dbghelp) +endif() + +if(MSVC) + # Keep Release optimizations but still emit a PDB, so a crash dump (see + # crash_handler.hpp) can actually be symbolicated afterwards — without this, + # .dmp files are close to useless. + target_compile_options(${PROJECT_NAME} PRIVATE /Zi) + target_link_options(${PROJECT_NAME} PRIVATE /DEBUG /OPT:REF /OPT:ICF) +endif() + install(TARGETS ${PROJECT_NAME} RUNTIME DESTINATION bin COMPONENT applications) option(AOG_TC_VALIDATE_IOP "Build and register the object pool validator test" diff --git a/docs/CONCURRENCY.md b/docs/CONCURRENCY.md new file mode 100644 index 0000000..8df81de --- /dev/null +++ b/docs/CONCURRENCY.md @@ -0,0 +1,79 @@ +# AOG-TaskController — Concurrency Model + +This document exists because the threading model here is easy to get wrong by analogy with the +main loop, and a wrong assumption here previously produced a crash with **zero trace** — no +exception, no log line, no console output. Read this before adding a new callback (a +`TaskControllerServer` override, an event-dispatcher listener, a raw +`add_global_parameter_group_number_callback`) or before touching state that such a callback reads +or writes. + +## The two threads + +`isobus::CANHardwareInterface::start()` (called once, in `Application::setup_can_hardware()`) +spawns its own background thread by default — this is standard AgIsoStack behavior, not something +this repo opted into. That thread calls `CANNetworkManager::CANNetwork.update()` internally, which +is what actually parses incoming CAN frames, handles address claims, and dispatches every PGN to +whatever is registered to receive it. + +Meanwhile, `Application::update()` — the loop driven from `main.cpp`, doing UDP handling and +periodic sends — runs on the **main thread**. It never calls `CANHardwareInterface::update()` +itself; it relies entirely on that background thread. + +So: **two threads exist for the lifetime of the process**, and the question for any new code is +which one it runs on. + +## Which callbacks run on which thread — checked, not assumed + +Don't guess this from a class's "manual update mode" comment (that pattern is unrelated to this +question and led to a wrong conclusion once already). Check how the *specific* callback is +registered: + +- **Runs on the CAN stack background thread by default** (CANHardwareInterface's updateThread): `TaskControllerServer` + (`MyTCServer`). Treat every `TaskControllerServer` override (`activate_object_pool`, + `on_value_command`, ...) as background-thread code unless you have traced the library + implementation and proven it is deferred to `TaskControllerServer::update()`. +- **Runs directly on the background thread, synchronously, as the frame is processed** — not + deferred to any `update()` call: `VirtualTerminalClient` (confirmed: `process_rx_message` is + registered via `add_global_parameter_group_number_callback` and invokes + `softKeyEventDispatcher`/`buttonEventDispatcher`/`changeNumericValueEventDispatcher`/etc. + synchronously). Any raw + `add_global_parameter_group_number_callback`/`add_any_control_function_parameter_group_number_callback` + registered directly in `app.cpp` is in this category too, unless proven otherwise the same way: + read the registration call, not the surrounding comments. + +If you're not sure which category a new callback falls into, trace it the way this file's history +did: find where it's registered with `CANNetworkManager`/`ControlFunction` and check whether that +registration point itself defers to an `update()` call, or invokes the listener/dispatcher inline. + +## What's protected today + +- **`MyTCServer::clients` / `uploadedPools`** (`task_controller.hpp`/`.cpp`) — guarded by + `MyTCServer::clientsMutex` (a `std::recursive_mutex`, since several methods call each other: + e.g. `update_section_states()` → `send_section_setpoint_states()`). Locked at the top of every + `TaskControllerServer` override and every method `Application` calls into. `get_clients()` + returns a **copy**, not a reference — call sites hold the result across multiple operations or + take a second `get_clients()` call, either of which would leave a dangling reference to a + temporary if it returned by reference. If you add a new method that reads or writes `clients`, + take `clientsMutex` at its top, even though (per the note above) the `TaskControllerServer` + overrides may turn out to already be main-thread-only — the lock is cheap insurance and keeps + every entry point consistent regardless of how the library's internals might change. + +## What's *not* protected yet — known gap + +Calls into `vtClient` (`isobus::VirtualTerminalClient`) happen from both the main thread +(`Application::update_vt_client()` and everything it calls: `send_vt_string_if_changed`, +`vtUpdateHelper`, etc.) **and** from the VT event-dispatcher lambdas registered in +`setup_vt_client()` (soft-key, button, numeric-value-change), which — per the confirmed +registration pattern above — run on the background thread. Whether `VirtualTerminalClient`'s own +internal state is safe under that concurrent access is not verified; no external mutex wraps +`vtClient` usage today. If a crash dump ever points into `isobus::VirtualTerminalClient`, or a +crash correlates with VT/AUX-N activity rather than DDOP upload, this is the first place to look. + +## Also worth knowing: `get_object_by_index()` can return null + +Unrelated to threading, but found while investigating a real crash report ("usually happens right +after DDOP loading"): `isobus::DeviceDescriptorObjectPool::get_object_by_index()` can return +`nullptr` for an index within `[0, size())` — most likely a symptom of a partially- or +incorrectly-parsed pool (e.g. a multi-chunk DDOP transfer). Two call sites in this file already +null-checked it; the rest didn't and were fixed alongside the mutex work above. **Any new call to +`get_object_by_index()` must null-check the result before dereferencing.** diff --git a/docs/PROTOCOL.md b/docs/PROTOCOL.md index c62faf2..e9d22e8 100644 --- a/docs/PROTOCOL.md +++ b/docs/PROTOCOL.md @@ -2,7 +2,7 @@ This document describes the interfaces AOG-TaskController exposes to the rest of the system. The intended audience is anyone writing a client — AgOpenGPS (AgIO), AgValoniaGPS, or a generic ISOBUS controller — that needs to talk to the TC. -For Linux deployment of the TC itself (systemd, CAN setup, the daemon footprint), see [LINUX_DAEMON.md](LINUX_DAEMON.md). +For Linux deployment of the TC itself (systemd, CAN setup, the daemon footprint), see [LINUX_DAEMON.md](LINUX_DAEMON.md). For the TC's threading model — which callbacks run on which thread, and what that means for any state you touch — see [CONCURRENCY.md](CONCURRENCY.md). --- diff --git a/include/crash_handler.hpp b/include/crash_handler.hpp new file mode 100644 index 0000000..ff945bc --- /dev/null +++ b/include/crash_handler.hpp @@ -0,0 +1,24 @@ +/** + * @file crash_handler.hpp + * @brief Last-resort crash diagnostics: writes a crash log (and, on Windows, a + * minidump) to the config "logs" directory before the process dies. + * + * This exists because AOG-TaskController normally runs with no visible console + * (launched by AgIO/AOG) and without --log2file, so a hard crash (access + * violation, uncaught exception, std::terminate) otherwise leaves zero trace. + * install_crash_handlers() should be called once, as early as possible in + * main/WinMain. + */ +#pragma once + +#include + +/// @brief Install process-wide crash handlers (SEH filter + minidump on +/// Windows, signal handlers on POSIX; std::terminate handler on both). +void install_crash_handlers(); + +/// @brief Append a line to the same crash log file the handlers above write to +/// (config dir's logs/crash_.log), regardless of whether --log2file +/// was passed. For fatal-but-caught conditions (e.g. an exception caught at the +/// top of main()) that should still leave a trace on disk. +void log_crash(const std::string &reason); diff --git a/include/task_controller.hpp b/include/task_controller.hpp index 00b7d82..705605e 100644 --- a/include/task_controller.hpp +++ b/include/task_controller.hpp @@ -16,6 +16,7 @@ #include #include +#include #include constexpr std::uint8_t NUMBER_SECTIONS_PER_CONDENSED_MESSAGE = 16; @@ -103,7 +104,14 @@ class MyTCServer : public isobus::TaskControllerServer std::int32_t processDataValue, std::uint8_t &errorCodes) override; bool store_device_descriptor_object_pool(std::shared_ptr partnerCF, const std::vector &binaryPool, bool appendToPool) override; - std::map, ClientState> &get_clients(); + + /// @brief Returns a snapshot copy of the client map, not a live reference. + /// See the concurrency note on clientsMutex below for why: the isobus stack + /// invokes the TaskControllerServer overrides above from its own background + /// thread, concurrently with whichever thread calls this. A returned reference + /// could be mutated (even reallocated, on insert/erase) out from under a caller + /// mid-iteration — a returned copy can't. + std::map, ClientState> get_clients(); void request_measurement_commands(); void update_section_states(std::vector §ionStates); void update_section_control_enabled(bool enabled); @@ -115,4 +123,23 @@ class MyTCServer : public isobus::TaskControllerServer std::map, ClientState> clients; std::map, std::queue>> uploadedPools; + + /// @brief Guards clients and uploadedPools. + /// + /// CONCURRENCY: the isobus/AgIsoStack stack runs its own background thread + /// (CANHardwareInterface's updateThread) that calls CANNetworkManager::update(), + /// which is what actually invokes every TaskControllerServer override in this + /// class (activate_object_pool, on_value_command, ...) — NOT the thread that + /// runs Application::update(). Meanwhile Application::update() (and the methods + /// it calls: request_measurement_commands, update_section_states/_control_enabled, + /// get_clients) reads/writes the SAME maps from the main thread. Without this + /// lock, that's a concurrent std::map read + insert/erase — undefined behavior, + /// seen in practice as the TC silently crashing (no exception, no log line) + /// whenever a new control function appeared on the bus. See docs/CONCURRENCY.md + /// before touching clients/uploadedPools or adding a new callback here: every + /// entry point the isobus stack can call into must take this lock + /// (std::recursive_mutex — some of these methods call each other, e.g. + /// update_section_states -> send_section_setpoint_states), and get_clients() + /// must keep returning a copy, not a reference (see its declaration above). + mutable std::recursive_mutex clientsMutex; }; diff --git a/src/app.cpp b/src/app.cpp index a5274df..3e9877e 100644 --- a/src/app.cpp +++ b/src/app.cpp @@ -1106,9 +1106,10 @@ void Application::update_vt_section_map() if (isobus::SystemTiming::time_expired_ms(lastVtSectionUpdateMs, 100)) { std::string sectionMap = "No sections connected"; - if (!tcServer->get_clients().empty()) + auto clients = tcServer->get_clients(); // snapshot copy — see get_clients()'s declaration + if (!clients.empty()) { - auto &state = tcServer->get_clients().begin()->second; + auto &state = clients.begin()->second; const auto sectionCount = std::min(state.get_number_of_sections(), 64); if (sectionCount > 0) { @@ -1184,7 +1185,7 @@ void Application::update_vt_status_strings(bool aogConnected) send_vt_string_if_changed(VTAogIPStr, udpConnections->get_bound_ip_address()); const std::string packetAge = (lastAogPacketMs == 0) ? "never" : (std::to_string(isobus::SystemTiming::get_time_elapsed_ms(lastAogPacketMs) / 1000) + " s"); const bool taskRunning = tcServer->get_task_totals_active(); - auto &clients = tcServer->get_clients(); + auto clients = tcServer->get_clients(); // snapshot copy — see get_clients()'s declaration std::uint32_t totalSections = 0; for (const auto &client : clients) diff --git a/src/crash_handler.cpp b/src/crash_handler.cpp new file mode 100644 index 0000000..ab9f0fe --- /dev/null +++ b/src/crash_handler.cpp @@ -0,0 +1,189 @@ +#include "crash_handler.hpp" + +#include "logging_utils.hpp" +#include "settings.hpp" + +#include +#include +#include +#include +#include +#include +#include +#include + +namespace +{ + std::string crash_file_path(const std::string &extension) + { + std::time_t now = std::time(nullptr); + std::tm localTime; +#if defined(_WIN32) + localtime_s(&localTime, &now); +#else + localtime_r(&now, &localTime); +#endif + char stamp[32]; + std::snprintf(stamp, sizeof(stamp), "%04d-%02d-%02d_%02d-%02d-%02d", localTime.tm_year + 1900, localTime.tm_mon + 1, localTime.tm_mday, localTime.tm_hour, localTime.tm_min, localTime.tm_sec); + // Settings::get_filename_path() throws if it can't create the directory; a crash + // handler must never throw, so fall back to the current directory on failure. + try + { + return Settings::get_filename_path(std::string("logs/crash_") + stamp + extension); + } + catch (...) + { + return std::string("crash_") + stamp + extension; + } + } + + void log_terminate_reason() + { + std::string reason = "std::terminate called"; + if (auto currentException = std::current_exception()) + { + try + { + std::rethrow_exception(currentException); + } + catch (const std::exception &e) + { + reason += std::string(" due to unhandled exception: ") + e.what(); + } + catch (...) + { + reason += " due to an unhandled exception of unknown type"; + } + } + else + { + reason += " with no active exception (direct std::terminate()/std::abort(), a noexcept " + "violation, or a failure during stack unwinding)"; + } + log_crash(reason); + } +} // namespace + +void log_crash(const std::string &reason) +{ + std::ofstream out(crash_file_path(".log"), std::ios::app); + std::ostream &sink = out.is_open() ? static_cast(out) : std::cout; + sink << "[" << get_timestamp() << "] [Crash] " << reason << std::endl; +} + +#if defined(_WIN32) + +#include + +#include + +#pragma comment(lib, "dbghelp.lib") + +namespace +{ + LONG WINAPI unhandled_exception_filter(EXCEPTION_POINTERS *exceptionPointers) + { + const std::string dumpPath = crash_file_path(".dmp"); + + { + std::ostringstream reason; + reason << "Unhandled SEH exception 0x" << std::hex << exceptionPointers->ExceptionRecord->ExceptionCode + << std::dec << " at address " << exceptionPointers->ExceptionRecord->ExceptionAddress + << ". Writing minidump to " << dumpPath; + log_crash(reason.str()); + } + + HANDLE dumpFile = CreateFileA(dumpPath.c_str(), GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr); + if (dumpFile != INVALID_HANDLE_VALUE) + { + MINIDUMP_EXCEPTION_INFORMATION mdInfo{}; + mdInfo.ThreadId = GetCurrentThreadId(); + mdInfo.ExceptionPointers = exceptionPointers; + mdInfo.ClientPointers = FALSE; + + MiniDumpWriteDump(GetCurrentProcess(), GetCurrentProcessId(), dumpFile, MiniDumpWithIndirectlyReferencedMemory, &mdInfo, nullptr, nullptr); + CloseHandle(dumpFile); + } + + // Let Windows terminate the process normally after we've captured diagnostics. + return EXCEPTION_EXECUTE_HANDLER; + } +} // namespace + +void install_crash_handlers() +{ + SetUnhandledExceptionFilter(unhandled_exception_filter); + std::set_terminate(log_terminate_reason); +} + +#else // POSIX + +#include +#include + +namespace +{ + void fatal_signal_handler(int signalNumber) + { + const char *name = "unknown signal"; + switch (signalNumber) + { + case SIGSEGV: + name = "SIGSEGV (segmentation fault)"; + break; + case SIGABRT: + name = "SIGABRT (abort)"; + break; + case SIGFPE: + name = "SIGFPE (arithmetic error)"; + break; + case SIGILL: + name = "SIGILL (illegal instruction)"; + break; + case SIGBUS: + name = "SIGBUS (bus error)"; + break; + default: + break; + } + + // Signal-handler-safe I/O only: no iostreams, no dynamic allocation, no + // get_timestamp(), and no strlen() — it's not on POSIX's async-signal-safe + // function list, so a hand-rolled length count is used instead. + const int fd = 2; // stderr + auto writeRaw = [fd](const char *text) { + std::size_t len = 0; + while (text[len] != '\0') + { + ++len; + } + (void)write(fd, text, len); + }; + writeRaw("[Crash] Fatal signal: "); + writeRaw(name); + writeRaw("\n"); + + // Re-trigger the signal with default disposition so the process terminates normally + // (and may produce a core dump), without calling non-async-signal-safe functions. + ::kill(::getpid(), signalNumber); + ::_exit(128 + signalNumber); + } +} // namespace + +void install_crash_handlers() +{ + struct sigaction sa + { + }; + sa.sa_handler = fatal_signal_handler; + sigemptyset(&sa.sa_mask); + sa.sa_flags = SA_RESETHAND; + const int fatalSignals[] = { SIGSEGV, SIGABRT, SIGFPE, SIGILL, SIGBUS }; + for (int sig : fatalSignals) + { + (void)sigaction(sig, &sa, nullptr); + } + std::set_terminate(log_terminate_reason); +} + +#endif diff --git a/src/main.cpp b/src/main.cpp index 642c2aa..b30c3e4 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1,4 +1,5 @@ #include "app.hpp" +#include "crash_handler.hpp" #include "logging.cpp" #include "settings.hpp" @@ -321,37 +322,53 @@ static std::shared_ptr prepare_application(const std: static int run_application_loop(std::shared_ptr canDriver) { Application app(canDriver); - if (!app.initialize()) + try { - std::cout << "Failed to initialize application..." << std::endl; - return -1; - } + if (!app.initialize()) + { + std::cout << "Failed to initialize application..." << std::endl; + return -1; + } - std::cout << "[" << get_timestamp() << "] Press Ctrl+C to stop the application..." << std::endl; + std::cout << "[" << get_timestamp() << "] Press Ctrl+C to stop the application..." << std::endl; - while (running) - { -#if defined(_WIN32) - // Pump the (hidden) message queue with a 1 ms timeout, mirroring the - // previous behavior so AOG can still close us via WM_CLOSE. - MSG msg; - DWORD result = MsgWaitForMultipleObjects(0, NULL, FALSE, 1, QS_ALLINPUT); - while (result == WAIT_OBJECT_0 && PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)) + while (running) { - TranslateMessage(&msg); - DispatchMessage(&msg); - } +#if defined(_WIN32) + // Pump the (hidden) message queue with a 1 ms timeout, mirroring the + // previous behavior so AOG can still close us via WM_CLOSE. + MSG msg; + DWORD result = MsgWaitForMultipleObjects(0, NULL, FALSE, 1, QS_ALLINPUT); + while (result == WAIT_OBJECT_0 && PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)) + { + TranslateMessage(&msg); + DispatchMessage(&msg); + } #else - // On POSIX we have no message loop; just sleep briefly between updates. - std::this_thread::sleep_for(std::chrono::milliseconds(1)); + // On POSIX we have no message loop; just sleep briefly between updates. + std::this_thread::sleep_for(std::chrono::milliseconds(1)); #endif - if (!app.update()) - { - std::cout << "Something unexpected happened, stopping application..." << std::endl; - break; + if (!app.update()) + { + std::cout << "Something unexpected happened, stopping application..." << std::endl; + break; + } } } + // A crash (access violation, SIGSEGV, ...) can't be caught here — that's what + // install_crash_handlers() is for. This is the safety net for ordinary C++ + // exceptions (e.g. a library call throwing) that would otherwise propagate all + // the way out and terminate the process with zero trace, since this app usually + // runs with no visible console and without --log2file. + catch (const std::exception &e) + { + log_crash(std::string("Unhandled exception escaped the main loop: ") + e.what()); + } + catch (...) + { + log_crash("Unhandled exception of unknown type escaped the main loop."); + } std::cout << "[" << get_timestamp() << "] Shutting down..." << std::endl; app.stop(); @@ -361,6 +378,10 @@ static int run_application_loop(std::shared_ptr canDr #if defined(_WIN32) int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nShowCmd) { + // Install first: this app usually runs with no visible console and without + // --log2file, so a crash otherwise leaves zero trace (see crash_handler.hpp). + install_crash_handlers(); + // Try to attach to the parent process's console if it exists if (AttachConsole(ATTACH_PARENT_PROCESS)) { @@ -399,6 +420,8 @@ int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine #else int main(int argc, char **argv) { + install_crash_handlers(); + std::signal(SIGINT, signal_handler); std::signal(SIGTERM, signal_handler); std::signal(SIGPIPE, SIG_IGN); diff --git a/src/task_controller.cpp b/src/task_controller.cpp index bc17ddd..660da73 100644 --- a/src/task_controller.cpp +++ b/src/task_controller.cpp @@ -336,12 +336,26 @@ MyTCServer::MyTCServer(std::shared_ptr internal { } -bool MyTCServer::activate_object_pool(std::shared_ptr partnerCF, ObjectPoolActivationError &, ObjectPoolErrorCodes &, std::uint16_t &, std::uint16_t &) +bool MyTCServer::activate_object_pool(std::shared_ptr partnerCF, ObjectPoolActivationError &activationError, ObjectPoolErrorCodes &objectPoolError, std::uint16_t &parentObjectIDOfFaultyObject, std::uint16_t &faultyObjectID) { - std::cout << "[" << get_timestamp() << "] [TC Server] Client " << partnerCF->get_NAME().get_full_name() << " requesting object pool activation" << std::endl; + std::lock_guard lock(clientsMutex); + + // Default to "no error" / "no faulty object" up front, so every early-return path + // below only needs to override what's actually wrong, per the meaning documented on + // TaskControllerServer::activate_object_pool() in the base class. + activationError = ObjectPoolActivationError::NoErrors; + objectPoolError = ObjectPoolErrorCodes::NoErrors; + parentObjectIDOfFaultyObject = isobus::NULL_OBJECT_ID; + faultyObjectID = isobus::NULL_OBJECT_ID; + + log("TC Server") << "Client " << partnerCF->get_NAME().get_full_name() << " requesting object pool activation" << std::endl; // Safety check to make sure partnerCF has uploaded a DDOP if (uploadedPools.find(partnerCF) == uploadedPools.end()) { + // Not a DDOP content problem (there's no DDOP to have one) — this is a client + // requesting activation without ever uploading, which isn't covered by a more + // specific error code. + activationError = ObjectPoolActivationError::AnyOtherError; return false; } @@ -359,20 +373,36 @@ bool MyTCServer::activate_object_pool(std::shared_ptr p } if (deserialized) { - std::cout << "[" << get_timestamp() << "] Successfully deserialized device descriptor object pool." << std::endl; + log() << "Successfully deserialized device descriptor object pool." << std::endl; // Save to NVM std::shared_ptr deviceObject; for (std::uint16_t i = 0; i < state.get_pool().size(); i++) { auto object = state.get_pool().get_object_by_index(i); - if (object->get_object_type() == isobus::task_controller_object::ObjectTypes::Device) + if (object && object->get_object_type() == isobus::task_controller_object::ObjectTypes::Device) { deviceObject = std::static_pointer_cast(object); break; } } + if (!deviceObject) + { + // A spec-compliant pool always has exactly one Device object at its root. + // If it's missing — a malformed pool, or a multi-chunk transfer that didn't + // concatenate correctly upstream — reject the activation instead of crashing + // on the dereference below. There's no single faulty object ID to point at + // here (the problem is an absence, not a bad object), so parent/faulty stay + // NULL_OBJECT_ID. + log("TC Server") << "Client " << partnerCF->get_NAME().get_full_name() + << " activation REJECTED: deserialized pool (" << state.get_pool().size() + << " objects) has no Device object." << std::endl; + activationError = ObjectPoolActivationError::ThereAreErrorsInTheDDOP; + objectPoolError = ObjectPoolErrorCodes::UnknownObjectReference; + return false; + } + auto labelBytes = deviceObject->get_localization_label(); std::string label(reinterpret_cast(labelBytes.data()), labelBytes.size()); // trim at first non-printable character (control chars, DEL, etc.) @@ -388,16 +418,16 @@ bool MyTCServer::activate_object_pool(std::shared_ptr p { outFile.write(reinterpret_cast(binaryPool.data()), binaryPool.size()); outFile.close(); - std::cout << "[" << get_timestamp() << "] Saved DDOP to file: " << fileName << std::endl; + log() << "Saved DDOP to file: " << fileName << std::endl; } else { - std::cout << "[" << get_timestamp() << "] Unable to save DDOP to NVM. (Failed to open file) file: " << fileName << std::endl; + log() << "Unable to save DDOP to NVM. (Failed to open file) file: " << fileName << std::endl; } } else { - std::cout << "[" << get_timestamp() << "] Unable to save DDOP to NVM. (Failed to generate binary object pool)" << std::endl; + log() << "Unable to save DDOP to NVM. (Failed to generate binary object pool)" << std::endl; } auto implement = isobus::DeviceDescriptorObjectPoolHelper::get_implement_geometry(state.get_pool()); @@ -452,7 +482,7 @@ bool MyTCServer::activate_object_pool(std::shared_ptr p for (std::uint32_t i = 0; i < state.get_pool().size(); i++) { auto object = state.get_pool().get_object_by_index(i); - if (object->get_object_type() == isobus::task_controller_object::ObjectTypes::DeviceProcessData) + if (object && object->get_object_type() == isobus::task_controller_object::ObjectTypes::DeviceProcessData) { auto processDataObject = std::dynamic_pointer_cast(object); auto ddi = processDataObject->get_ddi(); @@ -480,22 +510,22 @@ bool MyTCServer::activate_object_pool(std::shared_ptr p if (hasCondensedSetpoint) { // Modern: condensed setpoint DDI 290+ (always paired with DDI 289 for global work state) - std::cout << "[" << get_timestamp() << "] [TC Server] Attempting Section Control via: DDI 290 (SetpointCondensedWorkState) + DDI 289 (SetpointWorkState)" - << " for " << static_cast(numberOfSections) << " sections." << std::endl; + log("TC Server") << "Attempting Section Control via: DDI 290 (SetpointCondensedWorkState) + DDI 289 (SetpointWorkState)" + << " for " << static_cast(numberOfSections) << " sections." << std::endl; } else if (hasSettableCondensedActual) { // Old: settable condensed actual DDI 161+ - std::cout << "[" << get_timestamp() << "] [TC Server] Attempting Section Control via: DDI 161 (ActualCondensedWorkState, settable)" - << " for " << static_cast(numberOfSections) << " sections." << std::endl; + log("TC Server") << "Attempting Section Control via: DDI 161 (ActualCondensedWorkState, settable)" + << " for " << static_cast(numberOfSections) << " sections." << std::endl; } else if (hasSettableActualWorkState) { // Oldest: per-element settable DDI 141 state.set_uses_per_element_control(true); state.set_per_element_setpoint_ddi(static_cast(isobus::DataDescriptionIndex::ActualWorkState)); - std::cout << "[" << get_timestamp() << "] [TC Server] Attempting Section Control via: DDI 141 (ActualWorkState, settable per-element)" - << " for " << static_cast(numberOfSections) << " sections." << std::endl; + log("TC Server") << "Attempting Section Control via: DDI 141 (ActualWorkState, settable per-element)" + << " for " << static_cast(numberOfSections) << " sections." << std::endl; for (std::uint8_t i = 0; i < numberOfSections; i++) { std::cout << " Section " << static_cast(i) << " -> element " << sectionElementNumbers[i] << std::endl; @@ -503,19 +533,21 @@ bool MyTCServer::activate_object_pool(std::shared_ptr p } else { - std::cout << "[" << get_timestamp() << "] [TC Server] WARNING: No supported section control method detected! " - << "Device has no DDI 290, 161 (settable), or 141 (settable)." << std::endl; + log("TC Server") << "WARNING: No supported section control method detected! " + << "Device has no DDI 290, 161 (settable), or 141 (settable)." << std::endl; } } else { - std::cout << "[" << get_timestamp() << "] Failed to deserialize device descriptor object pool." << std::endl; + log() << "Failed to deserialize device descriptor object pool." << std::endl; + activationError = ObjectPoolActivationError::ThereAreErrorsInTheDDOP; + objectPoolError = ObjectPoolErrorCodes::AnyOtherError; return false; } clients[partnerCF] = state; - std::cout << "[" << get_timestamp() << "] [TC Server] Client " << partnerCF->get_NAME().get_full_name() << " registered successfully with " - << static_cast(state.get_number_of_sections()) << " sections." << std::endl; + log("TC Server") << "Client " << partnerCF->get_NAME().get_full_name() << " registered successfully with " + << static_cast(state.get_number_of_sections()) << " sections." << std::endl; return true; } @@ -526,6 +558,7 @@ bool MyTCServer::change_designator(std::shared_ptr, std bool MyTCServer::deactivate_object_pool(std::shared_ptr partnerCF) { + std::lock_guard lock(clientsMutex); clients.erase(partnerCF); uploadedPools.erase(partnerCF); return true; @@ -533,6 +566,7 @@ bool MyTCServer::deactivate_object_pool(std::shared_ptr bool MyTCServer::delete_device_descriptor_object_pool(std::shared_ptr partnerCF, ObjectPoolDeletionErrors &) { + std::lock_guard lock(clientsMutex); clients.erase(partnerCF); uploadedPools.erase(partnerCF); return true; @@ -569,6 +603,7 @@ void MyTCServer::identify_task_controller(std::uint8_t tcNumber) void MyTCServer::on_client_timeout(std::shared_ptr partner) { + std::lock_guard lock(clientsMutex); // Cleanup the client state std::cout << "[" << get_timestamp() << "] [TC Server] Client " << partner->get_NAME().get_full_name() << " has timed out!" << std::endl; clients.erase(partner); @@ -596,6 +631,7 @@ bool MyTCServer::on_value_command(std::shared_ptr partn std::int32_t processDataValue, std::uint8_t &errorCodes) { + std::lock_guard lock(clientsMutex); switch (dataDescriptionIndex) { case static_cast(isobus::DataDescriptionIndex::ActualCondensedWorkState1_16): @@ -658,6 +694,7 @@ bool MyTCServer::on_value_command(std::shared_ptr partn bool MyTCServer::store_device_descriptor_object_pool(std::shared_ptr partnerCF, const std::vector &binaryPool, bool appendToPool) { + std::lock_guard lock(clientsMutex); std::cout << "[" << get_timestamp() << "] [TC Server] Client " << partnerCF->get_NAME().get_full_name() << " requesting object pool transfer of " << binaryPool.size() << " bytes" << std::endl; if (uploadedPools.find(partnerCF) == uploadedPools.end()) { @@ -667,13 +704,15 @@ bool MyTCServer::store_device_descriptor_object_pool(std::shared_ptr, ClientState> &MyTCServer::get_clients() +std::map, ClientState> MyTCServer::get_clients() { - return clients; + std::lock_guard lock(clientsMutex); + return clients; // copy, taken while locked — see the declaration's comment } void MyTCServer::request_measurement_commands() { + std::lock_guard lock(clientsMutex); for (auto &client : clients) { // Skip clients with 0 sections (e.g. tractors) - sending measurement commands to a tractor ECU can cause unexpected behavior @@ -683,7 +722,7 @@ void MyTCServer::request_measurement_commands() for (std::uint32_t i = 0; i < client.second.get_pool().size(); i++) { auto object = client.second.get_pool().get_object_by_index(i); - if (object->get_object_type() == isobus::task_controller_object::ObjectTypes::DeviceProcessData) + if (object && object->get_object_type() == isobus::task_controller_object::ObjectTypes::DeviceProcessData) { auto processDataObject = std::dynamic_pointer_cast(object); if (processDataObject->get_ddi() == static_cast(isobus::DataDescriptionIndex::ActualWorkState) || @@ -696,7 +735,7 @@ void MyTCServer::request_measurement_commands() for (std::uint32_t j = 0; j < client.second.get_pool().size(); j++) { auto parentObject = client.second.get_pool().get_object_by_index(j); - if (parentObject->get_object_type() == isobus::task_controller_object::ObjectTypes::DeviceElement) + if (parentObject && parentObject->get_object_type() == isobus::task_controller_object::ObjectTypes::DeviceElement) { auto elementObject = std::dynamic_pointer_cast(parentObject); for (std::uint16_t elementObjectChild : elementObject->get_child_object_ids()) @@ -731,7 +770,7 @@ void MyTCServer::request_measurement_commands() for (std::uint32_t i = 0; i < client.second.get_pool().size(); i++) { auto object = client.second.get_pool().get_object_by_index(i); - if (object->get_object_type() == isobus::task_controller_object::ObjectTypes::DeviceProcessData) + if (object && object->get_object_type() == isobus::task_controller_object::ObjectTypes::DeviceProcessData) { auto processDataObject = std::dynamic_pointer_cast(object); if (processDataObject->get_ddi() == static_cast(isobus::DataDescriptionIndex::SectionControlState) || @@ -743,7 +782,7 @@ void MyTCServer::request_measurement_commands() for (std::uint32_t j = 0; j < client.second.get_pool().size(); j++) { auto parentObject = client.second.get_pool().get_object_by_index(j); - if (parentObject->get_object_type() == isobus::task_controller_object::ObjectTypes::DeviceElement) + if (parentObject && parentObject->get_object_type() == isobus::task_controller_object::ObjectTypes::DeviceElement) { auto elementObject = std::dynamic_pointer_cast(parentObject); for (std::uint16_t elementObjectChild : elementObject->get_child_object_ids()) @@ -781,6 +820,7 @@ void MyTCServer::request_measurement_commands() void MyTCServer::update_section_states(std::vector §ionStates) { + std::lock_guard lock(clientsMutex); for (auto &client : clients) { auto &state = client.second; @@ -826,6 +866,7 @@ void MyTCServer::update_section_states(std::vector §ionStates) void MyTCServer::update_section_control_enabled(bool enabled) { + std::lock_guard lock(clientsMutex); for (auto &client : clients) { // Always update the local flag @@ -845,6 +886,7 @@ void MyTCServer::update_section_control_enabled(bool enabled) void MyTCServer::send_section_setpoint_states(std::shared_ptr client, std::uint8_t ddiOffset) { + std::lock_guard lock(clientsMutex); std::uint8_t sectionOffset = ddiOffset * NUMBER_SECTIONS_PER_CONDENSED_MESSAGE; std::uint32_t value = 0; for (std::uint8_t i = 0; i < NUMBER_SECTIONS_PER_CONDENSED_MESSAGE; i++) @@ -928,15 +970,17 @@ void MyTCServer::send_section_setpoint_states(std::shared_ptr client, bool enabled) { + std::lock_guard lock(clientsMutex); send_set_value(client, static_cast(isobus::DataDescriptionIndex::SectionControlState), clients[client].get_element_number_for_ddi(isobus::DataDescriptionIndex::SectionControlState), enabled ? 1 : 0); } bool MyTCServer::is_ddi_settable(std::shared_ptr client, std::uint16_t ddi) { + std::lock_guard lock(clientsMutex); for (std::uint32_t i = 0; i < clients[client].get_pool().size(); i++) { auto object = clients[client].get_pool().get_object_by_index(i); - if (object->get_object_type() == isobus::task_controller_object::ObjectTypes::DeviceProcessData) + if (object && object->get_object_type() == isobus::task_controller_object::ObjectTypes::DeviceProcessData) { auto processDataObject = std::dynamic_pointer_cast(object); if (processDataObject->get_ddi() == ddi)