Fix ddop filename #22

gunicsba · 2025-12-30T11:22:18Z

Filename should be DDOP.
some implements have the Ascii 3 character meaning no more text which we should consider.

When the activity is suspended (e.g. AUX-N master switch) the painting is also suspended in AgOpenGPS.

… into gunicsba/section-fixes

…üller Jobrechner ECU.

…ement. Fixed the parameter names in the help function of the console.

…onfusing that it works with 2 boolean values at the same time.

…onfusing that it works with 2 boolean values at the same time. + Fix formatting

…AOG-TaskController into gunicsba/section-fixes

…onfusing that it works with 2 boolean values at the same time.

…AOG-TaskController into gunicsba/section-fixes

The ISO Wheel and Radar speed is coming from the Tractor BUS and is provided by the Tractor ECU. The NMEA2000 speed was x10

…enGPS other than enabling Zones)

…csba/AOG-TaskController into develop # Conflicts: # src/app.cpp Bump stack version to the latest and the minor version to 1.3

…ext" ASCII character in their name. Also the extension should be .ddop not .iop

GwnDaan · 2025-12-30T12:07:54Z

Run clang format

Copilot

Pull request overview

This PR fixes the DDOP (Device Descriptor Object Pool) filename generation and implements several enhancements to the Task Controller functionality. The main change updates the file extension from ".iop" to ".ddop" and adds proper handling for label trimming when null or ETX (0x03) characters are present.

Key changes:

Fixed DDOP filename extension from ".iop" to ".ddop" and improved label string handling to trim at null or ETX characters
Added per-element work state management to properly handle section states based on master element work states
Increased section capacity from 16 to 64 sections and added support for both modern ECU (SetpointCondensedWorkState) and fallback (ActualCondensedWorkState) communication patterns

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/task_controller.cpp	Fixed DDOP filename extension, added element work state tracking, improved section state calculation, and enhanced fallback logic for ECU compatibility
src/main.cpp	Updated help text to reflect correct command-line parameter names (can_adapter, can_channel)
src/app.cpp	Added separate TECU control function creation, improved null pointer guards for speed/NMEA interfaces, and expanded section data handling to support 64 sections
include/task_controller.hpp	Added new methods for per-element work state management and included data dictionary header
include/app.hpp	Added tecuCF member variable to store TECU control function reference
CMakeLists.txt	Updated version to 1.3.0 and updated isobus library dependency to newer commit

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-30T12:10:54Z

src/task_controller.cpp

+
+		auto labelBytes = deviceObject->get_localization_label();
+		std::string label(reinterpret_cast<const char *>(labelBytes.data()), labelBytes.size());
+		// trim at first occurrence of null or ETX (0x03)


The comment uses lowercase "ddop" but the actual file extension is ".ddop". For consistency and clarity, consider using uppercase "DDOP" in the comment to match the file extension and standard terminology.

Copilot · 2025-12-30T12:10:54Z

src/task_controller.cpp

 		value |= (clients[client].get_section_setpoint_state(sectionOffset + i) << (2 * i));
 	}

+	// Modern ECU? (DDI 290  SetpointCondensedWorkState1_16 exists)


There's an extra space in the comment between "290" and "SetpointCondensedWorkState1_16". Should be a single space for consistency.

Suggested change

// Modern ECU? (DDI 290 SetpointCondensedWorkState1_16 exists)

// Modern ECU? (DDI 290 SetpointCondensedWorkState1_16 exists)

Copilot · 2025-12-30T12:10:54Z

src/task_controller.cpp

+		return;
 	}
+
+	std::cout << "[TC Server] Neither condensed nor controllable-actual work state supported Missing DDI 290 and 141!" << std::endl;


The error message has a missing period before "Missing". Should be: "Neither condensed nor controllable-actual work state supported. Missing DDI 290 and 141!"

Suggested change

std::cout << "[TC Server] Neither condensed nor controllable-actual work state supported Missing DDI 290 and 141!" << std::endl;

std::cout << "[TC Server] Neither condensed nor controllable-actual work state supported. Missing DDI 290 and 141!" << std::endl;

Copilot · 2025-12-30T12:10:55Z

src/app.cpp

-				sectionStates.push_back(data[6] & (1 << i));
-			}
-			for (std::uint8_t i = 0; i < 8; i++)
+			for (std::uint8_t scb = 0; scb < 8; scb++) 


There's trailing whitespace after 'scb' on this line. Consider removing it for code cleanliness.

Suggested change

for (std::uint8_t scb = 0; scb < 8; scb++)

for (std::uint8_t scb = 0; scb < 8; scb++)

Copilot · 2025-12-30T12:10:55Z

