Conversation
… implement as of now.
…askController into tramlineSymmetrica18m6mhack # Conflicts: # include/task_controller.hpp # src/app.cpp # src/task_controller.cpp
…assymetrical. Requires a special AgOpenGPS version that sends the track information.
Contributor
Author
|
To test we need this version of AgOpenGPS: There's also a PR: |
… understand the random crash of our TC.
Same fixes already applied on fix/crash-handling-and-thread-safety (the develop-targeted branch this code was ported to): - clang-format the file (was never run before the previous commit). - cmake-format the new CMakeLists.txt comment block. - The POSIX fatal_signal_handler() called std::strlen() inside signal context; strlen isn't on POSIX's async-signal-safe function list, so using it here could itself deadlock/crash while handling SIGSEGV/SIGABRT. Replaced with a hand-rolled length count. - std::snprintf() was used without including <cstdio>, compiling only via a transitive include on this toolchain. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… Data ActualWorkingWidth/MaximumWorkingWidth/DefaultWorkingWidth (DDI 67/68/70) can be declared as a Device Process Data (DPD) object instead of a Device Property (DPT). A DPD has no value embedded in the DDOP at all - only a definition - so DeviceDescriptorObjectPoolHelper::get_implement_geometry(), a pure static-pool parser, could never see it. The KUHN ESPRO reports its width this way, which is why our logs never showed it despite the DDOP containing valid section/sub-boom structure. Subscribe to these DDIs per element in request_measurement_commands(), capture the reported value in on_value_command(), and fall back to it in derive_implement_details() whenever the static pool doesn't have a value - same Actual > Maximum > Default priority the library already uses. Verified against a live KUHN ESPRO: "Implement: ESPRO (2 sections, 6.00 m)". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
process_rx_messages() dereferenced rxMessage.get_source_control_function() unguarded in the TechnicalCapabilities case (directly via get_address() in two LOG_INFO calls, and indirectly via get_active_client(...)->reportedVersion). The source control function can legitimately be null - address-claim churn can queue a message before the network manager has resolved a ControlFunction for its source address - and process_rx_messages() drains that queue on a delay via the store_rx_message()/process_rx_messages() split, so a burst of claim activity (an implement timing out while a second VT claims an address) could crash with an access violation well after the message was received. Confirmed via a field minidump: SEH exception 0xC0000005 reading 0x19 inside ControlFunction::get_address(), called from TaskControllerServer::process_rx_messages(), called from TaskControllerServer::update(), called from Application::update() Line 787 - a real call stack from Visual Studio, not a heuristic guess. Fixed upstream in gunicsba/AgIsoStack-plus-plus@e79699e (null-checks both the source control function and get_active_client()'s return, mirroring the existing guard pattern in the DeviceDescriptor case). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AOG's own PGN 0xF4 guidance reference ID is only a 16-bit value scoped to whichever field is currently open in AOG - switching fields can land on the same raw ID a different field already used. An implement that caches per-track state (e.g. an offset) keyed on DDI 508 (Unique A-B Guidance Reference Line ID) alone can then apply a stale offset from the wrong field. Add a FieldRegistry that assigns each field name a stable, persistent index (backed by its own field_registry.csv under the app's config directory, kept separate from settings.json so it can be reset independently), and fold that index into the upper 16 bits of the 32-bit DDI 508 value while leaving AOG's own 16-bit ID in the lower 16 bits untouched - 100 tracks/field x 1000 fields needs 17 bits; this gives 16+16 with room to spare. Wires up PGN 0xF3 (Field Name) handling, which didn't exist in this codebase before. The documented wire layout (a length byte at offset 4, name at 5+) turned out not to match what AOG actually sends, confirmed against a live packet: the whole payload is just the raw UTF-8 name, no length prefix, and an empty payload means the field is closed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
TRACK negotiation currently marks clients “negotiation complete” even when DDI 506 echoes 0 (“no common level”), which can incorrectly start TRACK transmissions for unsupported implements.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds ISO 11783 TRACK (tramline) support and Tractor Facilities/Time-Date announcements, plus supporting diagnostics, persistence, and concurrency hardening to prevent crashes from concurrent client-map access.
Changes:
- Implement TECU Tractor Facilities (PGN 65033) responses and periodic Time/Date (FEE6) broadcasting with duplicate-provider suppression.
- Add TRACK (tramline) negotiation (DDI 505/506) and periodic track-context/DDI transmission from AOG PGN 0xF4, including GNSS quality mapping from PGN 0xD6.
- Introduce crash diagnostics, field-name indexing persistence for unique DDI 508 IDs, and unit tests + CI wiring.
File summaries
| File | Description |
|---|---|
| tools/test_tractor_facilities.cpp | New unit test binary validating PGN 65033 encode/decode bit layout. |
| src/tractor_facilities.cpp | Implements PGN 65033 encode/decode + request handling and PGN 65032 diagnostic listener. |
| include/tractor_facilities.hpp | Public API/types for Tractor Facilities support. |
| src/task_controller.cpp | Adds tramline capability detection, negotiation handling, TRACK DDI sending, and client-map locking. |
| include/task_controller.hpp | Adds tramline state to ClientState, TRACK APIs to MyTCServer, and documents concurrency constraints. |
| include/guidance_track_context.hpp | Parses AOG PGN 0xF4 into a TRACK-ready context with sequence freshness logic. |
| src/app.cpp | Wires in tractor facilities, FEE6 broadcasting, AOG PGN 0xD6/0xF4 handling, and TRACK transmission cadence. |
| include/app.hpp | Adds new state for FEE6 suppression, GNSS quality tracking, track context, and field registry. |
| src/main.cpp | Installs crash handlers and adds top-level exception logging. |
| src/crash_handler.cpp | New crash logging/minidump (Windows) + fatal-signal handler (POSIX). |
| include/crash_handler.hpp | Declares crash handler install + crash logging helper. |
| src/field_registry.cpp | Persists field-name→index mapping to avoid DDI 508 collisions across fields. |
| include/field_registry.hpp | Declares FieldRegistry and its persistence semantics. |
| docs/PROTOCOL.md | Expands UDP/ISOBUS protocol documentation for new PGNs, DDIs, and TRACK behavior. |
| docs/CONCURRENCY.md | Documents thread model and locking rules; calls out known remaining gaps. |
| CMakeLists.txt | Updates AgIsoStack-plus-plus pin, adds crash-handler link flags, and adds tractor facilities test target. |
| .github/workflows/validate-iop.yml | Builds the new unit test in CI alongside the validator. |
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Resolves conflicts from PR #74 (crash-handling and thread-safety), which landed on develop with overlapping crash_handler.cpp, clientsMutex documentation, and activate_object_pool error-reporting work. Kept develop's more robust POSIX signal handling (sigaction over signal/raise) and its ObjectPoolActivationError/ObjectPoolErrorCodes population, while keeping tramline-specific additions (tramline capability detection, send_tramline_track_data, tractor_facilities.cpp callback documentation). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A few new logging/persistence paths contain misleading or fragile behavior (log-level claims, “power-up” log labeling, newline persistence) that should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/tractor_facilities.cpp:442
- This block claims logging is "debug"/"guarded by the current log level", but it always prints to
std::coutregardless of--log_level. Either route it through the CAN stack logger (if intended) or adjust the comments/output so it doesn’t imply log-level filtering.
// Only log at debug level (guarded by the current log level).
// We use a simple hex dump to avoid pulling in the full Facilities
// decode for a diagnostic message.
std::ostringstream hex;
hex << std::hex;
- Files reviewed: 14/14 changed files
- Comments generated: 4
- Review effort level: Lite
app.cpp: add explicit <ctime> include for std::tm/time_t/mktime instead of relying on a transitive include. field_registry.cpp: strip CR/LF from field names (sourced from UDP input) before using them as a map key, persisting, or logging, so an embedded newline cannot split one registry entry across lines. tractor_facilities.hpp/.cpp: fix doc comments/log wording that claimed PGN 65032 handling is logged at debug level - there is no such gating today, it always prints. Also stop send_facilities_response() from unconditionally logging (power-up) when it is actually called from the PGN 65033 request handler. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
sujandumaru
reviewed
Sep 8, 2026
Collaborator
There was a problem hiding this comment.
Do we not want to put the implementation on the .cpp file?
|
|
||
| default: | ||
| // Handle ActualTramlineCondensedWorkState DDIs (Level 3 feedback from implement) | ||
| if ((dataDescriptionIndex >= static_cast<std::uint16_t>(isobus::DataDescriptionIndex::ActualTramlineCondensedWorkState1_16) && |
Collaborator
There was a problem hiding this comment.
Not confident about this DDI limit.
ActualTramlineCondensedWorkState1_16 = 518
.... other DDIs here...
ActualTramlineCondensedWorkState17_32 = 603
.... continue Tramline CWS till ...
ActualTramlineCondensedWorkState241_256 = 617
(https://www.isobus.net/isobus/site/exports?view=export)
So, we are going to allow all other DDIs in-between to pass in here as well even if they are not related to tramline. Also, don't we need CWS upto 241_256 section as of now?
sujandumaru
reviewed
Sep 8, 2026
| GuidanceTrackProvider trackProvider; | ||
| GuidanceTrackContext currentTrackContext; | ||
| bool aogWasConnectedForTrack = false; ///< Edge-detection for AOG connect/disconnect transitions | ||
| bool trackControlEnabled = false; ///< Track control enabled (separate from section control) |
Collaborator
There was a problem hiding this comment.
I don't see this being used in app.cpp file.
Contributor
Author
There was a problem hiding this comment.
Ah yes this is an artifact from the tests where I tried to generate a track number based on the Level3 like behaviour of AgOpenGPS.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces significant enhancements to both the codebase and documentation, primarily focused on supporting new ISOBUS features, improving protocol clarity, and expanding test coverage. The main themes are: expanded documentation for new PGNs and DDIs (especially for tramline/TRACK and tractor facilities), new code to support these features (including time/date and tractor facilities interfaces), and the addition of unit tests for tractor facilities encoding/decoding.
Documentation improvements and protocol clarifications:
docs/PROTOCOL.mdto document support for new inbound/outbound PGNs, especially GPS/IMU data (0xD6), machine data (0xEF), and guidance track context (0xF4). Added detailed explanations of their payloads, usage, and fallback behaviors. Also clarified source addresses and maximum payload sizes. [1] [2] [3] [4]Codebase enhancements for new ISOBUS features:
include/app.hppto add members and interfaces forTractorFacilities,TimeDateInterface, and related state (e.g., FEE6 broadcast timing, GNSS fix quality, guidance track context, and tramline/TRACK control state). This enables the application to broadcast tractor facilities, time/date, and track context, and to track AOG connectivity and GNSS quality. [1] [2] [3] [4]Testing improvements:
test_tractor_facilities) for Tractor Facilities (PGN 65033) encode/decode logic, ensuring correctness of this new protocol feature.Documentation and protocol expansion:
0xD6,0xEF,0xF4) and clarified their payloads, sources, and fallback behaviors indocs/PROTOCOL.md. [1] [2] [3] [4]ISOBUS feature support in code:
Applicationclass ininclude/app.hppto support tractor facilities, time/date interface, GNSS quality tracking, guidance track context, and tramline/TRACK control state. [1] [2] [3] [4]Testing:
CMakeLists.txt).