Modify functionality of single unit zero special case

[ChrisProto]

We recently changed our BMS Service to use FluentModbus instead of EasyModbus. Everything seemed to be working. However, for one of our customers their BMS Client was no longer getting data. Using WireShark, I determined that the problem was that the Client was sending in a Unit Identifier of 1, and FluentModbus was responding with a Unit Identifier of 0. (This is Modbus/TCP communications.) It seems that some BMS Clients ignore the Unit Identifier for TCP communications and some Clients don't.

I examined the source code and I see that when we instantiate ModbusTcpServer we get a default Unit Zero. Also, I see that there are AddUnit and RemoveUnit methods available (although they are protected for some reason). From that, I believe that the intent is to be able to create a Modbus Server that can contain many separate units, each having their own separate registers and coils.

So then the question is: what is Unit Zero and what does it do? I see that Unit Zero is a special case:

1. If it exists, it must be the only Unit present.
2. Unit Zero will accept requests for any Unit Identifier and respond with Unit Zero register values.
3. The response message contains a Unit Identifier of zero.

Since zero is not a legitimate Modbus Unit Identifier, BMS Clients generally won't request data from unit zero. I think that in many cases, BMS Clients don't do much with Unit Identifiers when communicating over Modbus/TCP, since they are addressing the device via IP address. Our early tests were using Shortbus Modbus Scanner as a test client. It was able to read registers specifying Unit 1, so we thought everything was working. Apparently it ignored the response Unit Identifier of zero.

However, in the case of our customer's BMS Client, it was rejecting the response because the Unit Identifier was zero.

I propose the following change in functionality for the Unit Zero special-case:
Since Unit Zero accepts incoming requests from all Unit Identifiers, it is pretending to be those units. When it responds with data, it should set the response Unit Identifier to the same value that was in the request.

I did modify the code locally to do this. The changes were actually pretty minimal. Also, I noticed that WireShark likes the modified response better. Since the modified response message contained the same Unit Identifier as the request message, WireShark was able to link them together. So when WireShark displayed the response message, it contained a link to the request, and WireShark displayed the register addresses of the response data.

I will check this in as soon as possible for you to review. However, this is the first time I am contributing to a GitHub project, so I am learning about how to fork and clone the repo to submit code changes. (I had cloned the repo without forking it first, created a local branch to modify the code, and I attempted to push that to origin. It didn't work, and that's when I discovered the fork procedure.)

Here are the changes I made:

1. In ModbusServer.cs:

   • I added a test to determine if we are in the Single Unit Zero Mode

public bool IsSingleZeroUnitMode => UnitIdentifiers.Count == 1 && UnitIdentifiers[0] == 0;

2. In ModbusTcpRequestHandler.cs, TryReceiveRequestAsync():

   • We always accept the incoming Unit Identifier. (I commented out the "if" statement.)

        //if (_handleUnitIdentifiers)   // Apollo3zehn: Is _handleUnitIdentifiers obsolete now?
        UnitIdentifier = unitIdentifier;

        // make sure that the incoming frame is actually addressed to this server
        // If we have only one UnitIdentifier, and it is zero, then we accept all incoming messages
        if (ModbusServer.IsSingleZeroUnitMode || ModbusServer.UnitIdentifiers.Contains(UnitIdentifier))

3. In ModbusServer, Find():
   • I modified Find() so that if we are in the Single Unit Zero Mode, we always return the register data from Unit Zero. Otherwise, we return the register data from the requested unit. We are still throwing the ModbusServer_ZeroUnitOverloadOnlyApplicableInSingleUnitMode and ModbusServer_UnitIdentifierNotFound exceptions where appropriate.

        private Span<byte> Find(byte unitIdentifier, Dictionary<byte, byte[]> map)
        {
            // If we are in the SingleZeroUnitMode then we want the Unit Zero buffer
            if (IsSingleZeroUnitMode)
                unitIdentifier = 0;
            else
            {
                // We are not in SingleZeroUnitMode, so we must not ask for Unit zero.
                if (unitIdentifier == 0)
                    throw new ArgumentException(ErrorMessage.ModbusServer_ZeroUnitOverloadOnlyApplicableInSingleUnitMode);
            }

            // Retrieve the buffer for the specified unitIdentifier (or zero)
            if (!map.TryGetValue(unitIdentifier, out var buffer))
                throw new KeyNotFoundException(ErrorMessage.ModbusServer_UnitIdentifierNotFound);

            return buffer;
        }

So that's it. This is similar to #85 in that I am proposing changes to the Find method. Also, I realize that I could derive my own server class from ModbusServer and make AddUnit and RemoveUnit be public. Then, after instantiating it I could remove unit zero and add unit 1 and it would work as long as the BMS Clients specified unit 1. However, as long as FluentModbus supports a default unit zero, I think my suggestion improves the behavior of unit zero.

I hope you like the idea. (Sorry for the long post.) I will keep working on forking and cloning the repo.

