Skip to content

String and string array fixes#513

Open
RobertoRoos wants to merge 5 commits into
stlehmann:masterfrom
RobertoRoos:bugfix/string-arrays
Open

String and string array fixes#513
RobertoRoos wants to merge 5 commits into
stlehmann:masterfrom
RobertoRoos:bugfix/string-arrays

Conversation

@RobertoRoos

Copy link
Copy Markdown
Contributor

Recreation of faulty PR:

Trying to improve string handling to solve some of the open issues.

Frankly I'm a bit lost what the issues are exactly, I'm trying to go through them slowly. EDIT: Okay, I think this is about it for now. I think this resolves all string issues?

Resolves:

Should resolve (but I haven't reproduced the issue before):

Obsoletes other PRs:

@stlehmann stlehmann left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

In process...

@stlehmann stlehmann left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for putting this together — the overall direction is really good. Centralising string/WSTRING decoding through get_value_from_ctype_data, removing the old PLCTYPE_WSTRING sentinel class in favour of a real ctypes type, and fixing the STRING(n) null-terminator off-by-one in get_type_from_str are all solid improvements.

Left two inline notes — one is a concrete suggestion to fix the failing test_read_wstring CI test, the other is just a platform portability observation. Otherwise the change looks good to me; once you rebase onto current master and push, CI should give a fresh green run.


Generated by Claude Code

Comment on lines 1454 to +1455
expected1.encode("utf-16-le") + b"\x00\x00",
constants.ADST_WSTRING, f"WSTRING({len(expected1)})"
constants.ADST_WSTRING, "WSTRING(80)"

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is what's causing the CI failure. The fixture now declares "WSTRING(80)", but the value bytes are still only as long as the actual content (len(expected1) * 2 + 2 = 26 bytes). On a real PLC a WSTRING(80) variable always occupies the full declared size (80 * 2 + 2 = 162 bytes), padded with zeros after the null terminator.

The adsSumRead path infers scalar-vs-array by dividing the symbol's reported .size by the per-element byte length derived from the declared type — so when .size (26) doesn't match element_size (162), it concludes it must be an array of int(26/162) == 0 elements and returns [].

Padding the value to the declared byte length fixes the immediate failure and makes the fixture match real PLC behaviour:

Suggested change
expected1.encode("utf-16-le") + b"\x00\x00",
constants.ADST_WSTRING, f"WSTRING({len(expected1)})"
constants.ADST_WSTRING, "WSTRING(80)"
(expected1.encode("utf-16-le") + b"\x00\x00").ljust(80 * 2 + 2, b"\x00"),
constants.ADST_WSTRING, "WSTRING(80)"

Generated by Claude Code

Comment thread src/pyads/constants.py
PLCTYPE_REAL = c_float
PLCTYPE_SINT = c_int8
PLCTYPE_STRING = c_char
PLCTYPE_WSTRING = c_wchar

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Small platform note: c_wchar is 2 bytes on Windows but 4 bytes on Linux/macOS. The good news is that bytes(ctypes_array) always returns raw memory regardless of ctypes type interpretation, so bytes(data).decode("utf-16-le").rstrip("\x00") in get_value_from_ctype_data works correctly on both platforms.

One side-effect though is that the read buffer allocated in adsSyncReadReqEx2 becomes c_wchar * STRING_BUFFER = 4 * 1024 = 4096 bytes on Linux, where the old code used c_uint8 * STRING_BUFFER = 1024 bytes. Requesting more bytes than the symbol holds should be harmless in practice (the ADS library returns what's available and the length check is skipped for strings), but it's worth a quick sanity check if you have a live PLC to test against.


Generated by Claude Code

@RobertoRoos

Copy link
Copy Markdown
Contributor Author

I'll try to have another look at this in the coming week. But it feels like a decade since I worked on this, so it's not so easy 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants