Latest modbus.py/Master.execute() fails to decode.

[kiml]

I was using pymodslave with my test app and it worked fine UNTIL I updated to the GitHub rev of modus_tk.

Data was no longer being decoded correctly. I traced it to these lines in modbus.py some of which which appear to be new. ~line 383: if (re.match("[>]?[sp]?",data_format)): result = data.decode() else: result = struct.unpack(data_format, data)

The re.match seems wrong. If one passes in data_format = “H” or “>H” the unpack should be called, instead everything calls decode() because [sp]? matches 0 (as well as 1). I suggest that at least it should be changed for "?" to "+" or maybe exactly one s or p is more appropriate given the following decode. I haven't thought through all option.

Summary: I think "[>]?[sp]+" is the minimum logical change required to make the code work again. I suspect something like "[sp]" would be better but I haven't considered this in detail.

[Jordi-aguilar]

Same issue here. Link to line of code mentioned: https://github.com/ljean/modbus-tk/blob/master/modbus_tk/modbus.py#L383 Personally no idea how it should be solved.

[jacksonmatheson]

Hey, I can have a look shortly.

Can you send me the code you were running so I can quickly re-create the issue. (I'm not the author dev, but can submit push request.) Should be a simple fix.

[Jordi-aguilar]

Hi Jackson, thanks a lot. In my case I'm using modbus-tk with a piece of hardware. Does the examples in the duplicated issue #178 that I created help you? There I show the values of the variables data and data_format while I was debugging the application right before entering this line In particular, the calls that I do is:

RtuMaster.execute(
    slave=1,
    function_code=cst.READ_HOLDING_REGISTERS,
    starting_address=32770,
    quantity_of_x=quantity_of_x
)

with values of quantity_of_x equal to 1 or 2

[jterce]

Hi,

I think there is a problem if Master.execute method accepts a struct format like ">s" or "sss" as a way to say "use decode instead of unpack", the intention from the user and the return of Master.execute would be ambiguous. What happens when someone actually wants to use those formats with struct.unpack? The return would not be the expected.

data = b'a'
struct.unpack(">s", data)

(b'a',)

data.decode()

'a'

Some options that would resolve this issue in my opinion:

• Compare with a different string literal, something like if data_format == "decode". If it is not going to be used with struct module, this would be less confusing.
• Remove the .decode posibility from this library, considering that kind of transformations out of the scope of this library and leaving it as part of the user code. This way, Master.execute would always follows its description

"Execute a modbus query and returns the data part of the answer as a tuple
...
data_format makes possible to extract the data like defined in the struct python module documentation
...

Using the scenario of the commit de178004476f45a2ff44bd2a11f2ed766c19daf4 as an example, something like this could be done instead:

result = master.execute(1, cst.READ_HOLDING_REGISTERS, 49, 3,data_format='6s')
result[0].decode()

Not sure, at this point, how compatibility breaking with previous changes may those two options be considered.

If those are not posible, maybe replacing [>]?[sp]? with [>]?[sp]$ to force to match only if s or p are also the last character. It is less flexible than adding a +. Considering that flexibility has no usage in this case, i think it is better to be more restrictive, giving more room to the common struct.unpack behaviour. For example, this pattern [>]?[sp] will match formats like >ssbb, which work fine with struct.unpack

data = b'ab\xFF\xEE'
struct.unpack(">ssbb", data)

(b'a', b'b', -1, -18)

but with data.decode() it will just throw an exception in some cases:

UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 2: invalid start byte

I can do a PR for this, but first, what do you think that is the best solution?

Thanks for the effort made by all contributors 😉

[ljean]

Hello, Thanks for your messages and sorry for late answer.

Thanks @jterce for your explanation and proposal. I don't remeber why we have this ".decode"... I agree that it is a bit strange

Rather than using a regex, I have added a parameter in order to avoid the struct things.

https://github.com/ljean/modbus-tk/tree/bug_172

What do you think?

[jterce]

Hello,

I agree with your solution, @ljean.

I proposed a regex change, as said by @kiml, as a minimal change to make it work. But i think using a new parameter is better.

Thanks!

[ljean]

Thanks for feedback