gijzelaerr / python-snap7

A Python wrapper for the snap7 PLC communication library
http://python-snap7.readthedocs.org/
MIT License
643 stars 246 forks source link

Support fixed size strings #375

Closed vmsh0 closed 2 years ago

vmsh0 commented 2 years ago

python-snap7 supports length-prefixed strings, but it does not support fixed-length strings.

This is not such an issue when manually unpacking reads with a pattern such as

b = client.db_read(10, 0, 20)
varA = snap7.util.get_byte(b, 0)
varB = snap7.util.get_byte(b, 1)
varString = ""
for i in range(18):
   varString = varString + snap7.util.get_char(b, 2+i)

However, when using the snap7.util.DB class, this doesn't work, as STRINGs are decoded with the expectation of being length-prefixed with no other option (but treating each character separately, which is not optimal).

If you're ok with it and there's the expectation that it will be pulled, I can provide a patch to implement a second set of string utility functions and support for the DB class, to deal specifically with fixed-length strings.

This could be implemented both as a new separate fstring type:

Or it could be implemented as a flag:

As mentioned, I can provide a patch for either approach. Please let me know whether you like any of those or if you have a different approach in mind to support this use case.

swamper123 commented 2 years ago

Hi,

sorry for the late response.

First: Helpfull PRs are always welcomed. :)

But tbh I still haven't understood 100%ly what you mean/your goal is.

STRINGS are defined with a prefix (max. string length and actual string length), because Siemens said so in the specs. And if I say this value is a STRING, I would expect to be able to reuse it to communicate with a real PLC. Also the values in a PLC are stored as single characters as well ((array like). That's why we got a byte_index parameter.

So why want you introduce/use a new FSTRING type, which is not defined in the specs?

vmsh0 commented 2 years ago

Thank you for the reply!

It's pretty common in industrial settings to find setups where strings represented as arrays of chars, either 0-terminated or space-padded, as opposed to using the actual length-prefixed string type.

So what I propose is adding a data type to this library to be able to parse those representations as strings. Yes, that would be out of spec (as in /not in the spec/, not /against the spec/), but it does represent an actual use case.

We are actually doing that already on an internal fork of the library, and it will be deployed in production some time in August. If you think it would be good to have in the library, I can offer it as a PR, as we would be happy to use upstream directly instead of rolling our own version. :)

Il ven 29 lug 2022, 13:11 Fabian Beitler @.***> ha scritto:

Hi,

sorry for the late response.

First: Helpfull PRs are always welcomed. :)

But tbh I still haven't understood 100%ly what you mean/your goal is.

STRINGS are defined with a prefix (max. string length and actual string length), because Siemens said so in the specs. And if I say this value is a STRING, I would expect to be able to reuse it to communicate with a real PLC. Also the values in a PLC are stored as single characters as well ((array like). That's why we got a byte_index parameter.

So why want you introduce/use a new FSTRING type, which is not defined in the specs?

— Reply to this email directly, view it on GitHub https://github.com/gijzelaerr/python-snap7/issues/375#issuecomment-1199156256, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAO5NKLWSHZ46VTSDDSRNFLVWO36TANCNFSM52SXCP4Q . You are receiving this because you authored the thread.Message ID: @.***>

vmsh0 commented 2 years ago

I have submitted the PR. As mentioned, we are using this already in the real world & we would love to see this into python-snap7 to go back to using upstream directly. However, I completely understand if this can't be integrated due to it not being officially specified.

gijzelaerr commented 2 years ago

merged, thanks!