Skip to content

[BUG]coCognexVision.TcoDataman_v_5_x_x Read should not prodduce error when data were not read #47

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
peterbarancek merged 2 commits into from
Mar 3, 2026
Merged

[BUG]coCognexVision.TcoDataman_v_5_x_x Read should not prodduce error when data were not read #47

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
570db01
Create draft PR for #46
github-actions[bot] Mar 3, 2026
10e9dd8
*trigger will not produce error if read is not successfull
peterbarancek Mar 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions ...on/src/XAE/XAE/TcoCognexVision/POUs/TcoDataman/v_5_x_x/Tasks/TcoDatamanRead_v_5_x_x.TcPOU
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,22 +87,31 @@ END_VAR
IF _progress = 6 THEN
THIS^.inoResults.Length := THIS^.inoIoData.ResultData.ResultLength;
THIS^.Messenger.OnCondition(_infoTimer.Q).Warning('<#Waiting for the ResultData copied!#>').Pin();
IF Tc2_System.MEMCPY(srcAddr := ADR(THIS^.inoIoData.ResultData.ResultData) , destAddr:= ADR(THIS^.inoResults.Data) , n:= THIS^.inoResults.Length) > 0 AND
Tc2_System.MEMSET(destAddr:= ADR(THIS^.inoResults.Data) + THIS^.inoResults.Length, fillByte:= 0 , n:= SIZEOF(THIS^.inoResults.Data) - THIS^.inoResults.Length)>0 THEN
IF THIS^.inoIoData.ResultData.ResultCode.0=1 THEN // read successfull
IF Tc2_System.MEMCPY(srcAddr := ADR(THIS^.inoIoData.ResultData.ResultData) , destAddr:= ADR(THIS^.inoResults.Data) , n:= THIS^.inoResults.Length) > 0 AND
Tc2_System.MEMSET(destAddr:= ADR(THIS^.inoResults.Data) + THIS^.inoResults.Length, fillByte:= 0 , n:= SIZEOF(THIS^.inoResults.Data) - THIS^.inoResults.Length)>0 THEN
Comment on lines 88 to +92
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MEMCPY uses n := inoResults.Length without any bounds check against the source buffer (inoIoData.ResultData.ResultData, 246 bytes) and destination buffer (inoResults.Data, 256 bytes). If ResultLength is larger than either buffer, this will read/write out of bounds and the subsequent SIZEOF(...) - Length can underflow. Clamp inoResults.Length to MIN(ResultLength, SIZEOF(src), SIZEOF(dst)) (and handle truncation explicitly).

Copilot uses AI. Check for mistakes.
CallTimers(FALSE);
_progress := _progress + 1;
END_IF;
ELSIF THIS^.inoIoData.ResultData.ResultCode.0=0 THEN // read not successfull
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the “read not successful” path, inoResults is not cleared. Because the task now completes successfully even when no data was read, callers can observe stale inoResults.Data from a previous successful read. Consider explicitly setting inoResults.Length := 0 and clearing inoResults.Data (and/or setting a status flag) when ResultCode indicates no-read.

Suggested change
ELSIF THIS^.inoIoData.ResultData.ResultCode.0=0 THEN // read not successfull
ELSIF THIS^.inoIoData.ResultData.ResultCode.0=0 THEN // read not successfull
// Clear results to avoid exposing stale data when read was not successful
THIS^.inoResults.Length := 0;
Tc2_System.MEMSET(destAddr:= ADR(THIS^.inoResults.Data), fillByte:= 0, n:= SIZEOF(THIS^.inoResults.Data));

Copilot uses AI. Check for mistakes.
Comment on lines +90 to +96
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The success criteria here (ResultCode.0=1) is inconsistent with the rest of the Dataman implementation, e.g. TcoDataman_v_5_x_x.ContinuousReading uses ResultCode > 0 AND ResultLength > 0 (TcoDataman_v_5_x_x.TcPOU:160). If ResultCode can be non-zero but not have bit0 set, this task will treat it as “not successful” and skip copying. Consider aligning the condition (or document why bit0 specifically is required).

Suggested change
IF THIS^.inoIoData.ResultData.ResultCode.0=1 THEN // read successfull
IF Tc2_System.MEMCPY(srcAddr := ADR(THIS^.inoIoData.ResultData.ResultData) , destAddr:= ADR(THIS^.inoResults.Data) , n:= THIS^.inoResults.Length) > 0 AND
Tc2_System.MEMSET(destAddr:= ADR(THIS^.inoResults.Data) + THIS^.inoResults.Length, fillByte:= 0 , n:= SIZEOF(THIS^.inoResults.Data) - THIS^.inoResults.Length)>0 THEN
CallTimers(FALSE);
_progress := _progress + 1;
END_IF;
ELSIF THIS^.inoIoData.ResultData.ResultCode.0=0 THEN // read not successfull
IF (THIS^.inoIoData.ResultData.ResultCode > 0) AND (THIS^.inoIoData.ResultData.ResultLength > 0) THEN // read successful
IF Tc2_System.MEMCPY(srcAddr := ADR(THIS^.inoIoData.ResultData.ResultData) , destAddr:= ADR(THIS^.inoResults.Data) , n:= THIS^.inoResults.Length) > 0 AND
Tc2_System.MEMSET(destAddr:= ADR(THIS^.inoResults.Data) + THIS^.inoResults.Length, fillByte:= 0 , n:= SIZEOF(THIS^.inoResults.Data) - THIS^.inoResults.Length)>0 THEN
CallTimers(FALSE);
_progress := _progress + 1;
END_IF;
ELSIF (THIS^.inoIoData.ResultData.ResultCode <= 0) OR (THIS^.inoIoData.ResultData.ResultLength = 0) THEN // read not successful

