Fix TC crash: unsynchronized MyTCServer::clients + null derefs in DDOP parsing

[gunicsba]

The isobus stack invokes every TaskControllerServer override (activate_object_pool, on_value_command, ...) from its own background thread (CANHardwareInterface's updateThread -> CANNetworkManager::update()), not from the thread that runs Application::update(). Both threads read and wrote MyTCServer::clients/uploadedPools with no synchronization, which crashed the TC with zero trace (no exception, no log line) whenever a new control function appeared on the bus. Fixed with MyTCServer::clientsMutex, taken at every entry point either thread can reach; get_clients() now returns a snapshot copy instead of a live reference, since several callers held the old reference across multiple operations.

Also fixed 8 unguarded isobus::DeviceDescriptorObjectPool::get_object_by_index() calls (it can return null for an in-range index, e.g. on a partially-parsed pool) - including a null-pointer crash in activate_object_pool() when a pool has no Device object, which reproduced reliably right after DDOP upload/activation.

Also added: a Windows SEH handler + minidump writer (crash_handler.hpp/.cpp) and a main-loop try/catch, so a future crash leaves a .dmp + log in the config dir instead of nothing - this app usually runs with no console and without --log2file. PDB generation (/Zi /DEBUG) enabled in Release so a dump can actually be symbolicated. docs/CONCURRENCY.md documents the threading model these fixes depend on, for future callback additions.

Found and fixed against the tramlineSymmetrica18m6mhack branch; ported here since both bugs are general TC-server issues, unrelated to that branch's tramline feature work.

[Copilot]

🟡 Changes recommended

There are correctness/build/documentation issues in the new crash handler and concurrency docs that should be resolved before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes silent Task Controller crashes by synchronizing MyTCServer shared state across threads, hardening DDOP object access against null returns, and adding last-resort crash diagnostics (logs/minidumps) to improve post-mortem debugging.

Changes:

• Add clientsMutex and lock all MyTCServer entry points that touch clients/uploadedPools; change get_clients() to return a snapshot copy.
• Add null checks around DeviceDescriptorObjectPool::get_object_by_index() usages and reject activation when the pool lacks a Device object.
• Add crash handler infrastructure (Windows SEH minidump + POSIX signal logging), top-level main-loop exception logging, and MSVC Release PDB generation; document concurrency model.

File summaries

File	Description
src/task_controller.cpp	Adds locking around shared client/pool maps and hardens DDOP parsing/activation against null objects.
include/task_controller.hpp	Introduces clientsMutex and updates get_clients() API to return a snapshot copy with concurrency documentation.
src/app.cpp	Updates VT status/section map code paths to use the get_clients() snapshot copy.
src/main.cpp	Installs crash handlers early and adds top-level try/catch logging for unhandled C++ exceptions in the main loop.
include/crash_handler.hpp	Declares crash handler APIs and documents intended usage.
src/crash_handler.cpp	Implements crash logging + Windows minidump writer + POSIX fatal-signal handler.
CMakeLists.txt	Links dbghelp on Windows and enables MSVC Release PDB generation for dump symbolication.
docs/CONCURRENCY.md	Adds concurrency model documentation and notes about DDOP null object behavior.
docs/PROTOCOL.md	Links to the new concurrency documentation.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

A few introduced diagnostics/build/doc paths have correctness issues (POSIX signal-handler safety, exception path exit codes, and some inaccurate concurrency documentation) that should be fixed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (6)

Previously missed (5) — in code that hasn't changed since the last review.

CMakeLists.txt:145

• The comment says this is for Release PDB generation, but /Zi and /DEBUG are currently applied to all MSVC configurations (including Debug). Applying these only to Release/RelWithDebInfo keeps Debug behavior unchanged and avoids forcing /OPT:* in Debug builds.
  src/main.cpp:367
• If an exception escapes the main loop, the process still returns 0 (success) after logging. That can cause service managers/watchdogs to treat a failure as a clean exit. Consider stopping the app and returning a non-zero exit code from this catch path.

This issue also appears on line 368 of the same file.
docs/CONCURRENCY.md:56

• This rationale for returning a copy is currently incorrect: returning by reference would not create a temporary (so it wouldn't be a dangling reference issue). The real problem is cross-thread concurrent access (data races) and potential iterator/reference invalidation if the map is mutated/erased while a caller iterates.
  include/task_controller.hpp:113
• The get_clients() doc comment describes std::map elements being "reallocated" on insert/erase; std::map is node-based, so the real issue is concurrent mutation/erase causing data races and iterator/reference invalidation (on erase), not reallocation. Tightening this wording will make the concurrency rationale more accurate.
  include/task_controller.hpp:143
• This comment states that "every entry point the isobus stack can call into must take this lock", but several overrides in this class do not lock. Either add locking to those overrides for consistency, or soften this comment so it matches the actual requirement (protect accesses to clients/uploadedPools).

src/main.cpp:371

• Same as above for the catch-all handler: logging is good, but returning success can mask an abnormal termination. Returning a non-zero exit code makes failures visible to callers/service managers.

	catch (...)
	{
		log_crash("Unhandled exception of unknown type escaped the main loop.");
	}

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Lite

[gunicsba]

.

[gunicsba]

We also found a problem that resulted in a crash in upstream: Open-Agriculture/AgIsoStack-plus-plus#710

[sujandumaru]

I am trying to see if we should populate the ActivationError and PoolErrorCode here on top of sending just true/false for return value. This way we can know more of error types on the client side.

[sujandumaru]

We can use the log() function from logging_utils.hpp I created to simplify these print lines more.