[Apollo3zehn]

Thank your for your detailed problem description. I will try to understand your changes tomorrow and come back to you then.

[ChrisProto]

Ok, Apollo3zehn, I created a fork at ChrisProto/FluentModbus. My changes are in branch mstr-cp-single-unit-zero-special-case. I'm not sure what to do next. Should I create a pull request from that branch to your master branch?

[Apollo3zehn]

Yes, please create a pull request to the dev branch :-)

[Apollo3zehn]

I have some comments:

The spec says to the unit identifier:

This field is set by the MODBUS Client in the request and must be returned with the same value in the response by the server.

So I agree that the server should always return the incoming unit identifier when the frame is accepted.

The spec also states the following

On TCP/IP, the MODBUS server is addressed using its IP address; therefore, the
MODBUS Unit Identifier is useless. The value 0xFF has to be used.
When addressing a MODBUS server connected directly to a TCP/IP network,
it’s recommended not using a significant MODBUS slave address in the “Unit
Identifier” field. In the event of a re-allocation of the IP addresses within an
automated system and if a IP address previously assigned to a MODBUS
server is then assigned to a gateway, using a significant slave address may
cause trouble because of a bad routing by the gateway. Using a non-
significant slave address, the gateway will simply discard the MODBUS PDU
with no trouble. 0xFF is recommended for the “Unit Identifier" as non-
significant value.
Remark : The value 0 is also accepted to communicate directly to a
MODBUS/TCP device.

From my understanding of this statement, Modbus TCP clients should use the value 0xFF or 0x00 for the unit identifier, except when the TCP/IP server is a gateway to forward the Modbus frames to Modbus RTU slaves. So the value 0 is valid in the context of Modbus TCP.

However I think this is meant more as a recommendation rather than a must behaviour, so clients might still put other unit identifiers in their request and so FluentModbus should also return that value in its response.

I agree that in ModbusTcpRequestHandler.cs, method TryReceiveRequestAsync():, the condition should be removed so that UnitIdentifier is always set and that _handleUnitIdentifiers is then obsolete.

I am not sure why the changes in ModbusServer, method Find are needed. Find is only indirectly called from the server application itself (e.g. via GetHoldingRegisterBuffer()) and I see no reason why this application should call Find() with a unit identifier != 0 when it is running in single zero unit mode.

The methods AddUnit and RemoveUnit are protected because otherwise the user could easily create an invalid configuration where we have unit = 0 and units != 0 at the same time. But I see the problem now, the condition

if (unitIdentifier == 0)
{
    if (map.Count == 1)
        return map.First().Value;

    else
        ...
}

is not sufficient.

Maybe the Find() method implementation should simply be

if (!map.TryGetValue(unitIdentifier, out var buffer))
    throw new KeyNotFoundException(ErrorMessage.ModbusServer_UnitIdentifierNotFound);

return buffer;

And we then ensure correctness in method AddUnit (this will not compile because the Error messages do not yet exist):

protected void AddUnit(byte unitIdentifer)
{
    // there are some or more unit identifiers - check if the operation is allowed
    if (_unitIdentifiers.Any())
    {
        if (unitIdentifer == 0)
        {
            // we are not in single zero unit mode
            if (!_unitIdentifiers.Contains(0))
                throw new ArgumentException(ErrorMessage.ModbusServer_ZeroUnitOnlyApplicableInSingleUnitMode);
        }

        else
        {
            // we are in single zero unit mode
            if (_unitIdentifiers.Contains(0))
                throw new ArgumentException(ErrorMessage.ModbusServer_NonZeroUnitNotApplicableInSingleUnitMode);
        }
    }

    if (!_unitIdentifiers.Contains(unitIdentifer))
    {
        _unitIdentifiers.Add(unitIdentifer);
        _inputRegisterBufferMap[unitIdentifer] = new byte[_inputRegisterSize];
        _holdingRegisterBufferMap[unitIdentifer] = new byte[_holdingRegisterSize];
        _coilBufferMap[unitIdentifer] = new byte[_coilSize];
        _discreteInputBufferMap[unitIdentifer] = new byte[_discreteInputSize];
    }
}

What do you think? This way we would ensure correctness about the single zero unit identifier mode right when the list of unit identifiers is modified instead of checking it in the Find method where it is too late I think.

[Apollo3zehn]

When AddUnit is modified as suggested above, AddUnit and RemoveUnit should become public instead of being protected.

[Apollo3zehn]

Is there anything I can do to support you?

[Apollo3zehn]

I have released v5.0.3 with the changes we discussed here. The only exception is that I did not make IsSingleZeroUnitMode public since I did not understand why it is necessary.

I have seen that you have more changes in another branch. If you would like me to review it, please create a pull request with your desired changes :-)

[Apollo3zehn]

I will close this issue for now because the actual issue should have been solved. As mentioned above, please create a PR for other changes.