Copilot uses AI. Check for mistakes.
Comment on lines +90 to +96
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling in the inline comments: “successfull” should be “successful” (also in “not successfull”).

Suggested change
IF THIS^.inoIoData.ResultData.ResultCode.0=1 THEN // read successfull
IF Tc2_System.MEMCPY(srcAddr := ADR(THIS^.inoIoData.ResultData.ResultData) , destAddr:= ADR(THIS^.inoResults.Data) , n:= THIS^.inoResults.Length) > 0 AND
Tc2_System.MEMSET(destAddr:= ADR(THIS^.inoResults.Data) + THIS^.inoResults.Length, fillByte:= 0 , n:= SIZEOF(THIS^.inoResults.Data) - THIS^.inoResults.Length)>0 THEN
CallTimers(FALSE);
_progress := _progress + 1;
END_IF;
ELSIF THIS^.inoIoData.ResultData.ResultCode.0=0 THEN // read not successfull
IF THIS^.inoIoData.ResultData.ResultCode.0=1 THEN // read successful
IF Tc2_System.MEMCPY(srcAddr := ADR(THIS^.inoIoData.ResultData.ResultData) , destAddr:= ADR(THIS^.inoResults.Data) , n:= THIS^.inoResults.Length) > 0 AND
Tc2_System.MEMSET(destAddr:= ADR(THIS^.inoResults.Data) + THIS^.inoResults.Length, fillByte:= 0 , n:= SIZEOF(THIS^.inoResults.Data) - THIS^.inoResults.Length)>0 THEN
CallTimers(FALSE);
_progress := _progress + 1;
END_IF;
ELSIF THIS^.inoIoData.ResultData.ResultCode.0=0 THEN // read not successful

Copilot uses AI. Check for mistakes.
Tc2_System.MEMSET(destAddr:= ADR(THIS^.inoIoData) , fillByte:= 0 , n:= SIZEOF(THIS^.inoIoData));
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tc2_System.MEMSET(ADR(inoIoData), SIZEOF(inoIoData)) clears the entire IO struct, including mapped %Q outputs (PnIoBoxCtrl, AcquisitionControl, ResultsControl, etc.) and %I inputs. This can unintentionally reset device control signals (e.g., TriggerEnable) and also masks real input status values in the process image. Prefer clearing only the result-related fields (e.g., inoIoData.ResultData / inoResults) rather than zeroing the whole IO structure.

Suggested change
Tc2_System.MEMSET(destAddr:= ADR(THIS^.inoIoData) , fillByte:= 0 , n:= SIZEOF(THIS^.inoIoData));
Tc2_System.MEMSET(destAddr:= ADR(THIS^.inoIoData.ResultData), fillByte:= 0, n:= SIZEOF(THIS^.inoIoData.ResultData));
Tc2_System.MEMSET(destAddr:= ADR(THIS^.inoResults), fillByte:= 0, n:= SIZEOF(THIS^.inoResults));

Copilot uses AI. Check for mistakes.
CallTimers(FALSE);
_progress := _progress + 1;

END_IF
END_IF

IF _progress = 7 THEN
THIS^.inoIoData.AcquisitionControl.Trigger := FALSE;
THIS^.inoIoData.ResultsControl.EnableResultBuffering := FALSE;
THIS^.inoIoData.ResultsControl.ResultsAcknowledge := TRUE;
THIS^.ThrowWhen(THIS^.inoIoData.ResultData.ResultCode=0 OR THIS^.inoIoData.ResultData.ResultLength=0);
//THIS^.ThrowWhen(THIS^.inoIoData.ResultData.ResultCode=0 OR THIS^.inoIoData.ResultData.ResultLength=0);
THIS^.DoneWhen(TRUE);
_progress := 0;
END_IF

THIS^.ThrowWhen(THIS^.inoIoData.AcquisitionStatus.MissedAcquisition );
THIS^.ThrowWhen(THIS^.inoIoData.ResultsStatus.ErrorDetected);
Comment on lines +112 to +113
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new unconditional ThrowWhen(...) checks dereference inoIoData even when _progress is 0 and __ISVALIDREF(inoIoData) is false (the task does not early-return after pinning the invalid-reference message). This can cause a runtime fault when the FB is invoked with an invalid VAR_IN_OUT reference. Guard these ThrowWhen calls with __ISVALIDREF(inoIoData) (or execute them only after the reference-validation block has passed).

Suggested change
THIS^.ThrowWhen(THIS^.inoIoData.AcquisitionStatus.MissedAcquisition );
THIS^.ThrowWhen(THIS^.inoIoData.ResultsStatus.ErrorDetected);
IF __ISVALIDREF(inoIoData) THEN
THIS^.ThrowWhen(THIS^.inoIoData.AcquisitionStatus.MissedAcquisition );
THIS^.ThrowWhen(THIS^.inoIoData.ResultsStatus.ErrorDetected);
END_IF

Copilot uses AI. Check for mistakes.

CallTimers(TRUE);

THIS^.ThrowWhen(_errorTimer.Q);
Expand Down
Loading