[AUX-N]: Add preferred assignments functionality#655
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds preferred auxiliary function assignment functionality to the Virtual Terminal Client, enabling applications to persist auxiliary input device assignments through user-provided callback functions. The implementation follows ISO 11783 specifications with proper state machine handling, timeout/retry logic, and device lifetime management.
- Introduced state machine states for sending and waiting for auxiliary preferred assignment responses with retry logic (up to 3 attempts)
- Added callback mechanism for loading and storing auxiliary function assignments, allowing applications to implement persistent storage
- Implemented device timeout monitoring (300ms per ISO 11783-6) with automatic cleanup of offline devices
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| isobus/include/isobus/isobus/isobus_virtual_terminal_client.hpp | Added new state machine states, callback typedefs, timeout constants, retry counter, and modified struct to support timestamp tracking; corrected typo in documentation |
| isobus/src/isobus_virtual_terminal_client.cpp | Implemented callback setter, state machine logic for preferred assignments with retry/timeout handling, device timeout monitoring, assignment message construction with serialization, and storage callback invocations |
| examples/virtual_terminal/aux_functions/main.cpp | Added example implementation of load/store callbacks using in-memory map storage, and enabled Debug logging to demonstrate the new functionality |
| .gitignore | Added exclusion for .claude directory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::vector<std::uint8_t> buffer = { | ||
| static_cast<std::uint8_t>(Function::PreferredAssignmentCommand) | ||
| }; | ||
|
|
||
| /// @todo make the load operation asynchronous | ||
| // Load stored assignments into pending storage if callback is registered | ||
| if (nullptr != auxiliaryAssignmentLoadCallback) | ||
| { | ||
| buffer.push_back(static_cast<std::uint8_t>(devices.size())); | ||
| for (const AssignedAuxiliaryInputDevice &device : devices) | ||
| { | ||
| buffer.push_back(static_cast<std::uint8_t>(device.name)); | ||
| buffer.push_back(static_cast<std::uint8_t>(device.name >> 8)); | ||
| buffer.push_back(static_cast<std::uint8_t>(device.name >> 16)); | ||
| buffer.push_back(static_cast<std::uint8_t>(device.name >> 24)); | ||
| buffer.push_back(static_cast<std::uint8_t>(device.name >> 32)); | ||
| buffer.push_back(static_cast<std::uint8_t>(device.name >> 40)); | ||
| buffer.push_back(static_cast<std::uint8_t>(device.name >> 48)); | ||
| buffer.push_back(static_cast<std::uint8_t>(device.name >> 56)); | ||
| buffer.push_back(static_cast<std::uint8_t>(device.modelIdentificationCode)); | ||
| buffer.push_back(static_cast<std::uint8_t>(device.modelIdentificationCode >> 8)); | ||
|
|
||
| auto loadedAssignments = auxiliaryAssignmentLoadCallback(device.name, device.modelIdentificationCode, auxiliaryAssignmentCallbackContext); | ||
| LOG_DEBUG("[AUX-N]: Loaded " + isobus::to_string(loadedAssignments.size()) + " stored assignments for device, awaiting VT confirmation."); | ||
|
|
||
| buffer.push_back(static_cast<std::uint8_t>(loadedAssignments.size())); | ||
| for (const AssignedAuxiliaryFunction &function : loadedAssignments) | ||
| { | ||
| buffer.push_back(static_cast<std::uint8_t>(function.functionObjectID)); | ||
| buffer.push_back(static_cast<std::uint8_t>(function.functionObjectID >> 8)); | ||
| buffer.push_back(static_cast<std::uint8_t>(function.inputObjectID)); | ||
| buffer.push_back(static_cast<std::uint8_t>(function.inputObjectID >> 8)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The buffer is constructed with repeated push_back operations for serialization. For better performance, consider pre-calculating the required buffer size and using reserve() before the loop, or using a more efficient serialization method. The current approach may cause multiple reallocations as the buffer grows, especially when there are many devices and assignments.
| else | ||
| { | ||
| buffer.push_back(0); | ||
| LOG_WARNING("[AUX-N] Sending preferred assignemnts, but no auxiliary assignment load callback registered."); |
There was a problem hiding this comment.
Spelling error: "assignemnts" should be "assignments"
| LOG_WARNING("[AUX-N] Sending preferred assignemnts, but no auxiliary assignment load callback registered."); | |
| LOG_WARNING("[AUX-N] Sending preferred assignments, but no auxiliary assignment load callback registered."); |
| { | ||
| hasError = true; | ||
| LOG_WARNING("[AUX-N]: Unable to store preferred assignment due to unsupported function type: " + isobus::to_string(functionType)); | ||
| LOG_WARNING("[AUX-N]: Unable to to handle assignment due to unsupported function type: " + isobus::to_string(functionType)); |
There was a problem hiding this comment.
Spelling error: "to to handle" should be "to handle" (duplicate "to")
| LOG_WARNING("[AUX-N]: Unable to to handle assignment due to unsupported function type: " + isobus::to_string(functionType)); | |
| LOG_WARNING("[AUX-N]: Unable to handle assignment due to unsupported function type: " + isobus::to_string(functionType)); |
|
|
||
| isobus::CANStackLogger::set_can_stack_logger_sink(&logger); | ||
| isobus::CANStackLogger::set_log_level(isobus::CANStackLogger::LoggingLevel::Info); // Change this to Debug to see more information | ||
| isobus::CANStackLogger::set_log_level(isobus::CANStackLogger::LoggingLevel::Debug); // Change this to Debug to see more information |
There was a problem hiding this comment.
The log level has been changed from Info to Debug. While this may be helpful for development and testing of the new auxiliary assignment functionality, consider whether this should remain in the example code permanently, as it will generate significantly more log output for users. A comment explaining why Debug level is recommended for this example would be helpful.
| isobus::CANStackLogger::set_log_level(isobus::CANStackLogger::LoggingLevel::Debug); // Change this to Debug to see more information | |
| // Use Debug level here so this example shows detailed VT and auxiliary assignment activity. | |
| // This will generate significantly more log output; for production use, consider using Info instead. | |
| isobus::CANStackLogger::set_log_level(isobus::CANStackLogger::LoggingLevel::Debug); |
| LOG_WARNING("[AUX-N]: Preferred Assignment OK response timeout (attempt " + | ||
| isobus::to_string(static_cast<int>(auxiliaryAssignmentRetryCount)) + | ||
| "/3). Retrying..."); |
There was a problem hiding this comment.
The log message is potentially confusing. The retry count is incremented before logging (line 1707), so when it shows "attempt 1/3", this is actually the 2nd send attempt (1st retry). Consider adjusting the log message to clarify this is a retry count, not an attempt count. For example: "Retrying (retry 1/2)" or incrementing after the log to show "attempt 2/3".
| LOG_WARNING("[AUX-N]: Preferred Assignment OK response timeout (attempt " + | |
| isobus::to_string(static_cast<int>(auxiliaryAssignmentRetryCount)) + | |
| "/3). Retrying..."); | |
| LOG_WARNING("[AUX-N]: Preferred Assignment OK response timeout (retry " + | |
| isobus::to_string(static_cast<int>(auxiliaryAssignmentRetryCount)) + | |
| "/2). Retrying..."); |
| { | ||
| parentVT->auxiliaryAssignmentRetryCount++; | ||
| LOG_WARNING("[AUX-N]: Retrying preferred assignment (attempt " + | ||
| isobus::to_string(static_cast<int>(parentVT->auxiliaryAssignmentRetryCount)) + "/3)..."); |
There was a problem hiding this comment.
The log message shows "attempt X/3" but this could be inconsistent with line 1708-1710. Here the retry count is incremented first (line 2732) before logging, so the attempt number shown may be confusing to users. Consider using consistent messaging across both locations.
| isobus::to_string(static_cast<int>(parentVT->auxiliaryAssignmentRetryCount)) + "/3)..."); | |
| isobus::to_string(static_cast<int>(parentVT->auxiliaryAssignmentRetryCount + 1U)) + "/3)..."); |
| if (parentVT->get_is_connected()) | ||
| { | ||
| // Send preferred assignments to VT for confirmation | ||
| if (parentVT->send_auxiliary_functions_preferred_assignment({ inputDevice })) | ||
| { | ||
| LOG_DEBUG("[AUX-N]: Successfully sent preferred assignments for new device."); | ||
| } | ||
| else | ||
| { | ||
| LOG_WARNING("[AUX-N]: Failed to send preferred assignments for new device."); | ||
| } | ||
| } |
There was a problem hiding this comment.
When a new auxiliary input device is detected and the VT is already connected, preferred assignments are sent immediately (line 3341) without going through the state machine. This bypasses the retry logic and timeout handling that exists in the SendAuxiliaryPreferredAssignment and WaitForPreferredAssignmentResponse states. Consider whether this needs retry logic or if the direct send is intentional for this specific case.
| std::uint64_t name; ///< The NAME of the unit | ||
| std::uint16_t modelIdentificationCode; ///< The model identification code |
There was a problem hiding this comment.
The const qualifiers were removed from the name and modelIdentificationCode fields. While this change is necessary to support the timestamp updates (line 3354), consider whether these fields should still be conceptually immutable. If a device's NAME or model ID should never change after creation, consider alternative designs such as using the timestamp field as mutable while keeping these fields const, or using a key-based lookup structure.
|



This PR adds the functionality for auxiliary assignments to work. One needs to register a store and load callback in the application to make the assignments persistent.