src/app.cpp

-				sectionStates.push_back(data[6] & (1 << i));
-			}
-			for (std::uint8_t i = 0; i < 8; i++)
+			for (std::uint8_t scb = 0; scb < 8; scb++) 


The variable name 'scb' is unclear and cryptic. Consider using a more descriptive name like 'byteIndex' or 'sectionByte' to improve code readability.

Copilot · 2025-12-30T12:10:55Z

include/task_controller.hpp

 	void mark_measurement_commands_sent();
 	std::uint16_t get_element_number_for_ddi(isobus::DataDescriptionIndex ddi) const;
 	void set_element_number_for_ddi(isobus::DataDescriptionIndex ddi, std::uint16_t elementNumber);
+	// Element work state management these act like master / override for actual sections


The comment is missing proper capitalization at the start. It should be: "Element work state management: these act like master/override for actual sections" or split into a proper sentence structure.

Suggested change

// Element work state management these act like master / override for actual sections

// Element work state management: these act like master/override for actual sections

gunicsba added 14 commits December 24, 2025 11:02

Tested with 16 sections, and added the check for the master switch.

572b78a

When the activity is suspended (e.g. AUX-N master switch) the painting is also suspended in AgOpenGPS.

Merge branch 'develop' of https://github.com/GwnDaan/AOG-TaskController…

768677a

… into gunicsba/section-fixes

Added support for legacy section control DDI-s required by Damman / M…

b7e6273

…üller Jobrechner ECU.

Increased CTS packet count to get rid of message flood from some impl…

983c3fb

…ement. Fixed the parameter names in the help function of the console.

Renamed the get_element to try_get_element_work_state as I found it c…

c677e8c

…onfusing that it works with 2 boolean values at the same time.

Renamed the get_element to try_get_element_work_state as I found it c…

fccd331

…onfusing that it works with 2 boolean values at the same time. + Fix formatting

Merge branch 'gunicsba/section-fixes' of https://github.com/gunicsba/…

547bd67

…AOG-TaskController into gunicsba/section-fixes

Renamed the get_element to try_get_element_work_state as I found it c…

94369f9

…onfusing that it works with 2 boolean values at the same time.

Merge branch 'gunicsba/section-fixes' of https://github.com/gunicsba/…

d704409

…AOG-TaskController into gunicsba/section-fixes

Separate the Tractor ECU function from Task Controller.

ec80df3

The ISO Wheel and Radar speed is coming from the Tractor BUS and is provided by the Tractor ECU. The NMEA2000 speed was x10

Execute clang-format

69f9bf4

64 sections are now supported by TC (no changes are necessary in AgOp…

4287e03

…enGPS other than enabling Zones)

Merge branch 'gunicsba/fix-speed-add-tecu' of https://github.com/guni…

ff8c863

…csba/AOG-TaskController into develop # Conflicts: # src/app.cpp Bump stack version to the latest and the minor version to 1.3

Fix the DDOP filename generation. Some implements have the "no more t…

01d2bc0

…ext" ASCII character in their name. Also the extension should be .ddop not .iop

GwnDaan requested a review from Copilot December 30, 2025 12:07

Copilot started reviewing on behalf of GwnDaan December 30, 2025 12:08 View session

Copilot AI reviewed Dec 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix ddop filename #22

Fix ddop filename #22

Uh oh!

gunicsba commented Dec 30, 2025

Uh oh!

GwnDaan commented Dec 30, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 30, 2025

Uh oh!

Copilot AI Dec 30, 2025

Uh oh!

Copilot AI Dec 30, 2025

Uh oh!

Copilot AI Dec 30, 2025

Uh oh!

Copilot AI Dec 30, 2025

Uh oh!

Copilot AI Dec 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	// Modern ECU? (DDI 290 SetpointCondensedWorkState1_16 exists)
	// Modern ECU? (DDI 290 SetpointCondensedWorkState1_16 exists)

	std::cout << "[TC Server] Neither condensed nor controllable-actual work state supported Missing DDI 290 and 141!" << std::endl;
	std::cout << "[TC Server] Neither condensed nor controllable-actual work state supported. Missing DDI 290 and 141!" << std::endl;

	for (std::uint8_t scb = 0; scb < 8; scb++)
	for (std::uint8_t scb = 0; scb < 8; scb++)

	// Element work state management these act like master / override for actual sections
	// Element work state management: these act like master/override for actual sections

Uh oh!

Fix ddop filename #22

Are you sure you want to change the base?

Fix ddop filename #22

Uh oh!

Conversation

gunicsba commented Dec 30, 2025

Uh oh!

GwnDaan commented Dec 30, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants