Docstring of processIncomingFrame is outdated

[smurfix]

The docstring for processIncomingFrame still documents the old callback-based interface.

Also, looking at that code it seems that there is no way _processIncomingFrame could ever return a (nbytes>0, None) tuple. Thus the split between process… and _process… is outdated and should be reverted.

Also I'd like to propose adding a usage example to the docstring, along the lines of

buffer = bytearray()
while True:
    more = read_more_data()
    if not more:
        break
    buffer += more

    while True:
        used,pdu = decoder.processIncomingFrame(buffer)
        del buffer[:used]
        if pdu is None:
            break
        process(pdu)

[janiversen]

Looking at _processIncomingFrame, it contains among other lines:

        if not frame_data:
            return used_len, None

So I am not sure why you think it could not return what the signature defines: tuple[int, ModbusPDU | None]

The doc string of ProcessIncommingPacket seems correct me:

"""Process new packet pattern.

        This takes in a new request packet, adds it to the current
        packet stream, and performs framing on it. That is, checks
        for complete messages, and once found, will process all that
        exist.
        """

Having 2 functions makes the code simpler, and they each have different purposes.

ProcessIncommingPacket is an internal function, which apps should never touch, so adding a usage example is just adding more maintenance.

However pull requests that enhance the library are always welcome.

[smurfix]

So I am not sure why you think it could not return what the signature defines: tuple[int, ModbusPDU | None]

Because in if the PDU is None, used_len is always zero as far as I can tell, thus process… will always return whatever _process… returned, thus it can be skipped.

adds it to the current packet stream … process all that exist.

I don't see any packet stream which ProcessIncomingFrame adds a packet. It will take exactly one packet from the incoming bytestream, if it's complete, and return that single packet.

ProcessIncomingPacket is an internal function, which apps should never touch

I need to call pymodbus from Trio or anyio (i.e. a reasonable Python async framework, which asyncio decidedly is not) thus I do need to call it with my own buffer.

[janiversen]

Because in if the PDU is None, used_len is always zero as far as I can tell, thus process… will always return whatever _process… returned, thus it can be skipped.

used_len is non-zero when garbage is detected and as I explained the two functions have different purposes....anyhow this is how we decided to implement if you want it differently submit a pull request with a proposal.

I don't see any packet stream which ProcessIncomingFrame adds a packet. It will take exactly one packet from the incoming bytestream, if it's complete, and return that single packet.

The text can always be discussed, but it actually adds a packet (which is not the same as a request/response) to the buffer, when a request/response is received as multiple packets.

Feel free to submit a PR with a better text.

I need to call pymodbus from Trio or anyio (i.e. a reasonable Python async framework, which asyncio decidedly is not) thus I do need to call it with my own buffer.

Well at least asyncio is builtin, so some developers think it is good, but that is what we have chosen to support.

Calling ProcessIncommingPacket with your own buffer is really cutting transaction handling in half and are very likely to create a lot of unwanted side effects, the better choice is to call callback_data, and thus have the normal transaction handling. Also remember ProcessIncommingPacket can/will change without warning, whereas callback_data is much more stable. But of course it is your choice.

[janiversen]

Closing as this do not seem to be a bug in pymodbus.

If you want a better/different doctoring, then feel free to make a pull request, but please remember this is an internal function, that we basically do not document for external use.

The function is likely to change, because right now part of the function resides in the transaction handler.