Support providing framer type Enum or class

[pjkundert]

Historically, one could create new Framer classes and supply them. For example, to implement "evil" framers, to test sending various errors in Modbus.

This fix maintains support for supplying a FramerType Enum, and restores the ability to pass a Framer class.

[janiversen]

This is only the server, if we are to include this feature again, at least it must also support client and sync client.

[pjkundert]

OK, I think I've generalized this to all servers and clients. I couldn't figure out where in the tests to be able to add an "evil" framer, though...

[janiversen]

Did you look at the test directory, it has a sub directory called framer, which is where framer testing goes. Please do not change existing tests, but add tests.

FramerBase is an internal class which we do not want public, because having it public makes it a lot harder to change, please see my bullet list.

[pjkundert]

I did look at it, but didn't find any tests of framers in a full I/O cycle. I'll look again.

Agreed; I don't think FramerBase should be public -- users can derive "evil" framers from any of the existing public FramerXxx classes.

[janiversen]

I am not convinced, allowing custom framers is a good way, because the framers are pretty complicated and a lot more tightly knitted into transport and transaction, than it was before. The framer functionality is defined in the modbus standard, so what would a custom framers implement ?

Why do you need to replace the framer, and not just use the byte stream/ pdu trace points that is provided. The trace points allows you to intercept and modify non-standard byte stream or even the pdu contents.

A custom framer would need a number of items:

• most importantly a documentation why it is needed, because in general pymodbus only support the standard
• documentation of how the custom framer works
• example of how to use it in server/cliemt
• support for both clients as well as the server
• a custom class to inherit from, that implements validation of the inherited class, to avoid side effects in the rest of the library
• a test class (framer have 100% test coverage)

If you implement the above items, then it would become a feature in pymodbus.

[janiversen]

Your idea of "evil" framers is not a bad idea, the simulator does that using the trace points.

[pjkundert]

OK, I've extended support for creating custom (eg. "evil") FramerBase classes to the client API.

Here's an example of an evil FramerBase class. After using FRAMER_NAME_TO_CLASS[framer] with a FramerType to receive one of the support FramerBase classes, eg. FramerRtu, we can implement a Framer with a random delay:

        class EvilFramerDelayResponse( framer ):
            delay               = 5

            def buildFrame(self, message):
                ''' Creates a ready to send modbus packet but delays the return.

                :param message: The populated request/response to send
                '''
                packet          = super( EvilFramerDelayResponse, self ).buildFrame( message )

                log.info( "Delaying response for %s seconds", self.delay )
                delay           = self.delay
                if isinstance( delay, (list,tuple) ):
                    delay       = random.uniform( *delay )
                time.sleep( delay )

                return packet

or that truncates a response:

        class EvilFramerTruncateResponse( framer ):
            def buildFrame(self, message):
                ''' Creates a *truncated* ready to send modbus packet.  Truncates from 1
                to all of the bytes, before returning response.

                :param message: The populated request/response to send
                '''
                packet          = super( EvilFramerTruncateResponse, self ).buildFrame( message )
                datalen         = len( packet )
                corrlen         = datalen - random.randint( 1, datalen )

                log.info( "Corrupting response; truncating from %d to %d bytes", datalen, corrlen )

                return packet[:corrlen]

[janiversen]

Your examples are good, but you can do exactly the same with the trace methods, and then it will work for tcp/tls/udp and serial, whereas you framer is limited to one type.

I will have a look at your code, once it passes CI, assuming your update includes the points I raised.

[janiversen]

To help with ci errors, check_ci.sh is your friend, as it runs the same tests as ci.

[janiversen]

I hope you are aware that using time.sleep() in an async program blocks everything, which probably is not what wanted.

[pjkundert]

Ya, I suspect you're right about using the "trace" methods, but I couldn't follow the simulator well enough to easily translate my code -- the "evil" framer seems simpler. Also -- since I use the base class returned by the FramerType lookup - my "evil" framer does extend the appropriate RTU/TLS/TCP or Serial base class.

[janiversen]

How can it extend all the framer types using the base class, framerRTU adds different bytes to the stream than framerTCP does, this means your framer implements something different.

Please also be aware the framer class is not part of the recv/send structure but merely a utility class to convert a pdu to/from a byte stream. your class probably works in sunny day situations but most likely not in a number of problem sets.

Anyhow we will not expose FramerBase, and based on your example a custom base class needs to guarantee that a user supplied class do not endanger the async work flow.

There are at least one example of how to use the tracer methods (all 4).

[pjkundert]

I can get a public FramerXxx class using some method (eg. the FramerType Enum and FRAMER_NAME_TO...).

