Refactor transaction handling to better separate async and sync code.

[jameshilliard]

Splitting off the transaction refactoring from #2230.

[janiversen]

This a generally not the way we are moving, we do NOT want 2 implementations, so we are are currently moving more and more of the async into the sync code.

The goal is that the sync client is only a shallow shell over async code, meaning 1 implementation.

[jameshilliard]

This a generally not the way we are moving, we do NOT want 2 implementations, so we are are currently moving more and more of the async into the sync code.

So currently the async code is using a ModbusTransactionManager class that has a bunch of logic which is only used by the sync codepaths, this is merely separating it out better so that it's clear which methods are used by both and which are sync only. The idea is that code shared by both sync and async clients would go in the ModbusTransactionManager.

The goal is that the sync client is only a shallow shell over async code, meaning 1 implementation.

Um, wouldn't this then be what we want? I've made it so that ModbusTransactionManager is the transaction code for both sync and async clients. I then have SyncModbusTransactionManager which is extending ModbusTransactionManager with the sync only methods so it's deriving from the common implementation.

[janiversen]

We do not want 2 implementations ! the current difference between sync and async are basically in the transport layer, and we do not want to expand these differences.

[janiversen]

Generally speaking this PR adds code complexity, where our aim is to simplify the code.

We are currently reworking the framer layer for version 3.7.0 and next up is the transaction layer, so adding complexity there is really not a goal.

[jameshilliard]

the current difference between sync and async are basically in the transport layer, and we do not want to expand these differences.

Clearly there's a huge difference at the transaction layer as well, keep in mind the code I moved into SyncModbusTransactionManager was already only being used by the sync client, so I'm not creating more differences so much as making it more obvious what the differences are, the async code only used a small percentage of the ModbusTransactionManager code in the first place, most was dead code when using an async client(which was making the code extremely difficult to read and type check properly since the client variable only used by sync clients in transaction manager was getting set to two very different client classes previously).

Generally speaking this PR adds code complexity, where our aim is to simplify the code.

I mean, I'm trying to simplify the code by making it clear which code is used by which client, I'm not changing the existing situation there or duplicating shared code. The transaction id tracking is the only part of the transaction code the async client was using, so it really shouldn't be importing a transaction class with a bunch of unused methods. Combining logic with async code can then be done afterwards where it makes sense but I think it's a good idea to do an initial refactoring to make it clear what code is used where essentially.

[janiversen]

You have a point in making it clear what is only sync and what is shared !
that is a point I can vouch for.

I would like to see, however, the sync part reduced, since this is the exercise I am currently doing with the frame layer and that is ultimately what we need to do on every layer.

[jameshilliard]

I would like to see, however, the sync part reduced

I don't disagree there, but I think this should be done in stages where we first ensure that we can identify which code is shared, which is async only and which is sync only, otherwise it's just too difficult IMO to reason about refactoring.

[alexrudd2]

This should be ModbusRequest | ModbusResponse