Then, I can dynamically create a class (above) based on that dynamically selected FramerXxx base class. Here's another example. Remember, 'framer' in this class definition was dynamically selected, and could be any valid FramerXxx base class:

        class EvilFramerCorruptResponse( framer ):
            what                        = "transaction"

            def buildFrame(self, message):
                ''' Creates a *corrupted* ready to send modbus packet.  Truncates from 1
                to all of the bytes, before returning response.

                :param message: The populated request/response to send

                WARNING: pymodbus seems to swallow any exceptions thrown by these
                methods.  This seems like a defect; it should log them, at least.
                '''
                try:
                    log.info("Encoding package")
                    message.encode()

                    if self.what == "transaction":
                        message.transaction_id ^= 0xFFFF
                        packet  = super( EvilFramerCorruptResponse, self ).buildFrame( message )
                        message.transaction_id ^= 0xFFFF
                    elif self.what == "registers":
                        if isinstance( message, ReadHoldingRegistersResponse ):
                            # These have '.registers' attribute, which is a list.
                            # Add/remove some
                            saveregs            = message.registers
                            if len( message.registers ) == 0 or random.randint( 0, 1 ):
                                message.registers += [999]
                            else:
                                message.registers = message.registers[:-1]
                            packet              = super( EvilFramerCorruptResponse, self ).buildFrame( message )
                            message.registers   = saveregs
                        elif isinstance( message, WriteSingleRegisterResponse ):
                            # Flip the responses address bits and then flip them back.
                            message.address    ^= 0xFFFF
                            packet              = super( EvilFramerCorruptResponse, self ).buildFrame( message )
                            message.address    ^= 0xFFFF
                        elif isinstance( message, WriteMultipleRegistersResponse ):
                            # Flip the responses address bits and then flip them back.
                            message.address    ^= 0xFFFF
                            packet              = super( EvilFramerCorruptResponse, self ).buildFrame( message )
                            message.address    ^= 0xFFFF
                        else:
                            raise NotImplementedException(
                                "Unhandled class for register corruption; not implemented" )
                    elif self.what == "protocol":
                        message.protocol_id    ^= 0xFFFF
                        packet                  = super( EvilFramerCorruptResponse, self ).buildFrame( message )
                        message.protocol_id    ^= 0xFFFF
                    elif self.what == "unit":
                        message.unit_id        ^= 0xFF
                        packet                  = super( EvilFramerCorruptResponse, self ).buildFrame( message )
                        message.unit_id        ^= 0xFF
                    elif self.what == "function":
                        message.function_code  ^= 0xFF
                        packet                  = super( EvilFramerCorruptResponse, self ).buildFrame( message )
                        message.function_code  ^= 0xFF
                    else:
                        raise NotImplementedException(
                            "Unknown corruption specified; not implemented" )
                except Exception:
                    log.info( "Could not build corrupt packet: %s", traceback.format_exc() )
                return packet

For the limited scope of this change, I think it provides a valuable set of capabilities to protocol implementers to use pymodbus...

[janiversen]

Yes you can, but you are using internal methods/classes that we do not want to be part of the public API.

Nearly anything can be done, when you overwrite/extend internals, and that is your choice to do so, but making it officially part of the API, makes it difficult to maintain because it only can be changed in x.y releases.

I have outlined earlier what is needed for custom framers to come back in the API.

And just for the record your example can be done with the 2 tracepdu methods.

[janiversen]

Your comment in the example is not correct, the exception is logged, and depending on the recv/send direction a retry or a close is initiated. You are mixing with internal classes so you need to look at how they are used.

[pjkundert]

Yes, this code was taking from a system implemented using early pymodbus 1.x; at the time, that was what I experienced. I had to monkey-patch a multitude of pymodbus internals to get Modus Serial RTU over RS-485 to work. Thankfully, most of those issues have now been addressed -- the remaining one was how to implement my coterie of "evil" framers.

Rather than try to re-architect them in terms of tracepdu methods, I was trying to get them to work, and the threshold to restoring the previous custom framer class functionality seemed quite low.

Custom Framers are always going to be based on modifying "internal" implementations, and I don't think writing a custom framer without the expectation of reading the implementation of existing framers is likely. Do you? In my 20+ years of Python, I have seldom ended up using a Python module without having to read the source code, and (often) fix/patch it. I rarely find that the documentation matches the implementation, so never depend on it, and seldom even read it -- why not just go to the definitive "source" of truth?

So, while I would like to retain this custom framer functionality as it was in the 1.x version branch, I know that this is your module, not mine! You should do what you feel is best for your future maintainability! I am burdened with maintaining some very complex python modules (cpppo, python-slip39), and have made some expensive decisions to publish functionality that is burdensome to maintain...

[janiversen]

Please close the PR if you do not to complete it.

[janiversen]

Please be aware that the framer structure have changed a lot since pymodbus v.1, and today it is a utility class not directly in the the send/recv line and finally v.1 did not have async in its core, so simply reusing a v.1 framer is going to cause a lot of problems (like e,g, your time.sleep).

[pjkundert]

Yes, timing-related "evil" framers will be impossible to implement cleanly due to asyncio, but structural framers will be a natural and simple implementation. There are probably a multitude of partially-broken Modbus devices out there that require slight modifications or extensions to the standard wire protocol in order to function, where a custom Framer would be the natural solution.

I'm trying to decide on my approach -- I need to deliver some functionality next week, so I may just fork the project and issue my own version on PyPI...

Alternatively, if I implement a minimal example and test case for a structural "evil" or enhanced-protocol Framer, are you saying you may merge the functionality?

[janiversen]

Why are they impossible to implement we do it in the tracers, without problems, you just cannot use time.sleep because it blocks everything.

The custom framer is really the wrong approach, mostly because framer is now a utility class so e.g. delay will not work correctly. did you investigate the tracer, they give you all you need. The simulator allows to delay packet, break packet into multiple send with delay, send malicious bytes before/after the packet, which we have used in several projects to help with bad modbus implementations, the code as you say the truth and easy to grasp and twist to your needs.

Custom framer will not be accepted without the list I wrote, simply because it does not solve e.g. delay and its far too complicated (if you want your server to actually work).

you are of course free to fork your own pymodbus, please remember to follow the foss rules, which means fork not copy, and a reference to the original pymodbus, otherwise you might run into problems with pypi.

[janiversen]

Thinking about it, why do you need your own version of pymodbus, you can simply use the standard Pymodbus and overwrite the framer object with your own object....that is far less maintenance burden for you.

I often take the approach to test out new features, mainly because it makes a clear break, between the product pymodbus and my experiment pymodbus.

[janiversen]

You remove one line and add the same information in 2 lines, that does not make sense ?

the app knows if it has requested slaves to be ignored.

Secondly this change seems unrelated to the title of this PR.